fix(remote-config)!: Expose HttpClientCapability in remote config#2252
Open
bwoebi wants to merge 2 commits into
Open
fix(remote-config)!: Expose HttpClientCapability in remote config#2252bwoebi wants to merge 2 commits into
bwoebi wants to merge 2 commits into
Conversation
Contributor
📚 Documentation Check Results📦
|
bwoebi
changed the title
Contributor
🔒 Cargo Deny Results📦
|
Contributor
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 3d29427 | Docs | Datadog PR Page | Give us feedback! |
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
More details
The body_bytes = response.into_body() change is correct: HttpClientCapability::request returns http::Response<bytes::Bytes> with the body fully buffered inside NativeHttpClient, so no further .collect().await is needed. All 24 existing unit tests pass, covering the full fetch pipeline through a local mock HTTP server. The timeout fix is legitimate — the body collection is now inside the timed future, closing a real hang risk in the old code.
📊 Validated against 24 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit 36072ea · What is Autotest? · Any feedback? Reach out in #autotest
![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: 36072ea7cf
ℹ️ 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".
| impl<S> ConfigFetcherState<S> { | ||
| impl<S> ConfigFetcherState<S, NativeHttpClient> { | ||
| pub fn new(invariants: ConfigInvariants) -> Self { | ||
| Self::with_client(invariants, NativeHttpClient::new_client()) |
There was a problem hiding this comment.
With the default constructor, every SingleFetcher/SharedFetcher now stores one NativeHttpClient for the whole polling loop. NativeHttpClient caches a new_default_client() in a OnceLock, and new_default_client() keeps idle connections, while http_common::new_client_periodic() explicitly disables pooling because the agent's low keep-alive can close the pipe on the next periodic request. For default RC polling, this changes the previous per-request client behavior into pooled reuse and can make subsequent polls fail intermittently; please use a periodic/no-idle native client for the default.
Useful? React with 👍 / 👎.
Comment on lines
130
to
133
| impl<S: FileStorage> SingleChangesFetcher<S, NativeHttpClient> | ||
| where | ||
| S::StoredFile: FilePath, | ||
| { |
There was a problem hiding this comment.
Although the struct is now generic over C, this only public constructor remains on SingleChangesFetcher<S, NativeHttpClient>, and the private changes field prevents downstream code from constructing SingleChangesFetcher<S, C> around SingleFetcher::with_client. Callers that use the change-tracking API rather than raw fetch_once still cannot inject a custom HttpClientCapability; add a with_client constructor mirroring SingleFetcher::with_client.
Useful? React with 👍 / 👎.
| client = [ | ||
| "libdd-trace-protobuf", | ||
| "libdd-capabilities", | ||
| "libdd-capabilities-impl", |
There was a problem hiding this comment.
Enabling the client feature now always enables libdd-capabilities-impl, even when a downstream only wants to instantiate ConfigFetcherState::with_client with its own HttpClientCapability. I checked libdd-capabilities-impl/src/http.rs, and that implementation imports native-only pieces such as libdd_common::connector::Connector, so custom-client builds for non-native targets still have to compile the native backend and fail before they can supply their own client. Please keep the native implementation behind the default/native constructors or a target-specific dependency instead of making it mandatory for client.
Useful? React with 👍 / 👎.
As a side-effect, this fixes the missing timeout on the remote config body awaiting. Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
bwoebi
force-pushed
the
bob/rc-native-client
branch
from
July 17, 2026 17:59
36072ea to
39d4b97
Compare
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
|
BenchmarksComparisonBenchmark execution time: 2026-07-20 14:03:32 Comparing candidate commit 3d29427 in PR branch Found 12 performance improvements and 4 performance regressions! Performance is the same for 126 metrics, 0 unstable metrics.
|
yannham
approved these changes
Jul 20, 2026
| /// Intended for clients that issue requests on a fixed interval (e.g. remote | ||
| /// config polling): the agent's low keep-alive setting can close an idle | ||
| /// connection between polls, which turns a pooled/reused connection into | ||
| /// intermittent request failures. |
Contributor
There was a problem hiding this comment.
I did something similar in libdd-http-client (disable pooling) because of similar comments in Endpoint, but after discussing with agent people, it seems there's no basis for this claim as of today. Might be worth checking if this whole periodic/connection pooling disabling business is really needed at all today.
Contributor
Author
There was a problem hiding this comment.
I don't know, but feels safer to me?
| /// config polling): the agent's low keep-alive setting can close an idle | ||
| /// connection between polls, which turns a pooled/reused connection into | ||
| /// intermittent request failures. | ||
| pub fn new_periodic_client() -> Self { |
Contributor
There was a problem hiding this comment.
Nit: I think calling it periodic is a bit too specific. At the level of an HTTP client, we don't really care why the caller disable connection pooling. IMHO we should be "lower" level in the description: what we do is that we disable connection pooling.
Contributor
Author
There was a problem hiding this comment.
So, what exactly would you propose as name then?
Contributor
There was a problem hiding this comment.
something like disable_connection_pooling if you go for a builder API, or new_no_connection_pooling or something better in that style, which is more factual IMHO
| let client = self | ||
| .client | ||
| .get_or_init(|| { | ||
| if periodic { |
Contributor
There was a problem hiding this comment.
Nit: I wonder if only having new() for the client, and then with_periodic(self) -> Self or something builder-like wouldn't make the interface a bit lighter.
Contributor
Author
There was a problem hiding this comment.
... and the callers a bit heavier. I don't quite like this.
Contributor
There was a problem hiding this comment.
I don't think there's a huge difference between new().with_periodic(true) and new_periodic() (or whatever variation depending on the precise names you use). But it's a matter of taste and not very important, so do you as you feel is best 🙂
| S::StoredFile: FilePath, | ||
| { | ||
| pub fn new(sink: S, target: Target, runtime_id: String, options: ConfigOptions) -> Self { | ||
| pub fn with_client( |
Contributor
There was a problem hiding this comment.
Nit: I would call that new_with_client. with_client sounds like it should have the signature with_client(self, client: C) -> Self
Contributor
Author
There was a problem hiding this comment.
Agree, this went through some refactor, putting new back.
bwoebi
changed the title
Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
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
2 participants
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.
As a side-effect, this fixes the missing timeout on the remote config body awaiting.