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

Improve read consistency observability #7193

Merged
merged 4 commits into from
Jan 24, 2024

Conversation

pracucci
Copy link
Collaborator

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:

  • Log read_consistency in 'query stats' even when query details are not available (the two are unrelated so I don't think we should bind them in the query-frontend middleware)
  • Log in a tracing span when the ruler requests strong read consistency

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 marked this pull request as ready for review January 23, 2024 16:50
@pracucci pracucci requested review from a team as code owners January 23, 2024 16:50
Copy link
Contributor

@56quarters 56quarters left a 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")
Copy link
Contributor

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?

Copy link
Collaborator Author

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Exactly, thanks!

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.

minor comments only. I agree with Nick on making it a tag. Otherwise, LGTM

pkg/frontend/transport/handler_test.go Show resolved Hide resolved
Comment on lines 123 to 125
// 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.
Copy link
Contributor

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?

Copy link
Collaborator Author

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.

Copy link
Contributor

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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 <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
@pracucci pracucci force-pushed the improve-read-consistency-observability branch from a6fd430 to e3525fb Compare January 24, 2024 10:31
Signed-off-by: Marco Pracucci <marco@pracucci.com>
@pracucci pracucci enabled auto-merge (squash) January 24, 2024 10:53
@pracucci pracucci merged commit 8e9fa31 into main Jan 24, 2024
28 checks passed
@pracucci pracucci deleted the improve-read-consistency-observability branch January 24, 2024 10:53
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

3 participants