-
Notifications
You must be signed in to change notification settings - Fork 569
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
Improve read consistency observability #7193
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.
LGTM
// Get details about the rule. | ||
detail := rules.FromOriginContext(ctx) | ||
|
||
// If the rule has dependencies then we should enforce strong read consistency, | ||
// otherwise we'll fallback to the per-tenant default. | ||
if !detail.NoDependencyRules { | ||
spanLog := spanlogger.FromContext(ctx, logger) | ||
spanLog.DebugLog("msg", "forced strong read consistency because the rule depends on other rules in the same rule group") |
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.
Would adding a tag / attribute to the span help so that we could query by it?
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.
Is e3525fb what you had in mind?
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.
Exactly, thanks!
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.
minor comments only. I agree with Nick on making it a tag. Otherwise, LGTM
pkg/ruler/rule_query_consistency.go
Outdated
// detect that the query is querying the ALERTS_FOR_STATE metric, otherwise we don't inject any specific read | ||
// consistency level and we fallback to the tenant's default. |
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.
should the falling back happen in this function or the caller is responsible for it?
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.
I think none of them. The fallback means "do not explicitly set the read consistency" which is what this function does.
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.
Falling back to the default happens in the ingester, not in the ruler. Maybe i'm nitpicking too much for a comment.
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.
I see what you mean now. Let me rephrase the comment.
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.
I rephrased it here: 61c6917
… available Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
a6fd430
to
e3525fb
Compare
Signed-off-by: Marco Pracucci <[email protected]>
What this PR does
This PR is part of the experimental ingest storage. In this PR I've done a couple of improvements to read consistency observability:
Which issue(s) this PR fixes or relates to
N/A
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]
.about-versioning.md
updated with experimental features.