feat(otel-thread-ctx)!: add shared thread context - #2168
Conversation
📚 Documentation Check Results📦
|
🔒 Cargo Deny Results✅ No issues found! 📦
|
|
🎯 Code Coverage (details) 🔗 Commit SHA: 0ae31cc | Docs | View more details | 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
|
003ab9e to
4f07a3b
Compare
79d5158 to
040545c
Compare
BenchmarksComparisonCandidateCandidate benchmark detailsBaselineBaseline benchmark details |
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: 56647649f3
ℹ️ 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".
| #[cfg(all(feature = "shared-context", not(feature = "owned-context")))] | ||
| mod shared; | ||
| #[cfg(all(feature = "shared-context", not(feature = "owned-context")))] | ||
| pub use shared::*; |
There was a problem hiding this comment.
When a final binary contains a shared-mode consumer alongside any dependency that enables the default owned-context feature—such as libdd-otel-thread-ctx-ffi—Cargo unifies both features, and this condition removes SharedThreadContext even though shared-context was requested. The shared consumer then fails to compile with an unresolved import; the shared-only and all-features CI runs cannot expose this mixed dependency case. Use a design that remains composable under additive feature unification, such as separate packages for the incompatible modes.
AGENTS.md reference: AGENTS.md:L76-L81
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is on purpose. I can't prevent separate packages from being used together. It's not ideal to have "falsely additive" features like that, but is the only way I know in Rust unfortunately have this kind of static exclusion policy guaranteeing that one can never use both APIs at the same time.
| // Though our `SharedThreadContext` might actually come from a different thread now, it's | ||
| // wrapped in an `Arc` that handles drop safety by synchronizing on the reference count. The | ||
| // record is already `valid = 1` and, being shared, immutable. | ||
| let record_ptr = Arc::into_raw(self.0) as *mut ThreadContextRecord; |
There was a problem hiding this comment.
If a thread terminates while a shared context is still attached—for example, because its task panics before calling detach—the strong reference converted here is lost when the raw assembly TLS slot is destroyed, because no TLS destructor ever reconstructs the Arc. Repeated short-lived threads therefore leak one 640-byte context allocation each (and any context attached on several exiting threads retains every leaked reference); add thread-exit cleanup or an RAII attachment guard that reliably reclaims the slot reference.
AGENTS.md reference: AGENTS.md:L77-L82
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
It's a legitimate concern, but unrelated to this PR IMHO. Was already the case for the previous owned API. I guess we can use a Rust thread-local dummy variable just for cleanup. Will consider this in a follow-up, if that sounds reasonable to reviewers
yannham
changed the title
…on-Linux The shared-context nextest invocation ran unconditionally on all runners, but libdd-otel-thread-ctx's functionality (and tests) are Linux-only, so macOS/Windows runners find zero tests and nextest exits with "no tests to run" (exit code 4). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 participant
What does this PR do?
This PR adds a different shared ownership model for the OTel thread context record, in addition the the current API that only supports owned context records. Doing so, we split the interface into corresponding
ownedandsharedsubmodules.Since the two modes are incompatible (trying to attach/detach in one mode and doing the other operation in the other mode is basically UB), the two modes are feature-gated in a way such that they can't co-exist (if both features are set,
ownedwins).The FFI is left unchanged (from the point of view of an external consumer at least).
Motivation
We want to integrate the work in dd-trace-rs. The current API assumes that the context is a heap-allocated, uniquely owned piece (which is indeed required for the update API). However, given open-telemetry/opentelemetry-rust#3585, this won't work for dd-trace-rs, where the context will be an immutable shared
Arc.There are two possible solutions:
Arc-specific), but more dangerous.Additional Notes
For reviewers: I've split the existing codebase, but almost all of the new code should be located in
shared.rs.owned.rsis just part of the previous API but renamed, with just a few functions factorized inlinux.rsas they are now used from both modules. Same for the FFI: beside movingThreadContextHandledirectly there, no major changes here.How to test the change?
tests included.