feat(http-client): add knob to disable connection pooling#1933
Conversation
Clippy Allow Annotation ReportComparing clippy allow annotations between branches:
Summary by Rule
Annotation Counts by File
Annotation Stats by Crate
About This ReportThis report tracks Clippy allow annotations for specific rules, showing how they've changed in this PR. Decreasing the number of these annotations generally improves code quality. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a47986f159
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let builder = http_common::client_builder(); | ||
|
|
||
| if !allow_connection_pooling { | ||
| builder = builder.pool_max_idle_per_host(0); |
There was a problem hiding this comment.
Make hyper client builder mutable before reassignment
In HyperBackend::new, builder is declared immutable and then reassigned inside the if !allow_connection_pooling branch. For builds that use the hyper backend (hyper-backend without reqwest-backend), this triggers a Rust compile error (cannot assign twice to immutable variable) and blocks the crate from compiling in that feature configuration.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Ah, this made me realize --features hyper-backend is (surprisingly I would say?) doesn't build with hyper backend. We need --no-default-features as well. It feels a bit like a foot gun, but I'm not sure what we can do, because I understand why we want to keep both features possible (for --all-features to work at least).
There was a problem hiding this comment.
I understand why we want to keep both features possible
Ideally, we wouldn't. But I don't think Rust handles mutually exclusive features well. Outside the scope of this PR but I think what we really need is a "feature matrix" for tests. --all-features is purely for tests. I'm not aware of any valid production use cases.
There was a problem hiding this comment.
Yeah, and beside --all-features, I suppose feature unification can come bite you as well. As you say I think Rust just assumes features are somehow additive in its model.
🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: e51df2b | Docs | Datadog PR Page | Give us feedback! |
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1933 +/- ##
==========================================
- Coverage 71.68% 71.68% -0.01%
==========================================
Files 434 434
Lines 70216 70254 +38
==========================================
+ Hits 50335 50359 +24
- Misses 19881 19895 +14
🚀 New features to boost your workflow:
|
ekump
left a comment
There was a problem hiding this comment.
LGTM once we have some test coverage
Co-authored-by: Edmund Kump <edmund.kump@datadoghq.com>
Add tests for libdd-http-client: - builder defaults to true - explicit false propagates Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
c7ebc58 to
e51df2b
Compare
|
/merge |
|
View all feedbacks in Devflow UI.
This pull request is not mergeable according to GitHub. Common reasons include pending required checks, missing approvals, or merge conflicts — but it could also be blocked by other repository rules or settings.
The expected merge time in
|
What does this PR do?
This PR adds a configuration knob to disable connection pooling for the HTTP client common component. This doesn't assume that the backend has specific support for connection pooling, but allows to say "if you have connection pooling, do not use it".
Motivation
This PR has been split off #1806, which builds an agent-level HTTP client on top of the basic HTTP client. To communicate with the agent, the current behavior of existing helpers in e.g. libdd-common is to disable connection pooling by default, because the agent has a low keep-alive timeout and the most common case is to flush data on a periodic schedule. There, connection pooling is detrimental as the connection will be invalidated as soon as it is re-used, see
libdatadog/libdd-common/src/http_common.rs
Lines 118 to 128 in cff7291
Additional Notes
N/A
How to test the change?
N/A