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: Run ingestion limiter after custom push wrappers #10754

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

Conversation

julienduchesne
Copy link
Member

@julienduchesne julienduchesne commented Feb 26, 2025

I was chasing an issue where a tenant was limited on a 10k/s limit, with a ~200/s received rate.

image

Turns out that the ingestion limit was applied and then custom push wrappers were removing timeseries and samples. The number of series recorded in the metrics was the one after custom push wrappers

Ingestion rate limit should apply after all push wrappers, since its purpose is to limit what we send to ingesters

With this bug, the Mimir / Tenants dashboard is also wrong because the limit is shown against the number of received samples (see screenshot for example of that) and we have no way to alert when we're approaching limits since we would typically use cortex_distributor_received_samples_total against the limit for that

Checklist

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

I was chasing an issue where a tenant was limited on a 10k/s limit, with a ~200/s received rate.

Turns out that the ingestion limit was applied and then custom push wrappers were removing timeseries and samples. The number of series recorded in the metrics was the one after custom push wrappers

Ingestion rate limit should apply after all push wrappers, since its purpose is to limit what we send to ingesters

With this bug, the `Mimir / Tenants` dashboard is also wrong because the limit is shown against the number of received samples
@julienduchesne julienduchesne marked this pull request as ready for review February 26, 2025 20:03
@julienduchesne julienduchesne requested a review from a team as a code owner February 26, 2025 20:03
@julienduchesne julienduchesne force-pushed the julienduchesne/distributor-run-ingester-limit branch from 0e9e553 to fd54d56 Compare February 26, 2025 20:12
Copy link
Contributor

@leizor leizor left a comment

Choose a reason for hiding this comment

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

LGTM!

@bboreham
Copy link
Contributor

bboreham commented Mar 7, 2025

I don’t agree with the premise. The point of the limit is to protect distributors.

@bboreham
Copy link
Contributor

bboreham commented Mar 8, 2025

Not related in this particular case, since the incoming rate was clearly over the limit, but since other people have erroneously picked up on this PR I will link it:

(If the limit line was on the panel to the left, this confusion might be eased)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants