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

Update dskit and mimir-prometheus #8632

Merged
merged 3 commits into from
Jul 5, 2024
Merged

Conversation

pracucci
Copy link
Collaborator

@pracucci pracucci commented Jul 5, 2024

What this PR does

In this PR I'm updating dskit and mimir-prometheus to get:

In this PR we're also introducing the new experimental PromQL functions limitk() and limit_ratio() done by @jjo. I've marked them as non shardable, just because I didn't want to spend an effort right now to brainstorm whether they're shardable or not and then add tests on them (they're disabled by default). We can revisit this later.

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.

Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
@pracucci pracucci marked this pull request as ready for review July 5, 2024 08:51
@pracucci pracucci requested review from stevesg, grafanabot and a team as code owners July 5, 2024 08:51
@@ -28,6 +28,8 @@ var NonParallelFuncs = []string{
"absent",
"absent_over_time",
"histogram_quantile",
"limitk",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think limitk is safe to parallelize, just like topk/bottomk.

Maybe it's worth creating a new good-first-issue issue and let someone send a PR adding some tests for it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Topk and bottok are not parallelizeable as well (see details in this internal ticket).

Why they're not mentioned here? Right now I don't remember all implementation details of how these functions are guaranteed not to be parallelized. I will investigate it further, but will not block this PR on this.

@pracucci pracucci merged commit 8df6e6f into main Jul 5, 2024
29 checks passed
@pracucci pracucci deleted the update-dskit-and-mimir-prometheus branch July 5, 2024 13:46
@jjo
Copy link

jjo commented Jul 5, 2024

What this PR does

In this PR I'm updating dskit and mimir-prometheus to get:

In this PR we're also introducing the new experimental PromQL functions limitk() and limit_ratio() done by @jjo. I've marked them as non shardable, just because I didn't want to spend an effort right now to brainstorm whether they're shardable or not and then add tests on them (they're disabled by default). We can revisit this later.

[...]

woOT!, thanks @pracucci 🎉

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.

3 participants