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

bug: promql/fragile should not warn when using without and joining with explicit labelset #1289

Closed
skl opened this issue Feb 14, 2025 · 4 comments · Fixed by #1299
Closed
Assignees

Comments

@skl
Copy link

skl commented Feb 14, 2025

There is a PromQL alert rule in the kubernetes-mixin for CPUThrottlingHigh which looks like this (abbreviated for readability):

sum without (id, metrics_path, name, image, endpoint, job, node) (
  increase(container_cpu_cfs_throttled_periods_total[5m])
)
/ on (cluster, namespace, pod, container) group_left
sum without (id, metrics_path, name, image, endpoint, job, node) (
  increase(container_cpu_cfs_periods_total[5m])
)
> ( 25 / 100 )

Source:

The warning from pint look like this:

Warning: Aggregation using without() can be fragile when used inside binary expression because both sides must have identical sets of labels to produce any results, adding or removing labels to metrics used here can easily break the query, consider aggregating using by() to ensure consistent labels. (promql/fragile)

I believe that the explicit usage of labels in the join on (cluster, namespace, pod, container) means that this query should not result in a fragile warning.

@prymitive
Copy link
Collaborator

Thanks, yes this is wrong.
I might go and remove the entire promql/fragile as it's pretty old, naive and can get a bit too spammy.

@skl
Copy link
Author

skl commented Feb 19, 2025

Sounds fine to me, we have this check disabled at the moment anyway 👍

@prymitive
Copy link
Collaborator

promql/fragile does other checks as well, so just removing the aggregation logic from it #1299

@skl
Copy link
Author

skl commented Feb 20, 2025

Thank you!

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 a pull request may close this issue.

2 participants