From 22890b1eb3eafb64a8fc37aa39e05aad1c10a676 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Sun, 25 Feb 2024 19:26:52 +0000 Subject: [PATCH 1/2] PromQL: improve warning for mixed values in aggregations Aggregations discard the metric name, so don't try to include it in the error message. Add a test that generates this warning. Signed-off-by: Bryan Boreham --- promql/engine.go | 6 ++---- promql/engine_test.go | 16 ++++++++++++++++ util/annotations/annotations.go | 13 +++++++++++-- 3 files changed, 29 insertions(+), 6 deletions(-) diff --git a/promql/engine.go b/promql/engine.go index 575ca58ea..cd955ff5e 100644 --- a/promql/engine.go +++ b/promql/engine.go @@ -2869,8 +2869,7 @@ func (ev *evaluator) aggregation(e *parser.AggregateExpr, grouping []string, par case parser.AVG: if aggr.hasFloat && aggr.hasHistogram { // We cannot aggregate histogram sample with a float64 sample. - metricName := aggr.labels.Get(labels.MetricName) - annos.Add(annotations.NewMixedFloatsHistogramsWarning(metricName, e.Expr.PositionRange())) + annos.Add(annotations.NewMixedFloatsHistogramsAggWarning(e.Expr.PositionRange())) continue } if aggr.hasHistogram { @@ -2923,8 +2922,7 @@ func (ev *evaluator) aggregation(e *parser.AggregateExpr, grouping []string, par case parser.SUM: if aggr.hasFloat && aggr.hasHistogram { // We cannot aggregate histogram sample with a float64 sample. - metricName := aggr.labels.Get(labels.MetricName) - annos.Add(annotations.NewMixedFloatsHistogramsWarning(metricName, e.Expr.PositionRange())) + annos.Add(annotations.NewMixedFloatsHistogramsAggWarning(e.Expr.PositionRange())) continue } if aggr.hasHistogram { diff --git a/promql/engine_test.go b/promql/engine_test.go index 5dc385942..105108d5b 100644 --- a/promql/engine_test.go +++ b/promql/engine_test.go @@ -4315,6 +4315,8 @@ func TestNativeHistogram_Sum_Count_Add_AvgOperator(t *testing.T) { ts := idx0 * int64(10*time.Minute/time.Millisecond) app := storage.Appender(context.Background()) + _, err := app.Append(0, labels.FromStrings("__name__", "float_series", "idx", "0"), ts, 42) + require.NoError(t, err) for idx1, h := range c.histograms { lbls := labels.FromStrings("__name__", seriesName, "idx", fmt.Sprintf("%d", idx1)) // Since we mutate h later, we need to create a copy here. @@ -4344,17 +4346,31 @@ func TestNativeHistogram_Sum_Count_Add_AvgOperator(t *testing.T) { res := qry.Exec(context.Background()) require.NoError(t, res.Err) + require.Empty(t, res.Warnings) vector, err := res.Vector() require.NoError(t, err) testutil.RequireEqual(t, exp, vector) } + queryAndCheckAnnotations := func(queryString string, ts int64, expWarnings annotations.Annotations) { + qry, err := engine.NewInstantQuery(context.Background(), storage, nil, queryString, timestamp.Time(ts)) + require.NoError(t, err) + + res := qry.Exec(context.Background()) + require.NoError(t, res.Err) + require.Equal(t, expWarnings, res.Warnings) + } // sum(). queryString := fmt.Sprintf("sum(%s)", seriesName) queryAndCheck(queryString, ts, []Sample{{T: ts, H: &c.expected, Metric: labels.EmptyLabels()}}) + queryString = `sum({idx="0"})` + var annos annotations.Annotations + annos.Add(annotations.NewMixedFloatsHistogramsAggWarning(posrange.PositionRange{Start: 4, End: 13})) + queryAndCheckAnnotations(queryString, ts, annos) + // + operator. queryString = fmt.Sprintf(`%s{idx="0"}`, seriesName) for idx := 1; idx < len(c.histograms); idx++ { diff --git a/util/annotations/annotations.go b/util/annotations/annotations.go index 29dafeb2e..c7340e4c1 100644 --- a/util/annotations/annotations.go +++ b/util/annotations/annotations.go @@ -105,7 +105,7 @@ var ( InvalidQuantileWarning = fmt.Errorf("%w: quantile value should be between 0 and 1", PromQLWarning) BadBucketLabelWarning = fmt.Errorf("%w: bucket label %q is missing or has a malformed value", PromQLWarning, model.BucketLabel) - MixedFloatsHistogramsWarning = fmt.Errorf("%w: encountered a mix of histograms and floats for metric name", PromQLWarning) + MixedFloatsHistogramsWarning = fmt.Errorf("%w: encountered a mix of histograms and floats for ", PromQLWarning) MixedClassicNativeHistogramsWarning = fmt.Errorf("%w: vector contains a mix of classic and native histograms for metric name", PromQLWarning) NativeHistogramNotCounterWarning = fmt.Errorf("%w: this native histogram metric is not a counter:", PromQLWarning) NativeHistogramNotGaugeWarning = fmt.Errorf("%w: this native histogram metric is not a gauge:", PromQLWarning) @@ -155,7 +155,16 @@ func NewBadBucketLabelWarning(metricName, label string, pos posrange.PositionRan func NewMixedFloatsHistogramsWarning(metricName string, pos posrange.PositionRange) error { return annoErr{ PositionRange: pos, - Err: fmt.Errorf("%w %q", MixedFloatsHistogramsWarning, metricName), + Err: fmt.Errorf("%w metric name %q", MixedFloatsHistogramsWarning, metricName), + } +} + +// NewMixedFloatsHistogramsAggWarning is used when the queried series includes both +// float samples and histogram samples in an aggregation. +func NewMixedFloatsHistogramsAggWarning(pos posrange.PositionRange) error { + return annoErr{ + PositionRange: pos, + Err: fmt.Errorf("%w aggregation", MixedFloatsHistogramsWarning), } } From 195a0f5ab8d34d68631562cf8086bc2d692344d1 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Mon, 26 Feb 2024 14:02:06 +0000 Subject: [PATCH 2/2] remove uneccessary space Co-authored-by: Julien Signed-off-by: Bryan Boreham --- util/annotations/annotations.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util/annotations/annotations.go b/util/annotations/annotations.go index c7340e4c1..16a920d57 100644 --- a/util/annotations/annotations.go +++ b/util/annotations/annotations.go @@ -105,7 +105,7 @@ var ( InvalidQuantileWarning = fmt.Errorf("%w: quantile value should be between 0 and 1", PromQLWarning) BadBucketLabelWarning = fmt.Errorf("%w: bucket label %q is missing or has a malformed value", PromQLWarning, model.BucketLabel) - MixedFloatsHistogramsWarning = fmt.Errorf("%w: encountered a mix of histograms and floats for ", PromQLWarning) + MixedFloatsHistogramsWarning = fmt.Errorf("%w: encountered a mix of histograms and floats for", PromQLWarning) MixedClassicNativeHistogramsWarning = fmt.Errorf("%w: vector contains a mix of classic and native histograms for metric name", PromQLWarning) NativeHistogramNotCounterWarning = fmt.Errorf("%w: this native histogram metric is not a counter:", PromQLWarning) NativeHistogramNotGaugeWarning = fmt.Errorf("%w: this native histogram metric is not a gauge:", PromQLWarning)