From 665d1ba0cc62a6c3bb58274e91a1a62082298100 Mon Sep 17 00:00:00 2001 From: Joshua Hesketh Date: Thu, 23 Jan 2025 00:35:43 +1100 Subject: [PATCH] Ensure metric name is present on histogram_quantile annotations (#15828) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ensure metric name is present on histogram_quantile annotations Previously the series __name__ label was dropped before the annotation was emitted causing the annotation message to have a blank value. This fix also allows delayed name removal for classic histograms. This also adds a test for malformed le label This annotation could be split into two to be clearer for the user (one for missing, and one for malformed). This is outside of the scope of this change. Fixes: #15411 --------- Signed-off-by: Joshua Hesketh Signed-off-by: Björn Rabenstein Co-authored-by: Björn Rabenstein --- promql/engine_test.go | 75 +++++++++++++++++++++++++++++++++++++++++++ promql/functions.go | 18 +++++++---- promql/quantile.go | 1 - 3 files changed, 87 insertions(+), 7 deletions(-) diff --git a/promql/engine_test.go b/promql/engine_test.go index 20d78a7d8..697e76116 100644 --- a/promql/engine_test.go +++ b/promql/engine_test.go @@ -3482,6 +3482,81 @@ func TestRateAnnotations(t *testing.T) { } } +func TestHistogramQuantileAnnotations(t *testing.T) { + testCases := map[string]struct { + data string + expr string + expectedWarningAnnotations []string + expectedInfoAnnotations []string + }{ + "info annotation for nonmonotonic buckets": { + data: ` + nonmonotonic_bucket{le="0.1"} 0+2x10 + nonmonotonic_bucket{le="1"} 0+1x10 + nonmonotonic_bucket{le="10"} 0+5x10 + nonmonotonic_bucket{le="100"} 0+4x10 + nonmonotonic_bucket{le="1000"} 0+9x10 + nonmonotonic_bucket{le="+Inf"} 0+8x10 + `, + expr: "histogram_quantile(0.5, nonmonotonic_bucket)", + expectedWarningAnnotations: []string{}, + expectedInfoAnnotations: []string{ + `PromQL info: input to histogram_quantile needed to be fixed for monotonicity (see https://prometheus.io/docs/prometheus/latest/querying/functions/#histogram_quantile) for metric name "nonmonotonic_bucket" (1:25)`, + }, + }, + "warning annotation for missing le label": { + data: ` + myHistogram{abe="0.1"} 0+2x10 + `, + expr: "histogram_quantile(0.5, myHistogram)", + expectedWarningAnnotations: []string{ + `PromQL warning: bucket label "le" is missing or has a malformed value of "" for metric name "myHistogram" (1:25)`, + }, + expectedInfoAnnotations: []string{}, + }, + "warning annotation for malformed le label": { + data: ` + myHistogram{le="Hello World"} 0+2x10 + `, + expr: "histogram_quantile(0.5, myHistogram)", + expectedWarningAnnotations: []string{ + `PromQL warning: bucket label "le" is missing or has a malformed value of "Hello World" for metric name "myHistogram" (1:25)`, + }, + expectedInfoAnnotations: []string{}, + }, + "warning annotation for mixed histograms": { + data: ` + mixedHistogram{le="0.1"} 0+2x10 + mixedHistogram{le="1"} 0+3x10 + mixedHistogram{} {{schema:0 count:10 sum:50 buckets:[1 2 3]}} + `, + expr: "histogram_quantile(0.5, mixedHistogram)", + expectedWarningAnnotations: []string{ + `PromQL warning: vector contains a mix of classic and native histograms for metric name "mixedHistogram" (1:25)`, + }, + expectedInfoAnnotations: []string{}, + }, + } + for name, testCase := range testCases { + t.Run(name, func(t *testing.T) { + store := promqltest.LoadedStorage(t, "load 1m\n"+strings.TrimSpace(testCase.data)) + t.Cleanup(func() { _ = store.Close() }) + + engine := newTestEngine(t) + query, err := engine.NewInstantQuery(context.Background(), store, nil, testCase.expr, timestamp.Time(0).Add(1*time.Minute)) + require.NoError(t, err) + t.Cleanup(query.Close) + + res := query.Exec(context.Background()) + require.NoError(t, res.Err) + + warnings, infos := res.Warnings.AsStrings(testCase.expr, 0, 0) + testutil.RequireEqual(t, testCase.expectedWarningAnnotations, warnings) + testutil.RequireEqual(t, testCase.expectedInfoAnnotations, infos) + }) + } +} + func TestHistogramRateWithFloatStaleness(t *testing.T) { // Make a chunk with two normal histograms of the same value. h1 := histogram.Histogram{ diff --git a/promql/functions.go b/promql/functions.go index 2d809571d..605661e5a 100644 --- a/promql/functions.go +++ b/promql/functions.go @@ -1387,14 +1387,13 @@ func funcHistogramQuantile(vals []parser.Value, args parser.Expressions, enh *Ev sample.Metric = labels.NewBuilder(sample.Metric). Del(excludedLabels...). Labels() - mb = &metricWithBuckets{sample.Metric, nil} enh.signatureToMetricWithBuckets[string(enh.lblBuf)] = mb } mb.buckets = append(mb.buckets, Bucket{upperBound, sample.F}) } - // Now deal with the histograms. + // Now deal with the native histograms. for _, sample := range histogramSamples { // We have to reconstruct the exact same signature as above for // a classic histogram, just ignoring any le label. @@ -1418,16 +1417,23 @@ func funcHistogramQuantile(vals []parser.Value, args parser.Expressions, enh *Ev }) } + // Now do classic histograms that have already been filtered for conflicting native histograms. for _, mb := range enh.signatureToMetricWithBuckets { if len(mb.buckets) > 0 { res, forcedMonotonicity, _ := BucketQuantile(q, mb.buckets) - enh.Out = append(enh.Out, Sample{ - Metric: mb.metric, - F: res, - }) if forcedMonotonicity { annos.Add(annotations.NewHistogramQuantileForcedMonotonicityInfo(mb.metric.Get(labels.MetricName), args[1].PositionRange())) } + + if !enh.enableDelayedNameRemoval { + mb.metric = mb.metric.DropMetricName() + } + + enh.Out = append(enh.Out, Sample{ + Metric: mb.metric, + F: res, + DropName: true, + }) } } diff --git a/promql/quantile.go b/promql/quantile.go index a6851810c..f3af82487 100644 --- a/promql/quantile.go +++ b/promql/quantile.go @@ -47,7 +47,6 @@ const smallDeltaTolerance = 1e-12 // excludedLabels are the labels to exclude from signature calculation for // quantiles. var excludedLabels = []string{ - labels.MetricName, labels.BucketLabel, }