From 87d7c12563c8e1786bce878bae78237c9315d467 Mon Sep 17 00:00:00 2001 From: beorn7 Date: Tue, 2 Sep 2025 22:32:36 +0200 Subject: [PATCH 1/7] promql: Fix trigger for `HistogramStatsIterator` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #16702 introduced a regression because it was too strict in detecting the condition for using the `HistogramStatsIterator`. It essentially required the triggering function to be buried at least one level deep. `histogram_count(sum(rate(native_histogram_series[2m]))` would not trigger anymore, but `1*histogram_count(sum(rate(native_histogram_series[2m]))` would. Ironically, PR #16682 made the performance of the `HistogramStatsIterator` so much worse that _not_ using it was often better, but this has to be addressed in a separate commit. This commit reinstates the previous `HistogramStatsIterator` detection behavior, as PR #16702 intended to keep it. Relevant benchmark changes with this commit (i.e. old is without using `HistogramStatsIterator`, new is with `HistogramStatsIterator`): name old time/op new time/op delta NativeHistograms/histogram_count_with_short_rate_interval-16 802ms ± 3% 837ms ± 3% +4.42% (p=0.008 n=5+5) NativeHistograms/histogram_count_with_long_rate_interval-16 1.22s ± 3% 1.11s ± 1% -9.46% (p=0.008 n=5+5) NativeHistogramsCustomBuckets/histogram_count_with_short_rate_interval-16 611ms ± 5% 751ms ± 6% +22.87% (p=0.008 n=5+5) NativeHistogramsCustomBuckets/histogram_count_with_long_rate_interval-16 975ms ± 4% 1131ms ±11% +16.04% (p=0.008 n=5+5) name old alloc/op new alloc/op delta NativeHistograms/histogram_count_with_short_rate_interval-16 222MB ± 0% 531MB ± 0% +139.63% (p=0.008 n=5+5) NativeHistograms/histogram_count_with_long_rate_interval-16 323MB ± 0% 528MB ± 0% +63.81% (p=0.008 n=5+5) NativeHistogramsCustomBuckets/histogram_count_with_short_rate_interval-16 179MB ± 0% 452MB ± 0% +153.07% (p=0.016 n=4+5) NativeHistogramsCustomBuckets/histogram_count_with_long_rate_interval-16 175MB ± 0% 452MB ± 0% +157.73% (p=0.016 n=4+5) name old allocs/op new allocs/op delta NativeHistograms/histogram_count_with_short_rate_interval-16 4.48M ± 0% 8.95M ± 0% +99.51% (p=0.008 n=5+5) NativeHistograms/histogram_count_with_long_rate_interval-16 5.02M ± 0% 8.84M ± 0% +75.89% (p=0.008 n=5+5) NativeHistogramsCustomBuckets/histogram_count_with_short_rate_interval-16 3.00M ± 0% 5.96M ± 0% +98.93% (p=0.008 n=5+5) NativeHistogramsCustomBuckets/histogram_count_with_long_rate_interval-16 2.89M ± 0% 5.86M ± 0% +102.69% (p=0.016 n=4+5) Signed-off-by: beorn7 --- promql/engine.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/promql/engine.go b/promql/engine.go index 9024df83da..2feb6c3c92 100644 --- a/promql/engine.go +++ b/promql/engine.go @@ -3891,7 +3891,7 @@ func detectHistogramStatsDecoding(expr parser.Expr) { return nil } - for i := len(path) - 1; i > 0; i-- { // Walk backwards up the path. + for i := len(path) - 1; i >= 0; i-- { // Walk backwards up the path. call, ok := path[i].(*parser.Call) if !ok { continue From 5010bd4bb1ad8964a802ce8e8b1ddd59cdbf2317 Mon Sep 17 00:00:00 2001 From: beorn7 Date: Tue, 2 Sep 2025 23:35:49 +0200 Subject: [PATCH 2/7] promql: Optimize `HistogramStatsIterator` by disallowing integer histograms MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `HistogramStatsIterator` is only meant to be used within PromQL. PromQL only ever uses float histograms. If `HistogramStatsIterator` is capable of handling integer histograms, it will still be used, for example by the `BufferedSeriesIterator`, which buffers samples and will use an integer `Histogram` for it, if the underlying chunk is an integer histogram chunk (which is common). However, we can simply intercept the `Next` and `Seek` calls and pretend to only ever be able te return float histograms. This has the welcome side effect that we do not have to handle a mix of float and integer histograms in the `HistogramStatsIterator` anymore. With this commit, the `AtHistogram` call has been changed to panic so that we ensure it is never called. Benchmark differences between this and the previous commit: name old time/op new time/op delta NativeHistograms/histogram_count_with_short_rate_interval-16 837ms ± 3% 616ms ± 2% -26.36% (p=0.008 n=5+5) NativeHistograms/histogram_count_with_long_rate_interval-16 1.11s ± 1% 0.91s ± 3% -17.75% (p=0.008 n=5+5) NativeHistogramsCustomBuckets/histogram_count_with_short_rate_interval-16 751ms ± 6% 581ms ± 1% -22.63% (p=0.008 n=5+5) NativeHistogramsCustomBuckets/histogram_count_with_long_rate_interval-16 1.13s ±11% 0.85s ± 2% -24.59% (p=0.008 n=5+5) name old alloc/op new alloc/op delta NativeHistograms/histogram_count_with_short_rate_interval-16 531MB ± 0% 148MB ± 0% -72.08% (p=0.008 n=5+5) NativeHistograms/histogram_count_with_long_rate_interval-16 528MB ± 0% 145MB ± 0% -72.60% (p=0.016 n=5+4) NativeHistogramsCustomBuckets/histogram_count_with_short_rate_interval-16 452MB ± 0% 145MB ± 0% -67.97% (p=0.016 n=5+4) NativeHistogramsCustomBuckets/histogram_count_with_long_rate_interval-16 452MB ± 0% 141MB ± 0% -68.70% (p=0.016 n=5+4) name old allocs/op new allocs/op delta NativeHistograms/histogram_count_with_short_rate_interval-16 8.95M ± 0% 1.60M ± 0% -82.15% (p=0.008 n=5+5) NativeHistograms/histogram_count_with_long_rate_interval-16 8.84M ± 0% 1.49M ± 0% -83.16% (p=0.008 n=5+5) NativeHistogramsCustomBuckets/histogram_count_with_short_rate_interval-16 5.96M ± 0% 1.57M ± 0% -73.68% (p=0.008 n=5+5) NativeHistogramsCustomBuckets/histogram_count_with_long_rate_interval-16 5.86M ± 0% 1.46M ± 0% -75.05% (p=0.016 n=5+4) Signed-off-by: beorn7 --- promql/histogram_stats_iterator.go | 136 ++++++++---------------- promql/histogram_stats_iterator_test.go | 87 +++++---------- 2 files changed, 72 insertions(+), 151 deletions(-) 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 From 0cef66b12a0a0e2d9d84cd8e9781250a68d8290f Mon Sep 17 00:00:00 2001 From: beorn7 Date: Thu, 4 Sep 2025 13:45:24 +0200 Subject: [PATCH 3/7] promql: Forget lastFH in HistogramStatsIterator after Seek After an effective Seek, the lastFH isn't the lastFH anymore, so we should nil it out. In practice, this should only matter is sub-queries, because we are otherwise not interested in a counter reset of the first sample returned after a Seek. Sub-queries, on the other hand, always do their own counter reset detection. (For that, they would prefer to see the whole histogram, so that's another problem for another commit.) Signed-off-by: beorn7 --- promql/histogram_stats_iterator.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/promql/histogram_stats_iterator.go b/promql/histogram_stats_iterator.go index bb8c610964..408b3cfa6d 100644 --- a/promql/histogram_stats_iterator.go +++ b/promql/histogram_stats_iterator.go @@ -61,6 +61,11 @@ func (hsi *HistogramStatsIterator) Next() chunkenc.ValueType { // Seek mostly relays to the underlying iterator, but changes a ValHistogram // return into a ValFloatHistogram return. func (hsi *HistogramStatsIterator) Seek(t int64) chunkenc.ValueType { + // If the Seek is going to move the iterator, we have to forget the + // lastFH. + if t > hsi.AtT() { + hsi.lastFH = nil + } vt := hsi.Iterator.Seek(t) if vt == chunkenc.ValHistogram { return chunkenc.ValFloatHistogram From 4e94ee0109ae58b60edb8d667f9c4b56769738db Mon Sep 17 00:00:00 2001 From: beorn7 Date: Thu, 4 Sep 2025 14:16:50 +0200 Subject: [PATCH 4/7] promqltest: Add test to demonstrate sub-query bug with HistogramStatsIterator To fix this, we need to make sure that HistogramStatsIterator is not used with a sub-query in the path. Signed-off-by: beorn7 --- .../promqltest/testdata/native_histograms.test | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/promql/promqltest/testdata/native_histograms.test b/promql/promqltest/testdata/native_histograms.test index 0958b8951e..eb8cfb3432 100644 --- a/promql/promqltest/testdata/native_histograms.test +++ b/promql/promqltest/testdata/native_histograms.test @@ -1608,6 +1608,24 @@ eval instant at 1m histogram_quantile(0.5, myHistogram2) eval instant at 1m histogram_quantile(0.5, mixedHistogram) expect warn msg: PromQL warning: vector contains a mix of classic and native histograms for metric name "mixedHistogram" +clear + +# A counter reset only in a bucket. Sub-queries still need to detect +# it via explicit counter reset detection. This test also runs it with +# histogram_count in the expression to make sure that the +# HistogramStatsIterator is not used. (The latter fails to correctly +# do the counter resets because Seek is used with sub-queries. And the +# explicit counter reset detection done with sub-queries cannot access +# the buckets anymore, if HistogramStatsIterator is used.) +load 1m + h{} {{schema:0 count:1 sum:10 buckets:[1]}}+{{}}x20 {{schema:0 count:1 sum:10 buckets:[0 1]}}+{{}}x20 + +# Both evals below should yield the same value for the count. +eval instant at 41m histogram_count(increase(h[40m:9m])) + {} 1.4814814814814814 + +eval instant at 41m increase(h[40m:9m]) + {} {{count:1.4814814814814814 sum:14.814814814814813 counter_reset_hint:gauge offset:1 buckets:[1.4814814814814814]}} clear From 0746f388b047e130e6864d145bb18c47483b3ad8 Mon Sep 17 00:00:00 2001 From: beorn7 Date: Thu, 4 Sep 2025 14:36:56 +0200 Subject: [PATCH 5/7] promql: Fix HistogramStatsIterator usage for subqueries Signed-off-by: beorn7 --- promql/engine.go | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/promql/engine.go b/promql/engine.go index 2feb6c3c92..91257eae37 100644 --- a/promql/engine.go +++ b/promql/engine.go @@ -3891,20 +3891,34 @@ func detectHistogramStatsDecoding(expr parser.Expr) { return nil } + pathLoop: for i := len(path) - 1; i >= 0; i-- { // Walk backwards up the path. + if _, ok := path[i].(*parser.SubqueryExpr); ok { + // If we ever see a subquery in the path, we + // will not skip the buckets. We need the + // buckets for correct counter reset detection. + n.SkipHistogramBuckets = false + break pathLoop + } call, ok := path[i].(*parser.Call) if !ok { - continue + continue pathLoop } switch call.Func.Name { case "histogram_count", "histogram_sum", "histogram_avg": + // We allow skipping buckets preliminarily. But + // we will continue through the path to see if + // we find a subquery (or a histogram function) + // further up (the latter wouldn't make sense, + // but no harm in detecting it). n.SkipHistogramBuckets = true case "histogram_quantile", "histogram_fraction": + // If we ever see a function that needs the + // whole histogram, we will not skip the + // buckets. n.SkipHistogramBuckets = false - default: - continue + break pathLoop } - break } return errors.New("stop") }) From 48c6c1a692a1e2e9a989e872542acc074348ff5a Mon Sep 17 00:00:00 2001 From: beorn7 Date: Thu, 4 Sep 2025 19:05:14 +0200 Subject: [PATCH 6/7] promql: Make HistogramStatsIterator.AtFloatHistogram idempotent Previously, multiple calls returned a wrong counter reset hint. This commit also includes a bunch of refactorings that partially have value on their own. However, the need for them was triggered by the additional work needed for idempotency, so I included them in this commit. Signed-off-by: beorn7 --- promql/histogram_stats_iterator.go | 92 +++++++++++++++---------- promql/histogram_stats_iterator_test.go | 15 +++- 2 files changed, 69 insertions(+), 38 deletions(-) diff --git a/promql/histogram_stats_iterator.go b/promql/histogram_stats_iterator.go index 408b3cfa6d..e58cc7d848 100644 --- a/promql/histogram_stats_iterator.go +++ b/promql/histogram_stats_iterator.go @@ -29,15 +29,16 @@ import ( type HistogramStatsIterator struct { chunkenc.Iterator - currentFH *histogram.FloatHistogram - lastFH *histogram.FloatHistogram + current *histogram.FloatHistogram + last *histogram.FloatHistogram + lastIsCurrent bool } // NewHistogramStatsIterator creates a new HistogramStatsIterator. func NewHistogramStatsIterator(it chunkenc.Iterator) *HistogramStatsIterator { return &HistogramStatsIterator{ - Iterator: it, - currentFH: &histogram.FloatHistogram{}, + Iterator: it, + current: &histogram.FloatHistogram{}, } } @@ -45,12 +46,14 @@ func NewHistogramStatsIterator(it chunkenc.Iterator) *HistogramStatsIterator { // objects already allocated where possible. func (hsi *HistogramStatsIterator) Reset(it chunkenc.Iterator) { hsi.Iterator = it - hsi.lastFH = nil + hsi.last = nil + hsi.lastIsCurrent = false } // Next mostly relays to the underlying iterator, but changes a ValHistogram // return into a ValFloatHistogram return. func (hsi *HistogramStatsIterator) Next() chunkenc.ValueType { + hsi.lastIsCurrent = false vt := hsi.Iterator.Next() if vt == chunkenc.ValHistogram { return chunkenc.ValFloatHistogram @@ -62,9 +65,10 @@ func (hsi *HistogramStatsIterator) Next() chunkenc.ValueType { // return into a ValFloatHistogram return. func (hsi *HistogramStatsIterator) Seek(t int64) chunkenc.ValueType { // If the Seek is going to move the iterator, we have to forget the - // lastFH. + // lastFH and mark the currentFH as not current anymore. if t > hsi.AtT() { - hsi.lastFH = nil + hsi.last = nil + hsi.lastIsCurrent = false } vt := hsi.Iterator.Seek(t) if vt == chunkenc.ValHistogram { @@ -83,47 +87,65 @@ func (*HistogramStatsIterator) AtHistogram(*histogram.Histogram) (int64, *histog // 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) - if value.IsStaleNaN(hsi.currentFH.Sum) { - return t, &histogram.FloatHistogram{Sum: hsi.currentFH.Sum} + populateFH := func(src *histogram.FloatHistogram, detectReset bool) { + h := histogram.FloatHistogram{ + CounterResetHint: src.CounterResetHint, + Count: src.Count, + Sum: src.Sum, + } + if detectReset { + h.CounterResetHint = hsi.getResetHint(src.CounterResetHint) + } + if fh == nil { + // Note that we cannot simply write `fh = &h` here + // because that would let h escape to the heap. + fh = &histogram.FloatHistogram{} + *fh = h + } else { + h.CopyTo(fh) + } } - if fh == nil { - fh = &histogram.FloatHistogram{ - CounterResetHint: hsi.getFloatResetHint(hsi.currentFH.CounterResetHint), - Count: hsi.currentFH.Count, - Sum: hsi.currentFH.Sum, - } - hsi.setLastFH(hsi.currentFH) + if hsi.lastIsCurrent { + // Nothing changed since last AtFloatHistogram call. Return a + // copy of the stored last histogram rather than doing counter + // reset detection again (which would yield a potentially wrong + // result of "no counter reset"). + populateFH(hsi.last, false) + return hsi.AtT(), fh + } + + var t int64 + t, hsi.current = hsi.Iterator.AtFloatHistogram(hsi.current) + if value.IsStaleNaN(hsi.current.Sum) { + populateFH(hsi.current, false) return t, fh } - - returnValue := histogram.FloatHistogram{ - CounterResetHint: hsi.getFloatResetHint(hsi.currentFH.CounterResetHint), - Count: hsi.currentFH.Count, - Sum: hsi.currentFH.Sum, - } - returnValue.CopyTo(fh) - - hsi.setLastFH(hsi.currentFH) + populateFH(hsi.current, true) + hsi.setLastFromCurrent(fh.CounterResetHint) return t, fh } -func (hsi *HistogramStatsIterator) setLastFH(fh *histogram.FloatHistogram) { - if hsi.lastFH == nil { - hsi.lastFH = fh.Copy() +// setLastFromCurrent stores a copy of hsi.current as hsi.last. The +// CounterResetHint of hsi.last is set to the provided value, though. This is +// meant to store the value we have calculated on the fly so that we can return +// the same without re-calculation in case AtFloatHistogram is called multiple +// times. +func (hsi *HistogramStatsIterator) setLastFromCurrent(hint histogram.CounterResetHint) { + if hsi.last == nil { + hsi.last = hsi.current.Copy() } else { - fh.CopyTo(hsi.lastFH) + hsi.current.CopyTo(hsi.last) } + hsi.last.CounterResetHint = hint + hsi.lastIsCurrent = true } -func (hsi *HistogramStatsIterator) getFloatResetHint(hint histogram.CounterResetHint) histogram.CounterResetHint { +func (hsi *HistogramStatsIterator) getResetHint(hint histogram.CounterResetHint) histogram.CounterResetHint { if hint != histogram.UnknownCounterReset { return hint } - prevFH := hsi.lastFH - if prevFH == nil { + if hsi.last == 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 @@ -134,7 +156,7 @@ func (hsi *HistogramStatsIterator) getFloatResetHint(hint histogram.CounterReset // place. return histogram.UnknownCounterReset } - if hsi.currentFH.DetectReset(prevFH) { + if hsi.current.DetectReset(hsi.last) { return histogram.CounterReset } return histogram.NotCounterReset diff --git a/promql/histogram_stats_iterator_test.go b/promql/histogram_stats_iterator_test.go index b9d37ce3b9..80bfee519d 100644 --- a/promql/histogram_stats_iterator_test.go +++ b/promql/histogram_stats_iterator_test.go @@ -116,10 +116,16 @@ func TestHistogramStatsDecoding(t *testing.T) { t.Run(tc.name, 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 typ := statsIterator.Next(); typ != chunkenc.ValNone; typ = statsIterator.Next() { + require.Equal(t, chunkenc.ValFloatHistogram, typ) + t1, h1 := statsIterator.AtFloatHistogram(nil) + // Call AtFloatHistogram again to check for idempotency. + t2, h2 := statsIterator.AtFloatHistogram(nil) + require.Equal(t, t1, t2) + require.True(t, h1.Equals(h2)) // require.Equal does not work with sum=NaN. + decodedStats = append(decodedStats, h1) } + require.NoError(t, statsIterator.Err()) for i := 0; i < len(tc.histograms); i++ { require.Equal(t, tc.expectedHints[i], decodedStats[i].CounterResetHint) fh := tc.histograms[i].ToFloat(nil) @@ -170,6 +176,9 @@ func TestHistogramStatsMixedUse(t *testing.T) { typ = statsIterator.Next() require.Equal(t, chunkenc.ValFloatHistogram, typ) _, h = statsIterator.AtFloatHistogram(nil) + // We call AtFloatHistogram here again "randomly" to check idempotency. + _, h2 := statsIterator.AtFloatHistogram(nil) + require.True(t, h.Equals(h2)) actualHints[1] = h.CounterResetHint typ = statsIterator.Next() require.Equal(t, chunkenc.ValFloatHistogram, typ) From 121de76cbb224b53582627363b1b4ccbaa5221ba Mon Sep 17 00:00:00 2001 From: beorn7 Date: Tue, 9 Sep 2025 13:40:54 +0200 Subject: [PATCH 7/7] promqltest: Remove now needless `1*` work-around Prior to #17127, we needed to add another level in the AST to trigger the usage of `HistogramStatsIterator`. This is fixed now. Signed-off-by: beorn7 --- promql/promqltest/testdata/native_histograms.test | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/promql/promqltest/testdata/native_histograms.test b/promql/promqltest/testdata/native_histograms.test index eb8cfb3432..83933e1e7f 100644 --- a/promql/promqltest/testdata/native_histograms.test +++ b/promql/promqltest/testdata/native_histograms.test @@ -1635,31 +1635,28 @@ load 1m # Trigger an annotation about conflicting counter resets by going through the # HistogramStatsIterator, which creates counter reset hints on the fly. -eval instant at 5m 1*histogram_count(sum_over_time(reset{timing="late"}[5m])) +eval instant at 5m histogram_count(sum_over_time(reset{timing="late"}[5m])) expect warn msg: PromQL warning: conflicting counter resets during histogram addition {timing="late"} 7 -eval instant at 5m 1*histogram_count(sum(reset)) +eval instant at 5m histogram_count(sum(reset)) expect warn msg: PromQL warning: conflicting counter resets during histogram aggregation {} 5 -eval instant at 5m 1*histogram_count(avg(reset)) +eval instant at 5m histogram_count(avg(reset)) expect warn msg: PromQL warning: conflicting counter resets during histogram aggregation {} 2.5 # No annotation with the right timing. -eval instant at 30s 1*histogram_count(sum(reset)) +eval instant at 30s histogram_count(sum(reset)) expect no_warn {} 3 -eval instant at 30s 1*histogram_count(avg(reset)) +eval instant at 30s histogram_count(avg(reset)) expect no_warn {} 1.5 # Ensure that the annotation does not happen with rate. -eval instant at 5m 1*histogram_count(rate(reset{timing="late"}[5m])) +eval instant at 5m histogram_count(rate(reset{timing="late"}[5m])) expect no_warn {timing="late"} 0.0175 - -# NOTE: The `1*` part in the expressions above should not be needed. -# It can be removed once https://github.com/prometheus/prometheus/pull/17127 is merged.