refactor(otel-thread-ctx): backport review suggestions from Polar Signals#2016
Merged
gh-worker-dd-mergequeue-cf854d[bot] merged 1 commit intoMay 20, 2026
Conversation
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 2448ca0 | Docs | Datadog PR Page | Give us feedback! |
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. |
yannham
force-pushed
the
yannham/otel-thread-ctx-polarsignals-review
branch
from
fc7cbd2 to
6ddbfa4
Compare
yannham
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2016 +/- ##
==========================================
- Coverage 72.71% 72.65% -0.06%
==========================================
Files 453 453
Lines 74934 74939 +5
==========================================
- Hits 54486 54445 -41
- Misses 20448 20494 +46
🚀 New features to boost your workflow:
|
Switch `get_tls_slot` from returning a reference with an unconstrained lifetime to a closure-based `with_tls_slot` API, matching the pattern used by `std::thread_local!`. This prevents leaking a reference to thread-local storage to another thread where it could become dangling. Also moves the `extern "C"` declaration inside `with_tls_slot`, making the raw FFI symbol structurally inaccessible outside the safe wrapper. Backported from upstream review feedback on polarsignals/custom-labels#15. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
yannham
force-pushed
the
yannham/otel-thread-ctx-polarsignals-review
branch
from
6ddbfa4 to
2448ca0
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
|
ivoanjo
approved these changes
May 20, 2026
Member
ivoanjo
left a comment
ivoanjo
left a comment
There was a problem hiding this comment.
👍 Looks good from my side. I'll admit that as usual I'm not the most knowledgeable about the rust low-level details so probably worth having an additional rusty human do a quick pass as well :)
paullegranddc
approved these changes
May 20, 2026
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-devflow-36fce6
Bot
added
mergequeue-status: done
and removed
mergequeue-status: in_progress
labels
gh-worker-dd-mergequeue-cf854d
Bot
deleted the
yannham/otel-thread-ctx-polarsignals-review
branch
gh-worker-dd-mergequeue-cf854d Bot
pushed a commit
that referenced
this pull request
May 20, 2026
…extRecord (#2018) # What does this PR do? Follow-up to #2016. Adds compile-time `mem::offset_of!` assertions for every field of `ThreadContextRecord`, catching accidental ABI breakage (field reordering, padding changes) at compile time rather than at runtime in a profiler. The existing size-only assertion from inside `new()` is moved to a top-level `const _: () = { ... }` block next to the struct definition so it fires unconditionally regardless of code path. # Motivation `ThreadContextRecord` has a fixed ABI read by out-of-process eBPF readers. The previous single `size_of == 640` check would still pass if fields were reordered without changing the total size. Per-field offset assertions make this impossible. # Additional Notes N/A # How to test the change? N/A Co-authored-by: yann.hamdaoui <yann.hamdaoui@datadoghq.com>
Member
|
Late to the party but super excited to see the stuff from the polar' PR feeding back here :) |
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
5 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.
What does this PR do?
Backports applicable review feedback from the upstream polarsignals/custom-labels#15 PR into our
libdd-otel-thread-ctxcrate.This PR switches
get_tls_slotfrom returning a reference with an unconstrained lifetime parameter to a closure-basedwith_tls_slotAPI, following the same pattern asstd::thread_local!, for improved safety. This PR also moves theextern "C"FFI declaration inside the function body so the raw symbol is structurally inaccessible outside the safe wrapper.More changes to follow in small themed PRs.
Motivation
We upstreamed libdatadog's OTel process context and thread context to the custom-labels repo, and got good suggestions (courtesy of @umanwizard). Several suggestions apply directly to our vanilla
otel-thread-ctxcrate:get_tls_slot<'a>() -> &'a AtomicPtr<...>signature has an unconstrained lifetime equivalent to'static, but the TLS storage is not actually'static(it dies with the thread). Plus, it's forbidden to mix atomic and non-atomic accesses (although we don't do it, it's not prevented for future code changes). The closure-based API prevents this structurally, matching howstd::thread_local!solves the same problem.extern "C"block relied on a doc comment ("CAUTION: do not use directly") to prevent misuse. Nesting it inside the accessor enforces this at the language level.Additional Notes
The closure is fully monomorphized and inlined by LLVM in release mode:
attach,detach, andupdateproduce byte-identical assembly before and after this change. No performance impact.How to test the change?
Tests pass (no change in behavior)