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.

Sign up for GitHub

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

Jump to bottom

Add Span#addLink, for adding link after span start #6084

Merged
merged 5 commits into from
Feb 7, 2024
Merged

Add Span#addLink, for adding link after span start #6084

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dc23b9d
Add Span#addLink, for adding link after span start
jack-berg Dec 18, 2023
3e92a5b
Minimize synchronized scope
jack-berg Dec 20, 2023
820ecc5
Apply suggestions from code review
jack-berg Jan 31, 2024
6c3ee82
Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…
jack-berg Jan 31, 2024
4588c3c
Move AddLink to incubating module
jack-berg Jan 31, 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
56 changes: 56 additions & 0 deletions ...ions/incubator/src/main/java/io/opentelemetry/extension/incubator/trace/ExtendedSpan.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.extension.incubator.trace;

import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.api.trace.Span;
import io.opentelemetry.api.trace.SpanBuilder;
import io.opentelemetry.api.trace.SpanContext;

/** Extended {@link Span} with experimental APIs. */
public interface ExtendedSpan extends Span {

/**
* Adds a link to this {@code Span}.
*
* <p>Links are used to link {@link Span}s in different traces. Used (for example) in batching
* operations, where a single batch handler processes multiple requests from different traces or
* the same trace.
*
* <p>Implementations may ignore calls with an {@linkplain SpanContext#isValid() invalid span
* context}.
*
* <p>Callers should prefer to add links before starting the span via {@link
* SpanBuilder#addLink(SpanContext)} if possible.
*
* @param spanContext the context of the linked {@code Span}.
* @return this.
*/
default Span addLink(SpanContext spanContext) {
return addLink(spanContext, Attributes.empty());
}
breedx-splk marked this conversation as resolved.
Show resolved Hide resolved

/**
* Adds a link to this {@code Span}.
*
* <p>Links are used to link {@link Span}s in different traces. Used (for example) in batching
* operations, where a single batch handler processes multiple requests from different traces or
* the same trace.
*
* <p>Implementations may ignore calls with an {@linkplain SpanContext#isValid() invalid span
* context}.
*
* <p>Callers should prefer to add links before starting the span via {@link
* SpanBuilder#addLink(SpanContext, Attributes)} if possible.
*
* @param spanContext the context of the linked {@code Span}.
* @param attributes the attributes of the {@code Link}.
* @return this.
*/
default Span addLink(SpanContext spanContext, Attributes attributes) {
return this;

Check warning on line 54 in extensions/incubator/src/main/java/io/opentelemetry/extension/incubator/trace/ExtendedSpan.java

View check run for this annotation

Codecov / codecov/patch

extensions/incubator/src/main/java/io/opentelemetry/extension/incubator/trace/ExtendedSpan.java#L54 

Added line #L54 was not covered by tests
breedx-splk marked this conversation as resolved.
Show resolved Hide resolved
}
}
65 changes: 20 additions & 45 deletions opencensus-shim/src/test/java/io/opentelemetry/opencensusshim/DelegatingSpanTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,18 @@
import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.Mockito.times;

import io.opentelemetry.api.common.AttributeKey;
import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.api.trace.Span;
import io.opentelemetry.api.trace.StatusCode;
import io.opentelemetry.context.Context;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.lang.reflect.Modifier;
import java.time.Instant;
import java.util.Arrays;
import java.util.List;
import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -80,64 +86,33 @@ static List<Method> allInterfaceMethods(Class<?> clazz) {
static Stream<Arguments> delegateMethodsProvider() {
return Stream.of(
Arguments.of("end", new Class<?>[] {}, times(1)),
Arguments.of(
"end", new Class<?>[] {long.class, java.util.concurrent.TimeUnit.class}, times(1)),
Arguments.of("end", new Class<?>[] {java.time.Instant.class}, times(1)),
Arguments.of("end", new Class<?>[] {long.class, TimeUnit.class}, times(1)),
Arguments.of("end", new Class<?>[] {Instant.class}, times(1)),
Arguments.of("setAttribute", new Class<?>[] {String.class, String.class}, times(1)),
Arguments.of(
"setAttribute",
new Class<?>[] {io.opentelemetry.api.common.AttributeKey.class, int.class},
times(1)),
Arguments.of(
"setAttribute",
new Class<?>[] {io.opentelemetry.api.common.AttributeKey.class, Object.class},
times(1)),
Arguments.of("setAttribute", new Class<?>[] {AttributeKey.class, int.class}, times(1)),
Arguments.of("setAttribute", new Class<?>[] {AttributeKey.class, Object.class}, times(1)),
Arguments.of("setAttribute", new Class<?>[] {String.class, long.class}, times(1)),
Arguments.of("setAttribute", new Class<?>[] {String.class, double.class}, times(1)),
Arguments.of("setAttribute", new Class<?>[] {String.class, boolean.class}, times(1)),
Arguments.of(
"recordException",
new Class<?>[] {Throwable.class, io.opentelemetry.api.common.Attributes.class},
times(1)),
"recordException", new Class<?>[] {Throwable.class, Attributes.class}, times(1)),
Arguments.of("recordException", new Class<?>[] {Throwable.class}, times(1)),
Arguments.of(
"setAllAttributes",
new Class<?>[] {io.opentelemetry.api.common.Attributes.class},
times(1)),
Arguments.of("setAllAttributes", new Class<?>[] {Attributes.class}, times(1)),
Arguments.of("updateName", new Class<?>[] {String.class}, times(1)),
Arguments.of("storeInContext", new Class<?>[] {Context.class}, times(1)),
Arguments.of("addEvent", new Class<?>[] {String.class, Instant.class}, times(1)),
Arguments.of(
"storeInContext", new Class<?>[] {io.opentelemetry.context.Context.class}, times(1)),
Arguments.of("addEvent", new Class<?>[] {String.class, java.time.Instant.class}, times(1)),
"addEvent", new Class<?>[] {String.class, long.class, TimeUnit.class}, times(1)),
Arguments.of(
"addEvent",
new Class<?>[] {String.class, long.class, java.util.concurrent.TimeUnit.class},
times(1)),
Arguments.of(
"addEvent",
new Class<?>[] {
String.class, io.opentelemetry.api.common.Attributes.class, java.time.Instant.class
},
times(1)),
"addEvent", new Class<?>[] {String.class, Attributes.class, Instant.class}, times(1)),
Arguments.of("addEvent", new Class<?>[] {String.class}, times(1)),
Arguments.of(
"addEvent",
new Class<?>[] {
String.class,
io.opentelemetry.api.common.Attributes.class,
long.class,
java.util.concurrent.TimeUnit.class
},
new Class<?>[] {String.class, Attributes.class, long.class, TimeUnit.class},
times(1)),
Arguments.of(
"addEvent",
new Class<?>[] {String.class, io.opentelemetry.api.common.Attributes.class},
times(1)),
Arguments.of(
"setStatus",
new Class<?>[] {io.opentelemetry.api.trace.StatusCode.class, String.class},
times(1)),
Arguments.of(
"setStatus", new Class<?>[] {io.opentelemetry.api.trace.StatusCode.class}, times(1)),
Arguments.of("addEvent", new Class<?>[] {String.class, Attributes.class}, times(1)),
Arguments.of("setStatus", new Class<?>[] {StatusCode.class, String.class}, times(1)),
Arguments.of("setStatus", new Class<?>[] {StatusCode.class}, times(1)),
//
// special cases
//
Expand Down
2 changes: 2 additions & 0 deletions sdk/trace/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ dependencies {
api(project(":api:all"))
api(project(":sdk:common"))

implementation(project(":extensions:incubator"))

compileOnly(project(":sdk:trace-shaded-deps"))

annotationProcessor("com.google.auto.value:auto-value")
Expand Down
70 changes: 57 additions & 13 deletions sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import io.opentelemetry.api.trace.SpanKind;
import io.opentelemetry.api.trace.StatusCode;
import io.opentelemetry.context.Context;
import io.opentelemetry.extension.incubator.trace.ExtendedSpan;
import io.opentelemetry.sdk.common.Clock;
import io.opentelemetry.sdk.common.InstrumentationScopeInfo;
import io.opentelemetry.sdk.internal.AttributeUtil;
Expand All @@ -35,7 +36,7 @@

/** Implementation for the {@link Span} class that records trace events. */
@ThreadSafe
final class SdkSpan implements ReadWriteSpan {
final class SdkSpan implements ReadWriteSpan, ExtendedSpan {
Copy link
Contributor

Choose a reason for hiding this comment

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

Something about this gives me pause -- because it is implementing two interfaces both of which extend a common parent interface. Seems weird to me. Maybe it's nothing, but it seems like a smell to me.

Immediately makes me question whether or not ExtendedSpan really needs to extend Span.

Copy link
Member Author

Choose a reason for hiding this comment

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

Initially I switched ReadWriteSpan to extend ExtendedSpan instead of Span, but that doesn't work because ReadWriteSpan is part of the public API, and we can't have experimental surface area exposed in the stable API.

We have a long history of doing non-ideal things to be able to incubate things without committing to the stable API. While this is a smell, it will be short lived as it goes away as soon as the spec stabilizes and these methods are promoted to span.

Immediately makes me question whether or not ExtendedSpan really needs to extend Span.

Any other ideas?

Copy link
Member

Choose a reason for hiding this comment

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

We have a long history of doing non-ideal things to be able to incubate things without committing to the stable API.

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Immediately makes me question whether or not ExtendedSpan really needs to extend Span.

Any other ideas?

What if it just didn't? It can be its own stand-alone interface, right? And it's only used by the single implementation.

Copy link
Member Author

@jack-berg jack-berg Feb 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Well the idea is that a user casts spans to ExtendedSpan to access the the combination of stable and experimental methods:

ExtendedSpan span = (ExtendedSpan) tracer.spanBuilder("name").startSpan();
span.setAttribute("key", "value");
span.addLink(spanContext);

If ExtenedSpan doesn't extend Span, then its more awkward to use:

Span span = tracer.spanBuilder("name").startSpan();
span.setAttribute("key", "value");
// Cast span to ExtendedSpan just to access #addLink
((ExtendedSpan) span).addLink(spanContext);

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah ok, I get it. Thanks for clarifying.


private static final Logger logger = Logger.getLogger(SdkSpan.class.getName());

Expand All @@ -47,11 +48,6 @@
private final SpanContext parentSpanContext;
// Handler called when the span starts and ends.
private final SpanProcessor spanProcessor;
// The displayed name of the span.
// List of recorded links to parent and child spans.
private final List<LinkData> links;
// Number of links recorded.
private final int totalRecordedLinks;
// The kind of the span.
private final SpanKind kind;
// The clock used to get the time.
Expand Down Expand Up @@ -81,6 +77,16 @@
@GuardedBy("lock")
private int totalRecordedEvents = 0;

// The displayed name of the span.
// List of recorded links to parent and child spans.
@GuardedBy("lock")
@Nullable
List<LinkData> links;

// Number of links recorded.
@GuardedBy("lock")
private int totalRecordedLinks;

// The status of the span.
@GuardedBy("lock")
private StatusData status = StatusData.unset();
Expand All @@ -104,7 +110,7 @@
AnchoredClock clock,
Resource resource,
@Nullable AttributesMap attributes,
List<LinkData> links,
@Nullable List<LinkData> links,
int totalRecordedLinks,
long startEpochNanos) {
this.context = context;
Expand Down Expand Up @@ -151,7 +157,7 @@
Clock tracerClock,
Resource resource,
@Nullable AttributesMap attributes,
List<LinkData> links,
@Nullable List<LinkData> links,
int totalRecordedLinks,
long userStartEpochNanos) {
boolean createdAnchoredClock;
Expand Down Expand Up @@ -206,11 +212,12 @@
synchronized (lock) {
return SpanWrapper.create(
this,
links,
getImmutableLinks(),
getImmutableTimedEvents(),
getImmutableAttributes(),
(attributes == null) ? 0 : attributes.getTotalAddedValues(),
totalRecordedEvents,
totalRecordedLinks,
status,
name,
endEpochNanos,
Expand Down Expand Up @@ -428,6 +435,37 @@
return this;
}

@Override
public Span addLink(SpanContext spanContext, Attributes attributes) {
if (spanContext == null || !spanContext.isValid()) {
return this;
}
if (attributes == null) {
attributes = Attributes.empty();

Check warning on line 444 in sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java

View check run for this annotation

Codecov / codecov/patch

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java#L444 

Added line #L444 was not covered by tests
}
LinkData link =
LinkData.create(
spanContext,
AttributeUtil.applyAttributesLimit(
attributes,
spanLimits.getMaxNumberOfAttributesPerLink(),
spanLimits.getMaxAttributeValueLength()));
synchronized (lock) {
if (hasEnded) {
logger.log(Level.FINE, "Calling addLink() on an ended Span.");
return this;

Check warning on line 456 in sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java

View check run for this annotation

Codecov / codecov/patch

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java#L455-L456 

Added lines #L455 - L456 were not covered by tests
}
if (links == null) {
links = new ArrayList<>(spanLimits.getMaxNumberOfLinks());

Check warning on line 459 in sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java

View check run for this annotation

Codecov / codecov/patch

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java#L459 

Added line #L459 was not covered by tests
}
if (links.size() < spanLimits.getMaxNumberOfLinks()) {
links.add(link);
}
totalRecordedLinks++;
}
return this;
}

@Override
public void end() {
endInternal(clock.now());
Expand Down Expand Up @@ -475,10 +513,6 @@
return startEpochNanos;
}

int getTotalRecordedLinks() {
return totalRecordedLinks;
}

@GuardedBy("lock")
private List<EventData> getImmutableTimedEvents() {
if (events.isEmpty()) {
Expand Down Expand Up @@ -508,19 +542,29 @@
return attributes.immutableCopy();
}

@GuardedBy("lock")
private List<LinkData> getImmutableLinks() {
if (links == null || links.isEmpty()) {
return Collections.emptyList();
}
return Collections.unmodifiableList(links);
}

@Override
public String toString() {
String name;
String attributes;
String status;
long totalRecordedEvents;
long endEpochNanos;
long totalRecordedLinks;
synchronized (lock) {
name = this.name;
attributes = String.valueOf(this.attributes);
status = String.valueOf(this.status);
totalRecordedEvents = this.totalRecordedEvents;
endEpochNanos = this.endEpochNanos;
totalRecordedLinks = this.totalRecordedLinks;
}
return "SdkSpan{traceId="
+ context.getTraceId()
Expand Down
5 changes: 3 additions & 2 deletions sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpanBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,8 +179,9 @@ public Span startSpan() {
// New child span.
traceId = parentSpanContext.getTraceId();
}
List<LinkData> currentLinks = links;
List<LinkData> immutableLinks =
links == null ? Collections.emptyList() : Collections.unmodifiableList(links);
currentLinks == null ? Collections.emptyList() : Collections.unmodifiableList(currentLinks);
// Avoid any possibility to modify the links list by adding links to the Builder after the
// startSpan is called. If that happens all the links will be added in a new list.
links = null;
Expand Down Expand Up @@ -228,7 +229,7 @@ public Span startSpan() {
tracerSharedState.getClock(),
tracerSharedState.getResource(),
recordedAttributes,
immutableLinks,
currentLinks,
totalNumberOfLinksAdded,
startEpochNanos);
}
Expand Down
6 changes: 5 additions & 1 deletion sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SpanWrapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ abstract class SpanWrapper implements SpanData {

abstract int totalRecordedEvents();

abstract int totalRecordedLinks();

abstract StatusData status();

abstract String name();
Expand All @@ -62,6 +64,7 @@ static SpanWrapper create(
Attributes attributes,
int totalAttributeCount,
int totalRecordedEvents,
int totalRecordedLinks,
StatusData status,
String name,
long endEpochNanos,
Expand All @@ -73,6 +76,7 @@ static SpanWrapper create(
attributes,
totalAttributeCount,
totalRecordedEvents,
totalRecordedLinks,
status,
name,
endEpochNanos,
Expand Down Expand Up @@ -158,7 +162,7 @@ public int getTotalRecordedEvents() {

@Override
public int getTotalRecordedLinks() {
return delegate().getTotalRecordedLinks();
return totalRecordedLinks();
}

@Override
Expand Down
Loading
Loading