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. |
![datadog-official[bot]](https://avatars.githubusercontent.com/in/1358509?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
The completed static review raised no concrete defect. Runtime availability of the new libdatadog entry point and ABI compatibility of its by-value result remain unverified.
🤖 Bits Code Review · Commit 86b4460
BenchmarksBenchmark execution time: 2026-11-10 17:08:06 Comparing candidate commit 1e6ec78 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 72 metrics, 0 unstable metrics, 5 known flaky benchmarks, 120 flaky benchmarks without significant changes.
|
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (9583) and master. ✅ No regressions detected |
mcculls
left a comment
mcculls
left a comment
There was a problem hiding this comment.
Looks ok from apm-sdk-capabilities perspective
andrewlock
left a comment
andrewlock
left a comment
There was a problem hiding this comment.
LGTM in general, just a few questions
| var result = NativeInterop.LibraryConfig.IncludeOtelThreadContext(ptr); | ||
| if (result.Tag == ResultTag.Error) | ||
| { | ||
| var errorMessage = result.Error.Message.ToUtf8String(); | ||
| NativeInterop.Common.DropError(ref result.Error); | ||
| Log.Error("Failed to include OpenTelemetry thread context metadata with message: {Error}", errorMessage); |
There was a problem hiding this comment.
Should we wrap all this in its own try-catch, so that the StoreTracerMetadata call still runs if this throws? Each NativeInterop call may throw, so we may need two additional try-catch, one for each native call? WDYT?
There was a problem hiding this comment.
Minor take: Setting this option is a setting a boolean -- if this fails, something is seriously wrong so why bother?
There was a problem hiding this comment.
Because native interop always has risks - what if the native library isn't there (it's not always there), this throws, and then we don't call StoreTracerMetadata which has knock-on effects?
something is seriously wrong so why bother
"Something seriously wrong" could also just be "someone changed the libdatadog interface". Having every application crash on startup (even in CI) is a lot more confusing than having this path just error.
One of the "problems" with native interop is that you can't tell from this side what the actual constraints/requirements are... e.g. if I don't call DropError, what happens? Nothing I assume in practice (just a small memory leak) but that's not always the case.
In general, our libraries need to be robust. try-catch is just one of the mechanisms we have for that 😄
There was a problem hiding this comment.
Hmm but storing the tracer metadata is also in the native library, so again it's weird to me to paper over issues -- it should all succeed, or none of it. No trying to limp with an arrow on the knee? ;)
If there's something wonky already going on, it should by caught by a general "setting tracer metadata failed" it's kinda weird to me to spend complexity to try to do a piece-meal recovery.
There was a problem hiding this comment.
"Something seriously wrong" could also just be "someone changed the libdatadog interface". Having every application crash on startup (even in CI) is a lot more confusing than having this path just error.
Of course I do agree there should be one general try/catch to not break apps on a failed tracer metadata call, my point is more on the "don't recover on the small details, recover the whole thing overall".
There was a problem hiding this comment.
Out of interest, if this is "just" adding additional config to an existing call, why did we introduce an entirely new API, instead of using the existing NativeInterop.LibraryConfig.TracerMetadataSet method?
If we had, I would be 100% with you, just roll it in because it's all the same call. But the way IncludeOtelThreadContext is designed it is distinct. It returns its own pointer (that needs freeing, and which could itself throw).
There was a problem hiding this comment.
I guess to avoid having to change every caller, since not all of them want this feature? We could've changed the API instead -- see DataDog/libdatadog#2622 .
There was a problem hiding this comment.
I don't understand, I thought the whole point of TracerMetadataSet was that it's written to be extensible, so literally it's no extra APIs 😅
There was a problem hiding this comment.
Maybe? I don't have strong feelings on that one -- I think it'd be reasonable to move the argument again if you want to take a stab at it, there's not that many libraries consuming it so we can update them as well.
| namespace Datadog.Trace.LibDatadog.ServiceDiscovery; | ||
|
|
||
| /// <summary> | ||
| /// Libdatadog interop mapping of <c>ddog_VoidResult</c>. |
There was a problem hiding this comment.
Just going to assume this is the correct mapping 😅
If it's possible to include a static link to the definition in libdatadog, might be useful? 🤔
| // Must run after the process context has been published, since it extends it in place | ||
| OtelProcessContextAnnouncer.Announce(tracerSettings); |
There was a problem hiding this comment.
I notice that previously we were publishing this after calling StoreTracerMetadata, but AFAICT we're now doing that "in the middle" of the call... is that correct? Or should it be after the NativeInterop.LibraryConfig.StoreTracerMetadata(ptr) call?
| log.Should().NotContain( | ||
| "Unable to publish the OpenTelemetry thread context", | ||
| "publication must succeed on every thread"); | ||
|
|
||
| log.Should().NotContain( | ||
| "OpenTelemetry thread context publication was requested but is unavailable", | ||
| "the feature is supported on this platform, so it must not fall back to a no-op"); |
There was a problem hiding this comment.
I assume these too log messages can still be written? They're not in the deleted code?
Summary of changes
Use the official libdatadog API to notify OTel thread context propagation metadata
Reason for change
A new Linux notification mechanism is missing in the current implementation
Implementation details
Replace the hand coded metadata code by libdatadog
ddog_tracer_metadata_include_otel_thread_contextAPI.Test coverage
Updated
Other details
Require libdatadog bump to v47 - #9579