feat: check for failed reason in distributors #18128
Conversation
| level.Error(d.logger).Log("msg", "failed to check if request exceeds limits, request has been accepted", "err", err) | ||
| } else if len(streamsAfterLimits) == 0 { | ||
| // All streams have been dropped. | ||
| level.Debug(d.logger).Log("msg", "request exceeded limits, all streams will be dropped", "tenant", tenantID) |
There was a problem hiding this comment.
Got rid of these log lines, will be far too much volume.
| break | ||
| } | ||
| } | ||
| if !found || reason == uint32(limits.ReasonFailed) { |
There was a problem hiding this comment.
Since we stopped using iota, no valid reason can ever be 0, meaning we can do a check against the default value of uint32.
| Reason: uint32(limits.ReasonMaxStreams), | ||
| }}, | ||
| }, | ||
| expectedErr: "rpc error: code = Code(429) desc = request exceeded limits: max streams", |
There was a problem hiding this comment.
I got rid of the reason for now, I'm not sure if its useful as it doesn't mention which streams. I want to think about how to better communicate this data back to the user given that some requests can be really large.
grobinson-grafana
force-pushed
the
grobinson/check-failed-reason-distributors
branch
from
56bfab8 to
9c11c0b
Compare
| // All streams were rejected, the request should be failed. | ||
| return nil, httpgrpc.Error(http.StatusTooManyRequests, "request exceeded limits") | ||
| } | ||
| streams = accepted |
There was a problem hiding this comment.
Since we "shink" the streams slice to the accepted ones, I believe the next check of the ingestionRateLimiter is operating on the wrong calculated value validationContext.validationMetrics.aggregatedPushStats.lineSize and we need to redo it here, right?
Imagine accepting a subset of the incoming streams but rate limiting on the total incoming.
There was a problem hiding this comment.
I will think about how to solve this 😢 We had the same behavior before this PR where we are rate limiting on discarded streams over the stream limit, so I think let's keep this PR scoped to the feature and then I will open a second PR to address this problem.
| // the original backing array. See "Filtering without allocation" from | ||
| // https://go.dev/wiki/SliceTricks. | ||
| withinLimits := make([]KeyedStream, 0, len(streams)) | ||
| accepted := make([]KeyedStream, 0, len(streams)) |
There was a problem hiding this comment.
Maybe we need to consider doing the "filtering without allocation" trick here if other validation stats are in following updated by this. OTH the backing array includes shards and not streams right, so the trick might not be possible at all. WDYT?
| // TODO(grobinson): We have an O(N*M) loop here. Need to benchmark if | ||
| // its faster to do this or if we should create a map instead. | ||
| var ( | ||
| found bool | ||
| reason uint32 | ||
| ) | ||
| for _, res := range results { | ||
| if res.StreamHash == s.HashKeyNoShard { | ||
| found = true | ||
| reason = res.Reason | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
We should measure this adds any latency in case of ingest-limits degradation.
What this PR does / why we need it:
This pull request checks for the new
ReasonFailedin the distributors.Stack on top of #18055 and #18123.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Checklist
CONTRIBUTING.mdguide (required)featPRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.docs/sources/setup/upgrade/_index.mddeprecated-config.yamlanddeleted-config.yamlfiles respectively in thetools/deprecated-config-checkerdirectory. Example PR