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

pkg/ingester: limit total number of errors a stream can return on push #1071

Merged
merged 2 commits into from
Jan 10, 2020

Conversation

rfratto
Copy link
Member

@rfratto rfratto commented Sep 26, 2019

What this PR does / why we need it: Stops ingesters returning a giant error message on Loki clusters with large amounts of traffic.

Checklist

  • Documentation added
  • Tests updated

@rfratto
Copy link
Member Author

rfratto commented Sep 26, 2019

@wardbekker not limiting the number of error messages meant that large pushes (i.e., ones with upwards of 300 logs) would result in a giant error message being logged at the client end if most/all of those logs were out of order. I've tried to pick a sensible default limit here. WDYT?

@rfratto
Copy link
Member Author

rfratto commented Sep 26, 2019

On second thought, this is a little too dirty of a hack. We do want all the detail, we just don't want it being logged. I think the new error API that's in progress would help here: if errors were gRPC errors, then the info here could be stored as a detail, and wouldn't be logged, but still returned to the client. Closing.

@rfratto rfratto closed this Sep 26, 2019
@rfratto rfratto deleted the limit-stream-errors branch November 19, 2019 14:33
@rfratto rfratto restored the limit-stream-errors branch January 10, 2020 19:36
@rfratto
Copy link
Member Author

rfratto commented Jan 10, 2020

On third thought, we should have this anyway :) The log messages we see from out of order entries are just too huge and frequent.

@rfratto rfratto reopened this Jan 10, 2020
Copy link
Collaborator

@slim-bean slim-bean left a comment

Choose a reason for hiding this comment

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

LGTM, I stalled on the other PR for improving error handling and the volume we see in out of order entry logs makes this PR valuable, at some point returning 10 errors or 10k is irrelevant but 10k is spamming the logs unnecessarily.

@rfratto rfratto merged commit 9227eed into grafana:master Jan 10, 2020
@rfratto rfratto deleted the limit-stream-errors branch January 10, 2020 20:29
cyriltovena pushed a commit to cyriltovena/loki that referenced this pull request Jun 11, 2021
Make sure we cache empty results in the index cache.
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.

None yet

3 participants