fix(networking): measure adaptive concurrency per request attempt - #26124
Draft
stigglor wants to merge 6 commits into
Draft
fix(networking): measure adaptive concurrency per request attempt#26124stigglor wants to merge 6 commits into
stigglor wants to merge 6 commits into
Conversation
The adaptive concurrency test backend only returns static latency, so nothing exercised a backend that fails and then recovers. Give the harness a simulated outage and add a fixture asserting the observed round-trip time stays near the real backend latency when a request is retried. This fixture fails today: the limiter sits outside the retry layer, so it records 19.6s against a 100ms backend.
PR vectordotdev#9026 nested the concurrency limiter outside the retry layer and widened this fixture's observed_rtt max from 0.102 to 19.7 to accommodate the result. The widened bound hid the regression rather than recording it. Against a 100ms simulated backend the suite now reports max 19.611 and mean 5.263, so the limiter is charging retry backoff to service latency.
The concurrency limiter wraps the retry layer, so the only outcome it sees for a request is the outcome of the last attempt, timed from the start of the first. A request that is deferred and then retried into a success therefore records the retry backoff as service latency and never registers as back pressure at all. Against a 100ms simulated backend the suite measured a round-trip time of 19.6s and let the concurrency limit climb to 86. Share the controller with a `MeasureAttempt` service placed on the far side of the retry layer, where one call is one attempt, and report each attempt to it. The limiter keeps its permit and in-flight accounting across the whole retry sequence. The distributed service stack already nests the limiter inside the retry layer, so it keeps timing the whole call. The three deferral fixtures that vectordotdev#9026 widened now hold the round-trip time bounds they had before that change. Their concurrency bounds stay wider than pre-vectordotdev#9026, because retry attempts no longer consume a permit and the limit has to sit higher to produce the same load on the backend.
The retry layer returns a readiness error between attempts without calling the service, so `MeasureAttempt` never sees that outcome and the concurrency limiter recorded no back pressure for it. Sinks whose `poll_ready` can fail, such as `datadog_metrics` and `statsd`, lost the signal entirely. Classify the outcome on the completion path as well. The attempt that triggered the retry has usually already flagged back pressure, but the control interval is roughly one round-trip time while the retry backoff is a second or more, so that flag is normally reset before the readiness error arrives.
The concurrency limiter and `MeasureAttempt` have to share a controller, but nothing enforced that. A stack that wired the limiter without the reporter would never feed the controller a round-trip time, so it would never establish a past average, never reach limit management, and would hold the sink at the initial concurrency of 1 with no error and no warning. Hand out both layers from `measured_pair`, which owns the controller. That drops the two `layer_fn` closures in the sink service stack and returns `Controller` and both constructors to module visibility.
The fragment was named against vectordotdev#25212, which argues the current layer ordering is desirable rather than describing this defect. Point it at vectordotdev#26123.
github-actions
Bot
added
the
domain: sinks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adaptive concurrency measures the round trip time of a whole retry sequence rather than a single
attempt, and reads back pressure from the sequence's final outcome. So a request that is deferred,
retried and eventually succeeds is recorded as one very slow success that never registered as back
pressure. The controller then steers on a latency figure that can be hundreds of times the real one,
having lost the deferral signal that should have throttled it.
This moves the measurement inside the retry layer, where one call is one attempt, and reports both
round trip time and back pressure per attempt. The layer ordering is unchanged, so the rate limit
accounting #9026 fixed is untouched and the ordering #25212 prefers is preserved.
The two halves cannot be separated. Correcting only the round trip time makes the controller worse:
back pressure signalling is already broken, so the inflated latency is currently the only thing still
throttling on deferrals. Removing it alone lets the limit run away.
References
Closes: #26123
Related: #9026, #3493, #25212, #8647
Vector configuration
No configuration change. The behaviour is exercised through the existing adaptive concurrency test
harness, which drives a simulated backend rather than a real sink.
How did you test this PR?
cargo test --lib adaptive_concurrencypasses 17 of 17.A new fixture,
tests/data/adaptive-concurrency/retried-request-rtt.yaml, fails onmasterwithobserved_rtt max = 19.611against a 100ms backend and passes here. The harness gained afailures_aftercontrol so a backend can defer for a bounded number of attempts and then recover,which is required because only an eventually successful request contributes a round trip sample.
The three fixtures #9026 widened now hold their exact pre #9026
observed_rttandaveraged_rttbounds again.
Their concurrency bounds are rebaselined higher than pre #9026, deliberately. Retry attempts no longer
hold a concurrency permit, so the limit has to sit higher to place the same load on the backend.
defers-at-high-ratesettles at 29 against 16 before #9026 and 41 onmastertoday.Is this a breaking change?
Not breaking in the API sense, but it does change runtime behaviour: deferrals now throttle the
concurrency limit where previously a retried deferral did not. That is the point of the change, and it
means a pipeline whose backend defers under load will settle at a lower concurrency than before, which
some deployments will see as reduced throughput. Worth maintainer input on whether that warrants more
than a changelog note.
Does this PR include user facing changes?
no-changeloglabel to this PR.changelog.d/26123_arc_retry_rtt.fix.md