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

(otelarrowreceiver) Deprecate admission::waiter_limit #36100

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions .chloggen/otelarrow-deprecate-waiters-limit.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
# Use this changelog template to create an entry for release notes.

# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: deprecation

# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver)
component: otelarrowreceiver

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Deprecate the "admission::waiter_limit" flag for replacement by "admission::waiting_limit_mib"

# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists.
issues: [36074]

# (Optional) One or more lines of additional information to render under the primary note.
# These lines will be padded with 2 spaces and then inserted directly into the document.
# Use pipe (|) for multiline entries.
subtext:

# If your change doesn't affect end users or the exported elements of any package,
# you should instead start your pull request title with [chore] or use the "Skip Changelog" label.
# Optional: The change log or logs in which this entry should be included.
# e.g. '[user]' or '[user, api]'
# Include 'user' if the change is relevant to end users.
# Include 'api' if there is a change to a library API.
# Default: '[user]'
change_logs: [user]
4 changes: 2 additions & 2 deletions receiver/otelarrowreceiver/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

You are deprecating waiter_limit, but the replacement waiting_limit_mib doesn't exist yet? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I found a better approach.

I've sent one more PR with the replacement bounded queue in a new directory, #36140.

If we can merge that code first, then I'll get back to the original #36033 and it will be a small PR that replaces waiter_limit with waiting_limit_mib in a single change.


`request_limit_mib` and `waiter_limit` are arguments supplied to [admission.BoundedQueue](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/internal/otelarrow/admission). This custom semaphore is meant to be used within receivers to help limit memory within the collector pipeline.
`request_limit_mib` is supplied to [admission.BoundedQueue](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/internal/otelarrow/admission). This custom semaphore is meant to be used within receivers to help limit memory within the collector pipeline.

### Arrow-specific Configuration

Expand Down
18 changes: 12 additions & 6 deletions receiver/otelarrowreceiver/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,16 @@ type AdmissionConfig struct {
// for processing.
RequestLimitMiB uint64 `mapstructure:"request_limit_mib"`

// WaiterLimit is the limit on the number of waiters waiting to be processed and consumed.
// This is a dimension of memory limiting to ensure waiters are not consuming an
// unexpectedly large amount of memory in the arrow receiver.
WaiterLimit int64 `mapstructure:"waiter_limit"`
// DeprecatedWaiterLimit is deprecated. This field configures
// a limit defined in terms of the number of requests waiting
// when the request_limit_mib limit has been reached. This field
// will be replaced by a limit implemented in terms of waiting
// request size instead of waiting request count.
//
// This field will continue to function until a new field
// named `waiting_limit_mib` is introduced, at which time this
// field will have no effect.
DeprecatedWaiterLimit int64 `mapstructure:"waiter_limit"`
}

// ArrowConfig support configuring the Arrow receiver.
Expand Down Expand Up @@ -84,8 +90,8 @@ func (cfg *Config) Unmarshal(conf *confmap.Conf) error {
if cfg.Admission.RequestLimitMiB == 0 && cfg.Arrow.DeprecatedAdmissionLimitMiB != 0 {
cfg.Admission.RequestLimitMiB = cfg.Arrow.DeprecatedAdmissionLimitMiB
}
if cfg.Admission.WaiterLimit == 0 && cfg.Arrow.DeprecatedWaiterLimit != 0 {
cfg.Admission.WaiterLimit = cfg.Arrow.DeprecatedWaiterLimit
if cfg.Admission.DeprecatedWaiterLimit == 0 && cfg.Arrow.DeprecatedWaiterLimit != 0 {
cfg.Admission.DeprecatedWaiterLimit = cfg.Arrow.DeprecatedWaiterLimit
}
return nil
}
12 changes: 6 additions & 6 deletions receiver/otelarrowreceiver/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,8 @@ func TestUnmarshalConfig(t *testing.T) {
},
},
Admission: AdmissionConfig{
RequestLimitMiB: 80,
WaiterLimit: 100,
RequestLimitMiB: 80,
DeprecatedWaiterLimit: 100,
},
}, cfg)

Expand All @@ -106,8 +106,8 @@ func TestValidateDeprecatedConfig(t *testing.T) {
},
Admission: AdmissionConfig{
// cfg.Validate should now set these fields.
RequestLimitMiB: 80,
WaiterLimit: 100,
RequestLimitMiB: 80,
DeprecatedWaiterLimit: 100,
},
}, cfg)
}
Expand All @@ -133,8 +133,8 @@ func TestUnmarshalConfigUnix(t *testing.T) {
},
},
Admission: AdmissionConfig{
RequestLimitMiB: defaultRequestLimitMiB,
WaiterLimit: defaultWaiterLimit,
RequestLimitMiB: defaultRequestLimitMiB,
DeprecatedWaiterLimit: defaultWaiterLimit,
},
}, cfg)
}
Expand Down
4 changes: 2 additions & 2 deletions receiver/otelarrowreceiver/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,8 @@ func createDefaultConfig() component.Config {
},
},
Admission: AdmissionConfig{
RequestLimitMiB: defaultRequestLimitMiB,
WaiterLimit: defaultWaiterLimit,
RequestLimitMiB: defaultRequestLimitMiB,
DeprecatedWaiterLimit: defaultWaiterLimit,
},
}
}
Expand Down
2 changes: 1 addition & 1 deletion receiver/otelarrowreceiver/otelarrow.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ func newOTelArrowReceiver(cfg *Config, set receiver.Settings) (*otelArrowReceive
if err != nil {
return nil, err
}
bq := admission.NewBoundedQueue(set.TracerProvider, int64(cfg.Admission.RequestLimitMiB<<20), cfg.Admission.WaiterLimit)
bq := admission.NewBoundedQueue(set.TracerProvider, int64(cfg.Admission.RequestLimitMiB<<20), cfg.Admission.DeprecatedWaiterLimit)
r := &otelArrowReceiver{
cfg: cfg,
settings: set,
Expand Down