From 008314b5a8c099f7e2d191da85c53d2a235db518 Mon Sep 17 00:00:00 2001 From: Julius Volz Date: Tue, 21 May 2013 23:19:39 +0200 Subject: [PATCH 1/3] Ensure that all extracted samples are added to view. The current behavior only adds those samples to the view that are extracted by the last pass of the last processed op and throws other ones away. This is a bug. We need to append all samples that are extracted by each op pass. This also makes view.appendSamples() take an array of samples. --- storage/metric/memory.go | 14 ++++++++------ storage/metric/tiered.go | 8 +++----- storage/metric/view.go | 4 ++-- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/storage/metric/memory.go b/storage/metric/memory.go index f31e6ddee..8fa1a9ccb 100644 --- a/storage/metric/memory.go +++ b/storage/metric/memory.go @@ -181,21 +181,23 @@ func (s *memorySeriesStorage) AppendSample(sample model.Sample) error { return nil } -// Append raw sample, bypassing indexing. Only used to add data to views, which -// don't need to lookup by metric. -func (s *memorySeriesStorage) appendSampleWithoutIndexing(f *model.Fingerprint, timestamp time.Time, value model.SampleValue) { +// Append raw samples, bypassing indexing. Only used to add data to views, +// which don't need to lookup by metric. +func (s *memorySeriesStorage) appendSamplesWithoutIndexing(fingerprint *model.Fingerprint, samples model.Values) { s.RLock() - series, ok := s.fingerprintToSeries[*f] + series, ok := s.fingerprintToSeries[*fingerprint] s.RUnlock() if !ok { series = newStream(model.Metric{}) s.Lock() - s.fingerprintToSeries[*f] = series + s.fingerprintToSeries[*fingerprint] = series s.Unlock() } - series.add(timestamp, value) + for _, sample := range samples { + series.add(sample.Timestamp, sample.Value) + } } func (s *memorySeriesStorage) GetFingerprintsForLabelSet(l model.LabelSet) (fingerprints model.Fingerprints, err error) { diff --git a/storage/metric/tiered.go b/storage/metric/tiered.go index 98f9e1d87..7976d2420 100644 --- a/storage/metric/tiered.go +++ b/storage/metric/tiered.go @@ -348,12 +348,10 @@ func (t *TieredStorage) renderView(viewJob viewJob) { for op.CurrentTime() != nil && !op.CurrentTime().After(targetTime) { out = op.ExtractSamples(model.Values(currentChunk)) - } - } - // Append the extracted samples to the materialized view. - for _, sample := range out { - view.appendSample(scanJob.fingerprint, sample.Timestamp, sample.Value) + // Append the extracted samples to the materialized view. + view.appendSamples(scanJob.fingerprint, out) + } } // Throw away standing ops which are finished. diff --git a/storage/metric/view.go b/storage/metric/view.go index a0ef8af7f..1330e6eac 100644 --- a/storage/metric/view.go +++ b/storage/metric/view.go @@ -105,8 +105,8 @@ type view struct { *memorySeriesStorage } -func (v view) appendSample(fingerprint *model.Fingerprint, timestamp time.Time, value model.SampleValue) { - v.memorySeriesStorage.appendSampleWithoutIndexing(fingerprint, timestamp, value) +func (v view) appendSamples(fingerprint *model.Fingerprint, samples model.Values) { + v.memorySeriesStorage.appendSamplesWithoutIndexing(fingerprint, samples) } func newView() view { From 83d60bed89ce831d720b9a28646858372eae7ac4 Mon Sep 17 00:00:00 2001 From: Julius Volz Date: Tue, 21 May 2013 23:20:43 +0200 Subject: [PATCH 2/3] extractValuesAroundTime() code simplification. --- storage/metric/operation.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/storage/metric/operation.go b/storage/metric/operation.go index 9c3408c0d..595cfaf19 100644 --- a/storage/metric/operation.go +++ b/storage/metric/operation.go @@ -111,14 +111,14 @@ func extractValuesAroundTime(t time.Time, in model.Values) (out model.Values) { if in[i].Timestamp.Equal(t) && len(in) > i+1 { // We hit exactly the current sample time. Very unlikely in practice. // Return only the current sample. - out = append(out, in[i]) + out = in[i : i+1] } else { if i == 0 { // We hit before the first sample time. Return only the first sample. - out = append(out, in[0:1]...) + out = in[0:1] } else { // We hit between two samples. Return both surrounding samples. - out = append(out, in[i-1:i+1]...) + out = in[i-1 : i+1] } } } From f2b48b8c4ae253cef5f53ffa7673af6232bafcf5 Mon Sep 17 00:00:00 2001 From: Julius Volz Date: Wed, 22 May 2013 00:53:25 +0200 Subject: [PATCH 3/3] Make getValuesAtIntervalOp consume all chunk data in one pass. This is mainly a small performance improvement, since we skip past the last extracted time immediately if it was also the last sample in the chunk, instead of trying to extract non-existent values before the chunk end again and again and only gradually approaching the end of the chunk. --- storage/metric/operation.go | 6 +++--- storage/metric/operation_test.go | 10 ++++++++++ 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/storage/metric/operation.go b/storage/metric/operation.go index 595cfaf19..f390caaae 100644 --- a/storage/metric/operation.go +++ b/storage/metric/operation.go @@ -161,12 +161,12 @@ func (g *getValuesAtIntervalOp) ExtractSamples(in model.Values) (out model.Value lastExtractedTime := out[len(out)-1].Timestamp in = in.TruncateBefore(lastExtractedTime.Add(1)) g.from = g.from.Add(g.interval) - if lastExtractedTime.Equal(lastChunkTime) { - break - } for !g.from.After(lastExtractedTime) { g.from = g.from.Add(g.interval) } + if lastExtractedTime.Equal(lastChunkTime) { + break + } if g.from.After(g.through) { break } diff --git a/storage/metric/operation_test.go b/storage/metric/operation_test.go index 772f439da..a7744ce3a 100644 --- a/storage/metric/operation_test.go +++ b/storage/metric/operation_test.go @@ -1610,6 +1610,16 @@ func TestGetValuesAtIntervalOp(t *testing.T) { t.Fatalf("%d. expected length %d, got %d: %v", i, len(scenario.out), len(actual), scenario.op) t.Fatalf("%d. expected length %d, got %d", i, len(scenario.out), len(actual)) } + + if len(scenario.in) < 1 { + continue + } + opTime := scenario.op.CurrentTime() + lastExtractedTime := scenario.out[len(scenario.out)-1].Timestamp + if opTime != nil && opTime.Before(lastExtractedTime) { + t.Fatalf("%d. expected op.CurrentTime() to be nil or after current chunk, %v, %v", i, scenario.op.CurrentTime(), scenario.out) + } + for j, out := range scenario.out { if out != actual[j] { t.Fatalf("%d. expected output %v, got %v", i, scenario.out, actual)