Reject small subquery step size in query frontend #5323
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Ben Ye <[email protected]>
Signed-off-by: Ben Ye <[email protected]>
Signed-off-by: Ben Ye <[email protected]>
Signed-off-by: Ben Ye <[email protected]>
Signed-off-by: Ben Ye <[email protected]>
Signed-off-by: Ben Ye <[email protected]>
justinjung04
reviewed
May 8, 2023
pkg/querier/tripperware/subquery.go
Outdated
| ) | ||
|
|
||
| var ( | ||
| ErrSubQueryStepTooSmall = "exceeded maximum resolution of %d points per timeseries in subquery. Try decreasing the step size of your subquery" |
There was a problem hiding this comment.
I think you meant to recommend "increasing" the step size? or decreasing the "number of steps"?
There was a problem hiding this comment.
Good catch. I updated it to increasing the step size.
Comment on lines
53
to
59
| { | ||
| name: "two subqueries within functions", | ||
| query: "sum_over_time(up[60m:]) + avg_over_time(test[5m:1m])", | ||
| maxStep: 10, | ||
| defaultStep: time.Second, | ||
| err: httpgrpc.Errorf(http.StatusBadRequest, ErrSubQueryStepTooSmall, 10), | ||
| }, |
There was a problem hiding this comment.
Could we also add a test for two subqueries that's within the step limit?
There was a problem hiding this comment.
Thanks. Added another test case.
Signed-off-by: Ben Ye <[email protected]>
alanprot
approved these changes
May 9, 2023
|
THANKS! LGTM |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does:
In Prometheus, there is a step size check to make sure we don't have too many samples for a series for range query because it takes step and range parameters https://github.com/prometheus/prometheus/blob/main/web/api/v1/api.go#L503.
However, there is no such check for subquery, which makes subqueries to have very large steps such as
up[30d:1s]. This could easily OOM kill queriers.This pr adds the same step size check in QFE to reject such queries.
Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]