Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mimirtool rules prepare: do not aggregation label to on() clause if already present in group_left() or group_right() #7840

Merged
merged 2 commits into from
Apr 9, 2024

Conversation

pracucci
Copy link
Collaborator

@pracucci pracucci commented Apr 8, 2024

What this PR does

During the weekend, while on-call, I supported an engineer at Grafana Labs which hit an edge case in mimirtool rules prepare command. The tool is used to add an aggregation label (defaults to cluster) to any aggregation and binary operator vector matcher of each rule.

The edge case we found is that when the aggregation label is already present in group_left/right() it shouldn't be added to on() too, because incorrect. A PromQL query with the same label both specified in on() and group_left/right() doesn't even parse (parsing it returns the error: label "cluster" must not occur in ON and GROUP clause at once).

Example

As an example, consider the following original query:

count by(cluster, namespace) (label_replace(cortex_build_info, "cluster", "", "cluster", "(.*)"))
* on(namespace) group_left(cluster) group by(cluster, namespace) (cortex_build_info)

With the code in main, mimirtool rules prepare rewrites it as follow:

count by(cluster, namespace) (label_replace(cortex_build_info, "cluster", "", "cluster", "(.*)"))
* on(namespace, cluster) group_left(cluster) group by(cluster, namespace) (cortex_build_info)

But it's invalid, and a subsequent parsing will return label "cluster" must not occur in ON and GROUP clause at once. With the modified code in this PR, mimirtool rules prepare would keep the original query as is, because cluster label is already part of group_left() and so shouldn't be added to on() too.

Which issue(s) this PR fixes or relates to

N/A

Checklist

  • Tests updated.
  • Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • about-versioning.md updated with experimental features.

…lready present in group_left() or group_right()

Signed-off-by: Marco Pracucci <[email protected]>
Copy link

@rafa-munoz rafa-munoz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the time to fix it

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Oleg Zaytsev <[email protected]>
@pracucci pracucci marked this pull request as ready for review April 8, 2024 13:43
@pracucci pracucci requested a review from a team as a code owner April 8, 2024 13:43
@pracucci pracucci merged commit db3719b into main Apr 9, 2024
29 checks passed
@pracucci pracucci deleted the fix-mimirtool-rules-prepare branch April 9, 2024 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants