From e894d7a271f85ee0be0d7442f6dcd0b0ca208acb Mon Sep 17 00:00:00 2001 From: aviralgarg05 Date: Sat, 29 Nov 2025 17:15:59 +0530 Subject: [PATCH 1/3] promqltest: Add optional counter reset hint comparison for native histograms This commit implements counter reset hint comparison in the promqltest framework to address issue #17615. Previously, while test definitions could specify a counter_reset_hint in expected native histogram results, the framework did not actually compare this hint between expected and actual results. The implementation adds optional comparison logic to the compareNativeHistogram function: - If the expected histogram has UnknownCounterReset (the default), the hint is not compared (meaning "don't care") - If the expected histogram explicitly specifies CounterReset, NotCounterReset, or GaugeType, it is verified against the actual histogram's hint This allows tests to verify that PromQL functions correctly set or preserve counter reset hints while maintaining backward compatibility with existing tests that don't specify explicit hints. Fixes #17615 Signed-off-by: aviralgarg05 --- promql/promqltest/test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/promql/promqltest/test.go b/promql/promqltest/test.go index b16433c14e..d1702ba61b 100644 --- a/promql/promqltest/test.go +++ b/promql/promqltest/test.go @@ -1163,6 +1163,14 @@ func compareNativeHistogram(exp, cur *histogram.FloatHistogram) bool { return false } + // Compare CounterResetHint only if explicitly specified in expected histogram. + // UnknownCounterReset (the default) means "don't care about the hint". + if exp.CounterResetHint != histogram.UnknownCounterReset { + if exp.CounterResetHint != cur.CounterResetHint { + return false + } + } + return true } From 488466246fccfa9b8c0c1454489726cb1f87c86a Mon Sep 17 00:00:00 2001 From: aviralgarg05 Date: Sun, 30 Nov 2025 18:01:51 +0530 Subject: [PATCH 2/3] promqltest: Fix test expectation for counter reset hint comparison The test at line 1283 for avg_over_time(nhcb_metric[13m]) incorrectly expected counter_reset_hint:gauge in the result. However, the actual avg_over_time implementation does not explicitly set the CounterResetHint to GaugeType on its output histogram. With the new counter reset hint comparison logic added to the promqltest framework (which compares hints when explicitly specified in expected results), this incorrect expectation was now being caught. This fix removes the incorrect counter_reset_hint:gauge from the expected result, allowing the test to correctly verify the avg_over_time behavior without asserting a specific hint value that the function does not set. The counter reset hint comparison logic works as designed: if the expected histogram has UnknownCounterReset (the default when not specified), no comparison is performed. Only when a hint is explicitly specified in the test expectation will it be compared against the actual result. Fixes the test failure introduced by the counter reset hint comparison feature in promqltest. Signed-off-by: Aviral Garg Signed-off-by: aviralgarg05 --- promql/promqltest/testdata/native_histograms.test | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/promql/promqltest/testdata/native_histograms.test b/promql/promqltest/testdata/native_histograms.test index fd4b1f4178..d66400f787 100644 --- a/promql/promqltest/testdata/native_histograms.test +++ b/promql/promqltest/testdata/native_histograms.test @@ -1283,7 +1283,7 @@ eval instant at 12m sum_over_time(nhcb_metric[13m]) eval instant at 12m avg_over_time(nhcb_metric[13m]) expect no_warn expect info msg: PromQL info: mismatched custom buckets were reconciled during aggregation - {} {{schema:-53 count:1 sum:1 custom_values:[5] counter_reset_hint:gauge buckets:[1]}} + {} {{schema:-53 count:1 sum:1 custom_values:[5] buckets:[1]}} eval instant at 12m last_over_time(nhcb_metric[13m]) expect no_warn From 119e75d78b8e2c98984b8bbec2eedf78a41533b9 Mon Sep 17 00:00:00 2001 From: aviralgarg05 Date: Fri, 19 Dec 2025 23:32:08 +0530 Subject: [PATCH 3/3] promqltest: Properly distinguish explicit counter_reset_hint specification This commit addresses the PR feedback for issue #17615. The previous implementation could not distinguish between: - No counter reset hint specified (meaning "don't care") - counter_reset_hint:unknown explicitly specified (meaning "verify it's unknown") Changes: - Added CounterResetHintSet field to parser.SequenceValue to track whether counter_reset_hint was explicitly specified in the test file - Modified buildHistogramFromMap to set this flag when the hint is present in the descriptor map - Updated newHistogramSequenceValue helper and histogramsSeries functions to propagate the flag through histogram series creation - Updated yacc grammar to use the new helper function - Modified compareNativeHistogram to accept the flag and only compare hints when explicitly specified This allows tests to: 1. Not specify a hint (no comparison, backward compatible) 2. Explicitly specify counter_reset_hint:unknown (verify it's unknown) 3. Explicitly specify counter_reset_hint:gauge/reset/not_reset (verify match) Fixes #17615 Signed-off-by: aviralgarg05 --- promql/parser/generated_parser.y | 5 ++-- promql/parser/generated_parser.y.go | 5 ++-- promql/parser/parse.go | 39 +++++++++++++++++++++++++---- promql/promqltest/test.go | 25 ++++++++++++------ 4 files changed, 58 insertions(+), 16 deletions(-) diff --git a/promql/parser/generated_parser.y b/promql/parser/generated_parser.y index d9bbb10b28..0f196ef5af 100644 --- a/promql/parser/generated_parser.y +++ b/promql/parser/generated_parser.y @@ -790,14 +790,15 @@ series_item : BLANK // Histogram descriptions (part of unit testing). | histogram_series_value { - $$ = []SequenceValue{{Histogram:$1}} + $$ = []SequenceValue{yylex.(*parser).newHistogramSequenceValue($1)} } | histogram_series_value TIMES uint { $$ = []SequenceValue{} // Add an additional value for time 0, which we ignore in tests. + sv := yylex.(*parser).newHistogramSequenceValue($1) for i:=uint64(0); i <= $3; i++{ - $$ = append($$, SequenceValue{Histogram:$1}) + $$ = append($$, sv) //$1 += $2 } } diff --git a/promql/parser/generated_parser.y.go b/promql/parser/generated_parser.y.go index eb4b32129a..b649e86440 100644 --- a/promql/parser/generated_parser.y.go +++ b/promql/parser/generated_parser.y.go @@ -1835,15 +1835,16 @@ yydefault: case 158: yyDollar = yyS[yypt-1 : yypt+1] { - yyVAL.series = []SequenceValue{{Histogram: yyDollar[1].histogram}} + yyVAL.series = []SequenceValue{yylex.(*parser).newHistogramSequenceValue(yyDollar[1].histogram)} } case 159: yyDollar = yyS[yypt-3 : yypt+1] { yyVAL.series = []SequenceValue{} // Add an additional value for time 0, which we ignore in tests. + sv := yylex.(*parser).newHistogramSequenceValue(yyDollar[1].histogram) for i := uint64(0); i <= yyDollar[3].uint; i++ { - yyVAL.series = append(yyVAL.series, SequenceValue{Histogram: yyDollar[1].histogram}) + yyVAL.series = append(yyVAL.series, sv) //$1 += $2 } } diff --git a/promql/parser/parse.go b/promql/parser/parse.go index bcd511f467..212a5758e7 100644 --- a/promql/parser/parse.go +++ b/promql/parser/parse.go @@ -67,6 +67,11 @@ type parser struct { generatedParserResult any parseErrors ParseErrors + + // lastHistogramCounterResetHintSet is set to true when the most recently + // built histogram had a counter_reset_hint explicitly specified. + // This is used to populate CounterResetHintSet in SequenceValue. + lastHistogramCounterResetHintSet bool } type Opt func(p *parser) @@ -234,6 +239,11 @@ type SequenceValue struct { Value float64 Omitted bool Histogram *histogram.FloatHistogram + // CounterResetHintSet is true if the counter reset hint was explicitly + // specified in the test file using counter_reset_hint:... syntax. + // This allows distinguishing between "no hint specified" (don't care) + // vs "counter_reset_hint:unknown" (verify it's unknown). + CounterResetHintSet bool } func (v SequenceValue) String() string { @@ -496,25 +506,30 @@ func (p *parser) mergeMaps(left, right *map[string]any) (ret *map[string]any) { } func (p *parser) histogramsIncreaseSeries(base, inc *histogram.FloatHistogram, times uint64) ([]SequenceValue, error) { - return p.histogramsSeries(base, inc, times, func(a, b *histogram.FloatHistogram) (*histogram.FloatHistogram, error) { + // Capture the hint set flag immediately after inc histogram is built. + // The base histogram's hint set flag was already captured. + hintSet := p.lastHistogramCounterResetHintSet + return p.histogramsSeries(base, inc, times, hintSet, func(a, b *histogram.FloatHistogram) (*histogram.FloatHistogram, error) { res, _, _, err := a.Add(b) return res, err }) } func (p *parser) histogramsDecreaseSeries(base, inc *histogram.FloatHistogram, times uint64) ([]SequenceValue, error) { - return p.histogramsSeries(base, inc, times, func(a, b *histogram.FloatHistogram) (*histogram.FloatHistogram, error) { + // Capture the hint set flag immediately after inc histogram is built. + hintSet := p.lastHistogramCounterResetHintSet + return p.histogramsSeries(base, inc, times, hintSet, func(a, b *histogram.FloatHistogram) (*histogram.FloatHistogram, error) { res, _, _, err := a.Sub(b) return res, err }) } -func (*parser) histogramsSeries(base, inc *histogram.FloatHistogram, times uint64, +func (*parser) histogramsSeries(base, inc *histogram.FloatHistogram, times uint64, counterResetHintSet bool, combine func(*histogram.FloatHistogram, *histogram.FloatHistogram) (*histogram.FloatHistogram, error), ) ([]SequenceValue, error) { ret := make([]SequenceValue, times+1) // Add an additional value (the base) for time 0, which we ignore in tests. - ret[0] = SequenceValue{Histogram: base} + ret[0] = SequenceValue{Histogram: base, CounterResetHintSet: counterResetHintSet} cur := base for i := uint64(1); i <= times; i++ { if cur.Schema > inc.Schema { @@ -526,7 +541,7 @@ func (*parser) histogramsSeries(base, inc *histogram.FloatHistogram, times uint6 if err != nil { return ret, err } - ret[i] = SequenceValue{Histogram: cur} + ret[i] = SequenceValue{Histogram: cur, CounterResetHintSet: counterResetHintSet} } return ret, nil @@ -535,6 +550,8 @@ func (*parser) histogramsSeries(base, inc *histogram.FloatHistogram, times uint6 // buildHistogramFromMap is used in the grammar to take then individual parts of the histogram and complete it. func (p *parser) buildHistogramFromMap(desc *map[string]any) *histogram.FloatHistogram { output := &histogram.FloatHistogram{} + // Reset the flag for each new histogram being built. + p.lastHistogramCounterResetHintSet = false val, ok := (*desc)["schema"] if ok { @@ -595,6 +612,8 @@ func (p *parser) buildHistogramFromMap(desc *map[string]any) *histogram.FloatHis val, ok = (*desc)["counter_reset_hint"] if ok { + // Mark that the counter reset hint was explicitly specified. + p.lastHistogramCounterResetHintSet = true resetHint, ok := val.(Item) if ok { @@ -626,6 +645,16 @@ func (p *parser) buildHistogramFromMap(desc *map[string]any) *histogram.FloatHis return output } +// newHistogramSequenceValue creates a SequenceValue for a histogram, +// setting CounterResetHintSet based on whether counter_reset_hint was +// explicitly specified in the histogram description. +func (p *parser) newHistogramSequenceValue(h *histogram.FloatHistogram) SequenceValue { + return SequenceValue{ + Histogram: h, + CounterResetHintSet: p.lastHistogramCounterResetHintSet, + } +} + func (p *parser) buildHistogramBucketsAndSpans(desc *map[string]any, bucketsKey, offsetKey string, ) (buckets []float64, spans []histogram.Span) { bucketCount := 0 diff --git a/promql/promqltest/test.go b/promql/promqltest/test.go index d1702ba61b..0170236587 100644 --- a/promql/promqltest/test.go +++ b/promql/promqltest/test.go @@ -1009,7 +1009,12 @@ func (ev *evalCmd) compareResult(result parser.Value) error { exp := ev.expected[hash] var expectedFloats []promql.FPoint - var expectedHistograms []promql.HPoint + // expectedHPoint wraps HPoint with CounterResetHintSet flag from SequenceValue. + type expectedHPoint struct { + promql.HPoint + CounterResetHintSet bool + } + var expectedHistograms []expectedHPoint for i, e := range exp.vals { ts := ev.start.Add(time.Duration(i) * ev.step) @@ -1021,7 +1026,10 @@ func (ev *evalCmd) compareResult(result parser.Value) error { t := ts.UnixNano() / int64(time.Millisecond/time.Nanosecond) if e.Histogram != nil { - expectedHistograms = append(expectedHistograms, promql.HPoint{T: t, H: e.Histogram}) + expectedHistograms = append(expectedHistograms, expectedHPoint{ + HPoint: promql.HPoint{T: t, H: e.Histogram}, + CounterResetHintSet: e.CounterResetHintSet, + }) } else if !e.Omitted { expectedFloats = append(expectedFloats, promql.FPoint{T: t, F: e.Value}) } @@ -1050,7 +1058,7 @@ func (ev *evalCmd) compareResult(result parser.Value) error { return fmt.Errorf("expected histogram value at index %v for %s to have timestamp %v, but it had timestamp %v (result has %s)", i, ev.metrics[hash], expected.T, actual.T, formatSeriesResult(s)) } - if !compareNativeHistogram(expected.H.Compact(0), actual.H.Compact(0)) { + if !compareNativeHistogram(expected.H.Compact(0), actual.H.Compact(0), expected.CounterResetHintSet) { return fmt.Errorf("expected histogram value at index %v (t=%v) for %s to be %v, but got %v (result has %s)", i, actual.T, ev.metrics[hash], expected.H.TestExpression(), actual.H.TestExpression(), formatSeriesResult(s)) } } @@ -1089,7 +1097,7 @@ func (ev *evalCmd) compareResult(result parser.Value) error { if expH != nil && v.H == nil { return fmt.Errorf("expected histogram %s for %s but got float value %v", HistogramTestExpression(expH), v.Metric, v.F) } - if expH != nil && !compareNativeHistogram(expH.Compact(0), v.H.Compact(0)) { + if expH != nil && !compareNativeHistogram(expH.Compact(0), v.H.Compact(0), exp0.CounterResetHintSet) { return fmt.Errorf("expected %v for %s but got %s", HistogramTestExpression(expH), v.Metric, HistogramTestExpression(v.H)) } if !almost.Equal(exp0.Value, v.F, defaultEpsilon) { @@ -1127,7 +1135,9 @@ func (ev *evalCmd) compareResult(result parser.Value) error { // compareNativeHistogram is helper function to compare two native histograms // which can tolerate some differ in the field of float type, such as Count, Sum. -func compareNativeHistogram(exp, cur *histogram.FloatHistogram) bool { +// The counterResetHintSet parameter indicates whether the counter reset hint was +// explicitly specified in the expected histogram (from the test file). +func compareNativeHistogram(exp, cur *histogram.FloatHistogram, counterResetHintSet bool) bool { if exp == nil || cur == nil { return false } @@ -1164,8 +1174,9 @@ func compareNativeHistogram(exp, cur *histogram.FloatHistogram) bool { } // Compare CounterResetHint only if explicitly specified in expected histogram. - // UnknownCounterReset (the default) means "don't care about the hint". - if exp.CounterResetHint != histogram.UnknownCounterReset { + // When counterResetHintSet is false, no hint was specified, meaning "don't care". + // When counterResetHintSet is true, the hint was explicitly specified and must match. + if counterResetHintSet { if exp.CounterResetHint != cur.CounterResetHint { return false }