Repository navigation
Keep span events structured until serialization - #12768
PerfectSlayer wants to merge 5 commits into
Conversation
This would avoid encoding span tags as JSON, to then decode them as Java objects in order then to encode them as msgstruct
…ingBuilder than JsonWriter
This comment has been minimized.
This comment has been minimized.
🟢 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. |
No longer use hardcoded system time source but the tracer one.
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
Security findingsAdvisory findings (1)
ℹ️ 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. |
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: 8f00694578
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f00694578
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (events != NO_EVENT) { | ||
| events.add(event); | ||
| } else { | ||
| this.events = new CopyOnWriteArrayList<>(singletonList(event)); |
There was a problem hiding this comment.
Replace copy-on-write storage for span events
For spans that record many events—such as the concurrent GraphQL/DataLoader scenario covered by the new 16,000-event test—every add to this CopyOnWriteArrayList copies all preceding events, producing quadratic work and roughly 128 million copied references for that test alone. Span events are write-heavy until completion, so use synchronized append storage with a stable snapshot at finish rather than the link-oriented copy-on-write strategy.
AGENTS.md reference: AGENTS.md:L81-L81
Useful? React with 👍 / 👎.
Note
WIP - EARLY DRAFT - NOT READY FOR REVIEW
What Does This Do
This PR addresses my feedback from #11855:
OtelSpanand one inDDSpan;internal-api.About implementation details:
DDSpanEventobjects onDDSpan, created in core through two newAgentSpanmethods:addEvent(name, attributes)andaddEvent(name, attributes, timestamp, unit). Previously the OTel shim JSON-encoded them into theeventstag when the span finished.internal-apino longer has an event type.eventstag in core (DDSpanContext) at serialization time, next to the span links tag. This covers the tag-based protocols and writers: v0.4/v0.5, OTLP, CI Visibility, LLM Obs and file-based.TraceMapperV1no longer parses JSON.SystemTimeSource, which has only millisecond precision.Motivation
Events were JSON-encoded on the application thread for every protocol. v1 then parsed that JSON back to write native events: encode → decode → re-encode.
This also moves work off the application thread: attributes are copied into a map instead of being JSON-encoded when an event is added, and no tag is built when the span finishes.
Additional Notes
Behaviour changes:
eventstag is only added when the span is serialized, so trace interceptors no longer see it. This already applies to_dd.span_links.Fixes:
eventstag. A name containing",\or a control character used to produce invalid JSON, or to be read as a different name. Names without those characters are encoded exactly as before.Notes:
The v0.x tag keeps the previous
StringBuilderencoding and escaping rules. It now uses one builder per tag instead of one per event and per attribute set. I didn't usedatadog.json.JsonWriter: on JDK 17 and earlier itsOutputStreamWriterallocates an 8 KB buffer per call, and it encodes one character at a time, which is costly for exception stack traces.Known limitations, to address separately:
CopyOnWriteArrayList, so each add copies the list. That's fine for a few events per span but costly for spans with many. There is still no limit on the number of events, as before.Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]
🤖 Generated with Claude Code