include metric name in annotations from histogram_quantile if delayed… #13905
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| func (q *histogramQuantile) ComputeNativeHistogramResult(pointIndex int, seriesIndex int, h *histogram.FloatHistogram) (float64, annotations.Annotations) { | ||
| ph := q.phValues.Samples[pointIndex].F | ||
| return promql.HistogramQuantile(ph, h, q.innerSeriesMetricNames.GetMetricNameForSeries(seriesIndex), q.innerExpressionPosition) | ||
| return promql.HistogramQuantile(ph, h, q.getMetricNameForSeries(seriesIndex), q.innerExpressionPosition) |
There was a problem hiding this comment.
histogram_fraction ignores enableDelayedNameRemoval flag unlike histogram_quantile
The histogramFraction struct was not updated to include an enableDelayedNameRemoval field and corresponding getMetricNameForSeries method like histogramQuantile was. In ComputeNativeHistogramResult, histogram_fraction directly calls f.innerSeriesMetricNames.GetMetricNameForSeries(seriesIndex) unconditionally, while histogram_quantile now uses q.getMetricNameForSeries(seriesIndex) which respects the delayed name removal setting. This creates inconsistent behavior where histogram_fraction always includes metric names in annotations regardless of the enableDelayedNameRemoval flag, contradicting the PR's stated goal that metric names are "only included...if EnableDelayedNameRemoval is enabled."
Additional Locations (1)
There was a problem hiding this comment.
This PR relates only to histogram_quantile
There was a problem hiding this comment.
Why have these test cases been disabled? Weren't they previously passing?
|
|
||
| t.Run("Mimir's engine", func(t *testing.T) { | ||
| if strings.Contains(testFile, "name_label_dropping") { | ||
| if strings.Contains(testFile, "name_label_dropping") || strings.Contains(testFile, "delayed_name_removal_enabled") { |
There was a problem hiding this comment.
Could we bring this condition out of the two t.Run calls (here and on line 264 below), so that we ensure that we consistently use / don't use engines with delayed name removal?
| data string | ||
| expr string | ||
| expectedWarningAnnotations []string | ||
| // an alternate string for when delayed name removal is enabled. |
There was a problem hiding this comment.
[nit] This allows more than just one string.
| // an alternate string for when delayed name removal is enabled. | |
| // an alternate set of annotations for when delayed name removal is enabled. |
| // if not set the test will fall back to expectedWarningAnnotations | ||
| expectedWarningAnnotationsDelayedNameRemovalEnabled []string | ||
| expectedInfoAnnotations []string | ||
| // an alternate string for when delayed name removal is enabled. |
There was a problem hiding this comment.
[nit] This allows more than just one string.
| // an alternate string for when delayed name removal is enabled. | |
| // an alternate set of annotations for when delayed name removal is enabled. |
| for _, delayedNameRemovalEnabled := range []bool{true, false} { | ||
| results := make([]*promql.Result, 0, 2) |
There was a problem hiding this comment.
We should call t.Run here with a name that makes it clear if delayed name removal was enabled or disabled. (Otherwise we'll get two nested tests for each engine, both with the same name, eg. "Mimir's engine".)
| for _, delayedNameRemovalEnabled := range []bool{true, false} { | ||
| results := make([]*promql.Result, 0, 2) | ||
|
|
||
| for engineName, engine := range engines[delayedNameRemovalEnabled] { |
There was a problem hiding this comment.
| for _, delayedNameRemovalEnabled := range []bool{true, false} { | |
| results := make([]*promql.Result, 0, 2) | |
| for engineName, engine := range engines[delayedNameRemovalEnabled] { | |
| for engines, delayedNameRemovalEnabled := range engines { | |
| results := make([]*promql.Result, 0, 2) | |
| for engineName, engine := range engines { |
|
|
||
| // create 2 sets of engines - one with EnableDelayedNameRemoval=true and the other with EnableDelayedNameRemoval=false | ||
| // there are some histogram annotation test cases which will emit a different warning/info annotation string depending on the delayed name removal setting | ||
| engines := make(map[bool]map[string]promql.QueryEngine, 2) |
There was a problem hiding this comment.
It might be even simpler (and help reduce some nesting below) to define engines something like this:
| engines := make(map[bool]map[string]promql.QueryEngine, 2) | |
| engines := []struct{ | |
| engine promql.QueryEngine | |
| name string // eg. "Mimir's engine without delayed name removal" | |
| delayedNameRemovalEnabled bool | |
| }{} |
| // The name is dropped even if the function is the first one invoked on the vector selector. | ||
| // Mimir doesn't have delayed __name__ removal, so to stay close to prometheus, we never display the label in some annotations and warnings. | ||
| // This is only used for backwards compatibility when delayed __name__ removal is not enabled. | ||
| intentionallyEmptyMetricName = "" |
There was a problem hiding this comment.
Given this is only referenced in one place (getMetricNameForSeries) now, should we move this constant and the comment into there?
We could probably also drop the constant and just move the comment to explain why we return an empty value.
4 participants
What this PR does
Now that MQE supports delayed name removal, it is possible to include the metric name in
histogram_quantile()warning and info annotations.This brings parity to the Prometheus implementation.
The metric names are only included in the relevant warning/info annotations if
EnableDelayedNameRemovalis enabled.Which issue(s) this PR fixes or relates to
Fixes #3299
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]. If changelog entry is not needed, please add thechangelog-not-neededlabel to the PR.about-versioning.mdupdated with experimental features.Note
Adds metric name to
histogram_quantilewarning/info annotations when delayed__name__removal is enabled, aligning with Prometheus.getMetricNameForSeriesand plumbenableDelayedNameRemovalthrough histogram operatorsexpected*DelayedNameRemovalEnabledfields and new testdata for both modes; update engine tests to select appropriate engines; skip optimization tests fordelayed_name_removal_enabledhistogram_quantileannotations when delayed name removal is enabledWritten by Cursor Bugbot for commit b0a10e2. This will update automatically on new commits. Configure here.