feat(otel-thread-ctx)!: publish W3C trace flags - #2298
Conversation
Store the W3C trace-flags byte at offset 25 and pass it through the Rust and C APIs during construction and in-place updates. BREAKING CHANGE: ThreadContext::new, ThreadContext::update, ddog_otel_thread_ctx_new, and ddog_otel_thread_ctx_update now require a trace_flags argument.
BenchmarksComparisonCandidateCandidate benchmark detailsBaselineBaseline benchmark details |
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: afda47c | Docs | Datadog PR Page | 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
|
ivoanjo
left a comment
ivoanjo
left a comment
There was a problem hiding this comment.
👍 LGTM thanks for the addition
(For extra context -- the W3C trace flags field was a late addition to the spec, we had one leftover reserved byte and it was decided that we'd repurpose it for carrying those flags. The eBPF Profiler doesn't yet do anything with this data, but in the future it might...)
| let root_span_id = [3u8; 8]; | ||
|
|
||
| ThreadContext::new(trace_id, span_id, root_span_id, &[]).attach(); | ||
| ThreadContext::new(trace_id, span_id, 0, root_span_id, &[]).attach(); |
There was a problem hiding this comment.
Nit: should 0 be a named constant like NO_FLAGS or something? To make the default value more discoverable. Another possibility, if those flags are rarely set, is to keep the original interface but have a with_flags API. Though it doesn't really work for updates as well...
There was a problem hiding this comment.
Ergonomic improvements e.g. NO_FLAGS are possible but kind of limited because these things go over into FFI as well, and then you end up with having to name something like DDOG_OTEL_THREAD_CTX_NO_TRACE_FLAGS or something similarly horrible.
But if you mean for the sake of readability in tests, sure, NO_FLAGS is fine.
There was a problem hiding this comment.
Right, agreed on the FFI part - and I suppose it's so idiomatic in C that it probably doesn't deserve a constant on the C side... Yeah, I was thinking test but also the few potential pure Rust API consumers, like dd-trace-rs, or tracers that re-use libdatadog à la carte
|
/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
3 participants
What does this PR do?
Store the W3C trace-flags byte at offset 25 and pass it through the Rust and C APIs during construction and in-place updates.
BREAKING CHANGES: these functions now require a trace_flags argument:
ThreadContext::newThreadContext::updateddog_otel_thread_ctx_newddog_otel_thread_ctx_updateMotivation
The spec changed after the context work was initially done in libdatadog.
Additional Notes
PHP will use this in an upstream PR.
How to test the change?
Update your APIs for the above functions to include the trace flags, then proceed as normal.