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

Storage: Fix max_chunks_per_query functionality #11212

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

le0park
Copy link

@le0park le0park commented Nov 11, 2023

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 that max_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 write max_chunks_per_query logic on composite_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

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • Tests updated
  • CHANGELOG.md updated
    • If the change is worth mentioning in the release notes, add add-to-release-notes label
  • Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
  • For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR
  • If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR
Not Found
@le0park le0park requested a review from a team as a code owner November 11, 2023 08:00
@CLAassistant
Copy link

CLAassistant commented Nov 11, 2023

CLA assistant check
All committers have signed the CLA.

@le0park
Copy link
Author

le0park commented Nov 15, 2023

@jeschkies @owen-d
I'm still learning and would love some guidance on this PR.
Would someone be willing to review it and give me some feedback?

Copy link
Contributor

Trivy scan found the following vulnerabilities:

@jeschkies
Copy link
Contributor

@le0park could you merge main into you branch? I'm not with the Loki team anymore but I'll try to find a reviewer.

@le0park
Copy link
Author

le0park commented Feb 5, 2024

@jeschkies
I merged main into my branch. Thank you for checking this pr.

Comment on lines 177 to 182
// 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
}
Copy link
Contributor

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.

Copy link
Author

@le0park le0park Feb 5, 2024

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(..).

Copy link
Contributor

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 {
Copy link
Contributor

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?

Copy link
Author

@le0park le0park Mar 11, 2024

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.

@le0park le0park force-pushed the revive-max-chunks-per-query branch from ece159a to 195a8ad Compare March 11, 2024 15:20
- `MaxChunksPerQueryFromStore` is nooping in Loki (d88f399)
@le0park le0park requested a review from jeschkies March 11, 2024 15:54
@carlopalacio
Copy link

Any update on this PR?

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.

max_chunks_per_query not working
4 participants