Merge pull request #14611 from charleskorn/sum-and-avg-over-mixed-custom-exponential-histograms

promql: fix incorrect results and panics in `sum` and `avg` over mixed custom and exponential buckets, or incompatible custom buckets

promql/engine.go

@@ -2786,11 +2786,12 @@ type groupedAggregation struct {
 	heap           vectorByValueHeap
 
 	// All bools together for better packing within the struct.
-	seen              bool // Was this output groups seen in the input at this timestamp.
+	seen                   bool // Was this output groups seen in the input at this timestamp.
-	hasFloat          bool // Has at least 1 float64 sample aggregated.
+	hasFloat               bool // Has at least 1 float64 sample aggregated.
-	hasHistogram      bool // Has at least 1 histogram sample aggregated.
+	hasHistogram           bool // Has at least 1 histogram sample aggregated.
-	groupAggrComplete bool // Used by LIMITK to short-cut series loop when we've reached K elem on every group.
+	incompatibleHistograms bool // If true, group has seen mixed exponential and custom buckets, or incompatible custom buckets.
-	incrementalMean   bool // True after reverting to incremental calculation of the mean value.
+	groupAggrComplete      bool // Used by LIMITK to short-cut series loop when we've reached K elem on every group.
+	incrementalMean        bool // True after reverting to incremental calculation of the mean value.
 }
 
 // aggregation evaluates sum, avg, count, stdvar, stddev or quantile at one timestep on inputMatrix.
@@ -2814,10 +2815,11 @@ func (ev *evaluator) aggregation(e *parser.AggregateExpr, q float64, inputMatrix
 		// Initialize this group if it's the first time we've seen it.
 		if !group.seen {
 			*group = groupedAggregation{
-				seen:       true,
+				seen:                   true,
-				floatValue: f,
+				floatValue:             f,
-				floatMean:  f,
+				floatMean:              f,
-				groupCount: 1,
+				incompatibleHistograms: false,
+				groupCount:             1,
 			}
 			switch op {
 			case parser.AVG, parser.SUM:
@@ -2838,6 +2840,10 @@ func (ev *evaluator) aggregation(e *parser.AggregateExpr, q float64, inputMatrix
 			continue
 		}
 
+		if group.incompatibleHistograms {
+			continue
+		}
+
 		switch op {
 		case parser.SUM:
 			if h != nil {
@@ -2846,6 +2852,7 @@ func (ev *evaluator) aggregation(e *parser.AggregateExpr, q float64, inputMatrix
 					_, err := group.histogramValue.Add(h)
 					if err != nil {
 						handleAggregationError(err, e, inputMatrix[si].Metric.Get(model.MetricNameLabel), &annos)
+						group.incompatibleHistograms = true
 					}
 				}
 				// Otherwise the aggregation contained floats
@@ -2866,10 +2873,14 @@ func (ev *evaluator) aggregation(e *parser.AggregateExpr, q float64, inputMatrix
 					toAdd, 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 err != nil {
 						handleAggregationError(err, e, inputMatrix[si].Metric.Get(model.MetricNameLabel), &annos)
+						group.incompatibleHistograms = true
+						continue
 					}
 				}
 				// Otherwise the aggregation contained floats
@@ -2966,6 +2977,8 @@ func (ev *evaluator) aggregation(e *parser.AggregateExpr, q float64, inputMatrix
 				continue
 			}
 			switch {
+			case aggr.incompatibleHistograms:
+				continue
 			case aggr.hasHistogram:
 				aggr.histogramValue = aggr.histogramValue.Compact(0)
 			case aggr.incrementalMean:
@@ -2992,9 +3005,12 @@ func (ev *evaluator) aggregation(e *parser.AggregateExpr, q float64, inputMatrix
 				annos.Add(annotations.NewMixedFloatsHistogramsAggWarning(e.Expr.PositionRange()))
 				continue
 			}
-			if aggr.hasHistogram {
+			switch {
+			case aggr.incompatibleHistograms:
+				continue
+			case aggr.hasHistogram:
 				aggr.histogramValue.Compact(0)
-			} else {
+			default:
 				aggr.floatValue += aggr.floatKahanC
 			}
 		default:

promql/promqltest/testdata/native_histograms.test

@@ -785,6 +785,8 @@ eval_warn instant at 1m rate(some_metric[30s])
 eval_warn instant at 30s rate(some_metric[30s])
     # Should produce no results.
 
+clear
+
 # Histogram with constant buckets.
 load 1m
     const_histogram {{schema:0 sum:1 count:1 buckets:[1 1 1]}} {{schema:0 sum:1 count:1 buckets:[1 1 1]}} {{schema:0 sum:1 count:1 buckets:[1 1 1]}} {{schema:0 sum:1 count:1 buckets:[1 1 1]}} {{schema:0 sum:1 count:1 buckets:[1 1 1]}}
@@ -828,3 +830,59 @@ eval instant at 5m histogram_stddev(rate(const_histogram[5m]))
 # Zero buckets mean no observations, so there is no standard variance.
 eval instant at 5m histogram_stdvar(rate(const_histogram[5m]))
     {} NaN
+
+clear
+
+# Test mixing exponential and custom buckets.
+load 6m
+  metric{series="exponential"}         {{sum:4 count:3 buckets:[1 2 1]}}  _                                                                 {{sum:4 count:3 buckets:[1 2 1]}}
+  metric{series="other-exponential"}   {{sum:3 count:2 buckets:[1 1 1]}}  _                                                                 {{sum:3 count:2 buckets:[1 1 1]}}
+  metric{series="custom"}              _                                  {{schema:-53 sum:1 count:1 custom_values:[5 10] buckets:[1]}}     {{schema:-53 sum:1 count:1 custom_values:[5 10] buckets:[1]}}
+  metric{series="other-custom"}        _                                  {{schema:-53 sum:15 count:2 custom_values:[5 10] buckets:[0 2]}}  {{schema:-53 sum:15 count:2 custom_values:[5 10] buckets:[0 2]}}
+
+# T=0: only exponential
+# T=6: only custom
+# T=12: mixed, should be ignored and emit a warning
+eval_warn range from 0 to 12m step 6m sum(metric)
+  {} {{sum:7 count:5 buckets:[2 3 2]}} {{schema:-53 sum:16 count:3 custom_values:[5 10] buckets:[1 2]}} _
+
+eval_warn range from 0 to 12m step 6m avg(metric)
+  {} {{sum:3.5 count:2.5 buckets:[1 1.5 1]}} {{schema:-53 sum:8 count:1.5 custom_values:[5 10] buckets:[0.5 1]}} _
+
+clear
+
+# Test incompatible custom bucket schemas.
+load 6m
+  metric{series="1"} _                                                             {{schema:-53 sum:1 count:1 custom_values:[5 10] buckets:[1]}} {{schema:-53 sum:1 count:1 custom_values:[5 10] buckets:[1]}}
+  metric{series="2"} {{schema:-53 sum:1 count:1 custom_values:[2] buckets:[1]}}    _                                                             {{schema:-53 sum:1 count:1 custom_values:[2] buckets:[1]}}
+  metric{series="3"} {{schema:-53 sum:1 count:1 custom_values:[5 10] buckets:[1]}} {{schema:-53 sum:1 count:1 custom_values:[5 10] buckets:[1]}} {{schema:-53 sum:1 count:1 custom_values:[5 10] buckets:[1]}}
+
+# T=0: incompatible, should be ignored and emit a warning
+# T=6: compatible
+# T=12: incompatible followed by compatible, should be ignored and emit a warning
+eval_warn range from 0 to 12m step 6m sum(metric)
+  {} _ {{schema:-53 sum:2 count:2 custom_values:[5 10] buckets:[2]}} _
+
+eval_warn range from 0 to 12m step 6m avg(metric)
+  {} _ {{schema:-53 sum:1 count:1 custom_values:[5 10] buckets:[1]}} _
+
+clear
+
+load 1m
+  metric{group="just-floats", series="1"} 2
+  metric{group="just-floats", series="2"} 3
+  metric{group="just-exponential-histograms", series="1"} {{sum:3 count:4 buckets:[1 2 1]}}
+  metric{group="just-exponential-histograms", series="2"} {{sum:2 count:3 buckets:[1 1 1]}}
+  metric{group="just-custom-histograms", series="1"} {{schema:-53 sum:1 count:1 custom_values:[2] buckets:[1]}}
+  metric{group="just-custom-histograms", series="2"} {{schema:-53 sum:3 count:4 custom_values:[2] buckets:[7]}}
+  metric{group="floats-and-histograms", series="1"} 2
+  metric{group="floats-and-histograms", series="2"} {{sum:2 count:3 buckets:[1 1 1]}}
+  metric{group="exponential-and-custom-histograms", series="1"} {{sum:2 count:3 buckets:[1 1 1]}}
+  metric{group="exponential-and-custom-histograms", series="2"} {{schema:-53 sum:1 count:1 custom_values:[5 10] buckets:[1]}}
+  metric{group="incompatible-custom-histograms", series="1"} {{schema:-53 sum:1 count:1 custom_values:[5 10] buckets:[1]}}
+  metric{group="incompatible-custom-histograms", series="2"} {{schema:-53 sum:1 count:1 custom_values:[2] buckets:[1]}}
+
+eval_warn instant at 0 sum by (group) (metric)
+  {group="just-floats"} 5
+  {group="just-exponential-histograms"} {{sum:5 count:7 buckets:[2 3 2]}}
+  {group="just-custom-histograms"} {{schema:-53 sum:4 count:5 custom_values:[2] buckets:[8]}}