Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support addEvent Otel API #7408

Merged
merged 40 commits into from
Sep 16, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
3de8679
Initial span events implementation + tests
mtoffl01 Aug 9, 2024
d654056
use DDTags.SPAN_EVENTS constant for setting tag
mtoffl01 Aug 9, 2024
f2bb9ac
added more tests and used moshi for json encoding
mtoffl01 Aug 12, 2024
f8bc1db
remove unused method toString on SpanEvent instance
mtoffl01 Aug 12, 2024
fae43bc
Started work on recordException
mtoffl01 Aug 13, 2024
761ad85
Removed moshi dependency
mtoffl01 Aug 13, 2024
add0ef0
Merge branch 'master' into mtoff/span-events
mtoffl01 Aug 14, 2024
ea4d8df
Fixed case where custom attributes should override default excpeption…
mtoffl01 Aug 15, 2024
00e47a5
Finishing up record exception unit tests
mtoffl01 Aug 15, 2024
80b8adf
Added annotations for clarity
mtoffl01 Aug 15, 2024
4e84b96
nits: reverted little changes that were superfluous
mtoffl01 Aug 15, 2024
35dd084
nits: reverted little changes that were superfluous
mtoffl01 Aug 15, 2024
a7626bb
Include new helperClassNames
mtoffl01 Aug 16, 2024
c611567
Added support for not-stringifying span events attributes, while main…
mtoffl01 Aug 16, 2024
8e353f7
fixing attributes manipulation for arrays
mtoffl01 Aug 16, 2024
90bc619
Fixed json parsing for array Attributes values
mtoffl01 Aug 26, 2024
14db2ad
Added better javadocs to functions
mtoffl01 Aug 26, 2024
565e43d
Fix exception.escaped and timesource for SpanEvent
mtoffl01 Sep 3, 2024
7655446
Fix exception.escaped and timesource for SpanEvent
mtoffl01 Sep 3, 2024
3123288
Update internal-api/src/main/java/datadog/trace/bootstrap/instrumenta…
mtoffl01 Sep 3, 2024
b4ae914
Update internal-api/src/main/java/datadog/trace/bootstrap/instrumenta…
mtoffl01 Sep 3, 2024
a2026b2
Update javadoc
mtoffl01 Sep 3, 2024
6690d3f
merge w upstream
mtoffl01 Sep 3, 2024
ccca4f3
fix nits
mtoffl01 Sep 6, 2024
0f04383
change SpanEvent method to private-package access scope
mtoffl01 Sep 6, 2024
9efd98e
Changed span event Attributes to a separate class
mtoffl01 Sep 10, 2024
87a7698
Fix error stack formatting for exception.stacktrace tag
mtoffl01 Sep 11, 2024
e6d69db
Remove superfluous import
mtoffl01 Sep 11, 2024
4d9b456
Optimized defaultExceptionAttributes
mtoffl01 Sep 11, 2024
f8721e4
Reverted all previous changes to spanlinks
mtoffl01 Sep 11, 2024
c34e139
Remove missed changes to spanlinks files
mtoffl01 Sep 11, 2024
41692b6
Merge branch 'master' into mtoff/span-events
mtoffl01 Sep 11, 2024
6d6cbc3
Moved exception attributes processing into spanevent class
mtoffl01 Sep 12, 2024
71d4b69
Fix error meta behavior with record exception
mtoffl01 Sep 12, 2024
684170a
Update comments and fix little nits
mtoffl01 Sep 12, 2024
af71579
Improve javadoc
mtoffl01 Sep 12, 2024
5950ade
javadoc improvement
mtoffl01 Sep 12, 2024
4051d56
feat(otel): Improve convention handling
PerfectSlayer Sep 16, 2024
e143d28
feat(otel): Simplify time API usage
PerfectSlayer Sep 16, 2024
3af7937
feat(otel): Improve tests
PerfectSlayer Sep 16, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions dd-java-agent/agent-otel/otel-shim/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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'
mtoffl01 marked this conversation as resolved.
Show resolved Hide resolved

implementation project(':internal-api')
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,12 @@
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;
import io.opentelemetry.api.trace.SpanKind;
import java.util.List;
import javax.annotation.Nullable;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -122,6 +124,13 @@ public static void applyNamingConvention(AgentSpan span) {
}
}

public static void setEventsAsTag(AgentSpan span, List<SpanEvent> events) {
if (events == null || events.isEmpty()) {
return;
}
span.setTag(DDTags.SPAN_EVENTS, SpanEvent.toTag(events));
}

private static String computeOperationName(AgentSpan span) {
Object spanKingTag = span.getTag(SPAN_KIND);
SpanKind spanKind =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -27,6 +29,7 @@ public class OtelSpan implements Span {
private final AgentSpan delegate;
private StatusCode statusCode;
private boolean recording;
private List<SpanEvent> events;

public OtelSpan(AgentSpan delegate) {
this.delegate = delegate;
Expand Down Expand Up @@ -71,13 +74,23 @@ public <T> Span setAttribute(AttributeKey<T> key, T value) {

@Override
public Span addEvent(String name, Attributes attributes) {
// Not supported
if (this.recording) {
if (this.events == null || this.events.isEmpty()) {
mtoffl01 marked this conversation as resolved.
Show resolved Hide resolved
this.events = new ArrayList<>();
}
this.events.add(new SpanEvent(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()) {
mtoffl01 marked this conversation as resolved.
Show resolved Hide resolved
this.events = new ArrayList<>();
}
this.events.add(new SpanEvent(name, attributes, timestamp, unit));
}
return this;
}

Expand Down Expand Up @@ -118,13 +131,15 @@ public Span updateName(String name) {
public void end() {
this.recording = false;
applyNamingConvention(this.delegate);
setEventsAsTag(this.delegate, this.events);
this.delegate.finish();
}

@Override
public void end(long timestamp, TimeUnit unit) {
this.recording = false;
applyNamingConvention(this.delegate);
setEventsAsTag(this.delegate, this.events);
this.delegate.finish(unit.toMicros(timestamp));
}

Expand Down
Original file line number Diff line number Diff line change
@@ -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;
mtoffl01 marked this conversation as resolved.
Show resolved Hide resolved

public class SpanEvent {
mtoffl01 marked this conversation as resolved.
Show resolved Hide resolved

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() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why toString() is using a JSON encoding instead of a default Java object representation?

Copy link
Contributor Author

@mtoffl01 mtoffl01 Sep 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To ensure that the final String product contains nested quotes in the expected places. Like the keys need to have quotes around them, and only String values need to have quotes around them. E.g,
"{\"name\":\"Mikayla\",\"age\":27}".
Also, there is all the specific Attributes manipulation to do, so just using the native toString() method wasn't sufficient (in my experience testing it?).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To ensure that the final String product contains nested quotes in the expected places

It sounds like you are trying to enforce business logic related to the events serialisation into the human readable String representation of the object instance, used for debugging. There is no "expected" quotes for debug logging.

About your example, I would expect to have "{Makayla, 27yo}". That's way more readable for debugging and not tied to an existing format (so people won't try to rely and on it and parse it).

If you need a method to return the JSON-encoded instance, simply create a toJson() method and leave the toString() for debugging instead.

// TODO if attributes exist, process those
return "{\"name\":\"" + this.name + "\",\"time_unix_nano\":" + this.timestamp + "}";
}

@NotNull
public static String toTag(List<SpanEvent> events) {
mtoffl01 marked this conversation as resolved.
Show resolved Hide resolved
StringBuilder builder = new StringBuilder("[");
int index = 0;
while (index < events.size()) {
String linkAsJson = getEncoder().toJson(events.get(index));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned above we can avoid the moshi dependency if we wrote out own method(s) to write span event data as JSON. The structure is simple, so this shouldn't involve much code. Also we only need to handle writing JSON, not parsing it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question I have is what the RFC states about encoding when reaching tag max length?
Should the tracer take care of it? Should we limit event count or drop attributes?
Because if the tag value is cropped, I don’t know what will happen on the backend side with invalid JSON object 🤷

Copy link
Contributor Author

@mtoffl01 mtoffl01 Aug 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The RFC does not specify any limits, however:

  1. Otel Span Events API does not specify any limits on span event count
  2. However, Otel does enforce a [default limit of 128 span events per span and a maximum of 128 attributes per span event](Span Events, they include a default of 128 maximum span events per span and a maximum of 128 attributes per span event) within the SDK
  3. Our implementation of Span Links enforces a character limit with TAG_MAX_LENGTH

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();
mtoffl01 marked this conversation as resolved.
Show resolved Hide resolved
}

private static JsonAdapter<SpanEvent> getEncoder() {
return EncoderHolder.ENCODER;
}

private static class EncoderHolder {
static final JsonAdapter<SpanEvent> ENCODER = createEncoder();

private static JsonAdapter<SpanEvent> 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<String, String> attributes;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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")

Expand Down Expand Up @@ -175,29 +180,100 @@ class OpenTelemetry14Test extends AgentTestRunner {
}
}

def "test non-supported features do not crash"() {
def "test add single event"() {
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(name, timestamp, unit)
result.end()

then:
assertTraces(2) {
long expectTime = timestamp
if (unit == TimeUnit.MILLISECONDS) {
expectTime *= 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:
name | attributes | timestamp | unit
"evt2" | null | TIME_MILLIS | TimeUnit.MILLISECONDS
"evt3" | null | TIME_NANO | TimeUnit.NANOSECONDS
}

def "test multiple span events"() {
setup:
def builder = tracer.spanBuilder("some-name")

when:
def result = builder.startSpan()
result.addEvent(name, timestamp, TimeUnit.NANOSECONDS)
result.addEvent(name, timestamp, TimeUnit.NANOSECONDS)
result.end()

then:
def expectedEventTag = """
[
{ "name": "${name}",
"time_unix_nano": ${timestamp}
},
{ "name": "${name}",
"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:
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
Expand Down
1 change: 1 addition & 0 deletions dd-trace-api/src/main/java/datadog/trace/api/DDTags.java
Original file line number Diff line number Diff line change
Expand Up @@ -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";
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";
Expand Down
Loading