Repository navigation
Conversation
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. |
BenchmarksComparisonBenchmark execution time: 2026-10-07 09:30:20 Comparing candidate commit b8d37f1 in PR branch Found 1 performance improvements and 0 performance regressions! Performance is the same for 161 metrics, 0 unstable metrics.
|
2db36ac to
b5b35f0
Compare
📚 Documentation Check Results📦
|
🔒 Cargo Deny Results📦
|
|
✅ All CI checks and tests passed. 🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: b8d37f1 | Docs | View more details | Give us feedback! |
11fc753 to
91d5dae
Compare
yannham
left a comment
There was a problem hiding this comment.
Maybe @ivoanjo has an opinion on this, but I wonder if we shouldn't rather publish thread context-related metadata by default, without having to do anything else on the tracer side. Otherwise most likely other tracers will have to know they need to call this additional setter. This also let us delay the "add custom key maps" operation, for which we can implement a separate FFI function if and when we need it.
If it's important to be able to disable this behavior in some cases, we can have a setter to do so, e.g. ddog_tracer_metadata_exclude_otel_thread_context. Wdyt?
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
There was a problem hiding this comment.
👍 LGTM
Maybe @ivoanjo has an opinion on this, but I wonder if we shouldn't rather publish thread context-related metadata by default, without having to do anything else on the tracer side. Otherwise most likely other tracers will have to know they need to call this additional setter. This also let us delay the "add custom key maps" operation, for which we can implement a separate FFI function if and when we need it.
Good question! TBH no very strong opinion on this one.
The thread context by its definition needs collaboration from the caller so making this one bit slightly more automatic I'm not sure makes a big difference.
Anyway if you don't have that block, outside readers won't be able to work so it's not like you won't notice very quickly...
yannham
left a comment
There was a problem hiding this comment.
I don't have a strong opinion either, so I'll let you decide what you think is best (opt-out vs explicit opt-in). There are remaining comments to acknowledge but otherwise the change is sound 👍
Ack! We can always add it later if it looks like it's a pain point |
|
I decided to go for opt-in, since some tracers don't need this functionality |
bc9cf28 to
b8d37f1
Compare
|
/merge |
|
View all feedbacks in Devflow UI.
It will be processed automatically as soon as GitHub reports it as mergeable. View in MergeQueue UI.
Use ⏳ Waiting for pull request to become mergeable |
What does this PR do?
This PR adds
ddog_tracer_metadata_include_otel_therad_contextfunction, allowing C callers to include thread-context discovery metadata when storing a tracer-metadata builder.When metadata is absent, the operation configures the
tlsdesc_v1_devschema and a key map containingdatadog.local_root_span_id. Existing metadata is preserved, so repeated calls are safe.Motivation
The agent currently cannot discover the Ruby TLS without the thread-context metadata in the OTel process context.
libdatadogalready supports publishing this metadata, but the C API lacks a way to configure it. Exposing the operation lets SDKs use the existing publisher instead of maintaining their own publication workaround.Additional Notes
The operation updates the builder; publication happens when
ddog_tracer_metadata_storeis called.How to test the change?
cargo nextest run -p libdd-library-config --features otel-thread-ctx,process-context-reader cargo nextest run -p libdd-library-config-ffi cargo nextest run -p libdd-library-config-ffi --no-default-features cargo test -p libdd-library-config-ffi --doc cargo ffi-tes