🪞 12053 - Fix concurrent span event recording in OTel shim - #12301
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17f0ff1ae6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
More details
The event list uses one monitor for each write and for the copy before serialization, so concurrent event recording cannot corrupt the serialized list.
🤖 Datadog Autotest · Commit 17f0ff1 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
|
🎯 Code Coverage (details) 🔗 Commit SHA: 25583ae | Docs | View more details | Give us feedback! |
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
OtelSpan stored span events in a plain ArrayList that was mutated by addEvent()/recordException() without synchronization and iterated at span finish. When events are recorded from multiple threads (e.g. GraphQL DataLoaders running on virtual threads) while the span is being finished, the backing list gets corrupted, leaving a null hole that triggers a NullPointerException in OtelSpanEvent.toTag. That NPE escapes span finish (DDSpan.finishAndAddToTrace) and can surface to the application as a request failure. Guard all event-list mutations by synchronizing on the span, and copy the list under lock at finish so serialization iterates a private snapshot. The events field is volatile so onSpanFinished keeps a lock-free fast path (single volatile read) for the common case of a span with no events, avoiding overhead on the hot finish path. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
…ock instead of copying. This avoids the extra list allocation while still bounding the lock to a quick pointer swap.
7d0dee0 to
ccabd93
Compare
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
This PR mirrors the changes from the original community contribution to enable CI testing with maintainer privileges.
Original PR: #12053
Original Author: @Meemaw
Original Branch: Meemaw/dd-trace-java:fix-otel-span-events-concurrency
Closes #12053
This is an automated mirror created to run CI checks. See tooling/mirror-community-pull-request.sh for details.