From f21087736f2cab07e147ed4228079b482c78ba14 Mon Sep 17 00:00:00 2001 From: Meemaw Date: Thu, 23 Jul 2026 13:39:52 +0200 Subject: [PATCH] Fix concurrent span event recording in OTel shim 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) --- .../opentelemetry/shim/trace/OtelSpan.java | 44 +++++++++++------ .../opentelemetry14/OpenTelemetry14Test.java | 48 +++++++++++++++++++ 2 files changed, 77 insertions(+), 15 deletions(-) diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpan.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpan.java index 34bf7f2a486..4ed85f52b7d 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpan.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpan.java @@ -37,8 +37,14 @@ public class OtelSpan implements Span, WithAgentSpan, SpanWrapper { private StatusCode statusCode; private boolean recording; - /** Span events ({@code null} until an event is added). */ - private List events; + /** + * Span events ({@code null} until an event is added). Mutations are guarded by synchronizing on + * {@code this}: events can be recorded from application threads while the span is finished (and + * its events serialized) from a different thread, so unsynchronized access would corrupt the + * backing list. Declared {@code volatile} so {@link #onSpanFinished()} can take a lock-free fast + * path for the common case of a span with no events. + */ + private volatile List events; public OtelSpan(AgentSpan delegate) { this.delegate = delegate; @@ -85,10 +91,7 @@ public Span setAttribute(AttributeKey key, T value) { @Override public Span addEvent(String name, Attributes attributes) { if (this.recording) { - if (this.events == null) { - this.events = new ArrayList<>(); - } - this.events.add(new OtelSpanEvent(name, attributes)); + addEvent(new OtelSpanEvent(name, attributes)); } return this; } @@ -96,14 +99,18 @@ public Span addEvent(String name, Attributes attributes) { @Override public Span addEvent(String name, Attributes attributes, long timestamp, TimeUnit unit) { if (this.recording) { - if (this.events == null) { - this.events = new ArrayList<>(); - } - this.events.add(new OtelSpanEvent(name, attributes, timestamp, unit)); + addEvent(new OtelSpanEvent(name, attributes, timestamp, unit)); } return this; } + private synchronized void addEvent(OtelSpanEvent event) { + if (this.events == null) { + this.events = new ArrayList<>(); + } + this.events.add(event); + } + @Override public Span setStatus(StatusCode statusCode, String description) { if (this.recording) { @@ -123,12 +130,9 @@ public Span setStatus(StatusCode statusCode, String description) { @Override public Span recordException(Throwable exception, Attributes additionalAttributes) { if (this.recording) { - if (this.events == null) { - this.events = new ArrayList<>(); - } additionalAttributes = initializeExceptionAttributes(exception, additionalAttributes); applySpanEventExceptionAttributesAsTags(this.delegate, additionalAttributes); - this.events.add(new OtelSpanEvent(EXCEPTION_SPAN_EVENT_NAME, additionalAttributes)); + addEvent(new OtelSpanEvent(EXCEPTION_SPAN_EVENT_NAME, additionalAttributes)); } return this; } @@ -179,7 +183,17 @@ public AgentSpan asAgentSpan() { @Override public void onSpanFinished() { applyNamingConvention(this.delegate); - setEventsAsTag(this.delegate, this.events); + // Fast path: most spans have no events, so avoid taking the lock entirely (single volatile + // read). + List eventsSnapshot = this.events; + if (eventsSnapshot != null) { + // Copy under lock so serialization iterates a private snapshot that cannot be mutated + // concurrently by an application thread still recording events. + synchronized (this) { + eventsSnapshot = new ArrayList<>(this.events); + } + } + setEventsAsTag(this.delegate, eventsSnapshot); } private static class NoopSpan implements Span { diff --git a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/java/opentelemetry14/OpenTelemetry14Test.java b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/java/opentelemetry14/OpenTelemetry14Test.java index 2df2a1902a4..99f28aab382 100644 --- a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/java/opentelemetry14/OpenTelemetry14Test.java +++ b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/java/opentelemetry14/OpenTelemetry14Test.java @@ -67,9 +67,11 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import java.util.stream.Stream; import opentelemetry14.context.propagation.TextMap; +import org.json.JSONArray; import org.json.JSONException; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; @@ -289,6 +291,52 @@ void testAddMultipleSpanEvents() { tag(SPAN_EVENTS, isJson(expectedEventTag))))); } + @Test + void testConcurrentAddEvents() throws Exception { + // Regression test for the concurrent recording of span events. Events used to be stored in a + // plain ArrayList mutated without synchronization; recording events from multiple threads (as + // GraphQL DataLoaders do on virtual threads) corrupted the backing list, leaving null holes + // that + // triggered a NullPointerException in OtelSpanEvent.toTag when the span was finished. See trace + // b8b8e4edde47f90a92e832b63251a577. + int threadCount = 8; + int eventsPerThread = 2000; + Span span = this.otelTracer.spanBuilder("some-name").startSpan(); + + CountDownLatch start = new CountDownLatch(1); + List threads = new ArrayList<>(); + for (int t = 0; t < threadCount; t++) { + Thread thread = + new Thread( + () -> { + try { + start.await(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + return; + } + for (int i = 0; i < eventsPerThread; i++) { + span.addEvent("event"); + } + }); + thread.start(); + threads.add(thread); + } + + start.countDown(); + for (Thread thread : threads) { + thread.join(); + } + span.end(); + + writer.waitForTraces(1); + List firstTrace = writer.firstTrace(); + assertEquals(1, firstTrace.size()); + Object eventsTag = firstTrace.get(0).getTags().get(SPAN_EVENTS); + JSONArray events = new JSONArray((String) eventsTag); + assertEquals(threadCount * eventsPerThread, events.length()); + } + @Test void testSimpleSpanLinks() { String traceId = "1234567890abcdef1234567890abcdef";