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

Distributor: Add limit for exemplar per series per request #7989

Merged
merged 2 commits into from
Apr 29, 2024

Conversation

ying-jeanne
Copy link
Contributor

@ying-jeanne ying-jeanne commented Apr 26, 2024

What this PR does

In this pull request, we propose safeguarding the write path by implementing a restriction on the number of exemplars/series per request. Exemplars exceeding this limit will be discarded, and these discards will be monitored through the cortex_discarded_exemplars_total metric.

Which issue(s) this PR fixes or relates to

Fixes #

Checklist

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

@ying-jeanne ying-jeanne force-pushed the max-exemplars-per-request branch 6 times, most recently from a4a4338 to d169e7a Compare April 26, 2024 20:05
Signed-off-by: Ying WANG <ying.wang@grafana.com>
@ying-jeanne ying-jeanne marked this pull request as ready for review April 26, 2024 20:24
@ying-jeanne ying-jeanne requested review from jdbaldry and a team as code owners April 26, 2024 20:24
pkg/distributor/distributor_test.go Outdated Show resolved Hide resolved
pkg/distributor/distributor_test.go Outdated Show resolved Hide resolved
pkg/distributor/validate.go Outdated Show resolved Hide resolved
Copy link
Member

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some nits on top of Nick's comments.

CHANGELOG.md Outdated Show resolved Hide resolved
cmd/mimir/help-all.txt.tmpl Outdated Show resolved Hide resolved
@aknuds1 aknuds1 added enhancement New feature or request component/distributor labels Apr 29, 2024
Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found some typos. I also wonder if instead the flag name should be -validation.max-exemplars-per-series-per-request, to be consistent with -validation.max-label-names-per-series?

CHANGELOG.md Outdated Show resolved Hide resolved
pkg/distributor/distributor_test.go Outdated Show resolved Hide resolved
pkg/distributor/validate.go Outdated Show resolved Hide resolved
pkg/mimirpb/timeseries.go Outdated Show resolved Hide resolved
pkg/mimirpb/timeseries_test.go Outdated Show resolved Hide resolved
pkg/util/validation/limits.go Outdated Show resolved Hide resolved
Copy link
Contributor

@56quarters 56quarters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Approving but I'd like to see @aknuds1's suggestions implemented. Thanks!

@ying-jeanne ying-jeanne force-pushed the max-exemplars-per-request branch 2 times, most recently from 419d93b to 38a352d Compare April 29, 2024 19:49
Signed-off-by: Ying WANG <ying.wang@grafana.com>
@ying-jeanne ying-jeanne merged commit 6af4273 into main Apr 29, 2024
29 checks passed
@ying-jeanne ying-jeanne deleted the max-exemplars-per-request branch April 29, 2024 20:07
grafanabot pushed a commit that referenced this pull request Apr 29, 2024
* Distributor: Add limit for exemplar per series

Signed-off-by: Ying WANG <ying.wang@grafana.com>

* Address comments

Signed-off-by: Ying WANG <ying.wang@grafana.com>

---------

Signed-off-by: Ying WANG <ying.wang@grafana.com>
(cherry picked from commit 6af4273)
@grafanabot
Copy link
Contributor

The backport to r287 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-7989-to-r287 origin/r287
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x 6af4273313e901f97fe2bffc6dc07b6e8b11e7cb
# Push it to GitHub
git push --set-upstream origin backport-7989-to-r287
git switch main
# Remove the local backport branch
git branch -D backport-7989-to-r287

Then, create a pull request where the base branch is r287 and the compare/head branch is backport-7989-to-r287.

@grafana grafana deleted a comment from grafanabot Apr 29, 2024
@grafana grafana deleted a comment from grafanabot Apr 29, 2024
ying-jeanne added a commit that referenced this pull request Apr 29, 2024
* Distributor: Add limit for exemplar per series

Signed-off-by: Ying WANG <ying.wang@grafana.com>

* Address comments

Signed-off-by: Ying WANG <ying.wang@grafana.com>

---------

Signed-off-by: Ying WANG <ying.wang@grafana.com>
(cherry picked from commit 6af4273)

Co-authored-by: Ying WANG <74549700+ying-jeanne@users.noreply.github.com>
@@ -13,6 +13,7 @@
* [FEATURE] New `/ingester/unregister-on-shutdown` HTTP endpoint allows dynamic access to ingesters' `-ingester.ring.unregister-on-shutdown` configuration. #7739
* [FEATURE] Querier: add experimental streaming PromQL engine, enabled with `-querier.promql-engine=streaming`. #7693
* [FEATURE] Server: added experimental [PROXY protocol support](https://www.haproxy.org/download/2.3/doc/proxy-protocol.txt). The PROXY protocol support can be enabled via `-server.proxy-protocol-enabled=true`. When enabled, the support is added both to HTTP and gRPC listening ports. #7698
* [ENHANCEMENT] Distributor: add experimental limit for exemplars per series per request, enabled with `-distributor.max-exemplars-per-series-per-request`, the number of discarded exemplars are tracked with `cortex_discarded_exemplars_total{reason="too_many_exemplars_per_series"}` #7989
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the reason should include the fact that it's per request, so one doesn't have the impression that series have too many exemplars, and to reflect the flag name: "too_many_exemplars_per_series_per_request".

@aknuds1 aknuds1 changed the title Distributor: Add limit for exemplar per series Distributor: Add limit for exemplar per series per request Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants