feat(grpc connection management): add jitter controls to grpc max age#25924
feat(grpc connection management): add jitter controls to grpc max age#25924hiporox wants to merge 3 commits into
Conversation
github-actions
Bot
added
the
domain: sources
|
Thank you for your contribution! Before we can merge this PR, please sign our Contributor License Agreement. To sign, copy and post the phrase below as a new comment on this PR.
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8444db8552
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// maximum. The default of zero preserves the configured age exactly. | ||
| #[serde(default)] | ||
| #[configurable(validation(range(min = 0.0, max = 1.0)))] | ||
| pub max_connection_age_jitter_factor: f64, |
There was a problem hiding this comment.
This adds a user-facing #[configurable_component] option, but the generated source reference files were not updated: checked website/cue/reference/components/sources/generated/vector.cue and website/cue/reference/components/sources/generated/opentelemetry.cue, and both gRPC keepalive option maps still list only max_connection_age_secs and max_connection_age_grace_secs. As a result, the website/schema-based docs will omit max_connection_age_jitter_factor until the generated docs are committed.
AGENTS.md reference: AGENTS.md:L212-L212
Useful? React with 👍 / 👎.
github-actions
Bot
added
docs review on hold
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55a405a8c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| fn jittered_duration(duration: Duration, jitter_factor: f64, jitter: f64) -> Duration { | ||
| duration.mul_f64(1.0 + jitter_factor.clamp(0.0, 1.0) * jitter.clamp(-1.0, 1.0)) |
There was a problem hiding this comment.
When max_connection_age_secs is configured near u64::MAX and max_connection_age_jitter_factor allows a positive jitter, this multiplication can overflow Duration::mul_f64 and panic while accepting a connection, taking down the gRPC source instead of saturating like the grace addition does just above. Consider capping or using checked/saturating arithmetic before applying jitter.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed this edge case and the under flow edge case resulting in a zero duration connection age
1 participant
Summary
PR #19457 added max connection age. This adds the ability to add jitter to those connections to stagger the connection resets better. Jitter is provided as a percentage, modifying the max connection age +/- that percentage.
How did you test this PR?
Added tests to the parser and to make sure we process the bounds correctly. We also can see that existing max connection age tests still pass
Change Type
Is this a breaking change?
Does this PR include user facing changes?
no-changeloglabel to this PR.References
#19457
Notes
@vectordotdev/vectorto reach out to us regarding this PR.pre-pushhook, please see this template.make fmtmake check-clippy(if there are failures it's possible some of them can be fixed withmake clippy-fix)make testgit merge origin masterandgit push.Cargo.lock), pleaserun
make build-licensesto regenerate the license inventory and commit the changes (if any). More details on the dd-rust-license-tool.