-
Notifications
You must be signed in to change notification settings - Fork 9.7k
promql: fix topk error on NaN argument for non-existing series #16725
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
promql: fix topk error on NaN argument for non-existing series #16725
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for this, let's see what others think about it
50447ec
to
80d4329
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good overall. I just have a few suggestions related to the tests.
80d4329
to
cf0b039
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but maybe let's make use of the new msg:
syntax to test the precise message in the tests. I think it makes sense in this case.
cf0b039
to
6fda1cc
Compare
Signed-off-by: Michael Hoffmann <mhoffmann@cloudflare.com>
6fda1cc
to
44ee5e2
Compare
Added the explicit warning message into the acceptance tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you.
## Manual follow-ups @krajorama : I've updated the engine tests from upstream (prometheus/prometheus#16725). There was one that has a different error message with MQE so I only added it to *ours* tests. Maybe we want to fix the error message instead? Commented. @krajorama : I've followed up the block stats changes (prometheus/prometheus#16824) by adding the necessary code in Mimir and also more tests. I've followed BlockStats.NumSamples to find all occurrences. Even if I wasn't successful , we don't actually use the new fields for anything critical - so this should be safe. ## Update mimir-prometheus dependency *This PR was automatically created by the [update-vendored-mimir-prometheus.yml](https://github.com/grafana/mimir/blob/main/.github/workflows/update-vendored-mimir-prometheus.yml) workflow.* ### Details: - **Source branch/ref**: [`main`](https://github.com/grafana/mimir-prometheus/tree/main) - **Previous commit**: [`609b9c7c04c1`](grafana/mimir-prometheus@609b9c7c04c1) - **New commit**: [`8323300263566d178c6d571d985924fbd4270225`](grafana/mimir-prometheus@8323300) - **Changes**: [`609b9c7c04c1...8323300263566d178c6d571d985924fbd4270225`](grafana/mimir-prometheus@609b9c7...8323300) --------- Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com> Co-authored-by: mimir-github-bot[bot] <mimir-github-bot[bot]@users.noreply.github.com> Co-authored-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com> Co-authored-by: Charles Korn <charles.korn@grafana.com>
topk(NaN, non_existent)
used to return an error, it stopped after https://github.com/prometheus/prometheus/pull/16404/files.This is a minor issue, but broke thanos regression tests