loki icon indicating copy to clipboard operation
loki copied to clipboard

Storage: Fix `max_chunks_per_query` functionality

Open le0park opened this issue 1 year ago • 6 comments

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.

  • #9101
  • #11077

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:

  • Add the limit logic just before that CompositeStore returns chunks. (CompositeStore.GetChunks) I referred the code before #5650 and #5833.
  • Remove unused cortex util code.

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

  • [x] Reviewed the CONTRIBUTING.md guide (required)
  • [ ] Documentation added
  • [x] 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

le0park avatar Nov 11 '23 08:11 le0park

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Nov 11 '23 08:11 CLAassistant

@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?

le0park avatar Nov 15 '23 01:11 le0park

Trivy scan found the following vulnerabilities:

github-actions[bot] avatar Nov 23 '23 14:11 github-actions[bot]

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

jeschkies avatar Feb 05 '24 12:02 jeschkies

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

le0park avatar Feb 05 '24 13:02 le0park