diff --git a/promql/histogram_stats_iterator.go b/promql/histogram_stats_iterator.go index f5224825d3..bb8c610964 100644 --- a/promql/histogram_stats_iterator.go +++ b/promql/histogram_stats_iterator.go @@ -19,27 +19,24 @@ import ( "github.com/prometheus/prometheus/tsdb/chunkenc" ) -// HistogramStatsIterator is an iterator that returns histogram objects -// which have only their sum and count values populated. The iterator handles -// counter reset detection internally and sets the counter reset hint accordingly -// in each returned histogram object. +// HistogramStatsIterator is an iterator that returns histogram objects that +// have only their sum and count values populated. The iterator handles counter +// reset detection internally and sets the counter reset hint accordingly in +// each returned histogram object. The Next and Seek methods of the iterator +// will never return ValHistogram, but ValFloatHistogram instead. Effectively, +// the iterator enforces conversion of (integer) Histogram to FloatHistogram. +// The AtHistogram method must not be called (and will panic). type HistogramStatsIterator struct { chunkenc.Iterator - currentH *histogram.Histogram - lastH *histogram.Histogram - currentFH *histogram.FloatHistogram lastFH *histogram.FloatHistogram - - currentSeriesRead bool } // NewHistogramStatsIterator creates a new HistogramStatsIterator. func NewHistogramStatsIterator(it chunkenc.Iterator) *HistogramStatsIterator { return &HistogramStatsIterator{ Iterator: it, - currentH: &histogram.Histogram{}, currentFH: &histogram.FloatHistogram{}, } } @@ -48,44 +45,38 @@ func NewHistogramStatsIterator(it chunkenc.Iterator) *HistogramStatsIterator { // objects already allocated where possible. func (hsi *HistogramStatsIterator) Reset(it chunkenc.Iterator) { hsi.Iterator = it - hsi.currentSeriesRead = false + hsi.lastFH = nil } -// AtHistogram returns the next timestamp/histogram pair. The counter reset -// detection is guaranteed to be correct only when the caller does not switch -// between AtHistogram and AtFloatHistogram calls. -func (hsi *HistogramStatsIterator) AtHistogram(h *histogram.Histogram) (int64, *histogram.Histogram) { - var t int64 - t, hsi.currentH = hsi.Iterator.AtHistogram(hsi.currentH) - if value.IsStaleNaN(hsi.currentH.Sum) { - h = &histogram.Histogram{Sum: hsi.currentH.Sum} - return t, h +// Next mostly relays to the underlying iterator, but changes a ValHistogram +// return into a ValFloatHistogram return. +func (hsi *HistogramStatsIterator) Next() chunkenc.ValueType { + vt := hsi.Iterator.Next() + if vt == chunkenc.ValHistogram { + return chunkenc.ValFloatHistogram } - - if h == nil { - h = &histogram.Histogram{ - CounterResetHint: hsi.getResetHint(hsi.currentH), - Count: hsi.currentH.Count, - Sum: hsi.currentH.Sum, - } - hsi.setLastH(hsi.currentH) - return t, h - } - - returnValue := histogram.Histogram{ - CounterResetHint: hsi.getResetHint(hsi.currentH), - Count: hsi.currentH.Count, - Sum: hsi.currentH.Sum, - } - returnValue.CopyTo(h) - - hsi.setLastH(hsi.currentH) - return t, h + return vt } -// AtFloatHistogram returns the next timestamp/float histogram pair. The counter -// reset detection is guaranteed to be correct only when the caller does not -// switch between AtHistogram and AtFloatHistogram calls. +// Seek mostly relays to the underlying iterator, but changes a ValHistogram +// return into a ValFloatHistogram return. +func (hsi *HistogramStatsIterator) Seek(t int64) chunkenc.ValueType { + vt := hsi.Iterator.Seek(t) + if vt == chunkenc.ValHistogram { + return chunkenc.ValFloatHistogram + } + return vt +} + +// AtHistogram must never be called. +func (*HistogramStatsIterator) AtHistogram(*histogram.Histogram) (int64, *histogram.Histogram) { + panic("HistogramStatsIterator.AtHistogram must never be called") +} + +// AtFloatHistogram returns the next timestamp/float histogram pair. The method +// performs a counter reset detection on the fly. It will return an explicit +// hint (not UnknownCounterReset) if the previous sample has been accessed with +// the same iterator. func (hsi *HistogramStatsIterator) AtFloatHistogram(fh *histogram.FloatHistogram) (int64, *histogram.FloatHistogram) { var t int64 t, hsi.currentFH = hsi.Iterator.AtFloatHistogram(hsi.currentFH) @@ -114,26 +105,12 @@ func (hsi *HistogramStatsIterator) AtFloatHistogram(fh *histogram.FloatHistogram return t, fh } -func (hsi *HistogramStatsIterator) setLastH(h *histogram.Histogram) { - hsi.lastFH = nil - if hsi.lastH == nil { - hsi.lastH = h.Copy() - } else { - h.CopyTo(hsi.lastH) - } - - hsi.currentSeriesRead = true -} - func (hsi *HistogramStatsIterator) setLastFH(fh *histogram.FloatHistogram) { - hsi.lastH = nil if hsi.lastFH == nil { hsi.lastFH = fh.Copy() } else { fh.CopyTo(hsi.lastFH) } - - hsi.currentSeriesRead = true } func (hsi *HistogramStatsIterator) getFloatResetHint(hint histogram.CounterResetHint) histogram.CounterResetHint { @@ -141,44 +118,19 @@ func (hsi *HistogramStatsIterator) getFloatResetHint(hint histogram.CounterReset return hint } prevFH := hsi.lastFH - if prevFH == nil || !hsi.currentSeriesRead { - if hsi.lastH == nil || !hsi.currentSeriesRead { - // We don't know if there's a counter reset. - return histogram.UnknownCounterReset - } - prevFH = hsi.lastH.ToFloat(nil) + if prevFH == nil { + // We don't know if there's a counter reset. Note that this + // generally will trigger an explicit counter reset detection by + // the PromQL engine, which in turn isn't as reliable in this + // case because the PromQL engine will not see the buckets. + // However, we can assume that in cases where the counter reset + // detection is relevant, an iteration through the series has + // happened, and therefore we do not end up here in the first + // place. + return histogram.UnknownCounterReset } if hsi.currentFH.DetectReset(prevFH) { return histogram.CounterReset } return histogram.NotCounterReset } - -func (hsi *HistogramStatsIterator) getResetHint(h *histogram.Histogram) histogram.CounterResetHint { - if h.CounterResetHint != histogram.UnknownCounterReset { - return h.CounterResetHint - } - var prevFH *histogram.FloatHistogram - if hsi.lastH == nil || !hsi.currentSeriesRead { - if hsi.lastFH == nil || !hsi.currentSeriesRead { - // We don't know if there's a counter reset. Note that - // this generally will trigger an explicit counter reset - // detection by the PromQL engine, which in turn isn't - // as reliable in this case because the PromQL engine - // will not see the buckets. However, we can assume that - // in cases where the counter reset detection is - // relevant, an iteration through the series has - // happened, and therefore we do not end up here in the - // first place. - return histogram.UnknownCounterReset - } - prevFH = hsi.lastFH - } else { - prevFH = hsi.lastH.ToFloat(nil) - } - fh := h.ToFloat(nil) - if fh.DetectReset(prevFH) { - return histogram.CounterReset - } - return histogram.NotCounterReset -} diff --git a/promql/histogram_stats_iterator_test.go b/promql/histogram_stats_iterator_test.go index 3e3f2dd4b2..b9d37ce3b9 100644 --- a/promql/histogram_stats_iterator_test.go +++ b/promql/histogram_stats_iterator_test.go @@ -114,64 +114,32 @@ func TestHistogramStatsDecoding(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - t.Run("histogram_stats", func(t *testing.T) { - check := func(statsIterator *HistogramStatsIterator) { - decodedStats := make([]*histogram.Histogram, 0) - - for statsIterator.Next() != chunkenc.ValNone { - _, h := statsIterator.AtHistogram(nil) - decodedStats = append(decodedStats, h) - } - - for i := 0; i < len(tc.histograms); i++ { - require.Equalf(t, tc.expectedHints[i], decodedStats[i].CounterResetHint, "mismatch in counter reset hint for histogram %d", i) - h := tc.histograms[i] - if value.IsStaleNaN(h.Sum) { - require.True(t, value.IsStaleNaN(decodedStats[i].Sum)) - require.Equal(t, uint64(0), decodedStats[i].Count) - } else { - require.Equal(t, tc.histograms[i].Count, decodedStats[i].Count) - require.Equal(t, tc.histograms[i].Sum, decodedStats[i].Sum) - } + check := func(statsIterator *HistogramStatsIterator) { + decodedStats := make([]*histogram.FloatHistogram, 0) + for statsIterator.Next() != chunkenc.ValNone { + _, h := statsIterator.AtFloatHistogram(nil) + decodedStats = append(decodedStats, h) + } + for i := 0; i < len(tc.histograms); i++ { + require.Equal(t, tc.expectedHints[i], decodedStats[i].CounterResetHint) + fh := tc.histograms[i].ToFloat(nil) + if value.IsStaleNaN(fh.Sum) { + require.True(t, value.IsStaleNaN(decodedStats[i].Sum)) + require.Equal(t, float64(0), decodedStats[i].Count) + } else { + require.Equal(t, fh.Count, decodedStats[i].Count) + require.Equal(t, fh.Sum, decodedStats[i].Sum) } } + } - // Check that we get the expected results with a fresh iterator. - statsIterator := NewHistogramStatsIterator(newHistogramSeries(tc.histograms).Iterator(nil)) - check(statsIterator) + // Check that we get the expected results with a fresh iterator. + statsIterator := NewHistogramStatsIterator(newHistogramSeries(tc.histograms).Iterator(nil)) + check(statsIterator) - // Check that we get the same results if we reset and reuse that iterator. - statsIterator.Reset(newHistogramSeries(tc.histograms).Iterator(nil)) - check(statsIterator) - }) - t.Run("float_histogram_stats", func(t *testing.T) { - check := func(statsIterator *HistogramStatsIterator) { - decodedStats := make([]*histogram.FloatHistogram, 0) - for statsIterator.Next() != chunkenc.ValNone { - _, h := statsIterator.AtFloatHistogram(nil) - decodedStats = append(decodedStats, h) - } - for i := 0; i < len(tc.histograms); i++ { - require.Equal(t, tc.expectedHints[i], decodedStats[i].CounterResetHint) - fh := tc.histograms[i].ToFloat(nil) - if value.IsStaleNaN(fh.Sum) { - require.True(t, value.IsStaleNaN(decodedStats[i].Sum)) - require.Equal(t, float64(0), decodedStats[i].Count) - } else { - require.Equal(t, fh.Count, decodedStats[i].Count) - require.Equal(t, fh.Sum, decodedStats[i].Sum) - } - } - } - - // Check that we get the expected results with a fresh iterator. - statsIterator := NewHistogramStatsIterator(newHistogramSeries(tc.histograms).Iterator(nil)) - check(statsIterator) - - // Check that we get the same results if we reset and reuse that iterator. - statsIterator.Reset(newHistogramSeries(tc.histograms).Iterator(nil)) - check(statsIterator) - }) + // Check that we get the same results if we reset and reuse that iterator. + statsIterator.Reset(newHistogramSeries(tc.histograms).Iterator(nil)) + check(statsIterator) }) } } @@ -193,17 +161,18 @@ func TestHistogramStatsMixedUse(t *testing.T) { histogram.NotCounterReset, histogram.CounterReset, } + // Note that statsIterator always returns float histograms. actualHints := make([]histogram.CounterResetHint, 3) typ := statsIterator.Next() - require.Equal(t, chunkenc.ValHistogram, typ) - _, h := statsIterator.AtHistogram(nil) + require.Equal(t, chunkenc.ValFloatHistogram, typ) + _, h := statsIterator.AtFloatHistogram(nil) actualHints[0] = h.CounterResetHint typ = statsIterator.Next() - require.Equal(t, chunkenc.ValHistogram, typ) - _, h = statsIterator.AtHistogram(nil) + require.Equal(t, chunkenc.ValFloatHistogram, typ) + _, h = statsIterator.AtFloatHistogram(nil) actualHints[1] = h.CounterResetHint typ = statsIterator.Next() - require.Equal(t, chunkenc.ValHistogram, typ) + require.Equal(t, chunkenc.ValFloatHistogram, typ) _, fh := statsIterator.AtFloatHistogram(nil) actualHints[2] = fh.CounterResetHint