From 3de867985d55b9ddd5a8fc6bf4baf5bbbdb23f38 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Fri, 9 Aug 2024 16:29:12 -0400 Subject: [PATCH 01/37] Initial span events implementation + tests --- .../shim/trace/OtelConventions.java | 8 ++ .../opentelemetry/shim/trace/OtelSpan.java | 19 ++++- .../shim/trace/OtelSpanEvent.java | 54 +++++++++++++ .../test/groovy/OpenTelemetry14Test.groovy | 77 +++++++++++++++++-- .../main/java/datadog/trace/api/DDTags.java | 1 + 5 files changed, 150 insertions(+), 9 deletions(-) create mode 100644 dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java index 2b2ca408969..86f2ba30be7 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java @@ -18,6 +18,7 @@ import datadog.trace.bootstrap.instrumentation.api.Tags; import io.opentelemetry.api.common.AttributeKey; import io.opentelemetry.api.trace.SpanKind; +import java.util.List; import javax.annotation.Nullable; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -122,6 +123,13 @@ public static void applyNamingConvention(AgentSpan span) { } } + public static void setEventsAsTag(AgentSpan span, List events) { + if (events == null || events.isEmpty()) { + return; + } + span.setTag("events", OtelSpanEvent.toTag(events)); + } + private static String computeOperationName(AgentSpan span) { Object spanKingTag = span.getTag(SPAN_KIND); SpanKind spanKind = 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 64fb9f9a2cb..7a286029bf8 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 @@ -2,6 +2,7 @@ import static datadog.opentelemetry.shim.trace.OtelConventions.applyNamingConvention; import static datadog.opentelemetry.shim.trace.OtelConventions.applyReservedAttribute; +import static datadog.opentelemetry.shim.trace.OtelConventions.setEventsAsTag; import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activateSpan; import static io.opentelemetry.api.trace.StatusCode.ERROR; import static io.opentelemetry.api.trace.StatusCode.OK; @@ -18,6 +19,7 @@ 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; @@ -27,6 +29,7 @@ public class OtelSpan implements Span { private final AgentSpan delegate; private StatusCode statusCode; private boolean recording; + private List events; public OtelSpan(AgentSpan delegate) { this.delegate = delegate; @@ -71,13 +74,23 @@ public Span setAttribute(AttributeKey key, T value) { @Override public Span addEvent(String name, Attributes attributes) { - // Not supported + if (this.recording) { + if (this.events == null || this.events.isEmpty()) { + this.events = new ArrayList<>(); + } + this.events.add(new OtelSpanEvent(name, attributes)); + } return this; } @Override public Span addEvent(String name, Attributes attributes, long timestamp, TimeUnit unit) { - // Not supported + if (this.recording) { + if (this.events == null || this.events.isEmpty()) { + this.events = new ArrayList<>(); + } + this.events.add(new OtelSpanEvent(name, attributes, timestamp, unit)); + } return this; } @@ -118,6 +131,7 @@ public Span updateName(String name) { public void end() { this.recording = false; applyNamingConvention(this.delegate); + setEventsAsTag(this.delegate, this.events); this.delegate.finish(); } @@ -125,6 +139,7 @@ public void end() { public void end(long timestamp, TimeUnit unit) { this.recording = false; applyNamingConvention(this.delegate); + setEventsAsTag(this.delegate, this.events); this.delegate.finish(unit.toMicros(timestamp)); } diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java new file mode 100644 index 00000000000..6dba1a9cdda --- /dev/null +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java @@ -0,0 +1,54 @@ +package datadog.opentelemetry.shim.trace; + +import io.opentelemetry.api.common.Attributes; +import java.util.List; +import java.util.concurrent.TimeUnit; + +public class OtelSpanEvent { + private final long timestamp; + private final String name; + // attributes + + public OtelSpanEvent(String name, Attributes attributes) { + this.name = name; + this.timestamp = timeNano(); + } + + public OtelSpanEvent(String name, Attributes attributes, long timestamp, TimeUnit unit) { + this.name = name; + this.timestamp = timeNano(timestamp, unit); + } + + private static long timeNano() { + return System.nanoTime(); + } + + private static long timeNano(long timestamp, TimeUnit unit) { + return TimeUnit.NANOSECONDS.convert(timestamp, unit); + // MTOFF: Should we handle the case where the conversion returns Long.MIN_VALUE or + // Long.MAX_VALUE? see: + // https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/TimeUnit.html#convert-long-java.util.concurrent.TimeUnit- + } + + public String toString() { + // TODO if attributes exist, process those + return "{\"name\":\"" + + this.name + + "\",\"time_unix_nano\":" + + Long.toString(this.timestamp) + + "}"; + } + + // @NotNull + // TODO: give this a notnull annotation + public static String toTag(List events) { + StringBuilder builder = new StringBuilder("["); + for (int i = 0; i < events.size(); i++) { + if (i > 0) { + builder.append(","); + } + builder.append(events.get(i).toString()); + } + return builder.append("]").toString(); + } +} diff --git a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy index 7a0c4d7ee91..a75e1a9a486 100644 --- a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy +++ b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy @@ -15,6 +15,8 @@ import org.skyscreamer.jsonassert.JSONAssert import spock.lang.Subject import java.security.InvalidParameterException +import java.sql.Time +import java.util.concurrent.TimeUnit import static datadog.trace.api.DDTags.ERROR_MSG import static datadog.trace.bootstrap.instrumentation.api.Tags.SPAN_KIND @@ -33,6 +35,9 @@ import static io.opentelemetry.api.trace.StatusCode.OK import static io.opentelemetry.api.trace.StatusCode.UNSET class OpenTelemetry14Test extends AgentTestRunner { + static final TIME_MILLIS = 1723220824705 + static final TIME_NANO = TIME_MILLIS * 1_000_000L + @Subject def tracer = GlobalOpenTelemetry.get().tracerProvider.get("some-instrumentation") @@ -175,27 +180,85 @@ class OpenTelemetry14Test extends AgentTestRunner { } } - def "test non-supported features do not crash"() { + def "test multiple span events"() { setup: def builder = tracer.spanBuilder("some-name") - def anotherSpan = tracer.spanBuilder("another-name").startSpan() - anotherSpan.end() when: // Adding event is not supported def result = builder.startSpan() - result.addEvent("some-event") + result.addEvent(name1, timestamp, TimeUnit.NANOSECONDS) + result.addEvent(name2, timestamp, TimeUnit.NANOSECONDS) result.end() then: - assertTraces(2) { + def expectedEventTag = """ + [ + { "name": "${name1}", + "time_unix_nano": ${timestamp} + }, + { "name": "${name2}", + "time_unix_nano": ${timestamp} + } + ]""" + assertTraces(1) { trace(1) { - span {} + span { + tags { + defaultTags() + "$SPAN_KIND" "$SPAN_KIND_INTERNAL" + tag("events", { JSONAssert.assertEquals(expectedEventTag, it as String, false); return true }) + } + } } + } + where: + name1 = "evt1" + name2 = "evt2" + timestamp = 1723220824705 * 1_000_000L + } + + def "test add single event"() { + setup: + def builder = tracer.spanBuilder("some-name") + + when: + // Adding event is not supported + def result = builder.startSpan() + result.addEvent(name, timestamp, unit) + result.end() + + then: + long expectTime = timestamp + if (unit != TimeUnit.NANOSECONDS) { + // unit is milliseconds + expectTime = timestamp * 1_000_000L + } + + def expectedEventTag = """ + [ + { "name": "${name}", + "time_unix_nano": ${expectTime}} + ]""" + assertTraces(1) { trace(1) { - span {} + span { + tags { + defaultTags() + "$SPAN_KIND" "$SPAN_KIND_INTERNAL" + tag("events", { JSONAssert.assertEquals(expectedEventTag, it as String, false); return true }) + } + } } } + + where: + // NOTE: Don't know how to test where timestamp is null. + name | attributes | timestamp | unit + // null | null | null | null + // "evt1" | null | null | null + "evt2" | null | TIME_MILLIS | TimeUnit.MILLISECONDS + "evt3" | null | TIME_NANO | TimeUnit.NANOSECONDS } def "test simple span links"() { diff --git a/dd-trace-api/src/main/java/datadog/trace/api/DDTags.java b/dd-trace-api/src/main/java/datadog/trace/api/DDTags.java index fff1793fdcf..5e373b5e7e6 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/DDTags.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/DDTags.java @@ -59,6 +59,7 @@ public class DDTags { public static final String LANGUAGE_TAG_VALUE = "jvm"; public static final String ORIGIN_KEY = "_dd.origin"; public static final String SPAN_LINKS = "_dd.span_links"; + private static final String SPAN_EVENTS = "events"; public static final String LIBRARY_VERSION_TAG_KEY = "library_version"; public static final String CI_ENV_VARS = "_dd.ci.env_vars"; public static final String CI_ITR_TESTS_SKIPPED = "_dd.ci.itr.tests_skipped"; From d654056d45ade534b2d08df121e4ffcded43811c Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Fri, 9 Aug 2024 16:32:57 -0400 Subject: [PATCH 02/37] use DDTags.SPAN_EVENTS constant for setting tag --- .../java/datadog/opentelemetry/shim/trace/OtelConventions.java | 3 ++- dd-trace-api/src/main/java/datadog/trace/api/DDTags.java | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java index 86f2ba30be7..fb4ade9163a 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java @@ -14,6 +14,7 @@ import static java.lang.Boolean.parseBoolean; import static java.util.Locale.ROOT; +import datadog.trace.api.DDTags; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import datadog.trace.bootstrap.instrumentation.api.Tags; import io.opentelemetry.api.common.AttributeKey; @@ -127,7 +128,7 @@ public static void setEventsAsTag(AgentSpan span, List events) { if (events == null || events.isEmpty()) { return; } - span.setTag("events", OtelSpanEvent.toTag(events)); + span.setTag(DDTags.SPAN_EVENTS, OtelSpanEvent.toTag(events)); } private static String computeOperationName(AgentSpan span) { diff --git a/dd-trace-api/src/main/java/datadog/trace/api/DDTags.java b/dd-trace-api/src/main/java/datadog/trace/api/DDTags.java index 5e373b5e7e6..b5b3e4ac07a 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/DDTags.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/DDTags.java @@ -59,7 +59,7 @@ public class DDTags { public static final String LANGUAGE_TAG_VALUE = "jvm"; public static final String ORIGIN_KEY = "_dd.origin"; public static final String SPAN_LINKS = "_dd.span_links"; - private static final String SPAN_EVENTS = "events"; + public static final String SPAN_EVENTS = "events"; public static final String LIBRARY_VERSION_TAG_KEY = "library_version"; public static final String CI_ENV_VARS = "_dd.ci.env_vars"; public static final String CI_ITR_TESTS_SKIPPED = "_dd.ci.itr.tests_skipped"; From f2bb9acd864397af7d8c0b5824fa835c6946291e Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Mon, 12 Aug 2024 12:32:25 -0400 Subject: [PATCH 03/37] added more tests and used moshi for json encoding --- .../agent-otel/otel-shim/build.gradle | 2 + .../shim/trace/OtelConventions.java | 4 +- .../opentelemetry/shim/trace/OtelSpan.java | 6 +- .../shim/trace/OtelSpanEvent.java | 54 ---------- .../opentelemetry/shim/trace/SpanEvent.java | 100 ++++++++++++++++++ .../test/groovy/OpenTelemetry14Test.groovy | 73 +++++++------ 6 files changed, 150 insertions(+), 89 deletions(-) delete mode 100644 dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java create mode 100644 dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java diff --git a/dd-java-agent/agent-otel/otel-shim/build.gradle b/dd-java-agent/agent-otel/otel-shim/build.gradle index d622712f484..79addb86442 100644 --- a/dd-java-agent/agent-otel/otel-shim/build.gradle +++ b/dd-java-agent/agent-otel/otel-shim/build.gradle @@ -6,6 +6,8 @@ minimumBranchCoverage = 0.0 dependencies { // minimum OpenTelemetry API version this shim is compatible with compileOnly group: 'io.opentelemetry', name: 'opentelemetry-api', version: '1.4.0' + implementation 'org.jetbrains:annotations:24.0.0' + implementation 'com.squareup.moshi:moshi:1.15.0' implementation project(':internal-api') } diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java index fb4ade9163a..4a02ef936b3 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java @@ -124,11 +124,11 @@ public static void applyNamingConvention(AgentSpan span) { } } - public static void setEventsAsTag(AgentSpan span, List events) { + public static void setEventsAsTag(AgentSpan span, List events) { if (events == null || events.isEmpty()) { return; } - span.setTag(DDTags.SPAN_EVENTS, OtelSpanEvent.toTag(events)); + span.setTag(DDTags.SPAN_EVENTS, SpanEvent.toTag(events)); } private static String computeOperationName(AgentSpan span) { 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 7a286029bf8..0183062b373 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 @@ -29,7 +29,7 @@ public class OtelSpan implements Span { private final AgentSpan delegate; private StatusCode statusCode; private boolean recording; - private List events; + private List events; public OtelSpan(AgentSpan delegate) { this.delegate = delegate; @@ -78,7 +78,7 @@ public Span addEvent(String name, Attributes attributes) { if (this.events == null || this.events.isEmpty()) { this.events = new ArrayList<>(); } - this.events.add(new OtelSpanEvent(name, attributes)); + this.events.add(new SpanEvent(name, attributes)); } return this; } @@ -89,7 +89,7 @@ public Span addEvent(String name, Attributes attributes, long timestamp, TimeUni if (this.events == null || this.events.isEmpty()) { this.events = new ArrayList<>(); } - this.events.add(new OtelSpanEvent(name, attributes, timestamp, unit)); + this.events.add(new SpanEvent(name, attributes, timestamp, unit)); } return this; } diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java deleted file mode 100644 index 6dba1a9cdda..00000000000 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java +++ /dev/null @@ -1,54 +0,0 @@ -package datadog.opentelemetry.shim.trace; - -import io.opentelemetry.api.common.Attributes; -import java.util.List; -import java.util.concurrent.TimeUnit; - -public class OtelSpanEvent { - private final long timestamp; - private final String name; - // attributes - - public OtelSpanEvent(String name, Attributes attributes) { - this.name = name; - this.timestamp = timeNano(); - } - - public OtelSpanEvent(String name, Attributes attributes, long timestamp, TimeUnit unit) { - this.name = name; - this.timestamp = timeNano(timestamp, unit); - } - - private static long timeNano() { - return System.nanoTime(); - } - - private static long timeNano(long timestamp, TimeUnit unit) { - return TimeUnit.NANOSECONDS.convert(timestamp, unit); - // MTOFF: Should we handle the case where the conversion returns Long.MIN_VALUE or - // Long.MAX_VALUE? see: - // https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/TimeUnit.html#convert-long-java.util.concurrent.TimeUnit- - } - - public String toString() { - // TODO if attributes exist, process those - return "{\"name\":\"" - + this.name - + "\",\"time_unix_nano\":" - + Long.toString(this.timestamp) - + "}"; - } - - // @NotNull - // TODO: give this a notnull annotation - public static String toTag(List events) { - StringBuilder builder = new StringBuilder("["); - for (int i = 0; i < events.size(); i++) { - if (i > 0) { - builder.append(","); - } - builder.append(events.get(i).toString()); - } - return builder.append("]").toString(); - } -} diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java new file mode 100644 index 00000000000..6be79dfd351 --- /dev/null +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java @@ -0,0 +1,100 @@ +package datadog.opentelemetry.shim.trace; + +import com.squareup.moshi.FromJson; +import com.squareup.moshi.JsonAdapter; +import com.squareup.moshi.Moshi; +import com.squareup.moshi.ToJson; +import io.opentelemetry.api.common.Attributes; +import java.util.List; +import java.util.concurrent.TimeUnit; +import org.jetbrains.annotations.NotNull; + +public class SpanEvent { + + private final long timestamp; + private final String name; + // attributes + + public SpanEvent(String name, Attributes attributes) { + this.name = name; + this.timestamp = timeNano(); + } + + public SpanEvent(String name, Attributes attributes, long timestamp, TimeUnit unit) { + this.name = name; + this.timestamp = timeNano(timestamp, unit); + } + + private static long timeNano() { + return System.nanoTime(); + } + + private static long timeNano(long timestamp, TimeUnit unit) { + return TimeUnit.NANOSECONDS.convert(timestamp, unit); + } + + public String toString() { + // TODO if attributes exist, process those + return "{\"name\":\"" + this.name + "\",\"time_unix_nano\":" + this.timestamp + "}"; + } + + @NotNull + public static String toTag(List events) { + StringBuilder builder = new StringBuilder("["); + int index = 0; + while (index < events.size()) { + String linkAsJson = getEncoder().toJson(events.get(index)); + if (index > 0) { + builder.append(','); + } + builder.append(linkAsJson); + index++; + } + // TODO: Do we want to enforce a maximum tag size, like TAG_MAX_LENGTH in DDSpanLink? If so, + // should this limit exist in DDTags instead (to apply to all tags moving forward)? + // If so, then we can maybe share the json encoder code between SpanEvent and DDSpanLink classes + return builder.append("]").toString(); + } + + private static JsonAdapter getEncoder() { + return EncoderHolder.ENCODER; + } + + private static class EncoderHolder { + static final JsonAdapter ENCODER = createEncoder(); + + private static JsonAdapter createEncoder() { + Moshi moshi = new Moshi.Builder().add(new SpanEventAdapter()).build(); + return moshi.adapter(SpanEvent.class); + } + } + + private static class SpanEventAdapter { + @ToJson + SpanEventJson toSpanEventJson(SpanEvent event) { + SpanEventJson json = new SpanEventJson(); + json.name = event.name; + json.time_unix_nano = event.timestamp; + // if (!link.attributes().isEmpty()) { + // json.attributes = link.attributes().asMap(); + // } + return json; + } + + @FromJson + SpanEvent fromSpanEventJson(SpanEventJson json) { + return new SpanEvent( + json.name, + // attributes + null, + json.time_unix_nano, + TimeUnit.NANOSECONDS); + } + } + + private static class SpanEventJson { + String name; + long time_unix_nano; + // Map attributes; + } +} diff --git a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy index a75e1a9a486..f2e8dc5f144 100644 --- a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy +++ b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy @@ -180,26 +180,25 @@ class OpenTelemetry14Test extends AgentTestRunner { } } - def "test multiple span events"() { + def "test add single event"() { setup: def builder = tracer.spanBuilder("some-name") when: - // Adding event is not supported def result = builder.startSpan() - result.addEvent(name1, timestamp, TimeUnit.NANOSECONDS) - result.addEvent(name2, timestamp, TimeUnit.NANOSECONDS) + result.addEvent(name, timestamp, unit) result.end() then: + long expectTime = timestamp + if (unit == TimeUnit.MILLISECONDS) { + expectTime *= 1_000_000L + } + def expectedEventTag = """ [ - { "name": "${name1}", - "time_unix_nano": ${timestamp} - }, - { "name": "${name2}", - "time_unix_nano": ${timestamp} - } + { "name": "${name}", + "time_unix_nano": ${expectTime}} ]""" assertTraces(1) { trace(1) { @@ -212,33 +211,32 @@ class OpenTelemetry14Test extends AgentTestRunner { } } } + where: - name1 = "evt1" - name2 = "evt2" - timestamp = 1723220824705 * 1_000_000L + name | attributes | timestamp | unit + "evt2" | null | TIME_MILLIS | TimeUnit.MILLISECONDS + "evt3" | null | TIME_NANO | TimeUnit.NANOSECONDS } - def "test add single event"() { + def "test multiple span events"() { setup: def builder = tracer.spanBuilder("some-name") when: - // Adding event is not supported def result = builder.startSpan() - result.addEvent(name, timestamp, unit) + result.addEvent(name, timestamp, TimeUnit.NANOSECONDS) + result.addEvent(name, timestamp, TimeUnit.NANOSECONDS) result.end() then: - long expectTime = timestamp - if (unit != TimeUnit.NANOSECONDS) { - // unit is milliseconds - expectTime = timestamp * 1_000_000L - } - def expectedEventTag = """ [ { "name": "${name}", - "time_unix_nano": ${expectTime}} + "time_unix_nano": ${timestamp} + }, + { "name": "${name}", + "time_unix_nano": ${timestamp} + } ]""" assertTraces(1) { trace(1) { @@ -251,16 +249,31 @@ class OpenTelemetry14Test extends AgentTestRunner { } } } - where: - // NOTE: Don't know how to test where timestamp is null. - name | attributes | timestamp | unit - // null | null | null | null - // "evt1" | null | null | null - "evt2" | null | TIME_MILLIS | TimeUnit.MILLISECONDS - "evt3" | null | TIME_NANO | TimeUnit.NANOSECONDS + name | timestamp | attributes + "evt1" | TIME_NANO | null + "evt2" | TIME_NANO | null } + // def "test add event no timestamp"() { + // setup: + // def builder = tracer.spanBuilder("some-name") + // + // when: + // def result = builder.startSpan() + // result.addEvent("evt", null, null) + // result.end() + // + // then: + // long endBlock = System.nanoTime(); + // + // // I'd like to grab the span's `time_unix_nano` field under `events` tag is not null, and is more than beforeBlock and less than afterBlock + // // But I don't know how to access this without asserting the span tags look a specific way using assertTraces. + // + // where: + // long startBlock = System.nanoTime(); + // } + def "test simple span links"() { setup: def traceId = "1234567890abcdef1234567890abcdef" as String From f8bc1db9cc996fee949a5638cef9c3a2ec65b136 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Mon, 12 Aug 2024 12:34:31 -0400 Subject: [PATCH 04/37] remove unused method toString on SpanEvent instance --- .../java/datadog/opentelemetry/shim/trace/SpanEvent.java | 5 ----- 1 file changed, 5 deletions(-) diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java index 6be79dfd351..1b95f7f0ef1 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java @@ -33,11 +33,6 @@ private static long timeNano(long timestamp, TimeUnit unit) { return TimeUnit.NANOSECONDS.convert(timestamp, unit); } - public String toString() { - // TODO if attributes exist, process those - return "{\"name\":\"" + this.name + "\",\"time_unix_nano\":" + this.timestamp + "}"; - } - @NotNull public static String toTag(List events) { StringBuilder builder = new StringBuilder("["); From fae43bcef4e5116704d48bb1d24fe91a744e92d2 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Tue, 13 Aug 2024 14:54:41 -0400 Subject: [PATCH 05/37] Started work on recordException --- .../datadog/opentelemetry/shim/trace/OtelSpan.java | 10 ++++++++++ 1 file changed, 10 insertions(+) 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 0183062b373..89ab5f252e3 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 @@ -113,6 +113,16 @@ public Span setStatus(StatusCode statusCode, String description) { @Override public Span recordException(Throwable exception, Attributes additionalAttributes) { if (this.recording) { + if (this.events == null || this.events.isEmpty()) { + this.events = new ArrayList<>(); + } + // ref: https://docs.oracle.com/javase/8/docs/api/java/lang/Throwable.html + // create attribute exception.message from exception.getMessage(); + // create attribute exception.type using reflection: exception.getClass().getName(); + // create attribute exception.escaped ... Doesn't even look like Otel autp adds this field: + // https://github.com/open-telemetry/opentelemetry-java/blob/v1.41.0/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/internal/data/ImmutableExceptionEventData.java#L22 + // create attribute exception.stacktrace using exception.getStackTrace + this.events.add(new SpanEvent("exception", additionalAttributes)); // Store exception as span tags as span events are not supported yet this.delegate.addThrowable(exception, ErrorPriorities.UNSET); } From 761ad85dfef7a1da410469ec8207c37b068905fb Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Tue, 13 Aug 2024 16:20:15 -0400 Subject: [PATCH 06/37] Removed moshi dependency --- .../agent-otel/otel-shim/build.gradle | 2 - .../opentelemetry/shim/trace/SpanEvent.java | 70 ++++--------------- 2 files changed, 12 insertions(+), 60 deletions(-) diff --git a/dd-java-agent/agent-otel/otel-shim/build.gradle b/dd-java-agent/agent-otel/otel-shim/build.gradle index 79addb86442..d622712f484 100644 --- a/dd-java-agent/agent-otel/otel-shim/build.gradle +++ b/dd-java-agent/agent-otel/otel-shim/build.gradle @@ -6,8 +6,6 @@ minimumBranchCoverage = 0.0 dependencies { // minimum OpenTelemetry API version this shim is compatible with compileOnly group: 'io.opentelemetry', name: 'opentelemetry-api', version: '1.4.0' - implementation 'org.jetbrains:annotations:24.0.0' - implementation 'com.squareup.moshi:moshi:1.15.0' implementation project(':internal-api') } diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java index 1b95f7f0ef1..93d445d112c 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java @@ -1,13 +1,9 @@ package datadog.opentelemetry.shim.trace; -import com.squareup.moshi.FromJson; -import com.squareup.moshi.JsonAdapter; -import com.squareup.moshi.Moshi; -import com.squareup.moshi.ToJson; +import edu.umd.cs.findbugs.annotations.NonNull; import io.opentelemetry.api.common.Attributes; import java.util.List; import java.util.concurrent.TimeUnit; -import org.jetbrains.annotations.NotNull; public class SpanEvent { @@ -33,63 +29,21 @@ private static long timeNano(long timestamp, TimeUnit unit) { return TimeUnit.NANOSECONDS.convert(timestamp, unit); } - @NotNull + public String toString() { + // TODO if attributes exist, process those + return "{\"name\":\"" + this.name + "\",\"time_unix_nano\":" + this.timestamp + "}"; + } + + @NonNull public static String toTag(List events) { + // TODO: Do we want to enforce a maximum tag size, like TAG_MAX_LENGTH in DDSpanLink? StringBuilder builder = new StringBuilder("["); - int index = 0; - while (index < events.size()) { - String linkAsJson = getEncoder().toJson(events.get(index)); - if (index > 0) { - builder.append(','); + for (int i = 0; i < events.size(); i++) { + if (i > 0) { + builder.append(","); } - builder.append(linkAsJson); - index++; + builder.append(events.get(i).toString()); } - // TODO: Do we want to enforce a maximum tag size, like TAG_MAX_LENGTH in DDSpanLink? If so, - // should this limit exist in DDTags instead (to apply to all tags moving forward)? - // If so, then we can maybe share the json encoder code between SpanEvent and DDSpanLink classes return builder.append("]").toString(); } - - private static JsonAdapter getEncoder() { - return EncoderHolder.ENCODER; - } - - private static class EncoderHolder { - static final JsonAdapter ENCODER = createEncoder(); - - private static JsonAdapter createEncoder() { - Moshi moshi = new Moshi.Builder().add(new SpanEventAdapter()).build(); - return moshi.adapter(SpanEvent.class); - } - } - - private static class SpanEventAdapter { - @ToJson - SpanEventJson toSpanEventJson(SpanEvent event) { - SpanEventJson json = new SpanEventJson(); - json.name = event.name; - json.time_unix_nano = event.timestamp; - // if (!link.attributes().isEmpty()) { - // json.attributes = link.attributes().asMap(); - // } - return json; - } - - @FromJson - SpanEvent fromSpanEventJson(SpanEventJson json) { - return new SpanEvent( - json.name, - // attributes - null, - json.time_unix_nano, - TimeUnit.NANOSECONDS); - } - } - - private static class SpanEventJson { - String name; - long time_unix_nano; - // Map attributes; - } } From ea4d8dfbbfeed1f2701477ddeb0a855a85df8f32 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Thu, 15 Aug 2024 13:50:48 -0400 Subject: [PATCH 07/37] Fixed case where custom attributes should override default excpeption attributes if conflict --- .../shim/trace/OtelConventions.java | 34 ++++++- .../opentelemetry/shim/trace/OtelSpan.java | 11 +-- .../opentelemetry/shim/trace/SpanEvent.java | 15 +++- .../test/groovy/OpenTelemetry14Test.groovy | 89 +++++++++++-------- .../instrumentation/api/SpanAttributes.java | 41 ++++++++- 5 files changed, 142 insertions(+), 48 deletions(-) diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java index 76c1fcb1dd6..fbab56d64b4 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java @@ -20,8 +20,12 @@ import datadog.trace.bootstrap.instrumentation.api.Tags; import io.opentelemetry.api.common.AttributeKey; import io.opentelemetry.api.common.Attributes; +import io.opentelemetry.api.common.AttributesBuilder; import io.opentelemetry.api.trace.SpanKind; +import java.util.Arrays; +import java.util.HashMap; import java.util.List; +import java.util.Map; import javax.annotation.Nullable; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -237,7 +241,7 @@ private static String getStringAttribute(AgentSpan span, String key) { } public static AgentSpan.Attributes convertAttributes(Attributes attributes) { - if (attributes.isEmpty()) { + if (attributes == null || attributes.isEmpty()) { return SpanAttributes.EMPTY; } SpanAttributes.Builder builder = SpanAttributes.builder(); @@ -277,4 +281,32 @@ public static AgentSpan.Attributes convertAttributes(Attributes attributes) { }); return builder.build(); } + + public static Attributes processExceptionAttributes( + Throwable exception, Attributes additionalAttributes) { + // "exception.escaped" should be true if exception will "escape" the scope of the span. I'm not + // sure how to check that from within here. And it doesn't even look like Otel dynamically sets + // this field: + // https://github.com/open-telemetry/opentelemetry-java/blob/v1.41.0/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/internal/data/ImmutableExceptionEventData.java#L22 + boolean escaped = false; + Map defaultAttributes = + new HashMap() { + { + put("exception.message", exception.getMessage()); + put("exception.type", exception.getClass().getName()); + put("exception.escaped", String.valueOf(escaped)); + put("exception.stacktrace", Arrays.toString(exception.getStackTrace())); + } + }; + // Create an AttributesBuilder with the additionalAttributes + AttributesBuilder attrsBuilder = additionalAttributes.toBuilder(); + for (String key : defaultAttributes.keySet()) { + // Add defaultAttributes onto the builder iff an equivalent key was not provided in + // additionalAttributes + if (additionalAttributes.get(AttributeKey.stringKey(key)) == null) { + attrsBuilder.put(key, defaultAttributes.get(key)); + } + } + return attrsBuilder.build(); + } } 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 89ab5f252e3..79fa64d28ba 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 @@ -2,6 +2,7 @@ import static datadog.opentelemetry.shim.trace.OtelConventions.applyNamingConvention; import static datadog.opentelemetry.shim.trace.OtelConventions.applyReservedAttribute; +import static datadog.opentelemetry.shim.trace.OtelConventions.processExceptionAttributes; import static datadog.opentelemetry.shim.trace.OtelConventions.setEventsAsTag; import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activateSpan; import static io.opentelemetry.api.trace.StatusCode.ERROR; @@ -116,14 +117,8 @@ public Span recordException(Throwable exception, Attributes additionalAttributes if (this.events == null || this.events.isEmpty()) { this.events = new ArrayList<>(); } - // ref: https://docs.oracle.com/javase/8/docs/api/java/lang/Throwable.html - // create attribute exception.message from exception.getMessage(); - // create attribute exception.type using reflection: exception.getClass().getName(); - // create attribute exception.escaped ... Doesn't even look like Otel autp adds this field: - // https://github.com/open-telemetry/opentelemetry-java/blob/v1.41.0/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/internal/data/ImmutableExceptionEventData.java#L22 - // create attribute exception.stacktrace using exception.getStackTrace - this.events.add(new SpanEvent("exception", additionalAttributes)); - // Store exception as span tags as span events are not supported yet + this.events.add( + new SpanEvent("exception", processExceptionAttributes(exception, additionalAttributes))); this.delegate.addThrowable(exception, ErrorPriorities.UNSET); } return this; diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java index 93d445d112c..953cb5d1f67 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java @@ -1,5 +1,7 @@ package datadog.opentelemetry.shim.trace; +import datadog.trace.bootstrap.instrumentation.api.AgentSpan; +import datadog.trace.bootstrap.instrumentation.api.SpanAttributes; import edu.umd.cs.findbugs.annotations.NonNull; import io.opentelemetry.api.common.Attributes; import java.util.List; @@ -9,15 +11,17 @@ public class SpanEvent { private final long timestamp; private final String name; - // attributes + private final AgentSpan.Attributes attributes; public SpanEvent(String name, Attributes attributes) { this.name = name; + this.attributes = OtelConventions.convertAttributes(attributes); this.timestamp = timeNano(); } public SpanEvent(String name, Attributes attributes, long timestamp, TimeUnit unit) { this.name = name; + this.attributes = OtelConventions.convertAttributes(attributes); this.timestamp = timeNano(timestamp, unit); } @@ -30,8 +34,13 @@ private static long timeNano(long timestamp, TimeUnit unit) { } public String toString() { - // TODO if attributes exist, process those - return "{\"name\":\"" + this.name + "\",\"time_unix_nano\":" + this.timestamp + "}"; + StringBuilder builder = + new StringBuilder( + "{\"time_unix_nano\":" + this.timestamp + ",\"name\":\"" + this.name + "\""); + if (!this.attributes.isEmpty()) { + builder.append(",\"attributes\":").append(SpanAttributes.toJson(this.attributes.asMap())); + } + return builder.append("}").toString(); } @NonNull diff --git a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy index f2e8dc5f144..0b72de716f9 100644 --- a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy +++ b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy @@ -19,6 +19,7 @@ import java.sql.Time import java.util.concurrent.TimeUnit import static datadog.trace.api.DDTags.ERROR_MSG +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 @@ -34,6 +35,9 @@ import static io.opentelemetry.api.trace.StatusCode.ERROR import static io.opentelemetry.api.trace.StatusCode.OK import static io.opentelemetry.api.trace.StatusCode.UNSET +class TestHelpers { +} + class OpenTelemetry14Test extends AgentTestRunner { static final TIME_MILLIS = 1723220824705 static final TIME_NANO = TIME_MILLIS * 1_000_000L @@ -186,7 +190,7 @@ class OpenTelemetry14Test extends AgentTestRunner { when: def result = builder.startSpan() - result.addEvent(name, timestamp, unit) + result.addEvent(name, attributes, timestamp, unit) result.end() then: @@ -197,8 +201,10 @@ class OpenTelemetry14Test extends AgentTestRunner { def expectedEventTag = """ [ - { "name": "${name}", - "time_unix_nano": ${expectTime}} + { "time_unix_nano": ${expectTime}, + "name": "${name}" + ${ expectedAttributes == null ? "" : ", attributes: " + expectedAttributes } + } ]""" assertTraces(1) { trace(1) { @@ -213,9 +219,10 @@ class OpenTelemetry14Test extends AgentTestRunner { } where: - name | attributes | timestamp | unit - "evt2" | null | TIME_MILLIS | TimeUnit.MILLISECONDS - "evt3" | null | TIME_NANO | TimeUnit.NANOSECONDS + name | timestamp | unit | attributes | expectedAttributes + "evt1" | TIME_MILLIS | TimeUnit.MILLISECONDS | Attributes.empty() | null + "evt2" | TIME_NANO | TimeUnit.NANOSECONDS | Attributes.builder().put("string-key", "string-value").put("long-key", 123456789L).put("double-key", 1234.5678D).put("boolean-key-true", true).put("boolean-key-false", false).build() | '{ string-key: "string-value", long-key: "123456789", double-key: "1234.5678", boolean-key-true: "true", boolean-key-false: "false" }' + "evt3" | TIME_NANO | TimeUnit.NANOSECONDS | Attributes.builder().put("string-key-array", "string-value1", "string-value2", "string-value3").put("long-key-array", 123456L, 1234567L, 12345678L).put("double-key-array", 1234.5D, 1234.56D, 1234.567D).put("boolean-key-array", true, false, true).build() | '{ string-key-array.0: "string-value1", string-key-array.1: "string-value2", string-key-array.2: "string-value3", long-key-array.0: "123456", long-key-array.1: "1234567", long-key-array.2: "12345678", double-key-array.0: "1234.5", double-key-array.1: "1234.56", double-key-array.2: "1234.567", boolean-key-array.0: "true", boolean-key-array.1: "false", boolean-key-array.2: "true" }' } def "test multiple span events"() { @@ -224,18 +231,20 @@ class OpenTelemetry14Test extends AgentTestRunner { when: def result = builder.startSpan() - result.addEvent(name, timestamp, TimeUnit.NANOSECONDS) - result.addEvent(name, timestamp, TimeUnit.NANOSECONDS) + result.addEvent("evt1", null, TIME_NANO, TimeUnit.NANOSECONDS) + result.addEvent("evt2", Attributes.builder().put("string-key", "string-value").build(), TIME_NANO, TimeUnit.NANOSECONDS) result.end() then: + def expectedAttributes = '{"string-key": "string-value"}' def expectedEventTag = """ [ - { "name": "${name}", - "time_unix_nano": ${timestamp} + { "time_unix_nano": ${TIME_NANO}, + "name": "evt1" }, - { "name": "${name}", - "time_unix_nano": ${timestamp} + { "time_unix_nano": ${TIME_NANO}, + "name": "evt2", + ${"attributes: " + expectedAttributes} } ]""" assertTraces(1) { @@ -249,30 +258,26 @@ class OpenTelemetry14Test extends AgentTestRunner { } } } - where: - name | timestamp | attributes - "evt1" | TIME_NANO | null - "evt2" | TIME_NANO | null } - // def "test add event no timestamp"() { - // setup: - // def builder = tracer.spanBuilder("some-name") + // def "test add event no timestamp"() { + // setup: + // def builder = tracer.spanBuilder("some-name") // - // when: - // def result = builder.startSpan() - // result.addEvent("evt", null, null) - // result.end() + // when: + // def result = builder.startSpan() + // result.addEvent("evt", null, null) + // result.end() // - // then: - // long endBlock = System.nanoTime(); + // then: + // long endBlock = System.nanoTime(); // - // // I'd like to grab the span's `time_unix_nano` field under `events` tag is not null, and is more than beforeBlock and less than afterBlock - // // But I don't know how to access this without asserting the span tags look a specific way using assertTraces. + // // I'd like to grab the span's `time_unix_nano` field under `events` tag is not null, and is more than beforeBlock and less than afterBlock + // // But I don't know how to access this without asserting the span tags look a specific way using assertTraces. // - // where: - // long startBlock = System.nanoTime(); - // } + // where: + // long startBlock = System.nanoTime(); + // } def "test simple span links"() { setup: @@ -642,8 +647,6 @@ class OpenTelemetry14Test extends AgentTestRunner { def "test span record exception"() { setup: def result = tracer.spanBuilder("some-name").startSpan() - def message = "input can't be null" - def exception = new InvalidParameterException(message) expect: result.delegate.getTag(ERROR_MSG) == null @@ -652,11 +655,11 @@ class OpenTelemetry14Test extends AgentTestRunner { !result.delegate.isError() when: - result.recordException(exception) + result.recordException(exception, attributes) then: - result.delegate.getTag(ERROR_MSG) == message - result.delegate.getTag(DDTags.ERROR_TYPE) == InvalidParameterException.name + result.delegate.getTag(ERROR_MSG) == exception.getMessage() + result.delegate.getTag(DDTags.ERROR_TYPE) == exception.getClass().getName() result.delegate.getTag(DDTags.ERROR_STACK) != null !result.delegate.isError() @@ -664,6 +667,15 @@ class OpenTelemetry14Test extends AgentTestRunner { result.end() then: + // TODO: Same issue with time_unix_nano here -- I can't control what it is and don't know how to check the name and attributes without it + // Need some way to access sub-tags of `"events"` as you would a map. + def expectedEventTag = """ + [ + { "time_unix_nano": ${TIME_NANO}, + "name": "exception", + "attributes": ${expectedAttributes} + } + ]""" assertTraces(1) { trace(1) { span { @@ -675,10 +687,17 @@ class OpenTelemetry14Test extends AgentTestRunner { defaultTags() "$SPAN_KIND" "$SPAN_KIND_INTERNAL" errorTags(exception) + tag("events", { JSONAssert.assertEquals(expectedEventTag, it as String, false); return true }) } } } } + + where: + exception | attributes | expectedAttributes + // new NullPointerException("Null pointer") | Attributes.empty() | """{ exception.message: "${exception.getMessage()}", exception.type: "${exception.getClass().getName()}", exception.escaped: "false", exception.stacktrace: "${Arrays.toString(exception.getStackTrace())}"}""" + new NumberFormatException("Number format exception") | Attributes.builder().put("exception.message", "something-else").build() | """{exception.message: "something-else", exception.type: "${exception.getClass().getName()}", exception.escaped: "false", exception.stacktrace: "${Arrays.toString(exception.getStackTrace())}"}""" + // new NullPointerException("Null pointer") | Attributes.builder().put("key", "value").build() | """{"key": "value","exception.message": "${exception.getMessage()}"}""" } def "test span name update"() { diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java index 46123e43a13..7e1061195d7 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java @@ -50,7 +50,46 @@ public boolean isEmpty() { @Override public String toString() { - return "SpanAttributes{" + this.attributes + '}'; + return "SpanAttributes:{" + this.attributes + "}"; + } + + public static String toJson(Map map) { + StringBuilder jsonBuilder = new StringBuilder(); + jsonBuilder.append("{"); + + boolean first = true; + for (Map.Entry entry : map.entrySet()) { + if (!first) { + jsonBuilder.append(","); + } + first = false; + + // Append the key and value + appendJsonString(jsonBuilder, entry.getKey(), entry.getValue()); + } + + jsonBuilder.append("}"); + return jsonBuilder.toString(); + } + + private static void appendJsonString(StringBuilder jsonBuilder, String key, String value) { + // Append the key (enclosed in double quotes) + jsonBuilder.append("\"").append(escapeJson(key)).append("\":"); + + // Append the value (enclosed in double quotes) + jsonBuilder.append("\"").append(escapeJson(value)).append("\""); + } + + private static String escapeJson(String value) { + // Replace special characters with their escaped counterparts + return value + .replace("\\", "\\\\") + .replace("\"", "\\\"") + .replace("\b", "\\b") + .replace("\f", "\\f") + .replace("\n", "\\n") + .replace("\r", "\\r") + .replace("\t", "\\t"); } public static class Builder { From 00e47a5a7c27414a51ab9efdfc33a7a3578d3da6 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Thu, 15 Aug 2024 16:05:43 -0400 Subject: [PATCH 08/37] Finishing up record exception unit tests --- .../main/java/datadog/opentelemetry/shim/trace/SpanEvent.java | 2 ++ .../src/test/groovy/OpenTelemetry14Test.groovy | 2 ++ 2 files changed, 4 insertions(+) diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java index 953cb5d1f67..f2df01ffc9d 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java @@ -55,4 +55,6 @@ public static String toTag(List events) { } return builder.append("]").toString(); } + + protected void changeThis() {} } diff --git a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy index 0b72de716f9..a6c961de19c 100644 --- a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy +++ b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy @@ -1,3 +1,4 @@ +import datadog.opentelemetry.shim.trace.SpanEvent import datadog.trace.agent.test.AgentTestRunner import datadog.trace.api.DDSpanId import datadog.trace.api.DDTags @@ -187,6 +188,7 @@ class OpenTelemetry14Test extends AgentTestRunner { def "test add single event"() { setup: def builder = tracer.spanBuilder("some-name") + SpanEvent.changeThis() when: def result = builder.startSpan() From 80b8adf438c365d171b12e21e6cff5c15d4ba1e5 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Thu, 15 Aug 2024 16:41:23 -0400 Subject: [PATCH 09/37] Added annotations for clarity --- .../shim/trace/OtelConventions.java | 13 +++- .../opentelemetry/shim/trace/SpanEvent.java | 6 +- .../test/groovy/OpenTelemetry14Test.groovy | 7 +- .../instrumentation/api/SpanAttributes.java | 65 ++++++++++--------- 4 files changed, 52 insertions(+), 39 deletions(-) diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java index fbab56d64b4..949797bfd30 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java @@ -282,6 +282,17 @@ public static AgentSpan.Attributes convertAttributes(Attributes attributes) { return builder.build(); } + /** + * Generate Attributes about the exception using a default list + the additionalAttributes + * provided by the user + * + *

If the same key exists in defaultAttributes and additionalAttributes, the latter always + * wins. + * + * @param exception The Throwable from which to build default attributes + * @param additionalAttributes Attributes provided by the user + * @return Attributes collection that combines defaultAttributes and additionalAttributes + */ public static Attributes processExceptionAttributes( Throwable exception, Attributes additionalAttributes) { // "exception.escaped" should be true if exception will "escape" the scope of the span. I'm not @@ -298,7 +309,7 @@ public static Attributes processExceptionAttributes( put("exception.stacktrace", Arrays.toString(exception.getStackTrace())); } }; - // Create an AttributesBuilder with the additionalAttributes + // Create an AttributesBuilder with the additionalAttributes provided AttributesBuilder attrsBuilder = additionalAttributes.toBuilder(); for (String key : defaultAttributes.keySet()) { // Add defaultAttributes onto the builder iff an equivalent key was not provided in diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java index f2df01ffc9d..f2871b7a966 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java @@ -38,7 +38,9 @@ public String toString() { new StringBuilder( "{\"time_unix_nano\":" + this.timestamp + ",\"name\":\"" + this.name + "\""); if (!this.attributes.isEmpty()) { - builder.append(",\"attributes\":").append(SpanAttributes.toJson(this.attributes.asMap())); + builder + .append(",\"attributes\":") + .append(SpanAttributes.JSONParser.toJson(this.attributes.asMap())); } return builder.append("}").toString(); } @@ -55,6 +57,4 @@ public static String toTag(List events) { } return builder.append("]").toString(); } - - protected void changeThis() {} } diff --git a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy index a6c961de19c..330d9eaec4f 100644 --- a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy +++ b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy @@ -188,7 +188,6 @@ class OpenTelemetry14Test extends AgentTestRunner { def "test add single event"() { setup: def builder = tracer.spanBuilder("some-name") - SpanEvent.changeThis() when: def result = builder.startSpan() @@ -646,6 +645,7 @@ class OpenTelemetry14Test extends AgentTestRunner { } } + // This is failing because of the time_unix_nano issue described on the "test add event no timestamp" scenario def "test span record exception"() { setup: def result = tracer.spanBuilder("some-name").startSpan() @@ -670,7 +670,6 @@ class OpenTelemetry14Test extends AgentTestRunner { then: // TODO: Same issue with time_unix_nano here -- I can't control what it is and don't know how to check the name and attributes without it - // Need some way to access sub-tags of `"events"` as you would a map. def expectedEventTag = """ [ { "time_unix_nano": ${TIME_NANO}, @@ -697,9 +696,9 @@ class OpenTelemetry14Test extends AgentTestRunner { where: exception | attributes | expectedAttributes - // new NullPointerException("Null pointer") | Attributes.empty() | """{ exception.message: "${exception.getMessage()}", exception.type: "${exception.getClass().getName()}", exception.escaped: "false", exception.stacktrace: "${Arrays.toString(exception.getStackTrace())}"}""" + new NullPointerException("Null pointer") | Attributes.empty() | """{ exception.message: "${exception.getMessage()}", exception.type: "${exception.getClass().getName()}", exception.escaped: "false", exception.stacktrace: "${Arrays.toString(exception.getStackTrace())}"}""" new NumberFormatException("Number format exception") | Attributes.builder().put("exception.message", "something-else").build() | """{exception.message: "something-else", exception.type: "${exception.getClass().getName()}", exception.escaped: "false", exception.stacktrace: "${Arrays.toString(exception.getStackTrace())}"}""" - // new NullPointerException("Null pointer") | Attributes.builder().put("key", "value").build() | """{"key": "value","exception.message": "${exception.getMessage()}"}""" + new NullPointerException("Null pointer") | Attributes.builder().put("key", "value").build() | """{"key": "value","exception.message": "${exception.getMessage()}"}""" } def "test span name update"() { diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java index 7e1061195d7..c5bc0408e30 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java @@ -53,43 +53,46 @@ public String toString() { return "SpanAttributes:{" + this.attributes + "}"; } - public static String toJson(Map map) { - StringBuilder jsonBuilder = new StringBuilder(); - jsonBuilder.append("{"); - - boolean first = true; - for (Map.Entry entry : map.entrySet()) { - if (!first) { - jsonBuilder.append(","); + // Helper class for turning the Map that holds the attributes into a JSON string + public static class JSONParser { + public static String toJson(Map map) { + StringBuilder jsonBuilder = new StringBuilder(); + jsonBuilder.append("{"); + + boolean first = true; + for (Map.Entry entry : map.entrySet()) { + if (!first) { + jsonBuilder.append(","); + } + first = false; + + // Append the key and value + appendJsonString(jsonBuilder, entry.getKey(), entry.getValue()); } - first = false; - // Append the key and value - appendJsonString(jsonBuilder, entry.getKey(), entry.getValue()); + jsonBuilder.append("}"); + return jsonBuilder.toString(); } - jsonBuilder.append("}"); - return jsonBuilder.toString(); - } - - private static void appendJsonString(StringBuilder jsonBuilder, String key, String value) { - // Append the key (enclosed in double quotes) - jsonBuilder.append("\"").append(escapeJson(key)).append("\":"); + private static void appendJsonString(StringBuilder jsonBuilder, String key, String value) { + // Append the key (enclosed in double quotes) + jsonBuilder.append("\"").append(escapeJson(key)).append("\":"); - // Append the value (enclosed in double quotes) - jsonBuilder.append("\"").append(escapeJson(value)).append("\""); - } + // Append the value (enclosed in double quotes) + jsonBuilder.append("\"").append(escapeJson(value)).append("\""); + } - private static String escapeJson(String value) { - // Replace special characters with their escaped counterparts - return value - .replace("\\", "\\\\") - .replace("\"", "\\\"") - .replace("\b", "\\b") - .replace("\f", "\\f") - .replace("\n", "\\n") - .replace("\r", "\\r") - .replace("\t", "\\t"); + private static String escapeJson(String value) { + // Replace special characters with their escaped counterparts + return value + .replace("\\", "\\\\") + .replace("\"", "\\\"") + .replace("\b", "\\b") + .replace("\f", "\\f") + .replace("\n", "\\n") + .replace("\r", "\\r") + .replace("\t", "\\t"); + } } public static class Builder { From 4e84b96380e8db616f2c662fe7802d539bdf4ca4 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Thu, 15 Aug 2024 16:50:00 -0400 Subject: [PATCH 10/37] nits: reverted little changes that were superfluous --- .../src/test/groovy/OpenTelemetry14Test.groovy | 3 --- .../trace/bootstrap/instrumentation/api/SpanAttributes.java | 2 +- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy index 330d9eaec4f..5a4e98c8b93 100644 --- a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy +++ b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy @@ -36,9 +36,6 @@ import static io.opentelemetry.api.trace.StatusCode.ERROR import static io.opentelemetry.api.trace.StatusCode.OK import static io.opentelemetry.api.trace.StatusCode.UNSET -class TestHelpers { -} - class OpenTelemetry14Test extends AgentTestRunner { static final TIME_MILLIS = 1723220824705 static final TIME_NANO = TIME_MILLIS * 1_000_000L diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java index c5bc0408e30..1adc8f28428 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java @@ -50,7 +50,7 @@ public boolean isEmpty() { @Override public String toString() { - return "SpanAttributes:{" + this.attributes + "}"; + return "SpanAttributes{" + this.attributes + '}'; } // Helper class for turning the Map that holds the attributes into a JSON string From 35dd084761dbf0828a25bcc401263e666ac36669 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Thu, 15 Aug 2024 16:55:13 -0400 Subject: [PATCH 11/37] nits: reverted little changes that were superfluous --- .../src/test/groovy/OpenTelemetry14Test.groovy | 3 --- 1 file changed, 3 deletions(-) diff --git a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy index 5a4e98c8b93..9dd3e124416 100644 --- a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy +++ b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy @@ -15,12 +15,9 @@ import opentelemetry14.context.propagation.TextMap import org.skyscreamer.jsonassert.JSONAssert import spock.lang.Subject -import java.security.InvalidParameterException -import java.sql.Time import java.util.concurrent.TimeUnit import static datadog.trace.api.DDTags.ERROR_MSG -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 From a7626bbc77eca2b2824538b700a6fe4b504f32b1 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Fri, 16 Aug 2024 12:36:28 -0400 Subject: [PATCH 12/37] Include new helperClassNames --- .../opentelemetry14/OpenTelemetryInstrumentation.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/OpenTelemetryInstrumentation.java b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/OpenTelemetryInstrumentation.java index fa049fb6bc2..bfa825a6ad7 100644 --- a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/OpenTelemetryInstrumentation.java +++ b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/OpenTelemetryInstrumentation.java @@ -66,6 +66,7 @@ public String[] helperClassNames() { "datadog.opentelemetry.shim.trace.OtelExtractedContext", "datadog.opentelemetry.shim.trace.OtelConventions", "datadog.opentelemetry.shim.trace.OtelConventions$1", + "datadog.opentelemetry.shim.trace.OtelConventions$2", "datadog.opentelemetry.shim.trace.OtelSpan", "datadog.opentelemetry.shim.trace.OtelSpan$1", "datadog.opentelemetry.shim.trace.OtelSpan$NoopSpan", @@ -77,6 +78,7 @@ public String[] helperClassNames() { "datadog.opentelemetry.shim.trace.OtelTracer", "datadog.opentelemetry.shim.trace.OtelTracerBuilder", "datadog.opentelemetry.shim.trace.OtelTracerProvider", + "datadog.opentelemetry.shim.trace.SpanEvent", }; } From c61156756de734a5974439c12c337c31fd8b680d Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Fri, 16 Aug 2024 14:51:22 -0400 Subject: [PATCH 13/37] Added support for not-stringifying span events attributes, while maintaing stringification for span link attributes --- .../shim/trace/OtelConventions.java | 18 ++-- .../shim/trace/OtelSpanLink.java | 2 +- .../opentelemetry/shim/trace/SpanEvent.java | 4 +- .../test/groovy/OpenTelemetry14Test.groovy | 8 +- .../java/datadog/trace/core/DDSpanLink.java | 2 +- .../instrumentation/api/AgentSpan.java | 2 +- .../instrumentation/api/SpanAttributes.java | 85 +++++++++++++------ 7 files changed, 78 insertions(+), 43 deletions(-) diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java index 949797bfd30..c25a0895c01 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java @@ -240,11 +240,11 @@ private static String getStringAttribute(AgentSpan span, String key) { return (String) tag; } - public static AgentSpan.Attributes convertAttributes(Attributes attributes) { + public static AgentSpan.Attributes convertAttributes(Attributes attributes, boolean stringify) { if (attributes == null || attributes.isEmpty()) { return SpanAttributes.EMPTY; } - SpanAttributes.Builder builder = SpanAttributes.builder(); + SpanAttributes.Builder builder = SpanAttributes.builder(stringify); attributes.forEach( (attributeKey, value) -> { String key = attributeKey.getKey(); @@ -300,12 +300,12 @@ public static Attributes processExceptionAttributes( // this field: // https://github.com/open-telemetry/opentelemetry-java/blob/v1.41.0/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/internal/data/ImmutableExceptionEventData.java#L22 boolean escaped = false; - Map defaultAttributes = - new HashMap() { + Map defaultAttributes = + new HashMap() { { put("exception.message", exception.getMessage()); put("exception.type", exception.getClass().getName()); - put("exception.escaped", String.valueOf(escaped)); + put("exception.escaped", escaped); put("exception.stacktrace", Arrays.toString(exception.getStackTrace())); } }; @@ -315,7 +315,13 @@ public static Attributes processExceptionAttributes( // Add defaultAttributes onto the builder iff an equivalent key was not provided in // additionalAttributes if (additionalAttributes.get(AttributeKey.stringKey(key)) == null) { - attrsBuilder.put(key, defaultAttributes.get(key)); + // naive implementation, as the default attributes values re only strings and booleans + Object value = defaultAttributes.get(key); + if (value instanceof String) { + attrsBuilder.put(key, (String) defaultAttributes.get(key)); + } else if (value instanceof Boolean) { + attrsBuilder.put(key, (Boolean) defaultAttributes.get(key)); + } } } return attrsBuilder.build(); diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanLink.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanLink.java index 81255e15027..02eb4d7e8ee 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanLink.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanLink.java @@ -19,6 +19,6 @@ public OtelSpanLink(SpanContext spanContext, io.opentelemetry.api.common.Attribu DDSpanId.fromHex(spanContext.getSpanId()), spanContext.isSampled() ? SAMPLED_FLAG : DEFAULT_FLAGS, TraceStateHelper.encodeHeader(spanContext.getTraceState()), - convertAttributes(attributes)); + convertAttributes(attributes, true)); } } diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java index f2871b7a966..b6adb0ba505 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java @@ -15,13 +15,13 @@ public class SpanEvent { public SpanEvent(String name, Attributes attributes) { this.name = name; - this.attributes = OtelConventions.convertAttributes(attributes); + this.attributes = OtelConventions.convertAttributes(attributes, false); this.timestamp = timeNano(); } public SpanEvent(String name, Attributes attributes, long timestamp, TimeUnit unit) { this.name = name; - this.attributes = OtelConventions.convertAttributes(attributes); + this.attributes = OtelConventions.convertAttributes(attributes, false); this.timestamp = timeNano(timestamp, unit); } diff --git a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy index 9dd3e124416..738afa8bfca 100644 --- a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy +++ b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy @@ -216,8 +216,8 @@ class OpenTelemetry14Test extends AgentTestRunner { where: name | timestamp | unit | attributes | expectedAttributes "evt1" | TIME_MILLIS | TimeUnit.MILLISECONDS | Attributes.empty() | null - "evt2" | TIME_NANO | TimeUnit.NANOSECONDS | Attributes.builder().put("string-key", "string-value").put("long-key", 123456789L).put("double-key", 1234.5678D).put("boolean-key-true", true).put("boolean-key-false", false).build() | '{ string-key: "string-value", long-key: "123456789", double-key: "1234.5678", boolean-key-true: "true", boolean-key-false: "false" }' - "evt3" | TIME_NANO | TimeUnit.NANOSECONDS | Attributes.builder().put("string-key-array", "string-value1", "string-value2", "string-value3").put("long-key-array", 123456L, 1234567L, 12345678L).put("double-key-array", 1234.5D, 1234.56D, 1234.567D).put("boolean-key-array", true, false, true).build() | '{ string-key-array.0: "string-value1", string-key-array.1: "string-value2", string-key-array.2: "string-value3", long-key-array.0: "123456", long-key-array.1: "1234567", long-key-array.2: "12345678", double-key-array.0: "1234.5", double-key-array.1: "1234.56", double-key-array.2: "1234.567", boolean-key-array.0: "true", boolean-key-array.1: "false", boolean-key-array.2: "true" }' + "evt2" | TIME_NANO | TimeUnit.NANOSECONDS | Attributes.builder().put("string-key", "string-value").put("long-key", 123456789L).put("double-key", 1234.5678D).put("boolean-key-true", true).put("boolean-key-false", false).build() | '{ string-key: "string-value", long-key: 123456789, double-key: 1234.5678, boolean-key-true: true, boolean-key-false: false }' + "evt3" | TIME_NANO | TimeUnit.NANOSECONDS | Attributes.builder().put("string-key-array", "string-value1", "string-value2", "string-value3").put("long-key-array", 123456L, 1234567L, 12345678L).put("double-key-array", 1234.5D, 1234.56D, 1234.567D).put("boolean-key-array", true, false, true).build() | '{ string-key-array.0: "string-value1", string-key-array.1: "string-value2", string-key-array.2: "string-value3", long-key-array.0: 123456, long-key-array.1: 1234567, long-key-array.2: 12345678, double-key-array.0: 1234.5, double-key-array.1: 1234.56, double-key-array.2: 1234.567, boolean-key-array.0: true, boolean-key-array.1: false, boolean-key-array.2: true }' } def "test multiple span events"() { @@ -690,8 +690,8 @@ class OpenTelemetry14Test extends AgentTestRunner { where: exception | attributes | expectedAttributes - new NullPointerException("Null pointer") | Attributes.empty() | """{ exception.message: "${exception.getMessage()}", exception.type: "${exception.getClass().getName()}", exception.escaped: "false", exception.stacktrace: "${Arrays.toString(exception.getStackTrace())}"}""" - new NumberFormatException("Number format exception") | Attributes.builder().put("exception.message", "something-else").build() | """{exception.message: "something-else", exception.type: "${exception.getClass().getName()}", exception.escaped: "false", exception.stacktrace: "${Arrays.toString(exception.getStackTrace())}"}""" + new NullPointerException("Null pointer") | Attributes.empty() | """{ exception.message: "${exception.getMessage()}", exception.type: "${exception.getClass().getName()}", exception.escaped: false, exception.stacktrace: "${Arrays.toString(exception.getStackTrace())}"}""" + new NumberFormatException("Number format exception") | Attributes.builder().put("exception.message", "something-else").build() | """{exception.message: "something-else", exception.type: "${exception.getClass().getName()}", exception.escaped: false, exception.stacktrace: "${Arrays.toString(exception.getStackTrace())}"}""" new NullPointerException("Null pointer") | Attributes.builder().put("key", "value").build() | """{"key": "value","exception.message": "${exception.getMessage()}"}""" } diff --git a/dd-trace-core/src/main/java/datadog/trace/core/DDSpanLink.java b/dd-trace-core/src/main/java/datadog/trace/core/DDSpanLink.java index 641e6fe56ab..d3d8a5a6e5a 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/DDSpanLink.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/DDSpanLink.java @@ -140,6 +140,6 @@ private static class SpanLinkJson { String span_id; Byte flags; String tracestate; - Map attributes; + Map attributes; } } diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentSpan.java b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentSpan.java index 6b55ad91b9d..eb0566ff2dd 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentSpan.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentSpan.java @@ -235,7 +235,7 @@ interface Attributes { * * @return The attributes as an immutable map. */ - Map asMap(); + Map asMap(); /** * Checks whether the attributes are empty. diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java index 1adc8f28428..167ff6ecdf2 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java @@ -7,15 +7,16 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Set; /** This class is a base implementation of {@link Attributes}. */ public class SpanAttributes implements Attributes { /** Represent an empty attributes. */ public static final Attributes EMPTY = new SpanAttributes(Collections.emptyMap()); - private final Map attributes; + private final Map attributes; - protected SpanAttributes(Map attributes) { + protected SpanAttributes(Map attributes) { this.attributes = attributes; } @@ -24,8 +25,8 @@ protected SpanAttributes(Map attributes) { * * @return A builder to create attributes. */ - public static Builder builder() { - return new Builder(); + public static Builder builder(boolean stringify) { + return new Builder(stringify); } /** @@ -34,12 +35,12 @@ public static Builder builder() { * @param map A map representing the attributes. * @return The related attributes. */ - public static SpanAttributes fromMap(Map map) { + public static SpanAttributes fromMap(Map map) { return new SpanAttributes(new HashMap<>(map)); } @Override - public Map asMap() { + public Map asMap() { return this.attributes; } @@ -55,35 +56,45 @@ public String toString() { // Helper class for turning the Map that holds the attributes into a JSON string public static class JSONParser { - public static String toJson(Map map) { + public static String toJson(Map map) { StringBuilder jsonBuilder = new StringBuilder(); jsonBuilder.append("{"); - boolean first = true; - for (Map.Entry entry : map.entrySet()) { - if (!first) { + Set> entrySet = map.entrySet(); + int entryCount = 0; + int totalEntries = entrySet.size(); + + for (Map.Entry entry : entrySet) { + if (entryCount > 0) { jsonBuilder.append(","); } - first = false; - // Append the key and value - appendJsonString(jsonBuilder, entry.getKey(), entry.getValue()); + String key = entry.getKey(); + Object value = entry.getValue(); + + // Escape key and append it + jsonBuilder.append("\"").append(escapeJson(key)).append("\":"); + + // Append value based on its type + if (value instanceof String) { + jsonBuilder.append("\"").append(escapeJson((String) value)).append("\""); + } else if (value instanceof Number) { + jsonBuilder.append(value.toString()); + } else if (value instanceof Boolean) { + jsonBuilder.append(value.toString()); + } else { + jsonBuilder.append("null"); // For unsupported types, use null + } + + entryCount++; } jsonBuilder.append("}"); - return jsonBuilder.toString(); - } - - private static void appendJsonString(StringBuilder jsonBuilder, String key, String value) { - // Append the key (enclosed in double quotes) - jsonBuilder.append("\"").append(escapeJson(key)).append("\":"); - // Append the value (enclosed in double quotes) - jsonBuilder.append("\"").append(escapeJson(value)).append("\""); + return jsonBuilder.toString(); } private static String escapeJson(String value) { - // Replace special characters with their escaped counterparts return value .replace("\\", "\\\\") .replace("\"", "\\\"") @@ -96,10 +107,12 @@ private static String escapeJson(String value) { } public static class Builder { - private final Map attributes; + private final Map attributes; + private final boolean stringify; - protected Builder() { + protected Builder(boolean stringify) { this.attributes = new HashMap<>(); + this.stringify = stringify; } public Builder put(String key, String value) { @@ -112,19 +125,31 @@ public Builder put(String key, String value) { public Builder put(String key, boolean value) { requireNonNull(key, "key must not be null"); - this.attributes.put(key, Boolean.toString(value)); + if (this.stringify) { + this.attributes.put(key, Boolean.toString(value)); + } else { + this.attributes.put(key, value); + } return this; } public Builder put(String key, long value) { requireNonNull(key, "key must not be null"); - this.attributes.put(key, Long.toString(value)); + if (this.stringify) { + this.attributes.put(key, Long.toString(value)); + } else { + this.attributes.put(key, value); + } return this; } public Builder put(String key, double value) { requireNonNull(key, "key must not be null"); - this.attributes.put(key, Double.toString(value)); + if (this.stringify) { + this.attributes.put(key, Double.toString(value)); + } else { + this.attributes.put(key, value); + } return this; } @@ -150,7 +175,11 @@ protected Builder putArray(String key, List array) { for (int index = 0; index < array.size(); index++) { Object value = array.get(index); if (value != null) { - this.attributes.put(key + "." + index, value.toString()); + if (this.stringify) { + this.attributes.put(key + "." + index, value.toString()); + } else { + this.attributes.put(key + "." + index, value); + } } } } From 8e353f71de056364079c1dc0c75e46245b79d81c Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Fri, 16 Aug 2024 17:38:18 -0400 Subject: [PATCH 14/37] fixing attributes manipulation for arrays --- .../shim/trace/OtelConventions.java | 5 +- .../shim/trace/OtelSpanLink.java | 3 +- .../opentelemetry/shim/trace/SpanEvent.java | 6 ++- .../test/groovy/OpenTelemetry14Test.groovy | 5 +- .../instrumentation/api/SpanAttributes.java | 48 ++++++++++--------- .../instrumentation/api/SpanLinkTest.groovy | 8 ++-- 6 files changed, 42 insertions(+), 33 deletions(-) diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java index c25a0895c01..4ae7523a402 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java @@ -240,11 +240,12 @@ private static String getStringAttribute(AgentSpan span, String key) { return (String) tag; } - public static AgentSpan.Attributes convertAttributes(Attributes attributes, boolean stringify) { + public static AgentSpan.Attributes convertAttributes( + Attributes attributes, SpanAttributes.Builder.Format format) { if (attributes == null || attributes.isEmpty()) { return SpanAttributes.EMPTY; } - SpanAttributes.Builder builder = SpanAttributes.builder(stringify); + SpanAttributes.Builder builder = SpanAttributes.builder(format); attributes.forEach( (attributeKey, value) -> { String key = attributeKey.getKey(); diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanLink.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanLink.java index 02eb4d7e8ee..6055ba37942 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanLink.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanLink.java @@ -5,6 +5,7 @@ import datadog.opentelemetry.shim.context.propagation.TraceStateHelper; import datadog.trace.api.DDSpanId; import datadog.trace.api.DDTraceId; +import datadog.trace.bootstrap.instrumentation.api.SpanAttributes.Builder.Format; import datadog.trace.bootstrap.instrumentation.api.SpanLink; import io.opentelemetry.api.trace.SpanContext; @@ -19,6 +20,6 @@ public OtelSpanLink(SpanContext spanContext, io.opentelemetry.api.common.Attribu DDSpanId.fromHex(spanContext.getSpanId()), spanContext.isSampled() ? SAMPLED_FLAG : DEFAULT_FLAGS, TraceStateHelper.encodeHeader(spanContext.getTraceState()), - convertAttributes(attributes, true)); + convertAttributes(attributes, Format.LINKS)); } } diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java index b6adb0ba505..7c7aa4edcc0 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java @@ -15,13 +15,15 @@ public class SpanEvent { public SpanEvent(String name, Attributes attributes) { this.name = name; - this.attributes = OtelConventions.convertAttributes(attributes, false); + this.attributes = + OtelConventions.convertAttributes(attributes, SpanAttributes.Builder.Format.EVENTS); this.timestamp = timeNano(); } public SpanEvent(String name, Attributes attributes, long timestamp, TimeUnit unit) { this.name = name; - this.attributes = OtelConventions.convertAttributes(attributes, false); + this.attributes = + OtelConventions.convertAttributes(attributes, SpanAttributes.Builder.Format.EVENTS); this.timestamp = timeNano(timestamp, unit); } diff --git a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy index 738afa8bfca..513c6ff7211 100644 --- a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy +++ b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy @@ -15,6 +15,7 @@ import opentelemetry14.context.propagation.TextMap import org.skyscreamer.jsonassert.JSONAssert import spock.lang.Subject +import java.sql.Time import java.util.concurrent.TimeUnit import static datadog.trace.api.DDTags.ERROR_MSG @@ -217,7 +218,9 @@ class OpenTelemetry14Test extends AgentTestRunner { name | timestamp | unit | attributes | expectedAttributes "evt1" | TIME_MILLIS | TimeUnit.MILLISECONDS | Attributes.empty() | null "evt2" | TIME_NANO | TimeUnit.NANOSECONDS | Attributes.builder().put("string-key", "string-value").put("long-key", 123456789L).put("double-key", 1234.5678D).put("boolean-key-true", true).put("boolean-key-false", false).build() | '{ string-key: "string-value", long-key: 123456789, double-key: 1234.5678, boolean-key-true: true, boolean-key-false: false }' - "evt3" | TIME_NANO | TimeUnit.NANOSECONDS | Attributes.builder().put("string-key-array", "string-value1", "string-value2", "string-value3").put("long-key-array", 123456L, 1234567L, 12345678L).put("double-key-array", 1234.5D, 1234.56D, 1234.567D).put("boolean-key-array", true, false, true).build() | '{ string-key-array.0: "string-value1", string-key-array.1: "string-value2", string-key-array.2: "string-value3", long-key-array.0: 123456, long-key-array.1: 1234567, long-key-array.2: 12345678, double-key-array.0: 1234.5, double-key-array.1: 1234.56, double-key-array.2: 1234.567, boolean-key-array.0: true, boolean-key-array.1: false, boolean-key-array.2: true }' + "evt3" | TIME_NANO | TimeUnit.NANOSECONDS | Attributes.builder().put("string-key-array", "string-value1", "string-value2", "string-value3").put("long-key-array", 123456L, 1234567L, 12345678L).put("double-key-array", 1234.5D, 1234.56D, 1234.567D).put("boolean-key-array", true, false, true).build() | '{ string-key-array: [ "string-value1", "string-value2", "string-value3" ], long-key-array: [ 123456, 1234567, 12345678 ], double-key-array: [ 1234.5, 1234.56, 1234.567], boolean-key-array: [true, false, true] }' + // "evt4" | TIME_NANO | TimeUnit.NANOSECONDS | Attributes.builder().put("string-array", "1", "2").build() | '{ string-array: [ "1", "2"]}' + // evt4 is failing because for some reason string arrays whose values are numbers are treated like number (int) arrays. } def "test multiple span events"() { diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java index 167ff6ecdf2..3de9421cd4a 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java @@ -21,12 +21,12 @@ protected SpanAttributes(Map attributes) { } /** - * Gets a builder to create attributes. + * Gets a builder of the specified format to create attributes. * * @return A builder to create attributes. */ - public static Builder builder(boolean stringify) { - return new Builder(stringify); + public static Builder builder(Builder.Format format) { + return new Builder(format); } /** @@ -78,14 +78,11 @@ public static String toJson(Map map) { // Append value based on its type if (value instanceof String) { jsonBuilder.append("\"").append(escapeJson((String) value)).append("\""); - } else if (value instanceof Number) { - jsonBuilder.append(value.toString()); - } else if (value instanceof Boolean) { - jsonBuilder.append(value.toString()); + } else if (value instanceof Number || value instanceof Boolean || value instanceof List) { + jsonBuilder.append(value); } else { jsonBuilder.append("null"); // For unsupported types, use null } - entryCount++; } @@ -107,12 +104,17 @@ private static String escapeJson(String value) { } public static class Builder { + public enum Format { + LINKS, + EVENTS + } + private final Map attributes; - private final boolean stringify; + private final Format format; - protected Builder(boolean stringify) { + protected Builder(Format format) { this.attributes = new HashMap<>(); - this.stringify = stringify; + this.format = format; } public Builder put(String key, String value) { @@ -125,9 +127,9 @@ public Builder put(String key, String value) { public Builder put(String key, boolean value) { requireNonNull(key, "key must not be null"); - if (this.stringify) { + if (this.format == Format.LINKS) { this.attributes.put(key, Boolean.toString(value)); - } else { + } else if (this.format == Format.EVENTS) { this.attributes.put(key, value); } return this; @@ -135,9 +137,9 @@ public Builder put(String key, boolean value) { public Builder put(String key, long value) { requireNonNull(key, "key must not be null"); - if (this.stringify) { + if (this.format == Format.LINKS) { this.attributes.put(key, Long.toString(value)); - } else { + } else if (this.format == Format.EVENTS) { this.attributes.put(key, value); } return this; @@ -145,9 +147,9 @@ public Builder put(String key, long value) { public Builder put(String key, double value) { requireNonNull(key, "key must not be null"); - if (this.stringify) { + if (this.format == Format.LINKS) { this.attributes.put(key, Double.toString(value)); - } else { + } else if (this.format == Format.EVENTS) { this.attributes.put(key, value); } return this; @@ -172,15 +174,15 @@ public Builder putDoubleArray(String key, List array) { protected Builder putArray(String key, List array) { requireNonNull(key, "key must not be null"); if (array != null) { - for (int index = 0; index < array.size(); index++) { - Object value = array.get(index); - if (value != null) { - if (this.stringify) { + if (this.format == Format.LINKS) { + for (int index = 0; index < array.size(); index++) { + Object value = array.get(index); + if (value != null) { this.attributes.put(key + "." + index, value.toString()); - } else { - this.attributes.put(key + "." + index, value); } } + } else if (this.format == Format.EVENTS) { + this.attributes.put(key, array); } } return this; diff --git a/internal-api/src/test/groovy/datadog/trace/bootstrap/instrumentation/api/SpanLinkTest.groovy b/internal-api/src/test/groovy/datadog/trace/bootstrap/instrumentation/api/SpanLinkTest.groovy index a58e38f1094..60f2d291f66 100644 --- a/internal-api/src/test/groovy/datadog/trace/bootstrap/instrumentation/api/SpanLinkTest.groovy +++ b/internal-api/src/test/groovy/datadog/trace/bootstrap/instrumentation/api/SpanLinkTest.groovy @@ -53,13 +53,13 @@ class SpanLinkTest extends DDSpecification { def "test span link attributes api"() { when: - def attributes = SpanAttributes.builder().build() + def attributes = SpanAttributes.builder(SpanAttributes.Builder.Format.LINKS).build() then: attributes.isEmpty() when: - attributes = SpanAttributes.builder().put('key', 'value').build() + attributes = SpanAttributes.builder(SpanAttributes.Builder.Format.LINKS).put('key', 'value').build() then: !attributes.isEmpty() @@ -67,7 +67,7 @@ class SpanLinkTest extends DDSpecification { def "test span link attributes encoding"() { setup: - def builder = SpanAttributes.builder() + def builder = SpanAttributes.builder(SpanAttributes.Builder.Format.LINKS) when: builder.put('string', 'value') @@ -137,7 +137,7 @@ class SpanLinkTest extends DDSpecification { def "test span link attributes toString"() { when: - SpanAttributes.builder().build().toString() + SpanAttributes.builder(SpanAttributes.Builder.Format.LINKS).build().toString() then: notThrown(NullPointerException) From 90bc61999266737974a11cf6f60d2ad347f80fab Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Mon, 26 Aug 2024 15:46:43 -0400 Subject: [PATCH 15/37] Fixed json parsing for array Attributes values --- .../test/groovy/OpenTelemetry14Test.groovy | 2 - .../instrumentation/api/SpanAttributes.java | 46 ++++++++++++++----- 2 files changed, 34 insertions(+), 14 deletions(-) diff --git a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy index 513c6ff7211..c983173ebf5 100644 --- a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy +++ b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy @@ -219,8 +219,6 @@ class OpenTelemetry14Test extends AgentTestRunner { "evt1" | TIME_MILLIS | TimeUnit.MILLISECONDS | Attributes.empty() | null "evt2" | TIME_NANO | TimeUnit.NANOSECONDS | Attributes.builder().put("string-key", "string-value").put("long-key", 123456789L).put("double-key", 1234.5678D).put("boolean-key-true", true).put("boolean-key-false", false).build() | '{ string-key: "string-value", long-key: 123456789, double-key: 1234.5678, boolean-key-true: true, boolean-key-false: false }' "evt3" | TIME_NANO | TimeUnit.NANOSECONDS | Attributes.builder().put("string-key-array", "string-value1", "string-value2", "string-value3").put("long-key-array", 123456L, 1234567L, 12345678L).put("double-key-array", 1234.5D, 1234.56D, 1234.567D).put("boolean-key-array", true, false, true).build() | '{ string-key-array: [ "string-value1", "string-value2", "string-value3" ], long-key-array: [ 123456, 1234567, 12345678 ], double-key-array: [ 1234.5, 1234.56, 1234.567], boolean-key-array: [true, false, true] }' - // "evt4" | TIME_NANO | TimeUnit.NANOSECONDS | Attributes.builder().put("string-array", "1", "2").build() | '{ string-array: [ "1", "2"]}' - // evt4 is failing because for some reason string arrays whose values are numbers are treated like number (int) arrays. } def "test multiple span events"() { diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java index 3de9421cd4a..671a1e3a8ba 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java @@ -54,7 +54,8 @@ public String toString() { return "SpanAttributes{" + this.attributes + '}'; } - // Helper class for turning the Map that holds the attributes into a JSON string + // JSONParser is a helper class for turning the Map that holds the attributes into + // a JSON string public static class JSONParser { public static String toJson(Map map) { StringBuilder jsonBuilder = new StringBuilder(); @@ -62,7 +63,6 @@ public static String toJson(Map map) { Set> entrySet = map.entrySet(); int entryCount = 0; - int totalEntries = entrySet.size(); for (Map.Entry entry : entrySet) { if (entryCount > 0) { @@ -75,22 +75,42 @@ public static String toJson(Map map) { // Escape key and append it jsonBuilder.append("\"").append(escapeJson(key)).append("\":"); - // Append value based on its type - if (value instanceof String) { - jsonBuilder.append("\"").append(escapeJson((String) value)).append("\""); - } else if (value instanceof Number || value instanceof Boolean || value instanceof List) { - jsonBuilder.append(value); - } else { - jsonBuilder.append("null"); // For unsupported types, use null - } + // Append value to jsonBuilder + appendValue(value, jsonBuilder); entryCount++; } - jsonBuilder.append("}"); - return jsonBuilder.toString(); } + /** + * appendValue recursively adds the value of an Attribute to the active StringBuilder in JSON + * format, depending on the value's type + * + * @param value the value to append + * @param jsonBuilder the active 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"); // For unsupported types, use null + } + } + private static String escapeJson(String value) { return value .replace("\\", "\\\\") @@ -104,6 +124,8 @@ private static String escapeJson(String value) { } public static class Builder { + // SpanLinks and SpanEvents are encoded in different formats; Format represents the encoding + // standard public enum Format { LINKS, EVENTS From 14db2adc63127b6dd5b6d204d4f5046e2288ee69 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Mon, 26 Aug 2024 16:12:23 -0400 Subject: [PATCH 16/37] Added better javadocs to functions --- .../shim/trace/OtelConventions.java | 47 ++++++++++++------- 1 file changed, 30 insertions(+), 17 deletions(-) diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java index 4ae7523a402..d854fd11ecd 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java @@ -240,6 +240,15 @@ private static String getStringAttribute(AgentSpan span, String key) { return (String) tag; } + /** + * convertAttributes is a helper function for converting opentelemetry Attributes to + * AgentSpan.Attributes based on the AttributeKey type + * + * @param attributes the Attributes to convert + * @param format the format to convert the Attributes to, given attributes are handled differently + * for SpanLinks and SpanEvents + * @return the converted AgentSpan.Attributes + */ public static AgentSpan.Attributes convertAttributes( Attributes attributes, SpanAttributes.Builder.Format format) { if (attributes == null || attributes.isEmpty()) { @@ -284,8 +293,8 @@ public static AgentSpan.Attributes convertAttributes( } /** - * Generate Attributes about the exception using a default list + the additionalAttributes - * provided by the user + * processExceptionAttributes generates Attributes about the exception using a default list + the + * additionalAttributes provided by the user * *

If the same key exists in defaultAttributes and additionalAttributes, the latter always * wins. @@ -296,35 +305,39 @@ public static AgentSpan.Attributes convertAttributes( */ public static Attributes processExceptionAttributes( Throwable exception, Attributes additionalAttributes) { - // "exception.escaped" should be true if exception will "escape" the scope of the span. I'm not - // sure how to check that from within here. And it doesn't even look like Otel dynamically sets - // this field: - // https://github.com/open-telemetry/opentelemetry-java/blob/v1.41.0/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/internal/data/ImmutableExceptionEventData.java#L22 - boolean escaped = false; - Map defaultAttributes = - new HashMap() { - { - put("exception.message", exception.getMessage()); - put("exception.type", exception.getClass().getName()); - put("exception.escaped", escaped); - put("exception.stacktrace", Arrays.toString(exception.getStackTrace())); - } - }; + Map defaultAttributes = getAttributesFromException(exception); // Create an AttributesBuilder with the additionalAttributes provided AttributesBuilder attrsBuilder = additionalAttributes.toBuilder(); for (String key : defaultAttributes.keySet()) { // Add defaultAttributes onto the builder iff an equivalent key was not provided in // additionalAttributes if (additionalAttributes.get(AttributeKey.stringKey(key)) == null) { - // naive implementation, as the default attributes values re only strings and booleans Object value = defaultAttributes.get(key); + // Currently we only have Strings and booleans in defaultAttributes, but this could change. if (value instanceof String) { attrsBuilder.put(key, (String) defaultAttributes.get(key)); } else if (value instanceof Boolean) { attrsBuilder.put(key, (Boolean) defaultAttributes.get(key)); } + // Need a catchall else here? } } return attrsBuilder.build(); } + + private static Map getAttributesFromException(Throwable exception) { + // "exception.escaped" should be true if exception will "escape" the scope of the span. I'm not + // sure how to check that from within here. And it doesn't even look like Otel dynamically sets + // this field: + // https://github.com/open-telemetry/opentelemetry-java/blob/v1.41.0/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/internal/data/ImmutableExceptionEventData.java#L22 + boolean escaped = false; + return new HashMap() { + { + put("exception.message", exception.getMessage()); + put("exception.type", exception.getClass().getName()); + put("exception.escaped", escaped); + put("exception.stacktrace", Arrays.toString(exception.getStackTrace())); + } + }; + } } From 565e43d418c1f655f21c81c3c32126f059aae03a Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Tue, 3 Sep 2024 16:42:10 -0400 Subject: [PATCH 17/37] Fix exception.escaped and timesource for SpanEvent --- .../shim/trace/OtelConventions.java | 6 --- .../opentelemetry/shim/trace/OtelSpan.java | 9 ++++ .../opentelemetry/shim/trace/SpanEvent.java | 25 ++++++--- .../test/groovy/OpenTelemetry14Test.groovy | 54 ++++++++++++------- 4 files changed, 62 insertions(+), 32 deletions(-) diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java index d854fd11ecd..12ac23678c1 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java @@ -326,16 +326,10 @@ public static Attributes processExceptionAttributes( } private static Map getAttributesFromException(Throwable exception) { - // "exception.escaped" should be true if exception will "escape" the scope of the span. I'm not - // sure how to check that from within here. And it doesn't even look like Otel dynamically sets - // this field: - // https://github.com/open-telemetry/opentelemetry-java/blob/v1.41.0/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/internal/data/ImmutableExceptionEventData.java#L22 - boolean escaped = false; return new HashMap() { { put("exception.message", exception.getMessage()); put("exception.type", exception.getClass().getName()); - put("exception.escaped", escaped); put("exception.stacktrace", Arrays.toString(exception.getStackTrace())); } }; 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 79fa64d28ba..4fddc5f5aa8 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 @@ -111,6 +111,15 @@ public Span setStatus(StatusCode statusCode, String description) { return this; } + /** + * Records information about the Throwable as a span event + * exception.escaped cannot be determined by this function and therefore is not automatically recorded. Record this manually using additionalAttributes + * See: ... + * + * @param exception the Throwable to record + * @param additionalAttributes the additional attributes to record + * + */ @Override public Span recordException(Throwable exception, Attributes additionalAttributes) { if (this.recording) { diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java index 7c7aa4edcc0..eac9b71c5bc 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java @@ -4,35 +4,48 @@ import datadog.trace.bootstrap.instrumentation.api.SpanAttributes; import edu.umd.cs.findbugs.annotations.NonNull; import io.opentelemetry.api.common.Attributes; + +import java.sql.Time; import java.util.List; import java.util.concurrent.TimeUnit; +import datadog.trace.api.time.SystemTimeSource; +import datadog.trace.api.time.TimeSource; public class SpanEvent { private final long timestamp; private final String name; private final AgentSpan.Attributes attributes; + private static TimeSource timeSource; public SpanEvent(String name, Attributes attributes) { this.name = name; this.attributes = OtelConventions.convertAttributes(attributes, SpanAttributes.Builder.Format.EVENTS); - this.timestamp = timeNano(); + this.timestamp = getNanosFromTimeSource(); } public SpanEvent(String name, Attributes attributes, long timestamp, TimeUnit unit) { this.name = name; this.attributes = OtelConventions.convertAttributes(attributes, SpanAttributes.Builder.Format.EVENTS); - this.timestamp = timeNano(timestamp, unit); + this.timestamp = convertNano(timestamp, unit); } - private static long timeNano() { - return System.nanoTime(); + private static long convertNano(long timestamp, TimeUnit unit) { + return TimeUnit.NANOSECONDS.convert(timestamp, unit); } - private static long timeNano(long timestamp, TimeUnit unit) { - return TimeUnit.NANOSECONDS.convert(timestamp, unit); + private long getNanosFromTimeSource() { + if (timeSource == null) { + return SystemTimeSource.INSTANCE.getCurrentTimeNanos(); + } else { + return timeSource.getCurrentTimeNanos(); + } + } + + public static void setTimeSource(TimeSource newTimeSource) { + timeSource = newTimeSource; } public String toString() { diff --git a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy index c983173ebf5..b37e9b86afb 100644 --- a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy +++ b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy @@ -3,6 +3,7 @@ import datadog.trace.agent.test.AgentTestRunner import datadog.trace.api.DDSpanId import datadog.trace.api.DDTags import datadog.trace.api.DDTraceId +import datadog.trace.api.time.ControllableTimeSource import io.opentelemetry.api.GlobalOpenTelemetry import io.opentelemetry.api.common.AttributeKey import io.opentelemetry.api.common.Attributes @@ -256,24 +257,37 @@ class OpenTelemetry14Test extends AgentTestRunner { } } - // def "test add event no timestamp"() { - // setup: - // def builder = tracer.spanBuilder("some-name") - // - // when: - // def result = builder.startSpan() - // result.addEvent("evt", null, null) - // result.end() - // - // then: - // long endBlock = System.nanoTime(); - // - // // I'd like to grab the span's `time_unix_nano` field under `events` tag is not null, and is more than beforeBlock and less than afterBlock - // // But I don't know how to access this without asserting the span tags look a specific way using assertTraces. - // - // where: - // long startBlock = System.nanoTime(); - // } + def "test add event no timestamp"() { + setup: + def builder = tracer.spanBuilder("some-name") + def timeSource = new ControllableTimeSource(); + timeSource.set(1000); + SpanEvent.setTimeSource(timeSource); + + when: + def result = builder.startSpan() + result.addEvent("evt", null, ) + result.end() + + then: + def expectedEventTag = """ + [ + { "time_unix_nano": ${timeSource.getCurrentTimeNanos()}, + "name": "evt" + } + ]""" + assertTraces(1) { + trace(1) { + span { + tags { + defaultTags() + "$SPAN_KIND" "$SPAN_KIND_INTERNAL" + tag("events", { JSONAssert.assertEquals(expectedEventTag, it as String, false); return true }) + } + } + } + } + } def "test simple span links"() { setup: @@ -691,8 +705,8 @@ class OpenTelemetry14Test extends AgentTestRunner { where: exception | attributes | expectedAttributes - new NullPointerException("Null pointer") | Attributes.empty() | """{ exception.message: "${exception.getMessage()}", exception.type: "${exception.getClass().getName()}", exception.escaped: false, exception.stacktrace: "${Arrays.toString(exception.getStackTrace())}"}""" - new NumberFormatException("Number format exception") | Attributes.builder().put("exception.message", "something-else").build() | """{exception.message: "something-else", exception.type: "${exception.getClass().getName()}", exception.escaped: false, exception.stacktrace: "${Arrays.toString(exception.getStackTrace())}"}""" + new NullPointerException("Null pointer") | Attributes.empty() | """{ exception.message: "${exception.getMessage()}", exception.type: "${exception.getClass().getName()}", exception.stacktrace: "${Arrays.toString(exception.getStackTrace())}"}""" + new NumberFormatException("Number format exception") | Attributes.builder().put("exception.message", "something-else").build() | """{exception.message: "something-else", exception.type: "${exception.getClass().getName()}", exception.stacktrace: "${Arrays.toString(exception.getStackTrace())}"}""" new NullPointerException("Null pointer") | Attributes.builder().put("key", "value").build() | """{"key": "value","exception.message": "${exception.getMessage()}"}""" } From 76554463a2e7c3d2bec81819f4b7da00813cf1d7 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Tue, 3 Sep 2024 16:43:27 -0400 Subject: [PATCH 18/37] Fix exception.escaped and timesource for SpanEvent --- .../opentelemetry/shim/trace/OtelSpan.java | 8 ++-- .../opentelemetry/shim/trace/SpanEvent.java | 6 +-- .../test/groovy/OpenTelemetry14Test.groovy | 46 +++++++++---------- 3 files changed, 29 insertions(+), 31 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 4fddc5f5aa8..c002b2fc3f6 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 @@ -112,13 +112,13 @@ public Span setStatus(StatusCode statusCode, String description) { } /** - * Records information about the Throwable as a span event - * exception.escaped cannot be determined by this function and therefore is not automatically recorded. Record this manually using additionalAttributes - * See: ... + * Records information about the Throwable as a span event exception.escaped cannot be determined + * by this function and therefore is not automatically recorded. Record this manually using + * additionalAttributes See: ... * * @param exception the Throwable to record * @param additionalAttributes the additional attributes to record - * */ @Override public Span recordException(Throwable exception, Attributes additionalAttributes) { diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java index eac9b71c5bc..96ac09aeb39 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java @@ -1,15 +1,13 @@ package datadog.opentelemetry.shim.trace; +import datadog.trace.api.time.SystemTimeSource; +import datadog.trace.api.time.TimeSource; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import datadog.trace.bootstrap.instrumentation.api.SpanAttributes; import edu.umd.cs.findbugs.annotations.NonNull; import io.opentelemetry.api.common.Attributes; - -import java.sql.Time; import java.util.List; import java.util.concurrent.TimeUnit; -import datadog.trace.api.time.SystemTimeSource; -import datadog.trace.api.time.TimeSource; public class SpanEvent { diff --git a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy index b37e9b86afb..c3d86465104 100644 --- a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy +++ b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy @@ -257,37 +257,37 @@ class OpenTelemetry14Test extends AgentTestRunner { } } - def "test add event no timestamp"() { - setup: - def builder = tracer.spanBuilder("some-name") - def timeSource = new ControllableTimeSource(); - timeSource.set(1000); - SpanEvent.setTimeSource(timeSource); - - when: - def result = builder.startSpan() - result.addEvent("evt", null, ) - result.end() - - then: - def expectedEventTag = """ + def "test add event no timestamp"() { + setup: + def builder = tracer.spanBuilder("some-name") + def timeSource = new ControllableTimeSource() + timeSource.set(1000) + SpanEvent.setTimeSource(timeSource) + + when: + def result = builder.startSpan() + result.addEvent("evt", null, ) + result.end() + + then: + def expectedEventTag = """ [ { "time_unix_nano": ${timeSource.getCurrentTimeNanos()}, "name": "evt" } ]""" - assertTraces(1) { - trace(1) { - span { - tags { - defaultTags() - "$SPAN_KIND" "$SPAN_KIND_INTERNAL" - tag("events", { JSONAssert.assertEquals(expectedEventTag, it as String, false); return true }) - } - } + assertTraces(1) { + trace(1) { + span { + tags { + defaultTags() + "$SPAN_KIND" "$SPAN_KIND_INTERNAL" + tag("events", { JSONAssert.assertEquals(expectedEventTag, it as String, false); return true }) } } } + } + } def "test simple span links"() { setup: From 3123288936372804f170e89c7dd6e3eb1edec90a Mon Sep 17 00:00:00 2001 From: Mikayla Toffler <46911781+mtoffl01@users.noreply.github.com> Date: Tue, 3 Sep 2024 16:48:36 -0400 Subject: [PATCH 19/37] Update internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java Co-authored-by: datadog-datadog-prod-us1[bot] <88084959+datadog-datadog-prod-us1[bot]@users.noreply.github.com> --- .../trace/bootstrap/instrumentation/api/SpanAttributes.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java index 671a1e3a8ba..ced5a3c22b1 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java @@ -59,7 +59,7 @@ public String toString() { public static class JSONParser { public static String toJson(Map map) { StringBuilder jsonBuilder = new StringBuilder(); - jsonBuilder.append("{"); + jsonBuilder.append('{'); Set> entrySet = map.entrySet(); int entryCount = 0; From b4ae9149990a67fdafd288816ffd9810de37e146 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler <46911781+mtoffl01@users.noreply.github.com> Date: Tue, 3 Sep 2024 16:51:22 -0400 Subject: [PATCH 20/37] Update internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java Co-authored-by: datadog-datadog-prod-us1[bot] <88084959+datadog-datadog-prod-us1[bot]@users.noreply.github.com> --- .../trace/bootstrap/instrumentation/api/SpanAttributes.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java index ced5a3c22b1..4e084e495c2 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java @@ -79,7 +79,7 @@ public static String toJson(Map map) { appendValue(value, jsonBuilder); entryCount++; } - jsonBuilder.append("}"); + jsonBuilder.append('}'); return jsonBuilder.toString(); } From a2026b2327c19895f3ef52f0d5f3c0237ab7fe9e Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Tue, 3 Sep 2024 16:55:19 -0400 Subject: [PATCH 21/37] Update javadoc --- .../datadog/opentelemetry/shim/trace/OtelConventions.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java index 12ac23678c1..29bef993a21 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java @@ -325,6 +325,12 @@ public static Attributes processExceptionAttributes( return attrsBuilder.build(); } + /** + * generate default list of attribtues about a Throwable + * + * @param exception the Throwable about which to generate attribtues + * @return Map of attributes about the Throwable + */ private static Map getAttributesFromException(Throwable exception) { return new HashMap() { { From ccca4f34a832bd7cfa2db61428f9a8197a479d95 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Fri, 6 Sep 2024 11:20:46 -0400 Subject: [PATCH 22/37] fix nits --- .../shim/trace/OtelConventions.java | 4 ++-- .../opentelemetry/shim/trace/OtelSpan.java | 15 ++++++------ .../{SpanEvent.java => OtelSpanEvent.java} | 23 ++++++++----------- .../OpenTelemetryInstrumentation.java | 2 +- .../test/groovy/OpenTelemetry14Test.groovy | 11 +++++---- .../instrumentation/api/SpanAttributes.java | 2 +- 6 files changed, 27 insertions(+), 30 deletions(-) rename dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/{SpanEvent.java => OtelSpanEvent.java} (73%) diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java index 29bef993a21..f0cab93fc07 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java @@ -130,11 +130,11 @@ public static void applyNamingConvention(AgentSpan span) { } } - public static void setEventsAsTag(AgentSpan span, List events) { + public static void setEventsAsTag(AgentSpan span, List events) { if (events == null || events.isEmpty()) { return; } - span.setTag(DDTags.SPAN_EVENTS, SpanEvent.toTag(events)); + span.setTag(DDTags.SPAN_EVENTS, OtelSpanEvent.toTag(events)); } private static String computeOperationName(AgentSpan span) { 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 c002b2fc3f6..0883d81d636 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 @@ -30,7 +30,7 @@ public class OtelSpan implements Span { private final AgentSpan delegate; private StatusCode statusCode; private boolean recording; - private List events; + private List events; public OtelSpan(AgentSpan delegate) { this.delegate = delegate; @@ -76,10 +76,10 @@ public Span setAttribute(AttributeKey key, T value) { @Override public Span addEvent(String name, Attributes attributes) { if (this.recording) { - if (this.events == null || this.events.isEmpty()) { + if (this.events == null) { this.events = new ArrayList<>(); } - this.events.add(new SpanEvent(name, attributes)); + this.events.add(new OtelSpanEvent(name, attributes)); } return this; } @@ -87,10 +87,10 @@ 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.isEmpty()) { + if (this.events == null) { this.events = new ArrayList<>(); } - this.events.add(new SpanEvent(name, attributes, timestamp, unit)); + this.events.add(new OtelSpanEvent(name, attributes, timestamp, unit)); } return this; } @@ -123,11 +123,12 @@ public Span setStatus(StatusCode statusCode, String description) { @Override public Span recordException(Throwable exception, Attributes additionalAttributes) { if (this.recording) { - if (this.events == null || this.events.isEmpty()) { + if (this.events == null) { this.events = new ArrayList<>(); } this.events.add( - new SpanEvent("exception", processExceptionAttributes(exception, additionalAttributes))); + new OtelSpanEvent( + "exception", processExceptionAttributes(exception, additionalAttributes))); this.delegate.addThrowable(exception, ErrorPriorities.UNSET); } return this; diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java similarity index 73% rename from dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java rename to dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java index 96ac09aeb39..d5fcc237d13 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/SpanEvent.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java @@ -9,21 +9,21 @@ import java.util.List; import java.util.concurrent.TimeUnit; -public class SpanEvent { +public class OtelSpanEvent { private final long timestamp; private final String name; private final AgentSpan.Attributes attributes; - private static TimeSource timeSource; + private static TimeSource timeSource = SystemTimeSource.INSTANCE; - public SpanEvent(String name, Attributes attributes) { + public OtelSpanEvent(String name, Attributes attributes) { this.name = name; this.attributes = OtelConventions.convertAttributes(attributes, SpanAttributes.Builder.Format.EVENTS); this.timestamp = getNanosFromTimeSource(); } - public SpanEvent(String name, Attributes attributes, long timestamp, TimeUnit unit) { + public OtelSpanEvent(String name, Attributes attributes, long timestamp, TimeUnit unit) { this.name = name; this.attributes = OtelConventions.convertAttributes(attributes, SpanAttributes.Builder.Format.EVENTS); @@ -35,11 +35,7 @@ private static long convertNano(long timestamp, TimeUnit unit) { } private long getNanosFromTimeSource() { - if (timeSource == null) { - return SystemTimeSource.INSTANCE.getCurrentTimeNanos(); - } else { - return timeSource.getCurrentTimeNanos(); - } + return timeSource.getCurrentTimeNanos(); } public static void setTimeSource(TimeSource newTimeSource) { @@ -55,19 +51,18 @@ public String toString() { .append(",\"attributes\":") .append(SpanAttributes.JSONParser.toJson(this.attributes.asMap())); } - return builder.append("}").toString(); + return builder.append('}').toString(); } @NonNull - public static String toTag(List events) { - // TODO: Do we want to enforce a maximum tag size, like TAG_MAX_LENGTH in DDSpanLink? + public static String toTag(List events) { StringBuilder builder = new StringBuilder("["); for (int i = 0; i < events.size(); i++) { if (i > 0) { - builder.append(","); + builder.append(','); } builder.append(events.get(i).toString()); } - return builder.append("]").toString(); + return builder.append(']').toString(); } } diff --git a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/OpenTelemetryInstrumentation.java b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/OpenTelemetryInstrumentation.java index bfa825a6ad7..817fc68c60d 100644 --- a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/OpenTelemetryInstrumentation.java +++ b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/OpenTelemetryInstrumentation.java @@ -78,7 +78,7 @@ public String[] helperClassNames() { "datadog.opentelemetry.shim.trace.OtelTracer", "datadog.opentelemetry.shim.trace.OtelTracerBuilder", "datadog.opentelemetry.shim.trace.OtelTracerProvider", - "datadog.opentelemetry.shim.trace.SpanEvent", + "datadog.opentelemetry.shim.trace.OtelSpanEvent", }; } diff --git a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy index c3d86465104..f2390258871 100644 --- a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy +++ b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy @@ -1,4 +1,4 @@ -import datadog.opentelemetry.shim.trace.SpanEvent +import datadog.opentelemetry.shim.trace.OtelSpanEvent import datadog.trace.agent.test.AgentTestRunner import datadog.trace.api.DDSpanId import datadog.trace.api.DDTags @@ -262,7 +262,7 @@ class OpenTelemetry14Test extends AgentTestRunner { def builder = tracer.spanBuilder("some-name") def timeSource = new ControllableTimeSource() timeSource.set(1000) - SpanEvent.setTimeSource(timeSource) + OtelSpanEvent.setTimeSource(timeSource) when: def result = builder.startSpan() @@ -654,10 +654,12 @@ class OpenTelemetry14Test extends AgentTestRunner { } } - // This is failing because of the time_unix_nano issue described on the "test add event no timestamp" scenario def "test span record exception"() { setup: def result = tracer.spanBuilder("some-name").startSpan() + def timeSource = new ControllableTimeSource() + timeSource.set(1000) + OtelSpanEvent.setTimeSource(timeSource) expect: result.delegate.getTag(ERROR_MSG) == null @@ -678,10 +680,9 @@ class OpenTelemetry14Test extends AgentTestRunner { result.end() then: - // TODO: Same issue with time_unix_nano here -- I can't control what it is and don't know how to check the name and attributes without it def expectedEventTag = """ [ - { "time_unix_nano": ${TIME_NANO}, + { "time_unix_nano": ${timeSource.getCurrentTimeNanos()}, "name": "exception", "attributes": ${expectedAttributes} } diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java index 4e084e495c2..4af80ef6313 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java @@ -124,7 +124,7 @@ private static String escapeJson(String value) { } public static class Builder { - // SpanLinks and SpanEvents are encoded in different formats; Format represents the encoding + // SpanLinks and OtelSpanEvents are encoded in different formats; Format represents the encoding // standard public enum Format { LINKS, From 0f04383b2209fb1465fd3aa4d07c72075451732d Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Fri, 6 Sep 2024 11:38:35 -0400 Subject: [PATCH 23/37] change SpanEvent method to private-package access scope --- dd-trace-core/src/main/java/datadog/trace/core/DDSpanLink.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dd-trace-core/src/main/java/datadog/trace/core/DDSpanLink.java b/dd-trace-core/src/main/java/datadog/trace/core/DDSpanLink.java index d3d8a5a6e5a..a87762a3504 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/DDSpanLink.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/DDSpanLink.java @@ -65,7 +65,7 @@ public static SpanLink from(ExtractedContext context, Attributes attributes) { * @param links The span link collection to encode. * @return The encoded tag value, {@code null} if no links. */ - public static String toTag(List links) { + static String toTag(List links) { if (links == null || links.isEmpty()) { return null; } From 9efd98e008268a6953f1c8dd1a148667a64da0f6 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Tue, 10 Sep 2024 15:18:14 -0400 Subject: [PATCH 24/37] Changed span event Attributes to a separate class --- .../shim/trace/OtelSpanEvent.java | 220 +++++++++++++++++- 1 file changed, 216 insertions(+), 4 deletions(-) diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java index d5fcc237d13..e0d51bdcf13 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java @@ -1,29 +1,38 @@ package datadog.opentelemetry.shim.trace; +import static java.util.Objects.requireNonNull; + import datadog.trace.api.time.SystemTimeSource; import datadog.trace.api.time.TimeSource; -import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import datadog.trace.bootstrap.instrumentation.api.SpanAttributes; import edu.umd.cs.findbugs.annotations.NonNull; import io.opentelemetry.api.common.Attributes; +import java.util.Collections; +import java.util.HashMap; import java.util.List; +import java.util.Map; +import java.util.Set; import java.util.concurrent.TimeUnit; public class OtelSpanEvent { private final long timestamp; private final String name; - private final AgentSpan.Attributes attributes; + private final Attributes attributes; private static TimeSource timeSource = SystemTimeSource.INSTANCE; - public OtelSpanEvent(String name, Attributes attributes) { + public OtelSpanEvent(String name, io.opentelemetry.api.common.Attributes attributes) { this.name = name; this.attributes = OtelConventions.convertAttributes(attributes, SpanAttributes.Builder.Format.EVENTS); this.timestamp = getNanosFromTimeSource(); } - public OtelSpanEvent(String name, Attributes attributes, long timestamp, TimeUnit unit) { + public OtelSpanEvent( + String name, + io.opentelemetry.api.common.Attributes attributes, + long timestamp, + TimeUnit unit) { this.name = name; this.attributes = OtelConventions.convertAttributes(attributes, SpanAttributes.Builder.Format.EVENTS); @@ -65,4 +74,207 @@ public static String toTag(List events) { } return builder.append(']').toString(); } + + private static class Attributes { + /** Represent an empty attributes. */ + public static final Attributes EMPTY = new Attributes(Collections.emptyMap()); + + private final Map attributes; + + protected Attributes(Map attributes) { + this.attributes = attributes; + } + + /** + * Gets a builder of the specified format to create attributes. + * + * @return A builder to create attributes. + */ + // public static Attributes.Builder builder(Attributes.Builder.Format format) { + // return new Attributes.Builder(format); + // } + /** + * Create attributes from its map representation. + * + * @param map A map representing the attributes. + * @return The related attributes. + */ + public static Attributes fromMap(Map map) { + return new Attributes(new HashMap<>(map)); + } + + public Map asMap() { + return this.attributes; + } + + public boolean isEmpty() { + return this.attributes.isEmpty(); + } + + public String toString() { + return "Attributes{" + this.attributes + '}'; + } + + // JSONParser is a helper class for turning the Map that holds the attributes + // into + // a JSON string + public static class JSONParser { + public static String toJson(Map map) { + StringBuilder jsonBuilder = new StringBuilder(); + jsonBuilder.append('{'); + + Set> entrySet = map.entrySet(); + int entryCount = 0; + + for (Map.Entry entry : entrySet) { + if (entryCount > 0) { + jsonBuilder.append(","); + } + + String key = entry.getKey(); + Object value = entry.getValue(); + + // Escape key and append it + jsonBuilder.append("\"").append(escapeJson(key)).append("\":"); + + // Append value to jsonBuilder + appendValue(value, jsonBuilder); + entryCount++; + } + jsonBuilder.append('}'); + return jsonBuilder.toString(); + } + /** + * appendValue recursively adds the value of an Attribute to the active StringBuilder in JSON + * format, depending on the value's type + * + * @param value the value to append + * @param jsonBuilder the active 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"); // For unsupported types, use null + } + } + + 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 class Builder { + // SpanLinks and OtelSpanEvents are encoded in different formats; Format represents the + // encoding + // standard + public enum Format { + LINKS, + EVENTS + } + + private final Map attributes; + private final OtelSpanEvent.Attributes.Builder.Format format; + + protected Builder(OtelSpanEvent.Attributes.Builder.Format format) { + this.attributes = new HashMap<>(); + this.format = format; + } + + public OtelSpanEvent.Attributes.Builder put(String key, String value) { + requireNonNull(key, "key must not be null"); + if (value != null) { + this.attributes.put(key, value); + } + return this; + } + + public OtelSpanEvent.Attributes.Builder put(String key, boolean value) { + requireNonNull(key, "key must not be null"); + if (this.format == OtelSpanEvent.Attributes.Builder.Format.LINKS) { + this.attributes.put(key, Boolean.toString(value)); + } else if (this.format == OtelSpanEvent.Attributes.Builder.Format.EVENTS) { + this.attributes.put(key, value); + } + return this; + } + + public OtelSpanEvent.Attributes.Builder put(String key, long value) { + requireNonNull(key, "key must not be null"); + if (this.format == OtelSpanEvent.Attributes.Builder.Format.LINKS) { + this.attributes.put(key, Long.toString(value)); + } else if (this.format == OtelSpanEvent.Attributes.Builder.Format.EVENTS) { + this.attributes.put(key, value); + } + return this; + } + + public OtelSpanEvent.Attributes.Builder put(String key, double value) { + requireNonNull(key, "key must not be null"); + if (this.format == OtelSpanEvent.Attributes.Builder.Format.LINKS) { + this.attributes.put(key, Double.toString(value)); + } else if (this.format == OtelSpanEvent.Attributes.Builder.Format.EVENTS) { + this.attributes.put(key, value); + } + return this; + } + + public OtelSpanEvent.Attributes.Builder putStringArray(String key, List array) { + return putArray(key, array); + } + + public OtelSpanEvent.Attributes.Builder putBooleanArray(String key, List array) { + return putArray(key, array); + } + + public OtelSpanEvent.Attributes.Builder putLongArray(String key, List array) { + return putArray(key, array); + } + + public OtelSpanEvent.Attributes.Builder putDoubleArray(String key, List array) { + return putArray(key, array); + } + + protected OtelSpanEvent.Attributes.Builder putArray(String key, List array) { + requireNonNull(key, "key must not be null"); + if (array != null) { + if (this.format == OtelSpanEvent.Attributes.Builder.Format.LINKS) { + for (int index = 0; index < array.size(); index++) { + Object value = array.get(index); + if (value != null) { + this.attributes.put(key + "." + index, value.toString()); + } + } + } else if (this.format == OtelSpanEvent.Attributes.Builder.Format.EVENTS) { + this.attributes.put(key, array); + } + } + return this; + } + + public OtelSpanEvent.Attributes build() { + return new OtelSpanEvent.Attributes(this.attributes); + } + } + } } From 87a7698804902d13f2aef550887bc67b15c8b67f Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Wed, 11 Sep 2024 09:58:24 -0400 Subject: [PATCH 25/37] Fix error stack formatting for exception.stacktrace tag --- .../datadog/opentelemetry/shim/trace/OtelConventions.java | 7 +++++-- .../datadog/opentelemetry/shim/trace/OtelSpanEvent.java | 6 +++--- .../src/test/groovy/OpenTelemetry14Test.groovy | 6 +++--- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java index f0cab93fc07..3d4047e651b 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java @@ -22,7 +22,8 @@ import io.opentelemetry.api.common.Attributes; import io.opentelemetry.api.common.AttributesBuilder; import io.opentelemetry.api.trace.SpanKind; -import java.util.Arrays; +import java.io.PrintWriter; +import java.io.StringWriter; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -336,7 +337,9 @@ private static Map getAttributesFromException(Throwable exceptio { put("exception.message", exception.getMessage()); put("exception.type", exception.getClass().getName()); - put("exception.stacktrace", Arrays.toString(exception.getStackTrace())); + final StringWriter errorString = new StringWriter(); + exception.printStackTrace(new PrintWriter(errorString)); + put("exception.stacktrace", errorString.toString()); } }; } diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java index e0d51bdcf13..780ab8b8a10 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java @@ -4,9 +4,9 @@ import datadog.trace.api.time.SystemTimeSource; import datadog.trace.api.time.TimeSource; +import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import datadog.trace.bootstrap.instrumentation.api.SpanAttributes; import edu.umd.cs.findbugs.annotations.NonNull; -import io.opentelemetry.api.common.Attributes; import java.util.Collections; import java.util.HashMap; import java.util.List; @@ -18,7 +18,7 @@ public class OtelSpanEvent { private final long timestamp; private final String name; - private final Attributes attributes; + private final AgentSpan.Attributes attributes; private static TimeSource timeSource = SystemTimeSource.INSTANCE; public OtelSpanEvent(String name, io.opentelemetry.api.common.Attributes attributes) { @@ -75,7 +75,7 @@ public static String toTag(List events) { return builder.append(']').toString(); } - private static class Attributes { + private static class Attributes implements AgentSpan.Attributes { /** Represent an empty attributes. */ public static final Attributes EMPTY = new Attributes(Collections.emptyMap()); diff --git a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy index f2390258871..a50dc8a8b35 100644 --- a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy +++ b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy @@ -706,9 +706,9 @@ class OpenTelemetry14Test extends AgentTestRunner { where: exception | attributes | expectedAttributes - new NullPointerException("Null pointer") | Attributes.empty() | """{ exception.message: "${exception.getMessage()}", exception.type: "${exception.getClass().getName()}", exception.stacktrace: "${Arrays.toString(exception.getStackTrace())}"}""" - new NumberFormatException("Number format exception") | Attributes.builder().put("exception.message", "something-else").build() | """{exception.message: "something-else", exception.type: "${exception.getClass().getName()}", exception.stacktrace: "${Arrays.toString(exception.getStackTrace())}"}""" - new NullPointerException("Null pointer") | Attributes.builder().put("key", "value").build() | """{"key": "value","exception.message": "${exception.getMessage()}"}""" + new NullPointerException("Null pointer") | Attributes.empty() | """{ exception.message: "${exception.getMessage()}", exception.type: "${exception.getClass().getName()}", exception.stacktrace: "${final StringWriter errorString = new StringWriter(); exception.printStackTrace(new PrintWriter(errorString)); return errorString.toString()}"}""" + new NumberFormatException("Number format exception") | Attributes.builder().put("exception.message", "something-else").build() | """{exception.message: "something-else", exception.type: "${exception.getClass().getName()}", exception.stacktrace: "${final StringWriter errorString = new StringWriter(); exception.printStackTrace(new PrintWriter(errorString)); return errorString.toString()}"}""" + new NullPointerException("Null pointer") | Attributes.builder().put("key", "value").build() | """{"key": "value", exception.message: "${exception.getMessage()}", exception.type: "${exception.getClass().getName()}", exception.stacktrace: "${final StringWriter errorString = new StringWriter(); exception.printStackTrace(new PrintWriter(errorString)); return errorString.toString()}"}""" } def "test span name update"() { From e6d69dbb5b1ec7ce0635056e1a12872801077ebe Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Wed, 11 Sep 2024 10:53:06 -0400 Subject: [PATCH 26/37] Remove superfluous import --- .../shim/trace/OtelConventions.java | 5 ++-- .../test/groovy/OpenTelemetry14Test.groovy | 23 +++++++++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java index 3d4047e651b..417156fce88 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java @@ -327,7 +327,7 @@ public static Attributes processExceptionAttributes( } /** - * generate default list of attribtues about a Throwable + * generate default list of attributes about a Throwable * * @param exception the Throwable about which to generate attribtues * @return Map of attributes about the Throwable @@ -337,9 +337,10 @@ private static Map getAttributesFromException(Throwable exceptio { put("exception.message", exception.getMessage()); put("exception.type", exception.getClass().getName()); + // "stringify" error stack final StringWriter errorString = new StringWriter(); exception.printStackTrace(new PrintWriter(errorString)); - put("exception.stacktrace", errorString.toString()); + put("exception.stacktrace", exception.toString()); } }; } diff --git a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy index a50dc8a8b35..7e295f8d43a 100644 --- a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy +++ b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy @@ -711,6 +711,29 @@ class OpenTelemetry14Test extends AgentTestRunner { new NullPointerException("Null pointer") | Attributes.builder().put("key", "value").build() | """{"key": "value", exception.message: "${exception.getMessage()}", exception.type: "${exception.getClass().getName()}", exception.stacktrace: "${final StringWriter errorString = new StringWriter(); exception.printStackTrace(new PrintWriter(errorString)); return errorString.toString()}"}""" } + def "test span error meta on record multiple exceptions"() { + // Span's error tags should reflect the last recorded exception + setup: + def result = tracer.spanBuilder("some-name").startSpan() + def timeSource = new ControllableTimeSource() + timeSource.set(1000) + OtelSpanEvent.setTimeSource(timeSource) + def exception1 = new NullPointerException("Null pointer") + def exception2 = new NumberFormatException("Number format exception") + + when: + result.recordException(exception1) + result.recordException(exception2) + + then: + result.delegate.getTag(ERROR_MSG) == exception2.getMessage() + result.delegate.getTag(DDTags.ERROR_TYPE) == exception2.getClass().getName() + final StringWriter errorString = new StringWriter() + exception2.printStackTrace(new PrintWriter(errorString)) + result.delegate.getTag(DDTags.ERROR_STACK) == errorString.toString() + !result.delegate.isError() + } + def "test span name update"() { setup: def result = tracer.spanBuilder("some-name") From 4d9b4562647b90ac5a1450b4d0b2959ca6189417 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Wed, 11 Sep 2024 16:28:06 -0400 Subject: [PATCH 27/37] Optimized defaultExceptionAttributes --- .../shim/trace/OtelConventions.java | 59 ++-- .../shim/trace/OtelSpanEvent.java | 297 +++++------------- .../shim/trace/OtelSpanLink.java | 3 +- .../test/groovy/OpenTelemetry14Test.groovy | 7 +- .../instrumentation/api/SpanAttributes.java | 116 +------ 5 files changed, 118 insertions(+), 364 deletions(-) diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java index 417156fce88..331487fa0b1 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java @@ -27,6 +27,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.function.Function; import javax.annotation.Nullable; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -36,6 +37,22 @@ public final class OtelConventions { static final String OPERATION_NAME_SPECIFIC_ATTRIBUTE = "operation.name"; static final String ANALYTICS_EVENT_SPECIFIC_ATTRIBUTES = "analytics.event"; static final String HTTP_RESPONSE_STATUS_CODE_ATTRIBUTE = "http.response.status_code"; + // map of default attribute keys to apply to all SpanEvents generated with recordException, along + // with the function for getting the expected value from the provided Throwable + static final Map> defaultExceptionAttributes = + new HashMap>() { + { + put("exception.message", exception -> exception.getMessage()); + put("exception.type", exception -> exception.getClass().getName()); + put( + "exception.stacktrace", + exception -> { + final StringWriter errorString = new StringWriter(); + exception.printStackTrace(new PrintWriter(errorString)); + return errorString.toString(); + }); + } + }; private static final Logger LOGGER = LoggerFactory.getLogger(OtelConventions.class); @@ -246,16 +263,13 @@ private static String getStringAttribute(AgentSpan span, String key) { * AgentSpan.Attributes based on the AttributeKey type * * @param attributes the Attributes to convert - * @param format the format to convert the Attributes to, given attributes are handled differently - * for SpanLinks and SpanEvents * @return the converted AgentSpan.Attributes */ - public static AgentSpan.Attributes convertAttributes( - Attributes attributes, SpanAttributes.Builder.Format format) { + public static AgentSpan.Attributes convertAttributes(Attributes attributes) { if (attributes == null || attributes.isEmpty()) { return SpanAttributes.EMPTY; } - SpanAttributes.Builder builder = SpanAttributes.builder(format); + SpanAttributes.Builder builder = SpanAttributes.builder(); attributes.forEach( (attributeKey, value) -> { String key = attributeKey.getKey(); @@ -302,46 +316,21 @@ public static AgentSpan.Attributes convertAttributes( * * @param exception The Throwable from which to build default attributes * @param additionalAttributes Attributes provided by the user - * @return Attributes collection that combines defaultAttributes and additionalAttributes + * @return Attributes collection that combines defaultExceptionAttributes with + * additionalAttributes */ public static Attributes processExceptionAttributes( Throwable exception, Attributes additionalAttributes) { - Map defaultAttributes = getAttributesFromException(exception); // Create an AttributesBuilder with the additionalAttributes provided AttributesBuilder attrsBuilder = additionalAttributes.toBuilder(); - for (String key : defaultAttributes.keySet()) { + for (String key : defaultExceptionAttributes.keySet()) { // Add defaultAttributes onto the builder iff an equivalent key was not provided in // additionalAttributes if (additionalAttributes.get(AttributeKey.stringKey(key)) == null) { - Object value = defaultAttributes.get(key); - // Currently we only have Strings and booleans in defaultAttributes, but this could change. - if (value instanceof String) { - attrsBuilder.put(key, (String) defaultAttributes.get(key)); - } else if (value instanceof Boolean) { - attrsBuilder.put(key, (Boolean) defaultAttributes.get(key)); - } - // Need a catchall else here? + String value = defaultExceptionAttributes.get(key).apply(exception); + attrsBuilder.put(key, value); } } return attrsBuilder.build(); } - - /** - * generate default list of attributes about a Throwable - * - * @param exception the Throwable about which to generate attribtues - * @return Map of attributes about the Throwable - */ - private static Map getAttributesFromException(Throwable exception) { - return new HashMap() { - { - put("exception.message", exception.getMessage()); - put("exception.type", exception.getClass().getName()); - // "stringify" error stack - final StringWriter errorString = new StringWriter(); - exception.printStackTrace(new PrintWriter(errorString)); - put("exception.stacktrace", exception.toString()); - } - }; - } } diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java index 780ab8b8a10..8741611704f 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java @@ -1,14 +1,9 @@ package datadog.opentelemetry.shim.trace; -import static java.util.Objects.requireNonNull; - import datadog.trace.api.time.SystemTimeSource; import datadog.trace.api.time.TimeSource; -import datadog.trace.bootstrap.instrumentation.api.AgentSpan; -import datadog.trace.bootstrap.instrumentation.api.SpanAttributes; import edu.umd.cs.findbugs.annotations.NonNull; -import java.util.Collections; -import java.util.HashMap; +import io.opentelemetry.api.common.AttributeKey; import java.util.List; import java.util.Map; import java.util.Set; @@ -18,13 +13,12 @@ public class OtelSpanEvent { private final long timestamp; private final String name; - private final AgentSpan.Attributes attributes; + private final String attributes; private static TimeSource timeSource = SystemTimeSource.INSTANCE; public OtelSpanEvent(String name, io.opentelemetry.api.common.Attributes attributes) { this.name = name; - this.attributes = - OtelConventions.convertAttributes(attributes, SpanAttributes.Builder.Format.EVENTS); + this.attributes = AttributesJSONParser.toJson(attributes); this.timestamp = getNanosFromTimeSource(); } @@ -34,11 +28,85 @@ public OtelSpanEvent( long timestamp, TimeUnit unit) { this.name = name; - this.attributes = - OtelConventions.convertAttributes(attributes, SpanAttributes.Builder.Format.EVENTS); + this.attributes = AttributesJSONParser.toJson(attributes); this.timestamp = convertNano(timestamp, unit); } + // JSONParser is a helper class for turning the Map that holds the attributes + // into + // a JSON string + public static class AttributesJSONParser { + public static String toJson(io.opentelemetry.api.common.Attributes attributes) { + if (attributes == null) { + return ""; + } + StringBuilder jsonBuilder = new StringBuilder(); + jsonBuilder.append('{'); + + Set, Object>> entrySet = attributes.asMap().entrySet(); + int entryCount = 0; + + for (Map.Entry, Object> entry : entrySet) { + if (entryCount > 0) { + jsonBuilder.append(","); + } + + String key = + entry + .getKey() + .getKey(); // AttributeKey type has method `getKey()` that "stringifies" the key + Object value = entry.getValue(); + + // Escape key and append it + jsonBuilder.append("\"").append(escapeJson(key)).append("\":"); + + // Append value to jsonBuilder + appendValue(value, jsonBuilder); + entryCount++; + } + jsonBuilder.append('}'); + return jsonBuilder.toString(); + } + /** + * appendValue recursively adds the value of an Attribute to the active StringBuilder in JSON + * format, depending on the value's type + * + * @param value the value to append + * @param jsonBuilder the active 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"); + } + } + private static long convertNano(long timestamp, TimeUnit unit) { return TimeUnit.NANOSECONDS.convert(timestamp, unit); } @@ -56,9 +124,7 @@ public String toString() { new StringBuilder( "{\"time_unix_nano\":" + this.timestamp + ",\"name\":\"" + this.name + "\""); if (!this.attributes.isEmpty()) { - builder - .append(",\"attributes\":") - .append(SpanAttributes.JSONParser.toJson(this.attributes.asMap())); + builder.append(",\"attributes\":").append(this.attributes); } return builder.append('}').toString(); } @@ -74,207 +140,4 @@ public static String toTag(List events) { } return builder.append(']').toString(); } - - private static class Attributes implements AgentSpan.Attributes { - /** Represent an empty attributes. */ - public static final Attributes EMPTY = new Attributes(Collections.emptyMap()); - - private final Map attributes; - - protected Attributes(Map attributes) { - this.attributes = attributes; - } - - /** - * Gets a builder of the specified format to create attributes. - * - * @return A builder to create attributes. - */ - // public static Attributes.Builder builder(Attributes.Builder.Format format) { - // return new Attributes.Builder(format); - // } - /** - * Create attributes from its map representation. - * - * @param map A map representing the attributes. - * @return The related attributes. - */ - public static Attributes fromMap(Map map) { - return new Attributes(new HashMap<>(map)); - } - - public Map asMap() { - return this.attributes; - } - - public boolean isEmpty() { - return this.attributes.isEmpty(); - } - - public String toString() { - return "Attributes{" + this.attributes + '}'; - } - - // JSONParser is a helper class for turning the Map that holds the attributes - // into - // a JSON string - public static class JSONParser { - public static String toJson(Map map) { - StringBuilder jsonBuilder = new StringBuilder(); - jsonBuilder.append('{'); - - Set> entrySet = map.entrySet(); - int entryCount = 0; - - for (Map.Entry entry : entrySet) { - if (entryCount > 0) { - jsonBuilder.append(","); - } - - String key = entry.getKey(); - Object value = entry.getValue(); - - // Escape key and append it - jsonBuilder.append("\"").append(escapeJson(key)).append("\":"); - - // Append value to jsonBuilder - appendValue(value, jsonBuilder); - entryCount++; - } - jsonBuilder.append('}'); - return jsonBuilder.toString(); - } - /** - * appendValue recursively adds the value of an Attribute to the active StringBuilder in JSON - * format, depending on the value's type - * - * @param value the value to append - * @param jsonBuilder the active 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"); // For unsupported types, use null - } - } - - 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 class Builder { - // SpanLinks and OtelSpanEvents are encoded in different formats; Format represents the - // encoding - // standard - public enum Format { - LINKS, - EVENTS - } - - private final Map attributes; - private final OtelSpanEvent.Attributes.Builder.Format format; - - protected Builder(OtelSpanEvent.Attributes.Builder.Format format) { - this.attributes = new HashMap<>(); - this.format = format; - } - - public OtelSpanEvent.Attributes.Builder put(String key, String value) { - requireNonNull(key, "key must not be null"); - if (value != null) { - this.attributes.put(key, value); - } - return this; - } - - public OtelSpanEvent.Attributes.Builder put(String key, boolean value) { - requireNonNull(key, "key must not be null"); - if (this.format == OtelSpanEvent.Attributes.Builder.Format.LINKS) { - this.attributes.put(key, Boolean.toString(value)); - } else if (this.format == OtelSpanEvent.Attributes.Builder.Format.EVENTS) { - this.attributes.put(key, value); - } - return this; - } - - public OtelSpanEvent.Attributes.Builder put(String key, long value) { - requireNonNull(key, "key must not be null"); - if (this.format == OtelSpanEvent.Attributes.Builder.Format.LINKS) { - this.attributes.put(key, Long.toString(value)); - } else if (this.format == OtelSpanEvent.Attributes.Builder.Format.EVENTS) { - this.attributes.put(key, value); - } - return this; - } - - public OtelSpanEvent.Attributes.Builder put(String key, double value) { - requireNonNull(key, "key must not be null"); - if (this.format == OtelSpanEvent.Attributes.Builder.Format.LINKS) { - this.attributes.put(key, Double.toString(value)); - } else if (this.format == OtelSpanEvent.Attributes.Builder.Format.EVENTS) { - this.attributes.put(key, value); - } - return this; - } - - public OtelSpanEvent.Attributes.Builder putStringArray(String key, List array) { - return putArray(key, array); - } - - public OtelSpanEvent.Attributes.Builder putBooleanArray(String key, List array) { - return putArray(key, array); - } - - public OtelSpanEvent.Attributes.Builder putLongArray(String key, List array) { - return putArray(key, array); - } - - public OtelSpanEvent.Attributes.Builder putDoubleArray(String key, List array) { - return putArray(key, array); - } - - protected OtelSpanEvent.Attributes.Builder putArray(String key, List array) { - requireNonNull(key, "key must not be null"); - if (array != null) { - if (this.format == OtelSpanEvent.Attributes.Builder.Format.LINKS) { - for (int index = 0; index < array.size(); index++) { - Object value = array.get(index); - if (value != null) { - this.attributes.put(key + "." + index, value.toString()); - } - } - } else if (this.format == OtelSpanEvent.Attributes.Builder.Format.EVENTS) { - this.attributes.put(key, array); - } - } - return this; - } - - public OtelSpanEvent.Attributes build() { - return new OtelSpanEvent.Attributes(this.attributes); - } - } - } } diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanLink.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanLink.java index 6055ba37942..81255e15027 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanLink.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanLink.java @@ -5,7 +5,6 @@ import datadog.opentelemetry.shim.context.propagation.TraceStateHelper; import datadog.trace.api.DDSpanId; import datadog.trace.api.DDTraceId; -import datadog.trace.bootstrap.instrumentation.api.SpanAttributes.Builder.Format; import datadog.trace.bootstrap.instrumentation.api.SpanLink; import io.opentelemetry.api.trace.SpanContext; @@ -20,6 +19,6 @@ public OtelSpanLink(SpanContext spanContext, io.opentelemetry.api.common.Attribu DDSpanId.fromHex(spanContext.getSpanId()), spanContext.isSampled() ? SAMPLED_FLAG : DEFAULT_FLAGS, TraceStateHelper.encodeHeader(spanContext.getTraceState()), - convertAttributes(attributes, Format.LINKS)); + convertAttributes(attributes)); } } diff --git a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy index 7e295f8d43a..5034727ff1c 100644 --- a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy +++ b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy @@ -16,7 +16,6 @@ import opentelemetry14.context.propagation.TextMap import org.skyscreamer.jsonassert.JSONAssert import spock.lang.Subject -import java.sql.Time import java.util.concurrent.TimeUnit import static datadog.trace.api.DDTags.ERROR_MSG @@ -218,7 +217,7 @@ class OpenTelemetry14Test extends AgentTestRunner { where: name | timestamp | unit | attributes | expectedAttributes "evt1" | TIME_MILLIS | TimeUnit.MILLISECONDS | Attributes.empty() | null - "evt2" | TIME_NANO | TimeUnit.NANOSECONDS | Attributes.builder().put("string-key", "string-value").put("long-key", 123456789L).put("double-key", 1234.5678D).put("boolean-key-true", true).put("boolean-key-false", false).build() | '{ string-key: "string-value", long-key: 123456789, double-key: 1234.5678, boolean-key-true: true, boolean-key-false: false }' + "evt2" | TIME_NANO | TimeUnit.NANOSECONDS | Attributes.builder().put("string-key", "string-value").put("long-key", 123456789L).put("double-key", 1234.5678).put("boolean-key-true", true).put("boolean-key-false", false).build() | '{ string-key: "string-value", long-key: 123456789, double-key: 1234.5678, boolean-key-true: true, boolean-key-false: false }' "evt3" | TIME_NANO | TimeUnit.NANOSECONDS | Attributes.builder().put("string-key-array", "string-value1", "string-value2", "string-value3").put("long-key-array", 123456L, 1234567L, 12345678L).put("double-key-array", 1234.5D, 1234.56D, 1234.567D).put("boolean-key-array", true, false, true).build() | '{ string-key-array: [ "string-value1", "string-value2", "string-value3" ], long-key-array: [ 123456, 1234567, 12345678 ], double-key-array: [ 1234.5, 1234.56, 1234.567], boolean-key-array: [true, false, true] }' } @@ -395,7 +394,7 @@ class OpenTelemetry14Test extends AgentTestRunner { where: attributes | expectedAttributes Attributes.empty() | null - Attributes.builder().put("string-key", "string-value").put("long-key", 123456789L).put("double-key", 1234.5678D).put("boolean-key-true", true).put("boolean-key-false", false).build() | '{ string-key: "string-value", long-key: "123456789", double-key: "1234.5678", boolean-key-true: "true", boolean-key-false: "false" }' + Attributes.builder().put("string-key", "string-value").put("long-key", 123456789L).put("double-key", 1234.5678).put("boolean-key-true", true).put("boolean-key-false", false).build() | '{ string-key: "string-value", long-key: "123456789", double-key: "1234.5678", boolean-key-true: "true", boolean-key-false: "false" }' Attributes.builder().put("string-key-array", "string-value1", "string-value2", "string-value3").put("long-key-array", 123456L, 1234567L, 12345678L).put("double-key-array", 1234.5D, 1234.56D, 1234.567D).put("boolean-key-array", true, false, true).build() | '{ string-key-array.0: "string-value1", string-key-array.1: "string-value2", string-key-array.2: "string-value3", long-key-array.0: "123456", long-key-array.1: "1234567", long-key-array.2: "12345678", double-key-array.0: "1234.5", double-key-array.1: "1234.56", double-key-array.2: "1234.567", boolean-key-array.0: "true", boolean-key-array.1: "false", boolean-key-array.2: "true" }' } @@ -712,7 +711,7 @@ class OpenTelemetry14Test extends AgentTestRunner { } def "test span error meta on record multiple exceptions"() { - // Span's error tags should reflect the last recorded exception + // Span's "error" tags should reflect the last recorded exception setup: def result = tracer.spanBuilder("some-name").startSpan() def timeSource = new ControllableTimeSource() diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java index 4af80ef6313..61be2c762c8 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java @@ -7,7 +7,6 @@ import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.Set; /** This class is a base implementation of {@link Attributes}. */ public class SpanAttributes implements Attributes { @@ -25,8 +24,8 @@ protected SpanAttributes(Map attributes) { * * @return A builder to create attributes. */ - public static Builder builder(Builder.Format format) { - return new Builder(format); + public static Builder builder() { + return new Builder(); } /** @@ -54,89 +53,12 @@ public String toString() { return "SpanAttributes{" + this.attributes + '}'; } - // JSONParser is a helper class for turning the Map that holds the attributes into - // a JSON string - public static class JSONParser { - public static String toJson(Map map) { - StringBuilder jsonBuilder = new StringBuilder(); - jsonBuilder.append('{'); - - Set> entrySet = map.entrySet(); - int entryCount = 0; - - for (Map.Entry entry : entrySet) { - if (entryCount > 0) { - jsonBuilder.append(","); - } - - String key = entry.getKey(); - Object value = entry.getValue(); - - // Escape key and append it - jsonBuilder.append("\"").append(escapeJson(key)).append("\":"); - - // Append value to jsonBuilder - appendValue(value, jsonBuilder); - entryCount++; - } - jsonBuilder.append('}'); - return jsonBuilder.toString(); - } - - /** - * appendValue recursively adds the value of an Attribute to the active StringBuilder in JSON - * format, depending on the value's type - * - * @param value the value to append - * @param jsonBuilder the active 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"); // For unsupported types, use null - } - } - - 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 class Builder { - // SpanLinks and OtelSpanEvents are encoded in different formats; Format represents the encoding - // standard - public enum Format { - LINKS, - EVENTS - } private final Map attributes; - private final Format format; - protected Builder(Format format) { + protected Builder() { this.attributes = new HashMap<>(); - this.format = format; } public Builder put(String key, String value) { @@ -149,31 +71,19 @@ public Builder put(String key, String value) { public Builder put(String key, boolean value) { requireNonNull(key, "key must not be null"); - if (this.format == Format.LINKS) { - this.attributes.put(key, Boolean.toString(value)); - } else if (this.format == Format.EVENTS) { - this.attributes.put(key, value); - } + this.attributes.put(key, Boolean.toString(value)); return this; } public Builder put(String key, long value) { requireNonNull(key, "key must not be null"); - if (this.format == Format.LINKS) { - this.attributes.put(key, Long.toString(value)); - } else if (this.format == Format.EVENTS) { - this.attributes.put(key, value); - } + this.attributes.put(key, Long.toString(value)); return this; } public Builder put(String key, double value) { requireNonNull(key, "key must not be null"); - if (this.format == Format.LINKS) { - this.attributes.put(key, Double.toString(value)); - } else if (this.format == Format.EVENTS) { - this.attributes.put(key, value); - } + this.attributes.put(key, Double.toString(value)); return this; } @@ -195,16 +105,10 @@ public Builder putDoubleArray(String key, List array) { protected Builder putArray(String key, List array) { requireNonNull(key, "key must not be null"); - if (array != null) { - if (this.format == Format.LINKS) { - for (int index = 0; index < array.size(); index++) { - Object value = array.get(index); - if (value != null) { - this.attributes.put(key + "." + index, value.toString()); - } - } - } else if (this.format == Format.EVENTS) { - this.attributes.put(key, array); + for (int index = 0; index < array.size(); index++) { + Object value = array.get(index); + if (value != null) { + this.attributes.put(key + "." + index, value.toString()); } } return this; From f8721e45edc9e1ccf4ced34eee0943e1a7b2d75c Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Wed, 11 Sep 2024 16:48:31 -0400 Subject: [PATCH 28/37] Reverted all previous changes to spanlinks --- .../shim/trace/OtelConventions.java | 3 ++- .../opentelemetry/shim/trace/OtelSpan.java | 8 +++---- .../shim/trace/OtelSpanEvent.java | 4 +--- .../instrumentation/api/AgentSpan.java | 2 +- .../instrumentation/api/SpanAttributes.java | 22 ++++++++++--------- .../instrumentation/api/SpanLinkTest.groovy | 8 +++---- 6 files changed, 24 insertions(+), 23 deletions(-) diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java index 331487fa0b1..ac2cbbd9d38 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java @@ -38,7 +38,7 @@ public final class OtelConventions { static final String ANALYTICS_EVENT_SPECIFIC_ATTRIBUTES = "analytics.event"; static final String HTTP_RESPONSE_STATUS_CODE_ATTRIBUTE = "http.response.status_code"; // map of default attribute keys to apply to all SpanEvents generated with recordException, along - // with the function for getting the expected value from the provided Throwable + // with the logic to generate the expected value from the provided Throwable static final Map> defaultExceptionAttributes = new HashMap>() { { @@ -327,6 +327,7 @@ public static Attributes processExceptionAttributes( // Add defaultAttributes onto the builder iff an equivalent key was not provided in // additionalAttributes if (additionalAttributes.get(AttributeKey.stringKey(key)) == null) { + // get the value using the function found at `key` in defaultExceptionAttributes String value = defaultExceptionAttributes.get(key).apply(exception); attrsBuilder.put(key, value); } 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 0883d81d636..a7f8aee7f0a 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 @@ -112,13 +112,13 @@ public Span setStatus(StatusCode statusCode, String description) { } /** - * Records information about the Throwable as a span event exception.escaped cannot be determined - * by this function and therefore is not automatically recorded. Record this manually using - * additionalAttributes See: ... * * @param exception the Throwable to record - * @param additionalAttributes the additional attributes to record + * @param additionalAttributes the additional attributes to record on this span event */ @Override public Span recordException(Throwable exception, Attributes additionalAttributes) { diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java index 8741611704f..89e17232005 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java @@ -32,9 +32,7 @@ public OtelSpanEvent( this.timestamp = convertNano(timestamp, unit); } - // JSONParser is a helper class for turning the Map that holds the attributes - // into - // a JSON string + // JSONParser is a helper class for JSON-encoding OtelSpanEvent attributes public static class AttributesJSONParser { public static String toJson(io.opentelemetry.api.common.Attributes attributes) { if (attributes == null) { diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentSpan.java b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentSpan.java index eb0566ff2dd..6b55ad91b9d 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentSpan.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentSpan.java @@ -235,7 +235,7 @@ interface Attributes { * * @return The attributes as an immutable map. */ - Map asMap(); + Map asMap(); /** * Checks whether the attributes are empty. diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java index 61be2c762c8..f57e1a87d0e 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java @@ -13,14 +13,14 @@ public class SpanAttributes implements Attributes { /** Represent an empty attributes. */ public static final Attributes EMPTY = new SpanAttributes(Collections.emptyMap()); - private final Map attributes; + private final Map attributes; - protected SpanAttributes(Map attributes) { + protected SpanAttributes(Map attributes) { this.attributes = attributes; } /** - * Gets a builder of the specified format to create attributes. + * Gets a builder to create attributes. * * @return A builder to create attributes. */ @@ -34,12 +34,12 @@ public static Builder builder() { * @param map A map representing the attributes. * @return The related attributes. */ - public static SpanAttributes fromMap(Map map) { + public static SpanAttributes fromMap(Map map) { return new SpanAttributes(new HashMap<>(map)); } @Override - public Map asMap() { + public Map asMap() { return this.attributes; } @@ -55,7 +55,7 @@ public String toString() { public static class Builder { - private final Map attributes; + private final Map attributes; protected Builder() { this.attributes = new HashMap<>(); @@ -105,10 +105,12 @@ public Builder putDoubleArray(String key, List array) { protected Builder putArray(String key, List array) { requireNonNull(key, "key must not be null"); - for (int index = 0; index < array.size(); index++) { - Object value = array.get(index); - if (value != null) { - this.attributes.put(key + "." + index, value.toString()); + if (array != null) { + for (int index = 0; index < array.size(); index++) { + Object value = array.get(index); + if (value != null) { + this.attributes.put(key + "." + index, value.toString()); + } } } return this; diff --git a/internal-api/src/test/groovy/datadog/trace/bootstrap/instrumentation/api/SpanLinkTest.groovy b/internal-api/src/test/groovy/datadog/trace/bootstrap/instrumentation/api/SpanLinkTest.groovy index 60f2d291f66..a58e38f1094 100644 --- a/internal-api/src/test/groovy/datadog/trace/bootstrap/instrumentation/api/SpanLinkTest.groovy +++ b/internal-api/src/test/groovy/datadog/trace/bootstrap/instrumentation/api/SpanLinkTest.groovy @@ -53,13 +53,13 @@ class SpanLinkTest extends DDSpecification { def "test span link attributes api"() { when: - def attributes = SpanAttributes.builder(SpanAttributes.Builder.Format.LINKS).build() + def attributes = SpanAttributes.builder().build() then: attributes.isEmpty() when: - attributes = SpanAttributes.builder(SpanAttributes.Builder.Format.LINKS).put('key', 'value').build() + attributes = SpanAttributes.builder().put('key', 'value').build() then: !attributes.isEmpty() @@ -67,7 +67,7 @@ class SpanLinkTest extends DDSpecification { def "test span link attributes encoding"() { setup: - def builder = SpanAttributes.builder(SpanAttributes.Builder.Format.LINKS) + def builder = SpanAttributes.builder() when: builder.put('string', 'value') @@ -137,7 +137,7 @@ class SpanLinkTest extends DDSpecification { def "test span link attributes toString"() { when: - SpanAttributes.builder(SpanAttributes.Builder.Format.LINKS).build().toString() + SpanAttributes.builder().build().toString() then: notThrown(NullPointerException) From c34e1391ad0b47a299ae8a15b14248aa7dfbf7f8 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Wed, 11 Sep 2024 16:50:26 -0400 Subject: [PATCH 29/37] Remove missed changes to spanlinks files --- dd-trace-core/src/main/java/datadog/trace/core/DDSpanLink.java | 2 +- .../trace/bootstrap/instrumentation/api/SpanAttributes.java | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/dd-trace-core/src/main/java/datadog/trace/core/DDSpanLink.java b/dd-trace-core/src/main/java/datadog/trace/core/DDSpanLink.java index a87762a3504..6b4491f48a1 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/DDSpanLink.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/DDSpanLink.java @@ -140,6 +140,6 @@ private static class SpanLinkJson { String span_id; Byte flags; String tracestate; - Map attributes; + Map attributes; } } diff --git a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java index f57e1a87d0e..46123e43a13 100644 --- a/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java +++ b/internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanAttributes.java @@ -54,7 +54,6 @@ public String toString() { } public static class Builder { - private final Map attributes; protected Builder() { From 6d6cbc36a70e5d1949ba627f7d07cf20908b0927 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Wed, 11 Sep 2024 21:49:26 -0400 Subject: [PATCH 30/37] Moved exception attributes processing into spanevent class --- .../shim/trace/OtelConventions.java | 50 ------------------- .../opentelemetry/shim/trace/OtelSpan.java | 36 ++++++++++++- .../shim/trace/OtelSpanEvent.java | 25 +++++++++- .../OpenTelemetryInstrumentation.java | 1 + 4 files changed, 58 insertions(+), 54 deletions(-) diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java index ac2cbbd9d38..636ff85ed1a 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java @@ -20,14 +20,8 @@ import datadog.trace.bootstrap.instrumentation.api.Tags; import io.opentelemetry.api.common.AttributeKey; import io.opentelemetry.api.common.Attributes; -import io.opentelemetry.api.common.AttributesBuilder; import io.opentelemetry.api.trace.SpanKind; -import java.io.PrintWriter; -import java.io.StringWriter; -import java.util.HashMap; import java.util.List; -import java.util.Map; -import java.util.function.Function; import javax.annotation.Nullable; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -37,22 +31,6 @@ public final class OtelConventions { static final String OPERATION_NAME_SPECIFIC_ATTRIBUTE = "operation.name"; static final String ANALYTICS_EVENT_SPECIFIC_ATTRIBUTES = "analytics.event"; static final String HTTP_RESPONSE_STATUS_CODE_ATTRIBUTE = "http.response.status_code"; - // map of default attribute keys to apply to all SpanEvents generated with recordException, along - // with the logic to generate the expected value from the provided Throwable - static final Map> defaultExceptionAttributes = - new HashMap>() { - { - put("exception.message", exception -> exception.getMessage()); - put("exception.type", exception -> exception.getClass().getName()); - put( - "exception.stacktrace", - exception -> { - final StringWriter errorString = new StringWriter(); - exception.printStackTrace(new PrintWriter(errorString)); - return errorString.toString(); - }); - } - }; private static final Logger LOGGER = LoggerFactory.getLogger(OtelConventions.class); @@ -306,32 +284,4 @@ public static AgentSpan.Attributes convertAttributes(Attributes attributes) { }); return builder.build(); } - - /** - * processExceptionAttributes generates Attributes about the exception using a default list + the - * additionalAttributes provided by the user - * - *

If the same key exists in defaultAttributes and additionalAttributes, the latter always - * wins. - * - * @param exception The Throwable from which to build default attributes - * @param additionalAttributes Attributes provided by the user - * @return Attributes collection that combines defaultExceptionAttributes with - * additionalAttributes - */ - public static Attributes processExceptionAttributes( - Throwable exception, Attributes additionalAttributes) { - // Create an AttributesBuilder with the additionalAttributes provided - AttributesBuilder attrsBuilder = additionalAttributes.toBuilder(); - for (String key : defaultExceptionAttributes.keySet()) { - // Add defaultAttributes onto the builder iff an equivalent key was not provided in - // additionalAttributes - if (additionalAttributes.get(AttributeKey.stringKey(key)) == null) { - // get the value using the function found at `key` in defaultExceptionAttributes - String value = defaultExceptionAttributes.get(key).apply(exception); - attrsBuilder.put(key, value); - } - } - return attrsBuilder.build(); - } } 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 a7f8aee7f0a..975bd22b5ae 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 @@ -2,7 +2,6 @@ import static datadog.opentelemetry.shim.trace.OtelConventions.applyNamingConvention; import static datadog.opentelemetry.shim.trace.OtelConventions.applyReservedAttribute; -import static datadog.opentelemetry.shim.trace.OtelConventions.processExceptionAttributes; import static datadog.opentelemetry.shim.trace.OtelConventions.setEventsAsTag; import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activateSpan; import static io.opentelemetry.api.trace.StatusCode.ERROR; @@ -15,6 +14,7 @@ import datadog.trace.bootstrap.instrumentation.api.ErrorPriorities; import io.opentelemetry.api.common.AttributeKey; import io.opentelemetry.api.common.Attributes; +import io.opentelemetry.api.common.AttributesBuilder; import io.opentelemetry.api.trace.Span; import io.opentelemetry.api.trace.SpanContext; import io.opentelemetry.api.trace.StatusCode; @@ -129,11 +129,43 @@ public Span recordException(Throwable exception, Attributes additionalAttributes this.events.add( new OtelSpanEvent( "exception", processExceptionAttributes(exception, additionalAttributes))); - this.delegate.addThrowable(exception, ErrorPriorities.UNSET); } return this; } + /** + * processExceptionAttributes generates Attributes about the exception using a default list + the + * additionalAttributes provided by the user. These attributes are also used to populate Span + * error tags. + * + *

If the same key exists in defaultAttributes and additionalAttributes, the latter always + * wins. + * + * @param exception The Throwable from which to build default attributes + * @param additionalAttributes Attributes provided by the user + * @return Attributes collection that combines defaultExceptionAttributes with + * additionalAttributes + */ + private Attributes processExceptionAttributes( + Throwable exception, Attributes additionalAttributes) { + // Create an AttributesBuilder with the additionalAttributes provided + AttributesBuilder attrsBuilder = additionalAttributes.toBuilder(); + for (String key : OtelSpanEvent.defaultExceptionAttributes.keySet()) { + // if(additionalAttributes.get(AttributeKey.stringKey(key)) != null) { + // + // } + // Add defaultAttributes onto the builder iff an equivalent key was not provided in + // additionalAttributes + if (additionalAttributes.get(AttributeKey.stringKey(key)) == null) { + // get the value using the function found at `key` in defaultExceptionAttributes + String value = OtelSpanEvent.defaultExceptionAttributes.get(key).apply(exception); + attrsBuilder.put(key, value); + } + } + this.delegate.addThrowable(exception, ErrorPriorities.UNSET); + return attrsBuilder.build(); + } + @Override public Span updateName(String name) { if (this.recording) { diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java index 89e17232005..68f96a37b85 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java @@ -4,16 +4,20 @@ import datadog.trace.api.time.TimeSource; import edu.umd.cs.findbugs.annotations.NonNull; import io.opentelemetry.api.common.AttributeKey; +import java.io.PrintWriter; +import java.io.StringWriter; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Set; import java.util.concurrent.TimeUnit; +import java.util.function.Function; public class OtelSpanEvent { private final long timestamp; private final String name; - private final String attributes; + private String attributes = ""; private static TimeSource timeSource = SystemTimeSource.INSTANCE; public OtelSpanEvent(String name, io.opentelemetry.api.common.Attributes attributes) { @@ -35,7 +39,7 @@ public OtelSpanEvent( // JSONParser is a helper class for JSON-encoding OtelSpanEvent attributes public static class AttributesJSONParser { public static String toJson(io.opentelemetry.api.common.Attributes attributes) { - if (attributes == null) { + if (attributes == null || attributes.isEmpty()) { return ""; } StringBuilder jsonBuilder = new StringBuilder(); @@ -138,4 +142,21 @@ public static String toTag(List events) { } return builder.append(']').toString(); } + + // map of default attribute keys to apply to all SpanEvents generated with recordException, along + // with the logic to generate the expected value from the provided Throwable + static final Map> defaultExceptionAttributes = + new HashMap>() { + { + put("exception.message", exception -> exception.getMessage()); + put("exception.type", exception -> exception.getClass().getName()); + put( + "exception.stacktrace", + exception -> { + final StringWriter errorString = new StringWriter(); + exception.printStackTrace(new PrintWriter(errorString)); + return errorString.toString(); + }); + } + }; } diff --git a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/OpenTelemetryInstrumentation.java b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/OpenTelemetryInstrumentation.java index 817fc68c60d..30dd3b9a40e 100644 --- a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/OpenTelemetryInstrumentation.java +++ b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/OpenTelemetryInstrumentation.java @@ -74,6 +74,7 @@ public String[] helperClassNames() { "datadog.opentelemetry.shim.trace.OtelSpanBuilder", "datadog.opentelemetry.shim.trace.OtelSpanBuilder$1", "datadog.opentelemetry.shim.trace.OtelSpanContext", + "datadog.opentelemetry.shim.trace.OtelSpanEvent$AttributesJSONParser", "datadog.opentelemetry.shim.trace.OtelSpanLink", "datadog.opentelemetry.shim.trace.OtelTracer", "datadog.opentelemetry.shim.trace.OtelTracerBuilder", From 71d4b69b314478cdd598612e43dd2143b84d450c Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Thu, 12 Sep 2024 16:37:54 -0400 Subject: [PATCH 31/37] Fix error meta behavior with record exception --- .../opentelemetry/shim/trace/OtelSpan.java | 58 ++++++++++++++----- .../shim/trace/OtelSpanEvent.java | 21 ------- .../OpenTelemetryInstrumentation.java | 1 - .../test/groovy/OpenTelemetry14Test.groovy | 36 +++++------- 4 files changed, 59 insertions(+), 57 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 975bd22b5ae..313648041f3 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 @@ -8,10 +8,10 @@ import static io.opentelemetry.api.trace.StatusCode.OK; import static io.opentelemetry.api.trace.StatusCode.UNSET; +import datadog.trace.api.DDTags; import datadog.trace.bootstrap.instrumentation.api.AgentScope; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import datadog.trace.bootstrap.instrumentation.api.AttachableWrapper; -import datadog.trace.bootstrap.instrumentation.api.ErrorPriorities; import io.opentelemetry.api.common.AttributeKey; import io.opentelemetry.api.common.Attributes; import io.opentelemetry.api.common.AttributesBuilder; @@ -20,6 +20,8 @@ import io.opentelemetry.api.trace.StatusCode; import io.opentelemetry.api.trace.TraceFlags; import io.opentelemetry.api.trace.TraceState; +import java.io.PrintWriter; +import java.io.StringWriter; import java.util.ArrayList; import java.util.List; import java.util.concurrent.TimeUnit; @@ -150,22 +152,52 @@ private Attributes processExceptionAttributes( Throwable exception, Attributes additionalAttributes) { // Create an AttributesBuilder with the additionalAttributes provided AttributesBuilder attrsBuilder = additionalAttributes.toBuilder(); - for (String key : OtelSpanEvent.defaultExceptionAttributes.keySet()) { - // if(additionalAttributes.get(AttributeKey.stringKey(key)) != null) { - // - // } - // Add defaultAttributes onto the builder iff an equivalent key was not provided in - // additionalAttributes - if (additionalAttributes.get(AttributeKey.stringKey(key)) == null) { - // get the value using the function found at `key` in defaultExceptionAttributes - String value = OtelSpanEvent.defaultExceptionAttributes.get(key).apply(exception); - attrsBuilder.put(key, value); - } + + // Check whether additionalAttributes contains any of the reserved exception attributes + + String key = "exception.message"; + String attrsValue = additionalAttributes.get(AttributeKey.stringKey(key)); + // reserved attribute "exception.message" found additionalAttributes + if (attrsValue != null) { + // use provided value to set ERROR_MSG tag + this.delegate.setTag(DDTags.ERROR_MSG, attrsValue); + } else { + // use exception to set ERROR_MSG tag + String value = exception.getMessage(); + this.delegate.setTag(DDTags.ERROR_MSG, value); + // and append to the builder + attrsBuilder.put(key, value); } - this.delegate.addThrowable(exception, ErrorPriorities.UNSET); + + key = "exception.type"; + attrsValue = additionalAttributes.get(AttributeKey.stringKey(key)); + if (attrsValue != null) { + this.delegate.setTag(DDTags.ERROR_TYPE, attrsValue); + } else { + String value = exception.getClass().getName(); + this.delegate.setTag(DDTags.ERROR_TYPE, value); + attrsBuilder.put(key, value); + } + + key = "exception.stacktrace"; + attrsValue = additionalAttributes.get(AttributeKey.stringKey(key)); + if (attrsValue != null) { + this.delegate.setTag(DDTags.ERROR_STACK, attrsValue); + } else { + String value = stringifyErrorStack(exception); + this.delegate.setTag(DDTags.ERROR_STACK, value); + attrsBuilder.put(key, value); + } + return attrsBuilder.build(); } + static String stringifyErrorStack(Throwable error) { + final StringWriter errorString = new StringWriter(); + error.printStackTrace(new PrintWriter(errorString)); + return errorString.toString(); + } + @Override public Span updateName(String name) { if (this.recording) { diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java index 68f96a37b85..3c3105745f8 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java @@ -4,14 +4,10 @@ import datadog.trace.api.time.TimeSource; import edu.umd.cs.findbugs.annotations.NonNull; import io.opentelemetry.api.common.AttributeKey; -import java.io.PrintWriter; -import java.io.StringWriter; -import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Set; import java.util.concurrent.TimeUnit; -import java.util.function.Function; public class OtelSpanEvent { @@ -142,21 +138,4 @@ public static String toTag(List events) { } return builder.append(']').toString(); } - - // map of default attribute keys to apply to all SpanEvents generated with recordException, along - // with the logic to generate the expected value from the provided Throwable - static final Map> defaultExceptionAttributes = - new HashMap>() { - { - put("exception.message", exception -> exception.getMessage()); - put("exception.type", exception -> exception.getClass().getName()); - put( - "exception.stacktrace", - exception -> { - final StringWriter errorString = new StringWriter(); - exception.printStackTrace(new PrintWriter(errorString)); - return errorString.toString(); - }); - } - }; } diff --git a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/OpenTelemetryInstrumentation.java b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/OpenTelemetryInstrumentation.java index 30dd3b9a40e..bc7774c57f5 100644 --- a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/OpenTelemetryInstrumentation.java +++ b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/OpenTelemetryInstrumentation.java @@ -66,7 +66,6 @@ public String[] helperClassNames() { "datadog.opentelemetry.shim.trace.OtelExtractedContext", "datadog.opentelemetry.shim.trace.OtelConventions", "datadog.opentelemetry.shim.trace.OtelConventions$1", - "datadog.opentelemetry.shim.trace.OtelConventions$2", "datadog.opentelemetry.shim.trace.OtelSpan", "datadog.opentelemetry.shim.trace.OtelSpan$1", "datadog.opentelemetry.shim.trace.OtelSpan$NoopSpan", diff --git a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy index 5034727ff1c..2eee1d8db43 100644 --- a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy +++ b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy @@ -1,4 +1,5 @@ import datadog.opentelemetry.shim.trace.OtelSpanEvent +import datadog.opentelemetry.shim.trace.OtelSpan import datadog.trace.agent.test.AgentTestRunner import datadog.trace.api.DDSpanId import datadog.trace.api.DDTags @@ -19,6 +20,8 @@ import spock.lang.Subject import java.util.concurrent.TimeUnit 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.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 @@ -653,29 +656,15 @@ class OpenTelemetry14Test extends AgentTestRunner { } } - def "test span record exception"() { + def "test span record exception event tags"() { setup: def result = tracer.spanBuilder("some-name").startSpan() def timeSource = new ControllableTimeSource() timeSource.set(1000) OtelSpanEvent.setTimeSource(timeSource) - expect: - result.delegate.getTag(ERROR_MSG) == null - result.delegate.getTag(DDTags.ERROR_TYPE) == null - result.delegate.getTag(DDTags.ERROR_STACK) == null - !result.delegate.isError() - when: result.recordException(exception, attributes) - - then: - result.delegate.getTag(ERROR_MSG) == exception.getMessage() - result.delegate.getTag(DDTags.ERROR_TYPE) == exception.getClass().getName() - result.delegate.getTag(DDTags.ERROR_STACK) != null - !result.delegate.isError() - - when: result.end() then: @@ -686,6 +675,7 @@ class OpenTelemetry14Test extends AgentTestRunner { "attributes": ${expectedAttributes} } ]""" + assertTraces(1) { trace(1) { span { @@ -696,18 +686,20 @@ class OpenTelemetry14Test extends AgentTestRunner { tags { defaultTags() "$SPAN_KIND" "$SPAN_KIND_INTERNAL" - errorTags(exception) tag("events", { JSONAssert.assertEquals(expectedEventTag, it as String, false); return true }) + tag(ERROR_MSG, errorMsg) + tag(ERROR_TYPE, errorType) + tag(ERROR_STACK, errorStack) } } } } where: - exception | attributes | expectedAttributes - new NullPointerException("Null pointer") | Attributes.empty() | """{ exception.message: "${exception.getMessage()}", exception.type: "${exception.getClass().getName()}", exception.stacktrace: "${final StringWriter errorString = new StringWriter(); exception.printStackTrace(new PrintWriter(errorString)); return errorString.toString()}"}""" - new NumberFormatException("Number format exception") | Attributes.builder().put("exception.message", "something-else").build() | """{exception.message: "something-else", exception.type: "${exception.getClass().getName()}", exception.stacktrace: "${final StringWriter errorString = new StringWriter(); exception.printStackTrace(new PrintWriter(errorString)); return errorString.toString()}"}""" - new NullPointerException("Null pointer") | Attributes.builder().put("key", "value").build() | """{"key": "value", exception.message: "${exception.getMessage()}", exception.type: "${exception.getClass().getName()}", exception.stacktrace: "${final StringWriter errorString = new StringWriter(); exception.printStackTrace(new PrintWriter(errorString)); return errorString.toString()}"}""" + exception | attributes | errorMsg | errorType | errorStack | expectedAttributes + new NullPointerException("Null pointer") | Attributes.empty() | exception.getMessage() | exception.getClass().getName() | OtelSpan.stringifyErrorStack(exception) | """{ exception.message: "$errorMsg", exception.type: "$errorType", exception.stacktrace: "$errorStack"}""" + new NumberFormatException("Number format exception") | Attributes.builder().put("exception.message", "something-else").build() | "something-else" | exception.getClass().getName() | OtelSpan.stringifyErrorStack(exception) | """{exception.message: "$errorMsg", exception.type: "$errorType", exception.stacktrace: "$errorStack"}""" + new NullPointerException("Null pointer") | Attributes.builder().put("key", "value").build() | exception.getMessage() | exception.getClass().getName() | OtelSpan.stringifyErrorStack(exception) | """{"key": "value", exception.message: "$errorMsg", exception.type: "$errorType", exception.stacktrace: "$errorStack"}""" } def "test span error meta on record multiple exceptions"() { @@ -726,10 +718,10 @@ class OpenTelemetry14Test extends AgentTestRunner { then: result.delegate.getTag(ERROR_MSG) == exception2.getMessage() - result.delegate.getTag(DDTags.ERROR_TYPE) == exception2.getClass().getName() + result.delegate.getTag(ERROR_TYPE) == exception2.getClass().getName() final StringWriter errorString = new StringWriter() exception2.printStackTrace(new PrintWriter(errorString)) - result.delegate.getTag(DDTags.ERROR_STACK) == errorString.toString() + result.delegate.getTag(ERROR_STACK) == errorString.toString() !result.delegate.isError() } From 684170ad36e2222537b2e1481cecc100ec3cb4b2 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Thu, 12 Sep 2024 16:50:38 -0400 Subject: [PATCH 32/37] Update comments and fix little nits --- .../shim/trace/OtelConventions.java | 9 +------ .../opentelemetry/shim/trace/OtelSpan.java | 26 +++++++++---------- .../java/datadog/trace/core/DDSpanLink.java | 2 +- 3 files changed, 14 insertions(+), 23 deletions(-) diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java index 636ff85ed1a..9ded57d7152 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java @@ -236,15 +236,8 @@ private static String getStringAttribute(AgentSpan span, String key) { return (String) tag; } - /** - * convertAttributes is a helper function for converting opentelemetry Attributes to - * AgentSpan.Attributes based on the AttributeKey type - * - * @param attributes the Attributes to convert - * @return the converted AgentSpan.Attributes - */ public static AgentSpan.Attributes convertAttributes(Attributes attributes) { - if (attributes == null || attributes.isEmpty()) { + if (attributes.isEmpty()) { return SpanAttributes.EMPTY; } SpanAttributes.Builder builder = SpanAttributes.builder(); 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 313648041f3..6ed83fd46f6 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 @@ -136,12 +136,12 @@ public Span recordException(Throwable exception, Attributes additionalAttributes } /** - * processExceptionAttributes generates Attributes about the exception using a default list + the - * additionalAttributes provided by the user. These attributes are also used to populate Span - * error tags. + * processExceptionAttributes generates Attributes about the exception and sets Span error tags + * using the processed information. * - *

If the same key exists in defaultAttributes and additionalAttributes, the latter always - * wins. + *

If additionalAttributes contains a key from defaultAttributes, the value set in + * additionalAttributes will be used in Attributes and in error tag Else, the value is determined + * from the exception itself. * * @param exception The Throwable from which to build default attributes * @param additionalAttributes Attributes provided by the user @@ -153,19 +153,17 @@ private Attributes processExceptionAttributes( // Create an AttributesBuilder with the additionalAttributes provided AttributesBuilder attrsBuilder = additionalAttributes.toBuilder(); - // Check whether additionalAttributes contains any of the reserved exception attributes + // Check whether additionalAttributes contains any of the reserved exception attribute keys String key = "exception.message"; String attrsValue = additionalAttributes.get(AttributeKey.stringKey(key)); - // reserved attribute "exception.message" found additionalAttributes - if (attrsValue != null) { - // use provided value to set ERROR_MSG tag - this.delegate.setTag(DDTags.ERROR_MSG, attrsValue); + if (attrsValue != null) { // additionalAttributes contains "exception.message" + this.delegate.setTag( + DDTags.ERROR_MSG, + attrsValue); // use the value provided in additionalAttributes to set ERROR_MSG tag } else { - // use exception to set ERROR_MSG tag - String value = exception.getMessage(); - this.delegate.setTag(DDTags.ERROR_MSG, value); - // and append to the builder + String value = exception.getMessage(); // use exception to set ERROR_MSG tag + this.delegate.setTag(DDTags.ERROR_MSG, value); // and append to the builder attrsBuilder.put(key, value); } diff --git a/dd-trace-core/src/main/java/datadog/trace/core/DDSpanLink.java b/dd-trace-core/src/main/java/datadog/trace/core/DDSpanLink.java index 6b4491f48a1..641e6fe56ab 100644 --- a/dd-trace-core/src/main/java/datadog/trace/core/DDSpanLink.java +++ b/dd-trace-core/src/main/java/datadog/trace/core/DDSpanLink.java @@ -65,7 +65,7 @@ public static SpanLink from(ExtractedContext context, Attributes attributes) { * @param links The span link collection to encode. * @return The encoded tag value, {@code null} if no links. */ - static String toTag(List links) { + public static String toTag(List links) { if (links == null || links.isEmpty()) { return null; } From af715791fbbbc311b8c12c09576ba41dee95bffd Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Thu, 12 Sep 2024 16:57:51 -0400 Subject: [PATCH 33/37] Improve javadoc --- .../java/datadog/opentelemetry/shim/trace/OtelSpan.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 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 6ed83fd46f6..09bc7601f31 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 @@ -139,9 +139,10 @@ public Span recordException(Throwable exception, Attributes additionalAttributes * processExceptionAttributes generates Attributes about the exception and sets Span error tags * using the processed information. * - *

If additionalAttributes contains a key from defaultAttributes, the value set in - * additionalAttributes will be used in Attributes and in error tag Else, the value is determined - * from the exception itself. + *

All exception span events get the following reserved Attributes: "exception.message", + * "exception.type" and "exception.stacktace". If additionalAttributes contains a reserved key, + * the value in additionalAttributes is used. Else, the value is determined from the provided + * Throwable. * * @param exception The Throwable from which to build default attributes * @param additionalAttributes Attributes provided by the user From 5950ade5da0755aca285ad1fce2a62390dad5e24 Mon Sep 17 00:00:00 2001 From: Mikayla Toffler Date: Thu, 12 Sep 2024 17:03:11 -0400 Subject: [PATCH 34/37] javadoc improvement --- .../datadog/opentelemetry/shim/trace/OtelSpan.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 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 09bc7601f31..27b0d7c4baa 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 @@ -136,15 +136,14 @@ public Span recordException(Throwable exception, Attributes additionalAttributes } /** - * processExceptionAttributes generates Attributes about the exception and sets Span error tags - * using the processed information. + * processExceptionAttributes generates Attributes about the exception and sets Span error tags. * *

All exception span events get the following reserved Attributes: "exception.message", * "exception.type" and "exception.stacktace". If additionalAttributes contains a reserved key, * the value in additionalAttributes is used. Else, the value is determined from the provided * Throwable. * - * @param exception The Throwable from which to build default attributes + * @param exception The Throwable from which to build reserved attributes * @param additionalAttributes Attributes provided by the user * @return Attributes collection that combines defaultExceptionAttributes with * additionalAttributes @@ -154,17 +153,18 @@ private Attributes processExceptionAttributes( // Create an AttributesBuilder with the additionalAttributes provided AttributesBuilder attrsBuilder = additionalAttributes.toBuilder(); - // Check whether additionalAttributes contains any of the reserved exception attribute keys + // Check whether additionalAttributes contains any of the reserved exception attribute keys. If + // not, append the newly created Attribute. String key = "exception.message"; String attrsValue = additionalAttributes.get(AttributeKey.stringKey(key)); if (attrsValue != null) { // additionalAttributes contains "exception.message" this.delegate.setTag( DDTags.ERROR_MSG, - attrsValue); // use the value provided in additionalAttributes to set ERROR_MSG tag + attrsValue); // use the value provided in additionalAttributes to set ERROR_MSG span tag } else { String value = exception.getMessage(); // use exception to set ERROR_MSG tag - this.delegate.setTag(DDTags.ERROR_MSG, value); // and append to the builder + this.delegate.setTag(DDTags.ERROR_MSG, value); // and append to the Attributes builder attrsBuilder.put(key, value); } From 4051d565a04e525c1975eb413e3b10b0f95dea6a Mon Sep 17 00:00:00 2001 From: Bruce Bujon Date: Mon, 16 Sep 2024 11:52:53 +0200 Subject: [PATCH 35/37] feat(otel): Improve convention handling Move convention related span event handling to OtelConventions Move span event related logic to OtelSpanEvent Add OTel attributes cache for performance Add comment about nullable field Rename OtelSpanEvent from toString to toJson to avoid confusion with Java Object semantic Add OtelSpanEvent toString method for debugging Add OtelSpanEvent recordException test cases Improve JSON encoding to use buffer length and char append when possible Rename AttributeJsonParser to follow Java naming conventions Clean up Javadoc --- .../shim/trace/OtelConventions.java | 17 ++- .../opentelemetry/shim/trace/OtelSpan.java | 85 +---------- .../shim/trace/OtelSpanEvent.java | 144 ++++++++++++------ .../OpenTelemetryInstrumentation.java | 2 +- .../test/groovy/OpenTelemetry14Test.groovy | 27 +++- 5 files changed, 142 insertions(+), 133 deletions(-) diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java index 9ded57d7152..3742f913880 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelConventions.java @@ -1,6 +1,13 @@ package datadog.opentelemetry.shim.trace; +import static datadog.opentelemetry.shim.trace.OtelSpanEvent.EXCEPTION_MESSAGE_ATTRIBUTE_KEY; +import static datadog.opentelemetry.shim.trace.OtelSpanEvent.EXCEPTION_STACK_TRACE_ATTRIBUTE_KEY; +import static datadog.opentelemetry.shim.trace.OtelSpanEvent.EXCEPTION_TYPE_ATTRIBUTE_KEY; import static datadog.trace.api.DDTags.ANALYTICS_SAMPLE_RATE; +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; @@ -14,7 +21,6 @@ import static java.lang.Boolean.parseBoolean; import static java.util.Locale.ROOT; -import datadog.trace.api.DDTags; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import datadog.trace.bootstrap.instrumentation.api.SpanAttributes; import datadog.trace.bootstrap.instrumentation.api.Tags; @@ -130,7 +136,14 @@ public static void setEventsAsTag(AgentSpan span, List events) { if (events == null || events.isEmpty()) { return; } - span.setTag(DDTags.SPAN_EVENTS, OtelSpanEvent.toTag(events)); + 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)); + span.setTag(ERROR_TYPE, exceptionAttributes.get(EXCEPTION_TYPE_ATTRIBUTE_KEY)); + span.setTag(ERROR_STACK, exceptionAttributes.get(EXCEPTION_STACK_TRACE_ATTRIBUTE_KEY)); } private static String computeOperationName(AgentSpan span) { 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 27b0d7c4baa..5e112d63b2b 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 @@ -2,26 +2,25 @@ 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.initializeExceptionAttributes; import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activateSpan; import static io.opentelemetry.api.trace.StatusCode.ERROR; import static io.opentelemetry.api.trace.StatusCode.OK; import static io.opentelemetry.api.trace.StatusCode.UNSET; -import datadog.trace.api.DDTags; import datadog.trace.bootstrap.instrumentation.api.AgentScope; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import datadog.trace.bootstrap.instrumentation.api.AttachableWrapper; import io.opentelemetry.api.common.AttributeKey; import io.opentelemetry.api.common.Attributes; -import io.opentelemetry.api.common.AttributesBuilder; import io.opentelemetry.api.trace.Span; import io.opentelemetry.api.trace.SpanContext; import io.opentelemetry.api.trace.StatusCode; import io.opentelemetry.api.trace.TraceFlags; import io.opentelemetry.api.trace.TraceState; -import java.io.PrintWriter; -import java.io.StringWriter; import java.util.ArrayList; import java.util.List; import java.util.concurrent.TimeUnit; @@ -32,6 +31,7 @@ public class OtelSpan implements Span { private final AgentSpan delegate; private StatusCode statusCode; private boolean recording; + /** Span events ({@code null} until an event is added). */ private List events; public OtelSpan(AgentSpan delegate) { @@ -113,90 +113,19 @@ public Span setStatus(StatusCode statusCode, String description) { return this; } - /** - * Records information about the Throwable as a span event. `exception.escaped` cannot be - * determined by this function and therefore is not automatically recorded. Record it manually - * using additionalAttributes See: ... - * - * @param exception the Throwable to record - * @param additionalAttributes the additional attributes to record on this span event - */ @Override public Span recordException(Throwable exception, Attributes additionalAttributes) { if (this.recording) { if (this.events == null) { this.events = new ArrayList<>(); } - this.events.add( - new OtelSpanEvent( - "exception", processExceptionAttributes(exception, additionalAttributes))); + additionalAttributes = initializeExceptionAttributes(exception, additionalAttributes); + applySpanEventExceptionAttributesAsTags(this.delegate, additionalAttributes); + this.events.add(new OtelSpanEvent(EXCEPTION_SPAN_EVENT_NAME, additionalAttributes)); } return this; } - /** - * processExceptionAttributes generates Attributes about the exception and sets Span error tags. - * - *

All exception span events get the following reserved Attributes: "exception.message", - * "exception.type" and "exception.stacktace". If additionalAttributes contains a reserved key, - * the value in additionalAttributes is used. Else, the value is determined from the provided - * Throwable. - * - * @param exception The Throwable from which to build reserved attributes - * @param additionalAttributes Attributes provided by the user - * @return Attributes collection that combines defaultExceptionAttributes with - * additionalAttributes - */ - private Attributes processExceptionAttributes( - Throwable exception, Attributes additionalAttributes) { - // Create an AttributesBuilder with the additionalAttributes provided - AttributesBuilder attrsBuilder = additionalAttributes.toBuilder(); - - // Check whether additionalAttributes contains any of the reserved exception attribute keys. If - // not, append the newly created Attribute. - - String key = "exception.message"; - String attrsValue = additionalAttributes.get(AttributeKey.stringKey(key)); - if (attrsValue != null) { // additionalAttributes contains "exception.message" - this.delegate.setTag( - DDTags.ERROR_MSG, - attrsValue); // use the value provided in additionalAttributes to set ERROR_MSG span tag - } else { - String value = exception.getMessage(); // use exception to set ERROR_MSG tag - this.delegate.setTag(DDTags.ERROR_MSG, value); // and append to the Attributes builder - attrsBuilder.put(key, value); - } - - key = "exception.type"; - attrsValue = additionalAttributes.get(AttributeKey.stringKey(key)); - if (attrsValue != null) { - this.delegate.setTag(DDTags.ERROR_TYPE, attrsValue); - } else { - String value = exception.getClass().getName(); - this.delegate.setTag(DDTags.ERROR_TYPE, value); - attrsBuilder.put(key, value); - } - - key = "exception.stacktrace"; - attrsValue = additionalAttributes.get(AttributeKey.stringKey(key)); - if (attrsValue != null) { - this.delegate.setTag(DDTags.ERROR_STACK, attrsValue); - } else { - String value = stringifyErrorStack(exception); - this.delegate.setTag(DDTags.ERROR_STACK, value); - attrsBuilder.put(key, value); - } - - return attrsBuilder.build(); - } - - static String stringifyErrorStack(Throwable error) { - final StringWriter errorString = new StringWriter(); - error.printStackTrace(new PrintWriter(errorString)); - return errorString.toString(); - } - @Override public Span updateName(String name) { if (this.recording) { diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java index 3c3105745f8..f72970a77f1 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java @@ -4,37 +4,101 @@ import datadog.trace.api.time.TimeSource; import edu.umd.cs.findbugs.annotations.NonNull; 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.Map; import java.util.Set; import java.util.concurrent.TimeUnit; public class OtelSpanEvent { - + public static final String EXCEPTION_SPAN_EVENT_NAME = "exception"; + public static final AttributeKey EXCEPTION_MESSAGE_ATTRIBUTE_KEY = + AttributeKey.stringKey("exception.message"); + public static final AttributeKey EXCEPTION_TYPE_ATTRIBUTE_KEY = + AttributeKey.stringKey("exception.type"); + public static final AttributeKey EXCEPTION_STACK_TRACE_ATTRIBUTE_KEY = + AttributeKey.stringKey("exception.stacktrace"); + + /** Event timestamp in nanoseconds. */ private final long timestamp; private final String name; - private String attributes = ""; + private final String attributes; private static TimeSource timeSource = SystemTimeSource.INSTANCE; - public OtelSpanEvent(String name, io.opentelemetry.api.common.Attributes attributes) { + public OtelSpanEvent(String name, Attributes attributes) { this.name = name; - this.attributes = AttributesJSONParser.toJson(attributes); + this.attributes = AttributesJsonParser.toJson(attributes); this.timestamp = getNanosFromTimeSource(); } - public OtelSpanEvent( - String name, - io.opentelemetry.api.common.Attributes attributes, - long timestamp, - TimeUnit unit) { + public OtelSpanEvent(String name, Attributes attributes, long timestamp, TimeUnit unit) { this.name = name; - this.attributes = AttributesJSONParser.toJson(attributes); + this.attributes = AttributesJsonParser.toJson(attributes); this.timestamp = convertNano(timestamp, unit); } - // JSONParser is a helper class for JSON-encoding OtelSpanEvent attributes - public static class AttributesJSONParser { - public static String toJson(io.opentelemetry.api.common.Attributes attributes) { + @NonNull + public static String toTag(List events) { + StringBuilder builder = new StringBuilder("["); + for (OtelSpanEvent event : events) { + if (builder.length() > 1) { + builder.append(','); + } + builder.append(event.toJson()); + } + return builder.append(']').toString(); + } + + /** + * Make sure exception related attributes are presents and generates them if needed. + * + *

All exception span events get the following reserved attributes: {@link + * #EXCEPTION_MESSAGE_ATTRIBUTE_KEY}, {@link #EXCEPTION_TYPE_ATTRIBUTE_KEY} and {@link + * #EXCEPTION_STACK_TRACE_ATTRIBUTE_KEY}. If additionalAttributes contains a reserved key, the + * value in additionalAttributes is used. Else, the value is determined from the provided + * Throwable. + * + * @param exception The Throwable from which to build reserved attributes + * @param additionalAttributes The user-provided attributes + * @return An {@link Attributes} collection with exception attributes. + */ + static Attributes initializeExceptionAttributes( + Throwable exception, Attributes additionalAttributes) { + // Create an AttributesBuilder with the additionalAttributes provided + AttributesBuilder builder = additionalAttributes.toBuilder(); + // Handle exception message + String value = additionalAttributes.get(EXCEPTION_MESSAGE_ATTRIBUTE_KEY); + if (value == null) { + value = exception.getMessage(); + builder.put(EXCEPTION_MESSAGE_ATTRIBUTE_KEY, value); + } + // Handle exception type + value = additionalAttributes.get(EXCEPTION_TYPE_ATTRIBUTE_KEY); + if (value == null) { + value = exception.getClass().getName(); + builder.put(EXCEPTION_TYPE_ATTRIBUTE_KEY, value); + } + // Handle exception stacktrace + value = additionalAttributes.get(EXCEPTION_STACK_TRACE_ATTRIBUTE_KEY); + if (value == null) { + value = stringifyErrorStack(exception); + builder.put(EXCEPTION_STACK_TRACE_ATTRIBUTE_KEY, value); + } + return builder.build(); + } + + static String stringifyErrorStack(Throwable error) { + final StringWriter errorString = new StringWriter(); + 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 ""; } @@ -42,50 +106,43 @@ public static String toJson(io.opentelemetry.api.common.Attributes attributes) { jsonBuilder.append('{'); Set, Object>> entrySet = attributes.asMap().entrySet(); - int entryCount = 0; for (Map.Entry, Object> entry : entrySet) { - if (entryCount > 0) { - jsonBuilder.append(","); + if (jsonBuilder.length() > 1) { + jsonBuilder.append(','); } - - String key = - entry - .getKey() - .getKey(); // AttributeKey type has method `getKey()` that "stringifies" the key + // 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("\":"); - + jsonBuilder.append('"').append(escapeJson(key)).append("\":"); // Append value to jsonBuilder appendValue(value, jsonBuilder); - entryCount++; } jsonBuilder.append('}'); return jsonBuilder.toString(); } /** - * appendValue recursively adds the value of an Attribute to the active StringBuilder in JSON - * format, depending on the value's type + * 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 StringBuilder + * @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("\""); + jsonBuilder.append('"').append(escapeJson((String) value)).append('"'); } else if (value instanceof List) { - jsonBuilder.append("["); + jsonBuilder.append('['); List valArray = (List) value; for (int i = 0; i < valArray.size(); i++) { if (i > 0) { - jsonBuilder.append(","); + jsonBuilder.append(','); } appendValue(valArray.get(i), jsonBuilder); } - jsonBuilder.append("]"); + jsonBuilder.append(']'); } else if (value instanceof Number || value instanceof Boolean) { jsonBuilder.append(value); } else { @@ -117,7 +174,7 @@ public static void setTimeSource(TimeSource newTimeSource) { timeSource = newTimeSource; } - public String toString() { + public String toJson() { StringBuilder builder = new StringBuilder( "{\"time_unix_nano\":" + this.timestamp + ",\"name\":\"" + this.name + "\""); @@ -127,15 +184,14 @@ public String toString() { return builder.append('}').toString(); } - @NonNull - public static String toTag(List events) { - StringBuilder builder = new StringBuilder("["); - for (int i = 0; i < events.size(); i++) { - if (i > 0) { - builder.append(','); - } - builder.append(events.get(i).toString()); - } - return builder.append(']').toString(); + @Override + public String toString() { + return "OtelSpanEvent{timestamp=" + + this.timestamp + + ", name='" + + this.name + + "', attributes='" + + this.attributes + + "'}"; } } diff --git a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/OpenTelemetryInstrumentation.java b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/OpenTelemetryInstrumentation.java index bc7774c57f5..63883a317f5 100644 --- a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/OpenTelemetryInstrumentation.java +++ b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/main/java/datadog/trace/instrumentation/opentelemetry14/OpenTelemetryInstrumentation.java @@ -73,7 +73,7 @@ public String[] helperClassNames() { "datadog.opentelemetry.shim.trace.OtelSpanBuilder", "datadog.opentelemetry.shim.trace.OtelSpanBuilder$1", "datadog.opentelemetry.shim.trace.OtelSpanContext", - "datadog.opentelemetry.shim.trace.OtelSpanEvent$AttributesJSONParser", + "datadog.opentelemetry.shim.trace.OtelSpanEvent$AttributesJsonParser", "datadog.opentelemetry.shim.trace.OtelSpanLink", "datadog.opentelemetry.shim.trace.OtelTracer", "datadog.opentelemetry.shim.trace.OtelTracerBuilder", diff --git a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy index 2eee1d8db43..822591be4bc 100644 --- a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy +++ b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy @@ -1,5 +1,4 @@ import datadog.opentelemetry.shim.trace.OtelSpanEvent -import datadog.opentelemetry.shim.trace.OtelSpan import datadog.trace.agent.test.AgentTestRunner import datadog.trace.api.DDSpanId import datadog.trace.api.DDTags @@ -19,6 +18,7 @@ import spock.lang.Subject import java.util.concurrent.TimeUnit +import static datadog.opentelemetry.shim.trace.OtelConventions.SPAN_KIND_INTERNAL import static datadog.trace.api.DDTags.ERROR_MSG import static datadog.trace.api.DDTags.ERROR_STACK import static datadog.trace.api.DDTags.ERROR_TYPE @@ -27,7 +27,6 @@ import static datadog.trace.bootstrap.instrumentation.api.Tags.SPAN_KIND_CLIENT import static datadog.trace.bootstrap.instrumentation.api.Tags.SPAN_KIND_CONSUMER import static datadog.trace.bootstrap.instrumentation.api.Tags.SPAN_KIND_PRODUCER import static datadog.trace.bootstrap.instrumentation.api.Tags.SPAN_KIND_SERVER -import static datadog.opentelemetry.shim.trace.OtelConventions.SPAN_KIND_INTERNAL import static io.opentelemetry.api.trace.SpanKind.CLIENT import static io.opentelemetry.api.trace.SpanKind.CONSUMER import static io.opentelemetry.api.trace.SpanKind.INTERNAL @@ -662,6 +661,16 @@ class OpenTelemetry14Test extends AgentTestRunner { def timeSource = new ControllableTimeSource() timeSource.set(1000) OtelSpanEvent.setTimeSource(timeSource) + def errorMessage = overridenMessage?:exception.getMessage() + def errorType = overridenType?:exception.getClass().getName() + def errorStackTrace = overridenStacktrace?:OtelSpanEvent.stringifyErrorStack(exception) + def expectedAttributes = + """{ + "exception.message": "${errorMessage}", + "exception.type": "${errorType}", + "exception.stacktrace": "${errorStackTrace}" + ${extraJson?:''} + }""" when: result.recordException(exception, attributes) @@ -687,19 +696,21 @@ class OpenTelemetry14Test extends AgentTestRunner { defaultTags() "$SPAN_KIND" "$SPAN_KIND_INTERNAL" tag("events", { JSONAssert.assertEquals(expectedEventTag, it as String, false); return true }) - tag(ERROR_MSG, errorMsg) + tag(ERROR_MSG, errorMessage) tag(ERROR_TYPE, errorType) - tag(ERROR_STACK, errorStack) + tag(ERROR_STACK, errorStackTrace) } } } } where: - exception | attributes | errorMsg | errorType | errorStack | expectedAttributes - new NullPointerException("Null pointer") | Attributes.empty() | exception.getMessage() | exception.getClass().getName() | OtelSpan.stringifyErrorStack(exception) | """{ exception.message: "$errorMsg", exception.type: "$errorType", exception.stacktrace: "$errorStack"}""" - new NumberFormatException("Number format exception") | Attributes.builder().put("exception.message", "something-else").build() | "something-else" | exception.getClass().getName() | OtelSpan.stringifyErrorStack(exception) | """{exception.message: "$errorMsg", exception.type: "$errorType", exception.stacktrace: "$errorStack"}""" - new NullPointerException("Null pointer") | Attributes.builder().put("key", "value").build() | exception.getMessage() | exception.getClass().getName() | OtelSpan.stringifyErrorStack(exception) | """{"key": "value", exception.message: "$errorMsg", exception.type: "$errorType", exception.stacktrace: "$errorStack"}""" + exception | attributes | overridenMessage | overridenType | overridenStacktrace | extraJson + new NullPointerException("Null pointer") | Attributes.empty() | null | null | null | null + new NumberFormatException("Number format exception") | Attributes.builder().put("exception.message", "something-else").build() | "something-else" | null | null | null + new NullPointerException("Null pointer") | Attributes.builder().put("exception.type", "CustomType").build() | null | "CustomType" | null | null + new NullPointerException("Null pointer") | Attributes.builder().put("exception.stacktrace", "CustomTrace").build() | null | null | "CustomTrace" | null + new NullPointerException("Null pointer") | Attributes.builder().put("key", "value").build() | null | null | null | ', "key": "value"' } def "test span error meta on record multiple exceptions"() { From e143d2836009a4f9b83b8fb33817f61b55b1e9f6 Mon Sep 17 00:00:00 2001 From: Bruce Bujon Date: Mon, 16 Sep 2024 13:59:02 +0200 Subject: [PATCH 36/37] feat(otel): Simplify time API usage --- .../opentelemetry/shim/trace/OtelSpanEvent.java | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java index f72970a77f1..e3c0680d2cb 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java @@ -26,18 +26,19 @@ public class OtelSpanEvent { private final long timestamp; private final String name; private final String attributes; + // TODO TimeSource instance is not retrieved from CoreTracer private static TimeSource timeSource = SystemTimeSource.INSTANCE; public OtelSpanEvent(String name, Attributes attributes) { this.name = name; this.attributes = AttributesJsonParser.toJson(attributes); - this.timestamp = getNanosFromTimeSource(); + 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 = convertNano(timestamp, unit); + this.timestamp = unit.toNanos(timestamp); } @NonNull @@ -162,14 +163,6 @@ private static String escapeJson(String value) { } } - private static long convertNano(long timestamp, TimeUnit unit) { - return TimeUnit.NANOSECONDS.convert(timestamp, unit); - } - - private long getNanosFromTimeSource() { - return timeSource.getCurrentTimeNanos(); - } - public static void setTimeSource(TimeSource newTimeSource) { timeSource = newTimeSource; } From 3af7937d33d090525dd68d7d85b21fcfd7974d26 Mon Sep 17 00:00:00 2001 From: Bruce Bujon Date: Mon, 16 Sep 2024 14:06:14 +0200 Subject: [PATCH 37/37] feat(otel): Improve tests Add test to check events behavior after span end Simplify expected JSON building Remove custom time source when not needed Used tag name from constants Ensure span close Improve time API usage Fix expected JSON keys --- .../shim/trace/OtelSpanEvent.java | 9 +- .../test/groovy/OpenTelemetry14Test.groovy | 105 +++++++++--------- 2 files changed, 55 insertions(+), 59 deletions(-) diff --git a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java index e3c0680d2cb..0f2f3f82805 100644 --- a/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java +++ b/dd-java-agent/agent-otel/otel-shim/src/main/java/datadog/opentelemetry/shim/trace/OtelSpanEvent.java @@ -22,13 +22,14 @@ public class OtelSpanEvent { public static final AttributeKey EXCEPTION_STACK_TRACE_ATTRIBUTE_KEY = AttributeKey.stringKey("exception.stacktrace"); - /** Event timestamp in nanoseconds. */ - private final long timestamp; - private final String name; - private final String attributes; // 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); diff --git a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy index 822591be4bc..532b2b44fc7 100644 --- a/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy +++ b/dd-java-agent/instrumentation/opentelemetry/opentelemetry-1.4/src/test/groovy/OpenTelemetry14Test.groovy @@ -16,12 +16,11 @@ import opentelemetry14.context.propagation.TextMap import org.skyscreamer.jsonassert.JSONAssert import spock.lang.Subject -import java.util.concurrent.TimeUnit - import static datadog.opentelemetry.shim.trace.OtelConventions.SPAN_KIND_INTERNAL 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 @@ -35,6 +34,8 @@ import static io.opentelemetry.api.trace.SpanKind.SERVER import static io.opentelemetry.api.trace.StatusCode.ERROR import static io.opentelemetry.api.trace.StatusCode.OK import static io.opentelemetry.api.trace.StatusCode.UNSET +import static java.util.concurrent.TimeUnit.MILLISECONDS +import static java.util.concurrent.TimeUnit.NANOSECONDS class OpenTelemetry14Test extends AgentTestRunner { static final TIME_MILLIS = 1723220824705 @@ -182,108 +183,103 @@ class OpenTelemetry14Test extends AgentTestRunner { } } - def "test add single event"() { + def "test add event"() { setup: def builder = tracer.spanBuilder("some-name") + def timeSource = new ControllableTimeSource() + timeSource.set(1000) + OtelSpanEvent.setTimeSource(timeSource) when: def result = builder.startSpan() - result.addEvent(name, attributes, timestamp, unit) + result.addEvent("event") result.end() then: - long expectTime = timestamp - if (unit == TimeUnit.MILLISECONDS) { - expectTime *= 1_000_000L - } - def expectedEventTag = """ - [ - { "time_unix_nano": ${expectTime}, - "name": "${name}" - ${ expectedAttributes == null ? "" : ", attributes: " + expectedAttributes } - } - ]""" + [ + { "time_unix_nano": ${timeSource.getCurrentTimeNanos()}, + "name": "event" + } + ]""" assertTraces(1) { trace(1) { span { tags { defaultTags() "$SPAN_KIND" "$SPAN_KIND_INTERNAL" - tag("events", { JSONAssert.assertEquals(expectedEventTag, it as String, false); return true }) + tag("$SPAN_EVENTS", { JSONAssert.assertEquals(expectedEventTag, it as String, false); return true }) } } } } - - where: - name | timestamp | unit | attributes | expectedAttributes - "evt1" | TIME_MILLIS | TimeUnit.MILLISECONDS | Attributes.empty() | null - "evt2" | TIME_NANO | TimeUnit.NANOSECONDS | Attributes.builder().put("string-key", "string-value").put("long-key", 123456789L).put("double-key", 1234.5678).put("boolean-key-true", true).put("boolean-key-false", false).build() | '{ string-key: "string-value", long-key: 123456789, double-key: 1234.5678, boolean-key-true: true, boolean-key-false: false }' - "evt3" | TIME_NANO | TimeUnit.NANOSECONDS | Attributes.builder().put("string-key-array", "string-value1", "string-value2", "string-value3").put("long-key-array", 123456L, 1234567L, 12345678L).put("double-key-array", 1234.5D, 1234.56D, 1234.567D).put("boolean-key-array", true, false, true).build() | '{ string-key-array: [ "string-value1", "string-value2", "string-value3" ], long-key-array: [ 123456, 1234567, 12345678 ], double-key-array: [ 1234.5, 1234.56, 1234.567], boolean-key-array: [true, false, true] }' } - def "test multiple span events"() { + def "test add single event"() { setup: def builder = tracer.spanBuilder("some-name") + def expectedEventTag = """ + [ + { "time_unix_nano": ${unit.toNanos(timestamp)}, + "name": "${name}" + ${expectedAttributes == null ? "" : ", attributes: " + expectedAttributes} + } + ]""" when: def result = builder.startSpan() - result.addEvent("evt1", null, TIME_NANO, TimeUnit.NANOSECONDS) - result.addEvent("evt2", Attributes.builder().put("string-key", "string-value").build(), TIME_NANO, TimeUnit.NANOSECONDS) + result.addEvent(name, attributes, timestamp, unit) result.end() then: - def expectedAttributes = '{"string-key": "string-value"}' - def expectedEventTag = """ - [ - { "time_unix_nano": ${TIME_NANO}, - "name": "evt1" - }, - { "time_unix_nano": ${TIME_NANO}, - "name": "evt2", - ${"attributes: " + expectedAttributes} - } - ]""" + assertTraces(1) { trace(1) { span { tags { defaultTags() "$SPAN_KIND" "$SPAN_KIND_INTERNAL" - tag("events", { JSONAssert.assertEquals(expectedEventTag, it as String, false); return true }) + tag("$SPAN_EVENTS", { JSONAssert.assertEquals(expectedEventTag, it as String, false); return true }) } } } } + + where: + name | timestamp | unit | attributes | expectedAttributes + "event1" | TIME_MILLIS | MILLISECONDS | Attributes.empty() | null + "event2" | TIME_NANO | NANOSECONDS | Attributes.builder().put("string-key", "string-value").put("long-key", 123456789L).put("double-key", 1234.5678).put("boolean-key-true", true).put("boolean-key-false", false).build() | '{"string-key": "string-value", "long-key": 123456789, "double-key": 1234.5678, "boolean-key-true": true, "boolean-key-false": false }' + "event3" | TIME_NANO | NANOSECONDS | Attributes.builder().put("string-key-array", "string-value1", "string-value2", "string-value3").put("long-key-array", 123456L, 1234567L, 12345678L).put("double-key-array", 1234.5D, 1234.56D, 1234.567D).put("boolean-key-array", true, false, true).build() | '{"string-key-array": [ "string-value1", "string-value2", "string-value3" ], "long-key-array": [ 123456, 1234567, 12345678 ], "double-key-array": [ 1234.5, 1234.56, 1234.567], "boolean-key-array": [true, false, true] }' } - def "test add event no timestamp"() { + def "test add multiple span events"() { setup: def builder = tracer.spanBuilder("some-name") - def timeSource = new ControllableTimeSource() - timeSource.set(1000) - OtelSpanEvent.setTimeSource(timeSource) when: def result = builder.startSpan() - result.addEvent("evt", null, ) + result.addEvent("event1", null, TIME_NANO, NANOSECONDS) + result.addEvent("event2", Attributes.builder().put("string-key", "string-value").build(), TIME_NANO, NANOSECONDS) result.end() then: def expectedEventTag = """ - [ - { "time_unix_nano": ${timeSource.getCurrentTimeNanos()}, - "name": "evt" - } - ]""" + [ + { "time_unix_nano": ${TIME_NANO}, + "name": "event1" + }, + { "time_unix_nano": ${TIME_NANO}, + "name": "event2", + "attributes": {"string-key": "string-value"} + } + ]""" assertTraces(1) { trace(1) { span { tags { defaultTags() "$SPAN_KIND" "$SPAN_KIND_INTERNAL" - tag("events", { JSONAssert.assertEquals(expectedEventTag, it as String, false); return true }) + tag("$SPAN_EVENTS", { JSONAssert.assertEquals(expectedEventTag, it as String, false); return true }) } } } @@ -655,7 +651,7 @@ class OpenTelemetry14Test extends AgentTestRunner { } } - def "test span record exception event tags"() { + def "test span record exception"() { setup: def result = tracer.spanBuilder("some-name").startSpan() def timeSource = new ControllableTimeSource() @@ -717,22 +713,19 @@ class OpenTelemetry14Test extends AgentTestRunner { // Span's "error" tags should reflect the last recorded exception setup: def result = tracer.spanBuilder("some-name").startSpan() - def timeSource = new ControllableTimeSource() - timeSource.set(1000) - OtelSpanEvent.setTimeSource(timeSource) def exception1 = new NullPointerException("Null pointer") def exception2 = new NumberFormatException("Number format exception") + def expectedStackTrace = OtelSpanEvent.stringifyErrorStack(exception2) when: result.recordException(exception1) result.recordException(exception2) + result.end() then: result.delegate.getTag(ERROR_MSG) == exception2.getMessage() result.delegate.getTag(ERROR_TYPE) == exception2.getClass().getName() - final StringWriter errorString = new StringWriter() - exception2.printStackTrace(new PrintWriter(errorString)) - result.delegate.getTag(ERROR_STACK) == errorString.toString() + result.delegate.getTag(ERROR_STACK) == expectedStackTrace !result.delegate.isError() } @@ -779,6 +772,8 @@ class OpenTelemetry14Test extends AgentTestRunner { result.updateName("other-name") result.setAttribute("string", "other-value") result.setStatus(OK) + result.addEvent("event") + result.recordException(new Throwable()) then: assertTraces(1) {