-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Storage: Fix max_chunks_per_query
functionality
#11212
base: main
Are you sure you want to change the base?
Conversation
@jeschkies @owen-d |
Trivy scan found the following vulnerabilities:
|
@le0park could you merge |
@jeschkies |
// Protect ourselves against OOMing. | ||
maxChunksPerQuery := c.limits.MaxChunksPerQueryFromStore(userID) | ||
if maxChunksPerQuery > 0 && len(chunkIDs) > maxChunksPerQuery { | ||
err := errors.QueryError(fmt.Sprintf("Query %v fetched too many chunks (%d > %d)", predicate, len(chunkIDs), maxChunksPerQuery)) | ||
return nil, nil, err | ||
} |
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 wonder if we could enforce this in the frontend. The index provides the number of chunks that will be queried.
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.
Yes, we could, but in this PR I just want to make max_chunks_per_query
functional.
max_chunks_per_query
is storage config, so I just add the code here to use store limit config.
Can I move max_chunks_per_query
config to querier config? If I could, then I will move this code to querier validation function validateQueryRequest(..)
.
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.
max_chunks_per_query is storage config
then it's fine 🙂
|
||
// NewQueryLimiter makes a new per-query limiter. Each query limiter | ||
// is configured using the `maxSeriesPerQuery` limit. | ||
func NewQueryLimiter(maxSeriesPerQuery, maxChunkBytesPerQuery int, maxChunksPerQuery int) *QueryLimiter { |
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.
Was this not used anywhere?
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.
@jeschkies Yes, it was.
This query_limiter
is copy of legacy cortex code, but any module doesn't use this query limiter. There is another query limiter actually using now, so remaining this code will confuse us.
ece159a
to
195a8ad
Compare
- `MaxChunksPerQueryFromStore` is nooping in Loki (d88f399)
Any update on this PR? |
What this PR does / why we need it:
There are some issues that
max_chunks_per_query
isn't working on limiting the chunks per query.I found that
max_chunks_per_query
is now totally unused configuration. I found thatmax_chunks_per_query
was using before #5650 and #5833. On those refactoring pr, limiting chunks per query logic had to move on CompositeStore, but it was missing. I think it's pretty important configuration to limit memory. So I writemax_chunks_per_query
logic oncomposite_store.go
and test.I do some works below:
Which issue(s) this PR fixes:
Fixes #9101
Special notes for your reviewer:
It's the first time to open pr on open source community for me.
I'm newbie with Loki. Please correct me if I'm wrong 😁 Thank you
@cyriltovena @owen-d Could you check my pull request?
Checklist
CONTRIBUTING.md
guide (required)CHANGELOG.md
updatedadd-to-release-notes
labeldocs/sources/setup/upgrade/_index.md
production/helm/loki/Chart.yaml
and updateproduction/helm/loki/CHANGELOG.md
andproduction/helm/loki/README.md
. Example PRdeprecated-config.yaml
anddeleted-config.yaml
files respectively in thetools/deprecated-config-checker
directory. Example PR