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

Consider query select time in max query length check #5808

Merged
merged 1 commit into from
Mar 11, 2024

Conversation

yeya24
Copy link
Contributor

@yeya24 yeya24 commented Mar 11, 2024

What this PR does:

Right now, there is a check for max query length in the query frontend query range middleware. It only checks the start and end parameter of range query API and see if it exceeds a maximum time range.

However, this check doesn't analyze the query string itself. For example, up[60d]. The time window 60d can be very large and exceeds the limit. However, it is not checked at query frontend.

To fix this issue, there is another checkfor max query length at Querier right now, which happens per Select only and it considers the time range of the query itself. For example, up[60d] can be rejected at Querier if the limit is configured as 30d.

However, this solution isn't perfect because a query can have multiple selects. rate(up[25d]) + rate(up[25d] offset 25d) + rate(up[25] offset 50d) actually queries 75 days of data but each select only queries 25 day time range so it will be allowed.

To address this issue in a better way, additional check logic is added in Query Frontend to analyze the whole query and see the max time range it queries. With this check, the check at Querier can actually be turned off.

Overall changes:

  1. Additional max query length check at Query Frontend Query Range Middleware
  2. Added the same logic to Query Frontend Instant Query Middleware. Previously there is no max query length check for instant query
  3. Added the same logic to Ruler. Previously Ruler relies on max query length at Querier level
  4. Added a new flag querier.ignore-max-query-length to disable checking max query length at Querier.

Which issue(s) this PR fixes:
Fixes #

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@yeya24 yeya24 force-pushed the max-query-length-qfe-ruler branch 2 times, most recently from 87b5803 to 6d72d46 Compare March 11, 2024 07:21
… query

Signed-off-by: Ben Ye <benye@amazon.com>
@yeya24 yeya24 force-pushed the max-query-length-qfe-ruler branch from 6d72d46 to 2d96cf1 Compare March 11, 2024 07:48
@yeya24 yeya24 requested a review from alanprot March 11, 2024 15:55
@yeya24 yeya24 merged commit e961770 into cortexproject:master Mar 11, 2024
16 checks passed
@yeya24 yeya24 deleted the max-query-length-qfe-ruler branch March 11, 2024 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants