RFC: Consistent timeout handling in Collector pipelines #11948
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #11948 +/- ##
==========================================
+ Coverage 91.59% 91.62% +0.03%
==========================================
Files 449 447 -2
Lines 23761 23731 -30
==========================================
- Hits 21763 21743 -20
+ Misses 1623 1613 -10
Partials 375 375 ☔ View full report in Codecov by Sentry. |
|
I will present this RFC in the next two Collector SIG meetings 1/7/2025 (APAC/PT) and 1/15/2025 (NA). |
| - A request arrives with an already-expired deadline. Should the component | ||
| immediately return a deadline-exceeded error status? | ||
| - A request arrives with a viable deadline, but the request does not | ||
| succeed in time. Should the component immediately return a deadline-exceeded | ||
| status, or should it wait for its response? |
There was a problem hiding this comment.
Would be also good to extend this into a requirement and have a "virtual" component after receivers and probably before exporters (not sure is useful for every processor as well) that checks for the deadline and returns immediately if expired.
There was a problem hiding this comment.
I agree, an implementation inside the pipeline apparatus will extend consistent timeout behavior to all components.
| - A request arrives and has to acquire a resource (e.g., space in a queue) that | ||
| is not immediately available. Should the component fail "fast" or stall the | ||
| request, hoping the resource will become available before the deadline? |
There was a problem hiding this comment.
As mentioned before, there are use-cases for both, so we should make sure we support both models.
| - A request arrives and the component calls for an additive timeout that is | ||
| greater than the request's deadline. For example: | ||
| - a batch processor is configured with `1s` timeout, and an arriving | ||
| request has a `0.5s` timeout | ||
| - timeout sender has `5s` configured timeout, arriving request has `2s` timeout | ||
| - retry sender has a maximum elapsed timeout of `1m`, arriving request has `5s` | ||
| timeout. |
There was a problem hiding this comment.
Not sure I understand what is the "expected" behavior here and especially the derived requirement related to timeout sender.
Also, keep in mind that users may use a persistent queue, when timeout has a different behavior probably.
There was a problem hiding this comment.
I'm also not sure what is expected, and I wonder if users have considered / wanted the ability to ignore a timeout. The use-case that I've seen is a OTLP endpoint accepting gRPC requests from a gateway collector. The gateway collector uses a 5s default timeout, and the service wants to permit its own backend 15s. The receiver cannot abide a 5s timeout, is there any way to simply disregard it?
For the batcher case, it's different: the batcher could bypass itself if the timeout is less than the interval.
For the persistent queue case, I believe we should interpret timeout as a request deadline, not as some kind of time-to-live parameter. That means, the timeout applies to the enqueue operation, and I expect timeout to happen if the disk gets slow.
|
|
||
| - Batch processor/sender propagates maximum deadline in batch | ||
| - If enabled, queue sender blocks until queue space is available | ||
| - Timeout sender configured not to lower an already-configured timeout. |
There was a problem hiding this comment.
This is the canonical golang behavior, why not doing this?
There was a problem hiding this comment.
With this behavior, there's no way for the user to request a longer timeout. There's no way for the exporter to allow the user-configured timeout to pass w/o imposing a timeout of its own. I would like to see behavior like "honor the user's timeout up to 1 minute, use 10 seconds for requests w/o a timeout".
| select { | ||
| case <-ctx.Done(): | ||
| // the context is canceled, maybe deadline-exceeded. | ||
| return ctx.Err() | ||
| default: | ||
| // OK to continue | ||
| } |
There was a problem hiding this comment.
I think is nicer to check for Err. See
// If Done is not yet closed, Err returns nil.
// If Done is closed, Err returns a non-nil error explaining why:
// Canceled if the context was canceled
// or DeadlineExceeded if the context's deadline passed.
// After Err returns a non-nil error, successive calls to Err return the same error.
Err() error
There was a problem hiding this comment.
Just do if err = ctx.Err(); err != nil { return err }
| ### Queue sender | ||
|
|
||
| A new field will be introduced, with default matching the | ||
| original behavior of this component. | ||
|
|
||
| ```golang | ||
| // FailFast indicates that the queue should immediately | ||
| // reject requests when the queue is full, without considering | ||
| // the request deadline. Default: true. | ||
| FailFast bool `mapstructure:"fail_fast"` | ||
| ``` | ||
|
|
||
| In case the new FailFast flag is false, there are two cases: | ||
|
|
||
| 1. The request has a deadline. In this case, wait until the deadline | ||
| to enqueue the request. | ||
| 2. The request has no deadline. In this case, let the request block | ||
| indefinitely until it can be enqueued. |
|
|
||
| ## Specific proposals | ||
|
|
||
| ### Timeout sender |
There was a problem hiding this comment.
Still struggle with this and what you try to achieve. Have you seen a real problem with this and what do you try to achieve.
| to use deadlines always and/or use separate pipelines for requests | ||
| with and without deadlines. | ||
|
|
||
| ### Receiver helper |
There was a problem hiding this comment.
There are some behaviors that we can automatically check for all the receivers like the deadline expires by adding virtual components in the processing graph while initialization the service.
Some components that require configuration do need a helper/user code change.
| - `min_timeout` (duration): Limits the allowable timeout for new requests to a minimum value. >=0 means deadline checking. | ||
| - `timeout` (duration): Limits the allowable timeout for new requests to a maximum value. Must be >= 0. |
There was a problem hiding this comment.
Why not having a processor for this?
Description
Calls for deadline-awareness across common Collector pipeline components, including batch processors, queue sender, retry and
timeout senders.
Link to tracking issue
Part of #11183
Testing
n/a
Documentation
This is a new RFC. As these changes are accepted and implemented, user-facing documentation will be added.