PromQL engine: Delay deletion of __name__ label to the end of the query evaluation by jcreixell · Pull Request #14477 · prometheus/prometheus

jcreixell · 2024-07-15T19:26:13Z

This change allows optionally preserving the __name__ label via the label_replace and label_join functions, and helps prevent the dreaded "vector cannot contain metrics with the same labelset" error.
The implementation extends the Series and Sample structs with a boolean flag indicating whether the __name__ label should be deleted at the end of the query evaluation.
The label_replace and label_join functions can still access the value of the __name__ label, even if it has been previously marked for deletion. If __name__ is used as target label, it won't be dropped at the end of the query evaluation.
Fixes Stop removing __name__ from results: prevent vector cannot contain metrics with the same labelset #11397
See POC2: Delay deletion of __name__ label (bool flags) jcreixell/prometheus#2 for previous discussion, including the decision to create this PR and benchmark it before considering other alternatives (like refactoring labels.Labels).
See POC1: Delay deletion of __name__ label (__deleted__name__ label) jcreixell/prometheus#1 for an alternative implementation using a special label instead of boolean flags.
Note: a feature flag promql-delayed-name-removal has been added as it changes the behavior of some "weird" queries (see Stop removing __name__ from results: prevent vector cannot contain metrics with the same labelset #11397 (comment))

Example (this always fails, as __name__ is being dropped by count_over_time):

count_over_time({__name__!=""}[1m])

=> Error executing query: vector cannot contain metrics with the same labelset

Before:

label_replace(count_over_time({__name__!=""}[1m]), "__name__", "count_$1", "__name__", "(.+)")

=> Error executing query: vector cannot contain metrics with the same labelset

After:

label_replace(count_over_time({__name__!=""}[1m]), "__name__", "count_$1", "__name__", "(.+)")

=> 
count_go_gc_cycles_automatic_gc_cycles_total{instance="localhost:9090", job="prometheus"} 1
count_go_gc_cycles_forced_gc_cycles_total{instance="localhost:9090", job="prometheus"} 1
...

bboreham · 2024-07-16T08:09:56Z

/prombench main

prombot · 2024-07-16T08:09:59Z

⏱️ Welcome to Prometheus Benchmarking Tool. ⏱️

Compared versions: PR-14477 and main

After successful deployment, the benchmarking results can be viewed at:

Other Commands:
To stop benchmark: /prombench cancel
To restart benchmark: /prombench restart main

bboreham · 2024-07-18T13:29:12Z

/prombench cancel

prombot · 2024-07-18T13:29:15Z

Benchmark cancel is in progress.

jcreixell · 2024-07-18T18:57:31Z

@bboreham I have pushed a change that removes what I believe are unnecessary duplicate-detection function calls. This is now done in one pass only at the end of the query evaluation. The rationale is the following:

Duplicate detection was initially introduced in Add duplicate-labelset check for range/instant vectors #4589 to prevent issues with duplicated metrics in certain scenarios like renaming an exporter metric and using a regexp to match both the old and new name, in combination with name-dropping operations.
Since the __name__ label is used to calculate the hash that is used to detect duplicates, and it's now not being dropped until the very end, it will never detect duplicates until .DropMetricName() is executed, so the other places where duplicate detection is called are mostly a no-op.

I would expect this to help with CPU usage. I have run some benchmarks and I don't see significant differences compared to main. Would it make sense to run another round of prombench to see how it behaves?

In the meantime I will try to reproduce the original issue to make sure we are not introducing a regression here.

also cc @beorn7 in case I am missing something important here

jcreixell · 2024-07-19T09:02:04Z

I have tried to reproduce the issue in #4562 by renaming a metric and running similar queries. I picked an internal prometheus metric (go_memstats_frees_total), collected some data, then added the following rule:

    metric_relabel_configs:
    - source_labels: [__name__]
      regex: '(.*go_memstats_frees_total.*)'
      replacement: '${1}_new'
      target_label: __name__

The rate still gives an error as expected:

The aggregation works and does not show gaps during the transition:

jcreixell · 2024-07-22T16:08:09Z

The feature is now behind a feature flag and this PR is ready for review 🙇

beorn7 · 2024-07-25T16:18:25Z

My (unplugged) vacations start ~now. I didn't get to this in time, sorry for that. I guess we should run prombench on this with the latest improvements (but of course, for that you have to hardcode setting the feature flag first so that prombench actually uses the new code). Happy to do the detailed review once I'm back, but if @bboreham or anyone else beats me to it, even better.

beorn7 · 2024-08-08T12:22:21Z

I'm finally back from vacations and the subsequent catch-up…

Note that we had some interesting discussion on the original issue starting here. The concerns are valid but we decided to go forward with the approach in this PR as we want to find out the practical implications and maybe get some inspiration how to deal with them. That's after all the reason why we are doing this as an experimental feature.

beorn7 · 2024-08-08T12:25:55Z

@jcreixell could you (temporarily) hard-wire the feature flag to "set" in this PR so that I can trigger another Prombench run?
At that same time, it might also be good to rebase on top of main so that we have a valid comparison (and the merge conflicts would also be gone).

beorn7 · 2024-08-08T12:26:09Z

Note for later: https://github.com/prometheus/prometheus/blob/main/docs/feature_flags.md needs an update, too.

jcreixell · 2024-08-12T13:06:10Z

@beorn7 I have now rebased and flipped the flag, feel free to trigger prombench.

I have also added some tentative content to the feature flags doc.

beorn7 · 2024-08-13T12:22:24Z

/prombench main

prombot · 2024-08-13T12:22:26Z

⏱️ Welcome to Prometheus Benchmarking Tool. ⏱️

Compared versions: PR-14477 and main

After successful deployment, the benchmarking results can be viewed at:

Other Commands:
To stop benchmark: /prombench cancel
To restart benchmark: /prombench restart main

beorn7 · 2024-08-14T13:39:10Z

/prombench cancel

prombot · 2024-08-14T13:39:13Z

Benchmark cancel is in progress.

beorn7 · 2024-08-14T13:42:23Z

Prombench looks good. If at all, I can only see a slightly lower CPU usage with this PR, no other noticeable differences.

Of course, Pormbench isn't perfectly measuring everything, but I think we can greenlight this from a performance perspective. (It's behind a feature flag, so if we see regressions in the wild, we can just rollback.)

@jcreixell could you re-enable the normal feature flag behavior?

@bboreham do you have any other concerns?

I will put this on my list for a detailed code-level review, provided there are no other objections against doing this at all.

jcreixell · 2024-08-14T15:58:18Z

@beorn7 ready on my side

beorn7 · 2024-08-15T14:09:48Z

Will review ASAP (more likely early next week than this week…).

beorn7

Looks very good overall. My comments are just about comments… :o)

Very general thought about naming: Why "ShouldDropName" and not just "DropName"? The "Should" sounds like there are code paths where it is not dropped or something. That feels misleading to me. Plus, the name seems unnecessarily long.

cmd/prometheus/main.go

docs/feature_flags.md

promql/engine.go

promql/value.go

…ry evaluation - This change allows optionally preserving the `__name__` label via the `label_replace` and `label_join` functions, and helps prevent the dreaded "vector cannot contain metrics with the same labelset" error. - The implementation extends the `Series` and `Sample` structs with a boolean flag indicating whether the `__name__` label should be deleted at the end of the query evaluation. - The `label_replace` and `label_join` functions can still access the value of the `__name__` label, even if it has been previously marked for deletion. If `__name__` is used as target label, it won't be dropped at the end of the query evaluation. - Fixes prometheus#11397 - See #2 for previous discussion, including the decision to create this PR and benchmark it before considering other alternatives (like refactoring `labels.Labels`). - See #1 for an alternative implementation using a special label instead of boolean flags. - Note: a feature flag `promql-delayed-name-removal` has been added as it changes the behavior of some "weird" queries (see prometheus#11397 (comment)) Example (this always fails, as `__name__` is being dropped by `count_over_time`): ``` count_over_time({__name__!=""}[1m]) => Error executing query: vector cannot contain metrics with the same labelset ``` Before: ``` label_replace(count_over_time({__name__!=""}[1m]), "__name__", "count_$1", "__name__", "(.+)") => Error executing query: vector cannot contain metrics with the same labelset ``` After: ``` label_replace(count_over_time({__name__!=""}[1m]), "__name__", "count_$1", "__name__", "(.+)") => count_go_gc_cycles_automatic_gc_cycles_total{instance="localhost:9090", job="prometheus"} 1 count_go_gc_cycles_forced_gc_cycles_total{instance="localhost:9090", job="prometheus"} 1 ... ``` Signed-off-by: Jorge Creixell <jcreixell@gmail.com>

jcreixell · 2024-08-29T10:00:27Z

@beorn7 I addressed your last comment and squashed the commits, this should be ready to go!

promql/value.go

Signed-off-by: Björn Rabenstein <github@rabenste.in>

beorn7

Will merge on green. Thank you very much.

bboreham · 2024-09-02T21:19:08Z

Should #10921 be closed?

beorn7 · 2024-09-03T13:56:13Z

Maybe. I'll comment on that issue.

#### What this PR does Implement prometheus/prometheus#14477 in MQE. #### Which issue(s) this PR fixes or relates to Fixes grafana/mimir-squad#3065 #### Checklist - [x] Tests updated. - [x] Documentation added. - [x] `CHANGELOG.md` updated - the order of entries should be `[CHANGE]`, `[FEATURE]`, `[ENHANCEMENT]`, `[BUGFIX]`. If changelog entry is not needed, please add the `changelog-not-needed` label to the PR. - [x] [`about-versioning.md`](https://github.com/grafana/mimir/blob/main/docs/sources/mimir/configure/about-versioning.md) updated with experimental features. --------- Co-authored-by: Charles Korn <charleskorn@users.noreply.github.com>

jcreixell requested a review from roidelapluie as a code owner July 15, 2024 19:26

prombot added the prombench label Jul 16, 2024

jcreixell force-pushed the delete-name-label-deletion branch 3 times, most recently from c445dc2 to c3c9d5e Compare July 22, 2024 15:49

jcreixell force-pushed the delete-name-label-deletion branch 2 times, most recently from aa3e625 to 43a4b0a Compare July 22, 2024 17:43

beorn7 mentioned this pull request Aug 7, 2024

Stop removing __name__ from results: prevent vector cannot contain metrics with the same labelset #11397

Closed

jcreixell force-pushed the delete-name-label-deletion branch 2 times, most recently from 3ee2c72 to aef7424 Compare August 12, 2024 11:31

beorn7 requested changes Aug 21, 2024

View reviewed changes

jcreixell force-pushed the delete-name-label-deletion branch from 781e680 to 658be86 Compare August 29, 2024 09:10

jcreixell requested review from Nexucis, aknuds1, brancz, bwplotka, cstyan, dgl, jesusvazquez, juliusv and tomwilkie as code owners August 29, 2024 09:10

jcreixell force-pushed the delete-name-label-deletion branch from 658be86 to d21dc53 Compare August 29, 2024 09:41

jcreixell force-pushed the delete-name-label-deletion branch from d21dc53 to a708458 Compare August 29, 2024 09:53

beorn7 reviewed Aug 29, 2024

View reviewed changes

promql/value.go Outdated Show resolved Hide resolved

Update promql/value.go

b9f2273

Signed-off-by: Björn Rabenstein <github@rabenste.in>

beorn7 approved these changes Aug 29, 2024

View reviewed changes

beorn7 merged commit e9e3d64 into prometheus:main Aug 29, 2024

beorn7 mentioned this pull request Sep 3, 2024

performance improvement idea: drop metric names later #10921

Closed

colega mentioned this pull request Oct 10, 2024

Test promql.Engine with querysharding & EnableDelayedNameRemoval=true grafana/mimir#9577

Closed

4 tasks

BrewTestBot mentioned this pull request Oct 22, 2024

prometheus 2.55.0 Homebrew/homebrew-core#195244

Merged

harry671003 mentioned this pull request Nov 21, 2024

Update prometheus thanos-io/promql-engine#487

Merged

This was referenced Jan 3, 2025

Implement Native histogram Binary operations and Change Functions for Compatibility with v3.1.0 thanos-io/promql-engine#503

Merged

Implement delayed name removal thanos-io/promql-engine#507

Open

charleskorn mentioned this pull request Jan 29, 2025

Proposal: Add type and unit metadata labels prometheus/proposals#39

Merged

zenador mentioned this pull request Aug 26, 2025

MQE: implement delayed name removal grafana/mimir#12509

Merged

4 tasks

zenador mentioned this pull request Sep 8, 2025

PromQL: Fix bug with inconsistent results for queries with OR expression and EnableDelayedNameRemoval #17161

Merged

Conversation

jcreixell commented Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bboreham commented Jul 16, 2024

Uh oh!

prombot commented Jul 16, 2024

Uh oh!

bboreham commented Jul 18, 2024

Uh oh!

prombot commented Jul 18, 2024

Uh oh!

jcreixell commented Jul 18, 2024

Uh oh!

jcreixell commented Jul 19, 2024

Uh oh!

jcreixell commented Jul 22, 2024

Uh oh!

beorn7 commented Jul 25, 2024

Uh oh!

beorn7 commented Aug 8, 2024

Uh oh!

beorn7 commented Aug 8, 2024

Uh oh!

beorn7 commented Aug 8, 2024

Uh oh!

jcreixell commented Aug 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

beorn7 commented Aug 13, 2024

Uh oh!

prombot commented Aug 13, 2024

Uh oh!

beorn7 commented Aug 14, 2024

Uh oh!

prombot commented Aug 14, 2024

Uh oh!

beorn7 commented Aug 14, 2024

Uh oh!

jcreixell commented Aug 14, 2024

Uh oh!

beorn7 commented Aug 15, 2024

Uh oh!

beorn7 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jcreixell commented Aug 29, 2024

Uh oh!

Uh oh!

beorn7 left a comment

Choose a reason for hiding this comment

Uh oh!

bboreham commented Sep 2, 2024

Uh oh!

beorn7 commented Sep 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jcreixell commented Jul 15, 2024 •

edited

Loading

jcreixell commented Aug 12, 2024 •

edited

Loading