refactor!: apply small timeout pooling strategy to libdd-http-client as well - #2449
Open
yannham wants to merge 6 commits into
Open
refactor!: apply small timeout pooling strategy to libdd-http-client as well#2449yannham wants to merge 6 commits into
yannham wants to merge 6 commits into
Conversation
Now that periodic clients pool connections with a small idle timeout instead of disabling pooling entirely, "new_without_connection_pooling" no longer describes what the constructor does. Rename it (and the related internal fields) to "new_periodic" to match the semantics. This is a breaking rename of the public capabilities API; all callers across the workspace are updated accordingly.
…client and libdd-agent-client Follow-up to the change that stopped entirely disabling connection pooling for periodic connections in libdd-common: - rename HttpClientBuilder::allow_connection_pooling to periodic, along with the HttpClientConfig getter and related fields - periodic clients now pool connections with a small idle timeout (5s) instead of disabling pooling entirely, in both the reqwest and hyper backends - rename AgentClientBuilder::allow_connection_pooling to periodic, now defaulting to true since the agent client is a periodic-flush writer by default
Contributor
📚 Documentation Check Results📦
|
Contributor
🔒 Cargo Deny Results📦
|
yannham
marked this pull request as ready for review
September 3, 2026 09:18
yannham
requested review from
btthomas and
typotter
and removed request for
a team
September 3, 2026 09:18
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ All CI checks and tests passed. 🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 0e5310f | Docs | View more details | Give us feedback! |
![datadog-official[bot]](https://avatars.githubusercontent.com/in/1358509?s=60&v=4){kind=small}
There was a problem hiding this comment.
Periodic clients can keep an unlimited number of idle sockets for five seconds after a concurrent request burst. The existing periodic client limits this pool to 20 sockets.
🤖 Datadog Autotest · Commit f3f9707 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| // while avoiding reusing connections the receiver may have closed. | ||
| builder | ||
| .pool_timer(hyper_util::rt::TokioTimer::new()) | ||
| .pool_idle_timeout(crate::config::PERIODIC_POOL_IDLE_TIMEOUT); |
There was a problem hiding this comment.
A concurrent request burst can keep many idle sockets open and cause short-term resource pressure.
Assertion details
- Input: A periodic client sends many concurrent requests to the same host.
- Expected:
Periodic clients must limit idle connections per host. The existing periodic client uses a limit of 20. - Actual:
The hyper and reqwest backends use the default unlimited idle-pool limit. They keep each socket until the five-second timeout ends.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest · Open Bits AI session
BenchmarksComparisonBenchmark execution time: 2026-09-03 13:16:49 Comparing candidate commit 0e5310f in PR branch Found 1 performance improvements and 0 performance regressions! Performance is the same for 154 metrics, 0 unstable metrics.
|
Contributor
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
|
There was a problem hiding this comment.
🟡 Changes recommended
There are a few documentation/wording issues introduced alongside the refactor (including misleading docs and minor grammar in public docs) that should be corrected before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR is a follow-up to #2440 that renames the “no_connection_pooling” constructor to better reflect its actual semantics and applies the same “short-lived pooling for periodic traffic” behavior across libdd-http-client and libdd-agent-client, along with updating call sites and tests throughout the workspace.
Changes:
- Renamed
new_without_connection_pooling→new_periodicacross theHttpClientCapabilitytrait, implementations, and workspace call sites. - Added/propagated a periodic-client configuration in
libdd-http-clientthat uses a small connection pool idle timeout instead of disabling pooling. - Updated
libdd-agent-clientto expose/forward the newperiodicsetting and default behavior.
File summaries
| File | Description |
|---|---|
| libdd-tracer-flare/src/lib.rs | Switches tracer flare capabilities to new_periodic(). |
| libdd-telemetry/src/worker/mod.rs | Uses new_periodic() capabilities for telemetry worker client construction. |
| libdd-remote-config/src/fetch/shared.rs | Updates remote-config tests to use new_periodic(). |
| libdd-remote-config/src/fetch/multitarget.rs | Updates multitarget fetcher tests to use new_periodic(). |
| libdd-remote-config/src/fetch/fetcher.rs | Updates fetcher tests to use new_periodic(). |
| libdd-remote-config/src/fetch/agentless/integration_tests.rs | Renames mock capability constructor to new_periodic(). |
| libdd-remote-config/src/fetch/agentless.rs | Updates agentless fetcher test wiring to use new_periodic(). |
| libdd-remote-config/examples/remote_config_fetch.rs | Updates example to use new_periodic(). |
| libdd-remote-config/examples/remote_config_agentless_bench.rs | Updates bench example to use new_periodic(). |
| libdd-http-client/src/config.rs | Replaces allow_connection_pooling with periodic config + adds periodic idle-timeout constant and tests. |
| libdd-http-client/src/backend/reqwest_backend.rs | Applies small pool idle timeout when periodic() is enabled. |
| libdd-http-client/src/backend/hyper_backend.rs | Applies small pool idle timeout (and timer) when periodic() is enabled. |
| libdd-http-client/Cargo.toml | Enables hyper-util’s tokio feature needed for TokioTimer. |
| libdd-ffe/src/telemetry/flagevaluation/sender.rs | Updates mock capability constructor to new_periodic(). |
| libdd-data-pipeline/tests/common/mock_http.rs | Updates mock capability constructor to new_periodic(). |
| libdd-data-pipeline/src/trace_exporter/mod.rs | Updates test capability wrapper to construct periodic capabilities. |
| libdd-data-pipeline-core/src/agentless/exporter.rs | Updates mock capability constructor to new_periodic(). |
| libdd-capabilities/src/http.rs | Renames trait constructor to new_periodic(). |
| libdd-capabilities-impl/src/lib.rs | Updates native capabilities to delegate to NativeHttpClient::new_periodic(). |
| libdd-capabilities-impl/src/http.rs | Renames/rewires native HTTP client periodic flag and constructor. |
| libdd-agent-client/src/builder.rs | Replaces pooling toggle with periodic toggle and forwards it to libdd-http-client. |
| datadog-sidecar/src/shm_remote_config.rs | Switches sidecar remote-config fetcher capabilities to new_periodic(). |
| datadog-sidecar/src/service/ffe_metrics_flusher.rs | Updates mock capability constructor to new_periodic(). |
| datadog-sidecar/src/service/ffe_exposures_flusher.rs | Updates mock capability constructor to new_periodic(). |
| datadog-live-debugger/src/sender.rs | Updates mock capability constructor to new_periodic(). |
Review details
- Files reviewed: 25/25 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
VianneyRuhlmann
approved these changes
Sep 4, 2026
Comment on lines
+191
to
+192
| /// streams). Set to `false` only for high-frequency continuous senders (e.g. a streaming | ||
| /// profiling exporter). |
Contributor
There was a problem hiding this comment.
Do we still need a non-peridodic case ? Since we are mostly talking to the agent, we are always limited by the agent keep-alive so all client should use the agent timeout.
Contributor
Author
There was a problem hiding this comment.
It's a good question. I think I added that to keep some compatibility with existing API/behavior in dd-trace-py (though the agent client isn't integrated yet). Note that it's set to true by default, so it's just an additional knob, but maybe it's useless... If I can't find a good reason in the next 30min or so I will get rid of it as part of this PR 🙂
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
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.
What does this PR do?
Follow-up of #2440. Rename
new_no_connection_poolingto reflect the more nuanced semantics of the function. Apply the same treatment to the equivalent functions in libdd-http-client and libdd-agent-client.Motivation
See #2440.
How to test the change?
Tested locally on the repro mentioned in #2440.