Refactored span events to avoid not needed JSON deserialization for v…#11855
Refactored span events to avoid not needed JSON deserialization for v…#11855AlexeyKuznetsov-DD wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a21f3b492
ℹ️ 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".
| } | ||
| span.setTag(SPAN_EVENTS, OtelSpanEvent.toTag(events)); | ||
|
|
||
| span.addSpanEvents(events); |
There was a problem hiding this comment.
Preserve events for pre-serialization span observers
When OpenTelemetry spans record events, this now only stores them in DDSpan.spanEvents; the legacy events tag is not materialized until processTagsAndBaggage runs during payload mapping. Trace interceptors and test/custom writers such as ListWriter inspect span.getTags() before that mapping step, so spans with addEvent/recordException now appear to have no events there, whereas the previous setTag(SPAN_EVENTS, ...) made them visible immediately. Please keep the tag populated or otherwise materialize structured events before pre-serialization observers read the span tags.
Useful? React with 👍 / 👎.
| encodeSpanLinks(writable, 11, meta.getSpanLinks()); | ||
| // events = 12, a collection of events that occurred during this span | ||
| encodeSpanEvents(writable, 12, tags.getObject(DDTags.SPAN_EVENTS)); | ||
| encodeSpanEvents(writable, 12, meta.getSpanEvents()); |
There was a problem hiding this comment.
Preserve legacy events tags in V1 payloads
For spans that still provide span events through the existing events tag as a List<Map<...>> (for example custom instrumentation or a trace interceptor calling setTag("events", eventPayload)), the V1 mapper now reads only Metadata.getSpanEvents(), so field 12 is emitted empty and the legacy tag is flattened as a normal span attribute instead of native events. The previous mapper explicitly encoded tags.getObject(DDTags.SPAN_EVENTS), so please keep that fallback when the structured event list is empty or migrate all producers before dropping it.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| def "test span events are encoded from events tag"() { | ||
| def "test span events are encoded from structured span events"() { |
There was a problem hiding this comment.
Move new span-event tests to JUnit 5
The repository instructions in AGENTS.md say, Test frameworks: Always use JUnit 5. Do not write new Groovy / Spock tests, but this adds new Spock coverage in a Groovy spec. Please move this new span-event payload coverage to a JUnit 5 test instead of extending the Groovy test suite.
Useful? React with 👍 / 👎.
| return Collections.emptyMap(); | ||
| } | ||
| return builder.append(']').toString(); | ||
| Map<String, Object> map = new LinkedHashMap<>(this.attributes.size()); |
There was a problem hiding this comment.
Is order of entries important? If not then this could be a HashMap which uses less space
There was a problem hiding this comment.
Pull request overview
This PR refactors span-event handling so events are retained as structured AgentSpanEvent instances on spans and encoded directly in the v1 payload (field 12), avoiding the previous JSON serialize→deserialize round-trip. It also keeps legacy v0.x behavior by emitting the events tag as JSON only when needed for those payloads.
Changes:
- Introduces
AgentSpanEventand addsAgentSpan.addSpanEvents(...), storing events onDDSpanand carrying them throughMetadata. - Updates v1 mapping (
TraceMapperV1) to encode events directly from structured events, and updates tag/baggage processing to optionally flatten events into the legacyeventsJSON tag for v0.x payloads. - Updates tests and test utilities to pass/verify structured span events and the v0.x compatibility tag path.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/ImmutableSpan.java | Adds no-op addSpanEvents(...) to satisfy the expanded AgentSpan interface. |
| internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentSpanEvent.java | Introduces a structured span-event abstraction with typed attributes and lazy JSON rendering. |
| internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentSpan.java | Extends the span API with addSpanEvents(...). |
| dd-trace-core/src/traceAgentTest/groovy/TraceGenerator.groovy | Updates Metadata construction to include span events. |
| dd-trace-core/src/test/java/datadog/trace/core/DDSpanSerializationTest.java | Adds v0.x tests for the legacy events tag assembled from structured events; refactors span context creation helpers. |
| dd-trace-core/src/test/java/datadog/trace/common/writer/TraceGenerator.java | Extends PojoSpan test span model to carry span events into Metadata. |
| dd-trace-core/src/test/java/datadog/trace/common/writer/FileBasedPayloadDispatcherTest.java | Updates mocked span construction to match updated metadata/span constructor signatures. |
| dd-trace-core/src/test/groovy/datadog/trace/common/writer/ddagent/TraceMapperV1PayloadTest.groovy | Updates v1 payload tests to validate structured event encoding and attribute filtering behavior. |
| dd-trace-core/src/main/java/datadog/trace/core/Metadata.java | Adds spanEvents storage and accessor to transport structured events to mappers. |
| dd-trace-core/src/main/java/datadog/trace/core/DDSpanContext.java | Adds optional flattening of structured events into legacy events tag and includes events in Metadata. |
| dd-trace-core/src/main/java/datadog/trace/core/DDSpan.java | Stores structured events and implements thread-safe addSpanEvents(...); renames structured processing method. |
| dd-trace-core/src/main/java/datadog/trace/core/CoreSpan.java | Renames/extends structured processing hook to include events. |
| dd-trace-core/src/main/java/datadog/trace/common/writer/ddagent/TraceMapperV1.java | Encodes span events from structured Metadata.getSpanEvents() and broadens string handling to CharSequence. |
| dd-java-agent/instrumentation-testing/src/main/groovy/datadog/trace/agent/test/TrackingSpanDecorator.groovy | Forwards addSpanEvents(...) to the delegate span. |
| dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java | Implements AgentSpanEvent, keeps raw OTel Attributes, provides typed attributes and lazy JSON rendering. |
| dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpan.java | Switches from tag-based event recording to structured addSpanEvents(...). |
| dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java | Renames event recording helper and routes events into structured storage instead of tags. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .append("\"name\":") | ||
| .append('"') | ||
| .append(this.name) | ||
| .append('"'); |
| boolean writeTopLevel = meta.topLevel(); | ||
| int tagCount = 0; | ||
| for (TagMap.EntryReader entry : tags) { | ||
| if (!DDTags.SPAN_EVENTS.equals(entry.tag())) { |
There was a problem hiding this comment.
If a user sets a DDTags.SPAN_EVENTS tag this will now write them as as a plain tag - is that acceptable?
|
|
||
| void addLink(AgentSpanLink link); | ||
|
|
||
| void addSpanEvents(List<? extends AgentSpanEvent> events); |
There was a problem hiding this comment.
addEvents would match better with addLink above
What Does This Do
Keeps span events as structured
AgentSpanEventobjects on the span instead of eagerly serializing them into the JSON_dd.span_eventstag, to encode them natively as structured data inv1protocol.AgentSpanEventinterface (mirrorsAgentSpanLink) exposingtimeNanos(),name(),attributes(), andtoJson().OtelSpanEventnow implementsAgentSpanEventand holds the raw OTelAttributesinstead of a pre-rendered JSON string; JSON is only produced on demand viatoJson().AgentSpan.addSpanEvents(...)stores events onDDSpan(thread-safety modeled onaddLink()), carried throughMetadata.getSpanEvents().TraceMapperV1encodes field12directly from typed events — dropping the JSON round-trip (parse-back) and theasLong/isEncodableSpanEventmap-inspection helpers._dd.span_eventsJSON tag, now assembled lazily inDDSpanContextonly when events are injected as tags.Motivation
For the
v1protocol the previous flow serialized span events to a JSON string tag and then deserialized that same JSON back in the mapper just to re-encode it as MessagePack — pure wasted work and allocation. Holding events as structured data lets each payload version choose its own representation:v1encodes them natively, whilev0.xkeeps the legacy tag.Additional Notes
processTagsAndBaggageWithStructuredLinkswas renamed toprocessTagsAndBaggageWithStructuredLinksAndEvents, andOtelConventions.setEventsAsTagtorecordSpanEvents.v1string-value writer now accepts anyCharSequence(not justString), sotoJson()'sCharSequenceoutput serializes without a debug warning.