fix(remote-config): notification of multi-processing and runtime deduplication#2082
Merged
Merged
Conversation
|
Contributor
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. |
![chatgpt-codex-connector[bot]](https://avatars.github.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: a3759f79d9
ℹ️ 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
+487
to
+488
| instance_id.session_id.clone(), | ||
| instance_id.runtime_id.clone(), |
There was a problem hiding this comment.
Passing instance_id.runtime_id here makes MultiTargetFetcher::add_runtime key its runtimes map only by the runtime id. When two forked/multiprocess sidecar sessions report the same runtime id for the same target, the second registration takes the occupied-target path and only increments a count, so its NotifyTarget/session id is never stored; later fetch notifications iterate that single runtime entry and only signal one process. This regresses the multi-process case that previously keyed registrations by the unique session id, so the fetcher needs to distinguish at least (session_id, runtime_id) while still deduplicating queues within one runtime.
Useful? React with 👍 / 👎.
bwoebi
force-pushed
the
bob/fix-rc-race
branch
2 times, most recently
from
994d3d9 to
38d448a
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2082 +/- ##
==========================================
- Coverage 73.55% 73.42% -0.14%
==========================================
Files 475 475
Lines 78799 78919 +120
==========================================
- Hits 57964 57948 -16
- Misses 20835 20971 +136
🚀 New features to boost your workflow:
|
We want the session_id as runtime_id key, but still deduplicate *processes* according to their runtime_id, otherwise notifcation will not reach all targeted processes. Additionally submit the remote_config_generation, so that processes which start up and have read the present configuration, get notified when a new configuration has intermittently been read. (race condition) Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
bwoebi
changed the title
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
|
morrisonlevi
approved these changes
Jun 4, 2026
morrisonlevi
left a comment
morrisonlevi
left a comment
Contributor
There was a problem hiding this comment.
I spent some time with Codex trying to understand it. I looks okay to me based on my weak understanding. I think Session ID == Runtime ID for runtimes which correctly regenerated the ID on fork, and the Root/Parent Session ID are related to tracking the parent/child relationships. Based on the idea that Session ID == Runtime ID, this looks fine. I will certainly not claim to be an expert here!
Contributor
Author
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: queued
mergequeue-status: in_progress
and removed
mergequeue-status: queued
labels
gh-worker-dd-mergequeue-cf854d
Bot
merged commit a76412c
into
main
98 of 101 checks passed
gh-worker-dd-mergequeue-cf854d Bot
pushed a commit
that referenced
this pull request
Jun 19, 2026
# Release v36.0.0 This release bumps the workspace version `35.0.0 → 36.0.0`. Below are the commits in `v35.0.0..HEAD` that directly modify the C ABI surface consumed by downstream SDKs. ## Major | Commit | FFI crate(s) affected | |---|---| | `refactor(shm)!: Extract one_way_shared_memory to IPC and prepare libdd-remote-config for python` (#2121) | `datadog-sidecar-ffi` | | `refactor(span)!: use VecMap for meta, metrics and meta_struct for v04 spans` (#2043) | `datadog-sidecar-ffi`, `libdd-data-pipeline-ffi` | | `feat(data-pipeline)!: add fork safety hooks and cancellation token for trace exporter FFI` (#2051) | `libdd-data-pipeline-ffi`, `libdd-profiling-ffi` | ## Minor | Commit | FFI crate(s) affected | |---|---| | `feat(sidecar): add retry interval configuration` (#2106) | `datadog-sidecar-ffi` | | `feat(profiling): Add setting to omit local root span id from serialized pprof` (#2104) | `libdd-profiling-ffi` | | `feat(live-debugger): agentless intake forwarding` (#2075) | `datadog-live-debugger-ffi` | | `feat(sidecar): forward FFE exposures to EVP proxy` (#2026) | `datadog-sidecar-ffi` | | `feat(sidecar): forward FFE evaluation metrics to OTLP intake` (#2052) | `datadog-sidecar-ffi` | | `feat: cross-language LTO to inline C TLS shim into Rust FFI` (#1982) | `libdd-otel-thread-ctx-ffi` (build-only: build.rs / scripts / README) | ## Patch | Commit | FFI crate(s) affected | |---|---| | `fix(ffe): honor shared fixture result metadata` (#2109) | `datadog-ffe-ffi` | | `fix(sidecar): Dedup VecMap spans before serialization` (#2107) | `datadog-sidecar-ffi` | | `fix(crashtracking): authenticate peer granted socket ptrace access` (#2098) | `libdd-crashtracker-ffi`, `datadog-sidecar-ffi` | | `fix(remote-config): notification of multi-processing and runtime deduplication` (#2082) | `datadog-sidecar-ffi` | | `fix(sidecar): configure OTLP endpoint for FFE metrics` (#2076) | `datadog-sidecar-ffi` | | `refactor(datadog-remote-config): rename as libdd-remote-config` (#2085) | `datadog-sidecar-ffi` (incl. `cbindgen.toml`) | Co-authored-by: julio.gonzalez <julio.gonzalez@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
3 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.
We want the session_id as runtime_id key, but still deduplicate processes according to their runtime_id, otherwise notifcation will not reach all targeted processes.
Additionally submit the remote_config_generation, so that processes which start up and have read the present configuration, get notified when a new configuration has intermittently been read. (race condition)