Repository navigation
Conversation
libdatadog v45 is source compatible with v38 for the profiling, crashtracker and library-config FFI (only additions). The musl builds now import fstatfs, mkdir, openat, pipe, readlinkat, rename and statfs, and no longer import dup. The version pins in FindLibdatadog.cmake and the vcpkg port are to be updated once the corresponding libdatadog-dotnet release exists. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Replaces the managed remote configuration protocol implementation (request building, response deserialization, TUF targets, applied configs cache) with the libdatadog remote config client (libdatadog-dotnet's ddog_rc_* FFI), which talks to the agent itself and validates the file hashes. The config contents stay in native memory: RemoteConfiguration.Contents is a RemoteConfigurationContent read in place via UnmanagedMemoryStream (or a span on .NET Core), so the payloads are no longer copied onto the managed heap. The subscription API and dispatch are unchanged. The protocol DTOs are moved to the test helpers, whose mock agent responses now carry real hashes.
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. |
![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: 878f20754c
ℹ️ 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".
Comment on lines
+76
to
+77
| [DllImport(DllName, EntryPoint = "ddog_rc_client_new")] | ||
| internal static extern VoidResult ClientNew(ref RcClientConfig config, IntPtr endpoint, out IntPtr client); |
There was a problem hiding this comment.
The packaged native dependency is still pinned to libdatadog-dotnet v2.1.1 in build/cmake/FindLibdatadog.cmake and build/vcpkg_local_ports/libdatadog/vcpkg.json, but this change begins importing the new ddog_rc_* entry points without updating that dependency or its hashes. With the normal packaged monitoring home, the first client creation therefore throws EntryPointNotFoundException; RemoteConfigurationManager.Poll() catches that as a client-factory failure and permanently exits its polling loop, disabling every Remote Configuration product. Update the libdatadog pins to a release containing this FFI as part of this change.
Useful? React with 👍 / 👎.
![datadog-prod-us1-4[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
The pinned native dependency lacks the remote-config exports, causing client creation to fail and polling to stop permanently. The new polling lifecycle also introduces application-scheduler blocking and races during manager replacement and shutdown.
🤖 Bits Code Review · Commit 878f207
| var nativeEndpoint = CreateEndpoint(endpoint); | ||
| try | ||
| { | ||
| NativeInterop.RemoteConfig.ClientNew(ref config, nativeEndpoint, out var client).ThrowIfError(); |
There was a problem hiding this comment.
Default builds still download libdatadog-dotnet 2.1.1 through FindLibdatadog.cmake and the Windows vcpkg port, but that release lacks the new ddog_rc_* exports. Client creation throws EntryPointNotFoundException, after which RemoteConfigurationManager permanently exits its polling loop. Remote configuration therefore stops working with the shipped dependency. Update the native release pins and platform checksums alongside this migration.
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
…onfigurationManager.cs Co-authored-by: datadog-prod-us1-4[bot] <266080418+datadog-prod-us1-4[bot]@users.noreply.github.com>
BenchmarksBenchmark execution time: 2026-10-14 19:48:33 Comparing candidate commit 68b7f28 in PR branch Found 0 performance improvements and 11 performance regressions! Performance is the same for 61 metrics, 0 unstable metrics, 75 known flaky benchmarks, 51 flaky benchmarks without significant changes.
|
The remote configuration managers of replaced tracers (Tracer.Configure) created a new client, whose change tracking started from zero: configs which no longer applied were never removed. Now RcmSubscriptionManager owns the client and rebinds it for other targets, and serializes the polls. The old manager is disposed before the new one starts, and the client is only cancelled on shutdown.
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (9406) and master. ✅ No regressions detected |
| } | ||
|
|
||
| private async Task<GetRcmResponse?> TrySendRequest(RcmClientTracer rcmClientTracer, Func<GetRcmRequest, Task<GetRcmResponse?>> func) | ||
| public void Cancel() => Volatile.Read(ref _client)?.Cancel(); |
Contributor
There was a problem hiding this comment.
🤖 Comment from Codex working with leoromanovsky:
[P2] Preserve shutdown cancellation while the first client is being created.
If shutdown calls Cancel() while clientFactory is running, _client is still null and the cancellation is lost. After the factory returns, Poll publishes the client and starts an uncancelled fetch. Keep a terminal cancellation flag and serialize client publication with cancellation under a separate short lock. Keep factory execution and fetching outside that lock.
Regression evidence and candidate fix
A gated factory reproduces the missed cancellation with subscriptions present and absent: both tests fail on this head. A candidate using a separate publication/cancellation lock passes all 15 focused tests, including cancellation during a pending fetch and repeated concurrent cancellation. Inverting the candidate guard causes five failures. These are managed tests with a fake client; they do not establish a process-exit delay or exercise native cancellation.
The candidate adds _cancellationLock and _isCancelled. After the factory returns, it publishes _client under that lock and calls client.Cancel() if shutdown has already occurred. Cancel() takes only that lock, sets the flag once, and cancels any published client. It never waits for _pollLock.
leoromanovsky
approved these changes
Oct 28, 2026
This branch has not been deployed
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.
Replaces the managed remote configuration protocol implementation (request building, response deserialization, TUF targets, applied configs cache) with the libdatadog remote config client (libdatadog-dotnet's ddog_rc_* FFI), which talks to the agent itself and validates the file hashes.
The config contents stay in native memory: RemoteConfiguration.Contents is a RemoteConfigurationContent read in place via UnmanagedMemoryStream, so the payloads are no longer copied onto the managed heap.
The subscription API and dispatch are unchanged. The protocol DTOs are moved to the test helpers, whose mock agent responses now carry real hashes.
It also not yet implements agentless remote config. This however is a rather minor change - mostly wiring up configuration at that point, and forwarding that to the native code.
This requires DataDog/libdatadog-dotnet#60. (The pipeline is broken as longas that isn't merged.)