Draft
Conversation
📚 Documentation Check Results📦
|
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. |
🔒 Cargo Deny Results✅ No issues found! 📦
|
BenchmarksComparisonBenchmark execution time: 2026-02-23 15:42:27 Comparing candidate commit 18eef42 in PR branch Found 5 performance improvements and 8 performance regressions! Performance is the same for 44 metrics, 2 unstable metrics. scenario:credit_card/is_card_number/37828224631000521389798
scenario:credit_card/is_card_number/x371413321323331
scenario:credit_card/is_card_number_no_luhn/ 378282246310005
scenario:credit_card/is_card_number_no_luhn/378282246310005
scenario:credit_card/is_card_number_no_luhn/37828224631000521389798
scenario:credit_card/is_card_number_no_luhn/x371413321323331
scenario:tags/replace_trace_tags
CandidateCandidate benchmark detailsGroup 1
Group 2
Group 3
Group 4
Group 5
Group 6
Group 7
Group 8
Group 9
Group 10
Group 11
Group 12
Group 13
Group 14
Group 15
Group 16
Group 17
Group 18
Group 19
BaselineOmitted due to size. |
yannham
force-pushed
the
yannham/process-context-sharing
branch
from
05a3bcd to
6598cc3
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1585 +/- ##
==========================================
+ Coverage 71.22% 71.30% +0.08%
==========================================
Files 423 424 +1
Lines 62130 62635 +505
==========================================
+ Hits 44253 44665 +412
- Misses 17877 17970 +93
🚀 New features to boost your workflow:
|
yannham
force-pushed
the
yannham/process-context-sharing
branch
from
6598cc3 to
43208fe
Compare
libdd-library-config/Cargo.toml
Outdated
| rand = "0.8.3" | ||
| rmp = "0.8.14" | ||
| rmp-serde = "1.3.0" | ||
| rustix = { version = "1.1.3", features = ["param", "mm", "process"] } |
Contributor
There was a problem hiding this comment.
why rustix rather than the libc crate?
Author
There was a problem hiding this comment.
IMHO it's higher-level and a nicer to use than libc (for example memfd_create returns a Result and an RAII file descriptor that is automatically closed upon drop, while the one from libc returns a c_int, etc.). I saw a bunch of occurrences already in Cargo.lock and thought it was pulled already anyway. But upon scrutiny it seems that the 1.1.3 major version bucket is mostly used by tempfile, which is a dev dependency most of the time? So maybe this is actually pulling some additional stuff.
There are a bunch of other dependencies that use the 0.38 version of rustix, so I can also downgrade to this one. But it's just a slight QoL improvement; happy to switch to libc if you think it's better for whatever reason.
Member
There was a problem hiding this comment.
Might be worth giving a quick check on the size of the resulting builds (I thought we had a github action that posted that but I'm not seeing it?).
In particular, we don't have a specific set target size that we need to stay under, but for many reasons we often have to ship variants so 1 MiB extra does add up if we need to ship e.g. N architectures and M versions.
Having said that, I feel like I'd seen rustix before but hadn't paid a lot of attention to it.
Looking at https://crates.io/crates/rustix it lists that it can work even without libc and that's amazing! If we could drop libc as a dependency from libdatadog would be super-unlock, since one situation where we end up needing to repeat builds is sometimes needing to have builds for both for glibc AND musl.
Contributor
There was a problem hiding this comment.
If we could drop libc as a dependency from libdatadog would be super-unlock, since one situation where we end up needing to repeat builds is sometimes needing to have builds for both for glibc AND musl.
That's not really possible, because std uses libc
Member
There was a problem hiding this comment.
That's not really possible, because std uses libc
Yeah, I know. Rust is very disapponting in this :P
Add an explicit rustix dependency (which was already pulled as a transitive dependency). Prep work for process context sharing.
yannham
force-pushed
the
yannham/process-context-sharing
branch
from
ed9af88 to
fedbeb2
Compare
yannham
force-pushed
the
yannham/process-context-sharing
branch
from
fedbeb2 to
74d2641
Compare
ivoanjo
reviewed
Feb 20, 2026
Member
ivoanjo
left a comment
ivoanjo
left a comment
There was a problem hiding this comment.
Did a pass on it! Sorry from my part if there is a bit of a confusion with older versions of the spec being implemented, I'll make sure to keep a close eye on the libdatadog impl so it doesn't fall behind while things are still sometimes moving in the spec.
Comment on lines
+61
to
+62
| /// We use `signature` as a release notification for publication, and `published_at_ns` for | ||
| /// updates. Ideally, those should be two `AtomicU64`, but this isn't compatible with |
Member
There was a problem hiding this comment.
We discussed this in yesterday's OTel Profiling SIG meeting and I'll go ahead and simplify this soon so that published_at_ns is the notification for both creation and updates. (I'll drop a note on the channel when I do so, so we can update all the impls as needed)
Comment on lines
410
to
418
| /// Checks if a mapping line refers to the OTEL_CTX mapping by name | ||
| /// | ||
| /// Handles both anonymous naming (`[anon:OTEL_CTX]`) and memfd naming | ||
| /// (`/memfd:OTEL_CTX` which may have ` (deleted)` suffix). | ||
| fn is_named_otel_mapping(line: &str) -> bool { | ||
| let trimmed = line.trim_end(); | ||
| trimmed.ends_with("[anon:OTEL_CTX]") | ||
| || trimmed.contains("/memfd:OTEL_CTX") | ||
| || trimmed.contains("memfd:OTEL_CTX") |
Member
There was a problem hiding this comment.
A few notes:
- Wait, did you see any variant with
memfdthat did not start with/memfd? - Rather than mixing ends_with and contains, I suggest always using start_with
- There was a slight oversight on my part and I forgot to list a third variant here -- I added it to the spec recently; you can see
[anon_shmem:OTEL_CTX]as well (the spec explains when that can happen)
Author
There was a problem hiding this comment.
To be honest, I took this directly from Scott's prototype. I'll clean it up a bit following your remarks (we're looking at whole lines of /proc/self/map, which is why this doesn't use start_with - the name is the 6th column. But even for a test, I agree that it should match the spec better and be a bit more robust).
Comment on lines
435
to
437
| // The atomic alignment constraints are checked during publication. | ||
| let signature = unsafe { AtomicU64::from_ptr(ptr).load(Ordering::Acquire) }; | ||
| &signature.to_ne_bytes() == super::SIGNATURE |
Member
There was a problem hiding this comment.
Should probably use SeqCst as well (to match my suggestion above)
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
|
The wrong mapping name was used for discovery. Co-authored-by: Scott Gerring <scottgerring@users.noreply.github.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
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?
This PR implements the publication protocol of the process context sharing proposal.
This is intended as a minimally viable starting point. Next steps are kept for follow-up PRs, which could include for example:
Motivation
This feature allows a process to expose data to an external process, typically an eBPF profiler. Please refer to the OTEP linked above for a detailed motivation.
Additional Notes
Some notes on dependencies:
rustixfor that since it's already pulled as a transitive dependency (with the same major version bucket), and is nicely higher-level thanlibc.MemFdwrapper crate used (e.g herelibdatadog/libdd-library-config/src/tracer_metadata.rs
Line 54 in 0bd90fd
There are a number of design choices or assumptions that might be interesting to discuss further:
/proc/<pid>/mapsand syscalls to do so, so the concurrency model is a bit unclear. We basically settled on the mental model being that we use atomics as if the reader was another thread of the same program, which sounds like the best we can do and should prevent re-ordering at least on the writer side (using OS-level sync is another solution, but was deemed too costly for the upcoming thread-level context).Pin<Box<[u8]>>?), and maybe offer the option - or do it automatically, depending on the size - of moving the payload directly after the header, as allowed by the spec. This is left for future work.How to test the change?
TODO