From 262832029210788c3714d1ff4eb9cdeeb449fdce Mon Sep 17 00:00:00 2001 From: beorn7 Date: Wed, 27 Aug 2025 18:39:33 +0200 Subject: [PATCH] promql: Fix when to emit a `HistogramCounterResetCollisionWarning` So far, we emitted a `HistogramCounterResetCollisionWarning` when encountering conflicting counter resets in the calculation of (i)rate and friends. We even tested for that. However, in the rate calculation, we are not interested in those collisions. They are actually expected. On the other hand, we did not warn about those collisions when doing a `sum` aggregation, where such a warning would be appropriate. This commit removes the warning in the former case and adds it in the latter. Sadly, we cannot really test this as we still remove the counter reset hint for the first sample in a chunk. (And that's the only sample where we could get a `NotCounterReset` hint.) Signed-off-by: beorn7 --- promql/engine.go | 15 +++++++-- promql/functions.go | 24 +++++++------- promql/functions_internal_test.go | 52 ------------------------------- util/annotations/annotations.go | 3 +- 4 files changed, 25 insertions(+), 69 deletions(-) diff --git a/promql/engine.go b/promql/engine.go index 92bedc9ac3..414fecc5b8 100644 --- a/promql/engine.go +++ b/promql/engine.go @@ -3081,11 +3081,14 @@ func (ev *evaluator) aggregation(e *parser.AggregateExpr, q float64, inputMatrix if h != nil { group.hasHistogram = true if group.histogramValue != nil { - _, _, err := group.histogramValue.Add(h) + _, counterResetCollision, err := group.histogramValue.Add(h) if err != nil { handleAggregationError(err, e, inputMatrix[si].Metric.Get(model.MetricNameLabel), &annos) group.incompatibleHistograms = true } + if counterResetCollision { + annos.Add(annotations.NewHistogramCounterResetCollisionWarning(e.Expr.PositionRange(), annotations.HistogramAgg)) + } } // Otherwise the aggregation contained floats // previously and will be invalid anyway. No @@ -3134,18 +3137,24 @@ func (ev *evaluator) aggregation(e *parser.AggregateExpr, q float64, inputMatrix if group.histogramValue != nil { left := h.Copy().Div(group.groupCount) right := group.histogramValue.Copy().Div(group.groupCount) - toAdd, _, err := left.Sub(right) + toAdd, counterResetCollision, err := left.Sub(right) if err != nil { handleAggregationError(err, e, inputMatrix[si].Metric.Get(model.MetricNameLabel), &annos) group.incompatibleHistograms = true continue } - _, _, err = group.histogramValue.Add(toAdd) + if counterResetCollision { + annos.Add(annotations.NewHistogramCounterResetCollisionWarning(e.Expr.PositionRange(), annotations.HistogramAgg)) + } + _, counterResetCollision, err = group.histogramValue.Add(toAdd) if err != nil { handleAggregationError(err, e, inputMatrix[si].Metric.Get(model.MetricNameLabel), &annos) group.incompatibleHistograms = true continue } + if counterResetCollision { + annos.Add(annotations.NewHistogramCounterResetCollisionWarning(e.Expr.PositionRange(), annotations.HistogramAgg)) + } } // Otherwise the aggregation contained floats // previously and will be invalid anyway. No diff --git a/promql/functions.go b/promql/functions.go index 01b37603e4..675403f43a 100644 --- a/promql/functions.go +++ b/promql/functions.go @@ -251,7 +251,10 @@ func histogramRate(points []HPoint, isCounter bool, metricName string, pos posra } h := last.CopyToSchema(minSchema) - _, counterResetCollision, err := h.Sub(prev) + // This subtraction may deliberately include conflicting counter resets. + // Counter resets are treated explicitly in this function, so the + // information about conflicting counter resets is ignored here. + _, _, err := h.Sub(prev) if err != nil { if errors.Is(err, histogram.ErrHistogramsIncompatibleSchema) { return nil, annotations.New().Add(annotations.NewMixedExponentialCustomHistogramsWarning(metricName, pos)) @@ -260,16 +263,13 @@ func histogramRate(points []HPoint, isCounter bool, metricName string, pos posra } } - if counterResetCollision { - annos.Add(annotations.NewHistogramCounterResetCollisionWarning(pos, annotations.HistogramSub)) - } - if isCounter { // Second iteration to deal with counter resets. for _, currPoint := range points[1:] { curr := currPoint.H if curr.DetectReset(prev) { - _, counterResetCollision, err := h.Add(prev) + // Counter reset conflict ignored here for the same reason as above. + _, _, err := h.Add(prev) if err != nil { if errors.Is(err, histogram.ErrHistogramsIncompatibleSchema) { return nil, annotations.New().Add(annotations.NewMixedExponentialCustomHistogramsWarning(metricName, pos)) @@ -277,9 +277,6 @@ func histogramRate(points []HPoint, isCounter bool, metricName string, pos posra return nil, annotations.New().Add(annotations.NewIncompatibleCustomBucketsHistogramsWarning(metricName, pos)) } } - if counterResetCollision { - annos.Add(annotations.NewHistogramCounterResetCollisionWarning(pos, annotations.HistogramAdd)) - } } prev = curr } @@ -393,15 +390,16 @@ func instantValue(vals Matrix, args parser.Expressions, out Vector, isRate bool) annos.Add(annotations.NewNativeHistogramNotGaugeWarning(metricName, args.PositionRange())) } if !isRate || !ss[1].H.DetectReset(ss[0].H) { - _, counterResetCollision, err := resultSample.H.Sub(ss[0].H) + // This subtraction may deliberately include conflicting + // counter resets. Counter resets are treated explicitly + // in this function, so the information about + // conflicting counter resets is ignored here. + _, _, err := resultSample.H.Sub(ss[0].H) if errors.Is(err, histogram.ErrHistogramsIncompatibleSchema) { return out, annos.Add(annotations.NewMixedExponentialCustomHistogramsWarning(metricName, args.PositionRange())) } else if errors.Is(err, histogram.ErrHistogramsIncompatibleBounds) { return out, annos.Add(annotations.NewIncompatibleCustomBucketsHistogramsWarning(metricName, args.PositionRange())) } - if counterResetCollision { - annos.Add(annotations.NewHistogramCounterResetCollisionWarning(args.PositionRange(), annotations.HistogramSub)) - } } resultSample.H.CounterResetHint = histogram.GaugeType resultSample.H.Compact(0) diff --git a/promql/functions_internal_test.go b/promql/functions_internal_test.go index fc7e604dc0..85c6cd7973 100644 --- a/promql/functions_internal_test.go +++ b/promql/functions_internal_test.go @@ -19,10 +19,6 @@ import ( "testing" "github.com/stretchr/testify/require" - - "github.com/prometheus/prometheus/model/histogram" - "github.com/prometheus/prometheus/promql/parser/posrange" - "github.com/prometheus/prometheus/util/annotations" ) func TestKahanSumInc(t *testing.T) { @@ -83,51 +79,3 @@ func TestKahanSumInc(t *testing.T) { }) } } - -func newAnnotations(errs ...error) annotations.Annotations { - var annos annotations.Annotations - for _, err := range errs { - annos.Add(err) - } - return annos -} - -// TODO(juliusmh): this test ensures histogramRate sets correct annotations. -// This test can be removed in favor of a user facing promqltest when -// https://github.com/prometheus/prometheus/issues/15346 is resolved. -func TestHistogramRate_Annotations(t *testing.T) { - const metricName = "test" - pos := posrange.PositionRange{} - for _, tc := range []struct { - name string - points []HPoint - wantAnnotations annotations.Annotations - }{ - { - name: "empty histograms", - points: []HPoint{ - {H: &histogram.FloatHistogram{}}, - {H: &histogram.FloatHistogram{}}, - }, - wantAnnotations: newAnnotations( - annotations.NewNativeHistogramNotGaugeWarning(metricName, pos), - ), - }, - { - name: "counter reset hint collision", - points: []HPoint{ - {H: &histogram.FloatHistogram{CounterResetHint: histogram.NotCounterReset}}, - {H: &histogram.FloatHistogram{CounterResetHint: histogram.CounterReset}}, - }, - wantAnnotations: newAnnotations( - annotations.NewNativeHistogramNotGaugeWarning(metricName, pos), - annotations.NewHistogramCounterResetCollisionWarning(pos, annotations.HistogramSub), - ), - }, - } { - t.Run(tc.name, func(t *testing.T) { - _, annos := histogramRate(tc.points, false, metricName, pos) - require.Equal(t, tc.wantAnnotations, annos) - }) - } -} diff --git a/util/annotations/annotations.go b/util/annotations/annotations.go index 37f983dde7..bbd3b0927f 100644 --- a/util/annotations/annotations.go +++ b/util/annotations/annotations.go @@ -362,13 +362,14 @@ type HistogramOperation string const ( HistogramAdd HistogramOperation = "addition" HistogramSub HistogramOperation = "subtraction" + HistogramAgg HistogramOperation = "aggregation" ) // NewHistogramCounterResetCollisionWarning is used when two counter histograms are added or subtracted where one has // a CounterReset hint and the other has NotCounterReset. func NewHistogramCounterResetCollisionWarning(pos posrange.PositionRange, operation HistogramOperation) error { switch operation { - case HistogramAdd, HistogramSub: + case HistogramAdd, HistogramSub, HistogramAgg: default: operation = "unknown operation" }