(otelarrowreceiver) Deprecate admission::waiter_limit #36100
Conversation
andrzej-stencel
changed the title
| @@ -84,9 +84,9 @@ In the `admission` configuration block the following settings are available: | |||
|
|
|||
| - `request_limit_mib` (default: 128): limits the number of requests that are received by the stream in terms of *uncompressed request size*. This should not be confused with `arrow.memory_limit_mib` which limits allocations made by the consumer when translating arrow records into pdata objects. i.e. request size is used to control how much traffic we admit, but does not control how much memory is used during request processing. | |||
|
|
|||
| - `waiter_limit` (default: 1000): limits the number of requests waiting on admission once `admission_limit_mib` is reached. This is another dimension of memory limiting that ensures waiters are not holding onto a significant amount of memory while waiting to be processed. | |||
| - `waiter_limit` (default: 1000): `waiter_limit` is [DEPRECATED and will be replaced by `waiting_limit_mib`](https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/36074). This configuration limits the number of requests waiting on admission once `admission_limit_mib` is reached. | |||
There was a problem hiding this comment.
You are deprecating waiter_limit, but the replacement waiting_limit_mib doesn't exist yet? 🤔
There was a problem hiding this comment.
That's my intention. This feature simply doesn't work in a useful way.
I am wary of sending a large PR that replaces the features in a single change, although I have prototyped it. #36033 shows this, and I think given how little this feature does for us today, we are best off to mark the feature as deprecated while working on its replacement in a series of smaller changes.
There was a problem hiding this comment.
By the way, I am open to not merging this PR and applying the residual parts of #36033 as a single PR.
However, when I review that PR it feels like a lot to re-implement the BoundedQueue implementation (LIFO based), deprecate an old feature based on it (waiter_limit), and support a new feature (waiting_limit_mib) all at once. However, it may be less confusing.
My expectation is that no one actually has configured this not-very-useful feature. After the rest of #36033 merges, it will be OK with me for the waiter_limit configuration to cause an error at startup. In that case, we can replace waiter_limit by waiting_limit_mib in a single PR.
There was a problem hiding this comment.
I see another approach: If you believe this feature brings more harm than benefit, then deprecate it without providing a (non-existent) alternative, but just say there is no alternative or that one may be created in the future. Do you think that would make sense?
There was a problem hiding this comment.
Co-authored-by: Andrzej Stencel <[email protected]>
Description
As described in #36074, the admission::waiters_limit feature is not working well and we will replace it with a waiter limit based on pending data size.
Link to tracking issue
Part of #36074.
Testing
Note that the feature is intact, and still tested. We will change tests when the feature is replaced.
Documentation
Updated.