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" }