-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Add ARC support #14144
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
base: main
Are you sure you want to change the base?
Add ARC support #14144
Conversation
|
|
Added the test results and generated metrics |
| } | ||
| } | ||
|
|
||
| // Acquire obtains a permit unless ARC is disabled or the context is cancelled. |
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.
My main feedback is this: we have here a large piece of code, it looks good, but it is very specific and opinionated. I would prefer to see this become an extension implementation. This PR is great, I just think it belongs in //extensions.
See #13902 explaining how extension APIs are added, it would begin with //extensions/extensioncontroller APIs, for example, then the bulk of the code would land in //extensions/arccontrollerextension (could be in contrib).
Could we invent an extension point to let you insert your ARC controller into the send pipeline for all exporters? Then, exporterhelper would only need to load and insert the extensions into the pipeline.
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.
Hi @jmacd, thanks for the detailed feedback on moving this to an extension.
My original thinking was that putting ARC in exporterhelper would make it easier to enable by default for any exporter that is using sending queue after ARC is battle-tested.
This isn't just a theoretical feature for us; it's based on our own painful experience of overwhelming and taking down downstream systems. We operate at a large scale, using both Vector and OTel to ingest hundreds of TBs of telemetry data, and we've seen the critical need for this kind of adaptive backpressure.
This experience is what led us to look for proven solutions. The Netflix tech blog (https://netflixtechblog.medium.com/performance-under-load-3e6fa9a60581) inspired Vector's implementation (https://vector.dev/blog/adaptive-request-concurrency/). We've seen the wins from this model firsthand in our own Vector deployments, and it's a key reason they enable it by default. My goal is to bring that same proven stability to OTel.
That said, I'm completely fine with moving this to an extension if the team prefers that. I did talk with @atoulme at KubeCon about how the sending_queue manages its goroutine pool, and I believe this implementation fits in correctly.
Since it's a big architectural choice, I will wait for feedback from the other exporterhelper code owners. @bogdandrutu and @dmitryax, I'd strongly request you read the two blog posts above if you have a moment. They provide the full context for why this feature is so important and why I'm proposing it.
I'll hold off on the refactoring until you've all had a chance to weigh in. Please let me know which direction you'd like me to take. I'm happy to go whichever way the project prefers.
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.
Here are the gradient-based ARC implementations from Netflix and Envoy.
7a98d38 to
b1c96fb
Compare
Description
This PR introduces an Adaptive Request Concurrency (ARC) controller to the
exporterhelper.When enabled via the new
sending_queue.arc.enabledflag, this controller dynamically manages the number of concurrent export requests, effectively overriding the staticnum_consumerssetting. It adjusts the concurrency limit based on observed RTT (Round-Trip Time) and backpressure signals (e.g., HTTP 429/503, gRPCResourceExhausted/Unavailable).The controller follows an AIMD (Additive Increase, Multiplicative Decrease) pattern to find the optimal concurrency limit, maximizing throughput during healthy operation and automatically backing off upon detecting export failures or RTT spikes.
This feature is disabled by default and introduces no behavior change unless explicitly enabled. It also adds a new set of
otelcol_exporter_arc_*metrics (detailed in the documentation) for observing its behavior.Link to tracking issue
Fixes #14080
Testing
internal/arc/controller_test.go, covering additive increase, multiplicative decrease (TestAdjustIncreaseAndDecrease), and the cold-start backoff heuristic (TestEarlyBackoffOnColdStart).shrinkSem(a custom shrinkable semaphore) to validate its concurrency, prioritization, and shutdown safety.TestController_Shutdown_UnblocksWaiters) to ensure that any goroutines blocked onAcquireare correctly unblocked with a shutdown error, preventing collector hangs.internal/queue_sender_test.go(TestQueueSender_ArcAcquireWaitMetric) that validates the end-to-end flow. It confirms that when the limit is reached, new requests block onAcquireand theexporter_arc_acquire_wait_msmetric records the wait time.internal/experr/back_pressure.goutility to verify its detection logic.Documentation
exporterhelper/README.mdto include the newsending_queue.arcblock with all its configuration options.exporterhelper/metadata.yamlto define all newotelcol_exporter_arc_*metrics, which are in turn reflected in the generateddocumentation.md.