Keep span events structured until serialization

dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java

@@ -7,7 +7,6 @@
 import static datadog.trace.api.DDTags.ERROR_MSG;
 import static datadog.trace.api.DDTags.ERROR_STACK;
 import static datadog.trace.api.DDTags.ERROR_TYPE;
-import static datadog.trace.api.DDTags.SPAN_EVENTS;
 import static datadog.trace.bootstrap.instrumentation.api.Tags.SPAN_KIND;
 import static datadog.trace.bootstrap.instrumentation.api.Tags.SPAN_KIND_CLIENT;
 import static datadog.trace.bootstrap.instrumentation.api.Tags.SPAN_KIND_CONSUMER;
@@ -136,13 +135,6 @@ public static void applyNamingConvention(AgentSpan span) {
     }
   }
 
-  public static void setEventsAsTag(AgentSpan span, List<OtelSpanEvent> events) {
-    if (events == null || events.isEmpty()) {
-      return;
-    }
-    span.setTag(SPAN_EVENTS, OtelSpanEvent.toTag(events));
-  }
-
   public static void applySpanEventExceptionAttributesAsTags(
       AgentSpan span, Attributes exceptionAttributes) {
     span.setTag(ERROR_MSG, exceptionAttributes.get(EXCEPTION_MESSAGE_ATTRIBUTE_KEY));

dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpan.java

@@ -3,8 +3,8 @@
 import static datadog.opentelemetry.shim.trace.OtelConventions.applyNamingConvention;
 import static datadog.opentelemetry.shim.trace.OtelConventions.applyReservedAttribute;
 import static datadog.opentelemetry.shim.trace.OtelConventions.applySpanEventExceptionAttributesAsTags;
-import static datadog.opentelemetry.shim.trace.OtelConventions.setEventsAsTag;
 import static datadog.opentelemetry.shim.trace.OtelSpanEvent.EXCEPTION_SPAN_EVENT_NAME;
+import static datadog.opentelemetry.shim.trace.OtelSpanEvent.eventAttributes;
 import static datadog.opentelemetry.shim.trace.OtelSpanEvent.initializeExceptionAttributes;
 import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activateSpan;
 import static io.opentelemetry.api.trace.StatusCode.ERROR;
@@ -26,7 +26,6 @@
 import io.opentelemetry.api.trace.StatusCode;
 import io.opentelemetry.api.trace.TraceFlags;
 import io.opentelemetry.api.trace.TraceState;
-import java.util.ArrayList;
 import java.util.List;
 import java.util.concurrent.TimeUnit;
 import javax.annotation.ParametersAreNonnullByDefault;
@@ -37,15 +36,6 @@ public class OtelSpan implements Span, WithAgentSpan, SpanWrapper {
   private StatusCode statusCode = UNSET;
   private volatile boolean recording = true;
 
-  /**
-   * Span events ({@code null} until an event is added).
-   *
-   * <p>Volatile so {@link #onSpanFinished()} can skip the lock in common no-events case; writes and
-   * the rare non-null read synchronize on {@code this} to guard against concurrent recording from
-   * another thread.
-   */
-  private volatile List<OtelSpanEvent> events;
-
   public OtelSpan(AgentSpan delegate) {
     this.delegate = delegate;
     if (delegate instanceof AttachableWrapper) {
@@ -89,27 +79,19 @@ public <T> Span setAttribute(AttributeKey<T> key, T value) {
   @Override
   public Span addEvent(String name, Attributes attributes) {
     if (this.recording) {
-      doAddEvent(new OtelSpanEvent(name, attributes));
+      this.delegate.addEvent(name, eventAttributes(attributes));
     }
     return this;
   }
 
   @Override
   public Span addEvent(String name, Attributes attributes, long timestamp, TimeUnit unit) {
     if (this.recording) {
-      doAddEvent(new OtelSpanEvent(name, attributes, timestamp, unit));
+      this.delegate.addEvent(name, eventAttributes(attributes), timestamp, unit);
     }
     return this;
   }
 
-  private synchronized void doAddEvent(OtelSpanEvent event) {
-    List<OtelSpanEvent> eventsSnapshot = this.events;
-    if (eventsSnapshot == null) {
-      this.events = eventsSnapshot = new ArrayList<>();
-    }
-    eventsSnapshot.add(event);
-  }
-
   @Override
   public Span setStatus(StatusCode statusCode, String description) {
     if (this.recording) {
@@ -131,7 +113,7 @@ public Span recordException(Throwable exception, Attributes additionalAttributes
     if (this.recording) {
       additionalAttributes = initializeExceptionAttributes(exception, additionalAttributes);
       applySpanEventExceptionAttributesAsTags(this.delegate, additionalAttributes);
-      doAddEvent(new OtelSpanEvent(EXCEPTION_SPAN_EVENT_NAME, additionalAttributes));
+      this.delegate.addEvent(EXCEPTION_SPAN_EVENT_NAME, eventAttributes(additionalAttributes));
     }
     return this;
   }
@@ -182,16 +164,6 @@ public AgentSpan asAgentSpan() {
   @Override
   public void onSpanFinished() {
     applyNamingConvention(this.delegate);
-    // Fast path: skip the lock when there are no events (the common case)
-    if (this.events != null) {
-      List<OtelSpanEvent> eventsSnapshot;
-      synchronized (this) {
-        // detach events for serialization
-        eventsSnapshot = this.events;
-        this.events = null;
-      }
-      setEventsAsTag(this.delegate, eventsSnapshot);
-    }
   }
 
   private static class NoopSpan implements Span {

dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java

@@ -1,19 +1,17 @@
 package datadog.opentelemetry.shim.trace;
 
-import datadog.trace.api.time.SystemTimeSource;
-import datadog.trace.api.time.TimeSource;
+import static java.util.Collections.emptyMap;
+
 import io.opentelemetry.api.common.AttributeKey;
 import io.opentelemetry.api.common.Attributes;
 import io.opentelemetry.api.common.AttributesBuilder;
 import java.io.PrintWriter;
 import java.io.StringWriter;
-import java.util.List;
+import java.util.HashMap;
 import java.util.Map;
-import java.util.Set;
-import java.util.concurrent.TimeUnit;
-import javax.annotation.Nonnull;
 
-public class OtelSpanEvent {
+/** Helpers to record OpenTelemetry span events on agent spans. */
+public final class OtelSpanEvent {
   public static final String EXCEPTION_SPAN_EVENT_NAME = "exception";
   public static final AttributeKey<String> EXCEPTION_MESSAGE_ATTRIBUTE_KEY =
       AttributeKey.stringKey("exception.message");
@@ -22,37 +20,22 @@ public class OtelSpanEvent {
   public static final AttributeKey<String> EXCEPTION_STACK_TRACE_ATTRIBUTE_KEY =
       AttributeKey.stringKey("exception.stacktrace");
 
-  // TODO TimeSource instance is not retrieved from CoreTracer
-  private static TimeSource timeSource = SystemTimeSource.INSTANCE;
-
-  private final String name;
-  private final String attributes;
-
-  /** Event timestamp in nanoseconds. */
-  private final long timestamp;
-
-  public OtelSpanEvent(String name, Attributes attributes) {
-    this.name = name;
-    this.attributes = AttributesJsonParser.toJson(attributes);
-    this.timestamp = OtelSpanEvent.timeSource.getCurrentTimeNanos();
-  }
-
-  public OtelSpanEvent(String name, Attributes attributes, long timestamp, TimeUnit unit) {
-    this.name = name;
-    this.attributes = AttributesJsonParser.toJson(attributes);
-    this.timestamp = unit.toNanos(timestamp);
-  }
+  private OtelSpanEvent() {}
 
-  @Nonnull
-  public static String toTag(List<OtelSpanEvent> events) {
-    StringBuilder builder = new StringBuilder("[");
-    for (OtelSpanEvent event : events) {
-      if (builder.length() > 1) {
-        builder.append(',');
-      }
-      builder.append(event.toJson());
+  /**
+   * Converts OpenTelemetry attributes into span event attributes.
+   *
+   * @param attributes The OpenTelemetry attributes to convert.
+   * @return The attributes keyed by attribute name.
+   */
+  static Map<String, ?> eventAttributes(Attributes attributes) {
+    if (attributes == null || attributes.isEmpty()) {
+      return emptyMap();
     }
-    return builder.append(']').toString();
+    // Size the map to hold all attributes without resizing given the default load factor
+    Map<String, Object> map = new HashMap<>((int) (attributes.size() / 0.75f) + 1);
+    attributes.forEach((key, value) -> map.put(key.getKey(), value));
+    return map;
   }
 
   /**
@@ -98,96 +81,4 @@ static String stringifyErrorStack(Throwable error) {
     error.printStackTrace(new PrintWriter(errorString));
     return errorString.toString();
   }
-
-  /** Helper class for JSON-encoding {@link OtelSpanEvent} {@link #attributes}. */
-  public static class AttributesJsonParser {
-    public static String toJson(Attributes attributes) {
-      if (attributes == null || attributes.isEmpty()) {
-        return "";
-      }
-      StringBuilder jsonBuilder = new StringBuilder();
-      jsonBuilder.append('{');
-
-      Set<Map.Entry<AttributeKey<?>, Object>> entrySet = attributes.asMap().entrySet();
-
-      for (Map.Entry<AttributeKey<?>, Object> entry : entrySet) {
-        if (jsonBuilder.length() > 1) {
-          jsonBuilder.append(',');
-        }
-        // AttributeKey type has method `getKey()` that "stringifies" the key
-        String key = entry.getKey().getKey();
-        Object value = entry.getValue();
-        // Escape key and append it
-        jsonBuilder.append('"').append(escapeJson(key)).append("\":");
-        // Append value to jsonBuilder
-        appendValue(value, jsonBuilder);
-      }
-      jsonBuilder.append('}');
-      return jsonBuilder.toString();
-    }
-
-    /**
-     * Recursively adds the value of an {@link Attributes} to the active StringBuilder in JSON
-     * format, depending on the value's type.
-     *
-     * @param value The value to append
-     * @param jsonBuilder The active {@link StringBuilder}
-     */
-    private static void appendValue(Object value, StringBuilder jsonBuilder) {
-      // Append value based on its type
-      if (value instanceof String) {
-        jsonBuilder.append('"').append(escapeJson((String) value)).append('"');
-      } else if (value instanceof List) {
-        jsonBuilder.append('[');
-        List<?> valArray = (List<?>) value;
-        for (int i = 0; i < valArray.size(); i++) {
-          if (i > 0) {
-            jsonBuilder.append(',');
-          }
-          appendValue(valArray.get(i), jsonBuilder);
-        }
-        jsonBuilder.append(']');
-      } else if (value instanceof Number || value instanceof Boolean) {
-        jsonBuilder.append(value);
-      } else {
-        jsonBuilder.append("null"); // null for unsupported types
-      }
-    }
-
-    private static String escapeJson(String value) {
-      return value
-          .replace("\\", "\\\\")
-          .replace("\"", "\\\"")
-          .replace("\b", "\\b")
-          .replace("\f", "\\f")
-          .replace("\n", "\\n")
-          .replace("\r", "\\r")
-          .replace("\t", "\\t");
-    }
-  }
-
-  public static void setTimeSource(TimeSource newTimeSource) {
-    timeSource = newTimeSource;
-  }
-
-  public String toJson() {
-    StringBuilder builder =
-        new StringBuilder(
-            "{\"time_unix_nano\":" + this.timestamp + ",\"name\":\"" + this.name + "\"");
-    if (!this.attributes.isEmpty()) {
-      builder.append(",\"attributes\":").append(this.attributes);
-    }
-    return builder.append('}').toString();
-  }
-
-  @Override
-  public String toString() {
-    return "OtelSpanEvent{timestamp="
-        + this.timestamp
-        + ", name='"
-        + this.name
-        + "', attributes='"
-        + this.attributes
-        + "'}";
-  }
 }

dd-java-agent/instrumentation-testing/src/main/groovy/datadog/trace/agent/test/TrackingSpanDecorator.groovy

@@ -13,6 +13,7 @@ import datadog.trace.bootstrap.instrumentation.api.AgentSpanLink
 import datadog.trace.core.DDSpan
 
 import java.util.concurrent.ConcurrentHashMap
+import java.util.concurrent.TimeUnit
 import javax.annotation.Nonnull
 
 /**
@@ -404,6 +405,16 @@ class TrackingSpanDecorator implements AgentSpan {
     delegate.addLink(link)
   }
 
+  @Override
+  void addEvent(String name, Map<String, ?> attributes) {
+    delegate.addEvent(name, attributes)
+  }
+
+  @Override
+  void addEvent(String name, Map<String, ?> attributes, long timestamp, TimeUnit unit) {
+    delegate.addEvent(name, attributes, timestamp, unit)
+  }
+
   @Override
   AgentSpan setMetaStruct(String field, Object value) {
     return delegate.setMetaStruct(field, value)

dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/java/opentelemetry14/OpenTelemetry14Test.java

@@ -42,14 +42,13 @@
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.params.provider.Arguments.arguments;
 
-import datadog.opentelemetry.shim.trace.OtelSpanEvent;
 import datadog.trace.agent.test.assertions.TagsMatcher;
 import datadog.trace.api.DDSpanId;
 import datadog.trace.api.DDTags;
 import datadog.trace.api.DDTraceId;
-import datadog.trace.api.time.ControllableTimeSource;
 import datadog.trace.bootstrap.instrumentation.api.WithAgentSpan;
 import datadog.trace.core.DDSpan;
+import datadog.trace.core.DDSpanEvent;
 import datadog.trace.test.junit.utils.assertions.Matcher;
 import datadog.trace.test.junit.utils.assertions.Matchers;
 import io.opentelemetry.api.GlobalOpenTelemetry;
@@ -172,9 +171,6 @@ void testNoParentToCreateNewRootSpan() {
   @Test
   void testAddEvent() {
     SpanBuilder builder = this.otelTracer.spanBuilder("some-name");
-    ControllableTimeSource timeSource = new ControllableTimeSource();
-    timeSource.set(1000);
-    OtelSpanEvent.setTimeSource(timeSource);
 
     Span result = builder.startSpan();
     result.addEvent("event");
@@ -183,7 +179,7 @@ void testAddEvent() {
     String expectedEventTag =
         "["
             + "{ \"time_unix_nano\": "
-            + timeSource.getCurrentTimeNanos()
+            + eventTimeWithinSpan(result, 0)
             + ", \"name\": \"event\" }"
             + "]";
     assertTraces(
@@ -328,8 +324,8 @@ void testConcurrentAddEvents() throws Exception {
     writer.waitForTraces(1);
     List<DDSpan> firstTrace = writer.firstTrace();
     assertEquals(1, firstTrace.size());
-    Object eventsTag = firstTrace.get(0).getTags().get(SPAN_EVENTS);
-    JSONArray events = new JSONArray((String) eventsTag);
+    String eventsTag = firstTrace.get(0).getTags().getString(SPAN_EVENTS);
+    JSONArray events = new JSONArray(eventsTag);
     assertEquals(threadCount * eventsPerThread, events.length());
   }
 
@@ -764,10 +760,6 @@ void testSpanRecordException(
       String overriddenType,
       String overriddenStacktrace,
       String extraJson) {
-    ControllableTimeSource timeSource = new ControllableTimeSource();
-    timeSource.set(1000);
-    OtelSpanEvent.setTimeSource(timeSource);
-
     Span result = this.otelTracer.spanBuilder("some-name").startSpan();
     result.recordException(exception, attributes);
     result.end();
@@ -791,7 +783,7 @@ void testSpanRecordException(
     String expectedEventTag =
         "["
             + "{ \"time_unix_nano\": "
-            + timeSource.getCurrentTimeNanos()
+            + eventTimeWithinSpan(result, 0)
             + ", \"name\": \"exception\", \"attributes\": "
             + expectedAttributes
             + " }"
@@ -887,6 +879,18 @@ private static DDSpan getDDSpan(Span span) {
     return (DDSpan) ((WithAgentSpan) span).asAgentSpan();
   }
 
+  /** Gets the time of the single span event, checking it comes from the same clock as the span. */
+  private static long eventTimeWithinSpan(Span span, int eventIndex) {
+    DDSpan ddSpan = getDDSpan(span);
+    List<DDSpanEvent> events = ddSpan.getEvents();
+    assertTrue(eventIndex < events.size());
+    long eventTime = events.get(eventIndex).timeUnixNano();
+    long startTime = ddSpan.getStartTime();
+    long endTime = startTime + ddSpan.getDurationNano();
+    assertTrue(startTime <= eventTime && eventTime <= endTime, "Event time outside span bounds");
+    return eventTime;
+  }
+
   private static Matcher<String> isJson(String expected) {
     return Matchers.validates(
         s -> {