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

Add conditional read-after–write support to rules evaluation #7142

Merged
merged 12 commits into from
Jan 22, 2024

Conversation

pracucci
Copy link
Collaborator

@pracucci pracucci commented Jan 16, 2024

What this PR does

This PR introduces conditional read-after-write (or strong read consistency) support for rule evaluations. This PR is based on grafana/mimir-prometheus#587.

How it works:

  • The Prometheus ruler evaluates whether each rule in a group is independent or not. A rule is independent if no other rule depends on it and it doesn't depend on any other in the group.
  • The information whether a rule is independent or not is stored in the RuleDetail inside the context passed down to the ruler's query function.
  • Mimir uses a custom query function. In this PR I've introduced a query function wrapper which reads the RuleDetail from the context and inject Mimir-specific read consistency requirement in the context.
  • The Mimir-specific read consistency requirement is passed, via context and HTTP header (remote querier), through the network so that it gets correctly enforced in the ingesters.

code easier in Prometheus

I will try to upstream the mimir-prometheus changes to Prometheus, in coordination with Danny's PR from which I've tool the dependency analysis code. However, I will not block on it.

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.

@pracucci pracucci force-pushed the add-conditional-read-after-write-for-rules branch from 6d89d88 to af18dbb Compare January 17, 2024 15:33
@pracucci pracucci changed the title [do not review] Add conditional read-after–write support to rules evaluation Add conditional read-after–write support to rules evaluation Jan 17, 2024
@pracucci pracucci force-pushed the add-conditional-read-after-write-for-rules branch 3 times, most recently from 3601fe3 to 201703e Compare January 18, 2024 18:27
Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

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

nicely done, i really don't have any comments.

Would be nice if we can eventually improve rule.Independant to only apply where we need it (today it might significantly increase the number of read-after-write queries without needing that), but i suppose that's future work?

@pracucci
Copy link
Collaborator Author

pracucci commented Jan 19, 2024

Would be nice if we can eventually improve rule.Independant to only apply where we need it (today it might significantly increase the number of read-after-write queries without needing that), but i suppose that's future work?

Yes, it's on my plate but I wanted to keep the scope of mimir-prometheus PR smaller.

@pracucci pracucci force-pushed the add-conditional-read-after-write-for-rules branch from e7fae40 to 6e5bae8 Compare January 19, 2024 15:19
@pracucci
Copy link
Collaborator Author

@dimitarvdimitrov I've swapped the mimir-prometheus implementation with a simpler one (here) which doesn't include the concurrent rules evaluation support (given we don't need it in this PR). In the new mimir-prometheus implementation I've also exposed both "dependent rules" and "dependency rules" info, so in Mimir we can only enforce strong consistency on rules with dependencies.

I haven't update this PR description yet. Can you review the changes (this commit) first, please?

@pracucci pracucci force-pushed the add-conditional-read-after-write-for-rules branch from 6e5bae8 to a9afbe3 Compare January 19, 2024 15:31
Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

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

I had a closer look at the second revision and still LGTM

Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
…ntextWhenQueryingAlertsForStateMetric

Signed-off-by: Marco Pracucci <marco@pracucci.com>
…lerAndQueryStatsEnabled

Signed-off-by: Marco Pracucci <marco@pracucci.com>
…DependentRulesWhenUsingTheIngestStorage

Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
@pracucci pracucci force-pushed the add-conditional-read-after-write-for-rules branch from a9afbe3 to 8edf3c8 Compare January 22, 2024 16:52
…asedOnRuleDetail

Signed-off-by: Marco Pracucci <marco@pracucci.com>
@pracucci pracucci marked this pull request as ready for review January 22, 2024 17:09
@pracucci pracucci requested review from grafanabot and a team as code owners January 22, 2024 17:09
@pracucci pracucci enabled auto-merge (squash) January 22, 2024 17:09
@pracucci pracucci enabled auto-merge (squash) January 22, 2024 17:16
@pracucci pracucci merged commit 094b110 into main Jan 22, 2024
28 checks passed
@pracucci pracucci deleted the add-conditional-read-after-write-for-rules branch January 22, 2024 17:22
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.

None yet

2 participants