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

Query-frontend: add -query-frontend.query-sharding-max-regexp-size-bytes #4632

Merged
merged 4 commits into from
Mar 31, 2023

Conversation

pracucci
Copy link
Collaborator

@pracucci pracucci commented Mar 31, 2023

What this PR does

Query sharding amplifies the number of partial queries executed by queriers, and so label matchers evaluated by ingesters and store-gateway. We've seen some edge cases where very complex regexp can spike the ingesters CPU significantly, up to the point of saturating the node CPU. Despite our efforts to optimise the regexp matcher (which in many cases are very effective), I would like to add an optional limit to query sharding, to skip it if the query contains a regexp longer than the configured limit.

Note to reviewers:

  • The regexp length is not synonymous of complexity, but evaluating the actual computatinal complexity of a regexp may be computationally expensive itself, so just checking the length is a simple approximation I suggest to begin with.
  • I removed TestQuerySharding_ShouldFallbackToDownstreamHandlerOnMappingFailure beacuse it expects to push down to queriers a query we can't parse, which is not correct (there's no point doing it). The test aims to
    ensure we push down to querier a query we fail to shard (but we can parse), but this can happen only in case of logic bugs in the query sharding, and so can't be reproduced unless mocking its internals. I tried to mock the internals to reproduce it, but the resulting code is pretty ugly, so I'm proposing to just get rid of that test.

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]

Signed-off-by: Marco Pracucci <marco@pracucci.com>
@pracucci pracucci requested review from a team as code owners March 31, 2023 03:09
…ailure beacuse it expects to push down to queriers a query we can't parse, which is not correct. The test aims to ensure we push down to querier a query we fail to shard, but this can happen only in case of logic bugs in the query sharding, and so can't be reproduced unless mocking its internals.

Signed-off-by: Marco Pracucci <marco@pracucci.com>
@pracucci pracucci marked this pull request as draft March 31, 2023 08:34
@pracucci
Copy link
Collaborator Author

Moving to draft because an integration test is failing in CI and I can't understand why. I'm investigating it.

Copy link
Member

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

PR lgtm. I can't see why integration tests would be failing.

pkg/frontend/querymiddleware/querysharding.go Outdated Show resolved Hide resolved
pracucci and others added 2 commits March 31, 2023 12:42
Co-authored-by: Peter Štibraný <pstibrany@gmail.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
@pracucci pracucci marked this pull request as ready for review March 31, 2023 11:25
@pracucci pracucci merged commit b228ece into main Mar 31, 2023
@pracucci pracucci deleted the add-query-sharding-max-regexp-limit branch March 31, 2023 12:50
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.

2 participants