Draft
Conversation
📚 Documentation Check Results✅ No documentation warnings found! 📦
|
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-20 16:55:55 Comparing candidate commit 74d2641 in PR branch Found 4 performance improvements and 9 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.20% -0.03%
==========================================
Files 423 424 +1
Lines 62130 62376 +246
==========================================
+ Hits 44253 44415 +162
- Misses 17877 17961 +84
🚀 New features to boost your workflow:
|
yannham
force-pushed
the
yannham/process-context-sharing
branch
from
6598cc3 to
43208fe
Compare
| 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.
| // Copyright 2021-Present Datadog, Inc. https://www.datadoghq.com/ | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| //! Implementation of the publisher part of the [process sharing protocol](https://github.com/open-telemetry/opentelemetry-specification/pull/4719) |
Member
There was a problem hiding this comment.
I know we already use a lot of name variants internally/informally to refer to this, but if possible, I'd like to use "OTel/OpenTelemetry Process Context" when referring to this in code (so it's easy, to e.g. google it).
Comment on lines
+12
to
+14
| //! Still, we typically want to avoid the compiler and the hardware to re-order the write to the | ||
| //! signature (which should be last according to the specification) with the writes to other fields | ||
| //! of the header. |
Member
There was a problem hiding this comment.
Minor: The rest of the comment I think makes sense at the top of the file, but this part I think should live nearer to the actual implementation bits?
Comment on lines
+21
to
+24
| /// Signature bytes for identifying process context mappings | ||
| pub const SIGNATURE: &[u8; 8] = b"OTEL_CTX"; | ||
| /// The discoverable name of the memory mapping. | ||
| pub const MAPPING_NAME: &str = "OTEL_CTX"; |
Member
There was a problem hiding this comment.
Minor: These are always expected to match so they can maybe be squeezed into one?
Comment on lines
+91
to
+92
| /// The file descriptor, if the mapping was successfully created from `memfd`. | ||
| fd: Option<OwnedFd>, |
Member
There was a problem hiding this comment.
You actually don't need to keep the fd around -- It's expected to close it after the mmap succeeds, that works fine (and one less thing to track ;D )
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)
Comment on lines
+421
to
+424
| /// Checks if a mapping line refers to the OTEL_CTX memfd mapping | ||
| fn is_memfd_otel_mapping(line: &str) -> bool { | ||
| line.contains("/memfd:OTEL_CTX") | ||
| } |
Member
There was a problem hiding this comment.
Minor: is_named_otel_mapping should be enough for all 3 variants -- no need to separate them out, I think?
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)
Comment on lines
+448
to
+450
| if !is_otel_mapping_candidate(&line) { | ||
| continue; | ||
| } |
Member
There was a problem hiding this comment.
This is no longer needed (also remnant of older version of spec)
Comment on lines
+464
to
+469
| // For unnamed mappings, verify by reading the signature | ||
| if let Some(addr) = parse_mapping_start(&line) { | ||
| if verify_signature_at(addr) { | ||
| return Ok(addr); | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
You can assume there's always a name now ;)
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
4 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