-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Conversation
@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? |
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. |
On third thought, we should have this anyway :) The log messages we see from out of order entries are just too huge and frequent. |
f5122f4
to
c02faee
Compare
There was a problem hiding this 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.
d186e38
to
ad1b552
Compare
Make sure we cache empty results in the index cache.
What this PR does / why we need it: Stops ingesters returning a giant error message on Loki clusters with large amounts of traffic.
Checklist