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

Remove DefaultSpan from public API. #1791

Merged
merged 5 commits into from
Oct 15, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

import io.opentelemetry.context.Context;
import io.opentelemetry.context.propagation.TextMapPropagator.Setter;
import io.opentelemetry.trace.DefaultSpan;
import io.opentelemetry.trace.Span;
import io.opentelemetry.trace.SpanContext;
import io.opentelemetry.trace.TraceFlags;
import io.opentelemetry.trace.TraceState;
Expand Down Expand Up @@ -75,7 +75,7 @@ private static SpanContext createTestSpanContext(String traceId, String spanId)
private static List<Context> createContexts(List<SpanContext> spanContexts) {
List<Context> contexts = new ArrayList<>();
for (SpanContext context : spanContexts) {
contexts.add(TracingContextUtils.withSpan(DefaultSpan.create(context), Context.root()));
contexts.add(TracingContextUtils.withSpan(Span.getPropagated(context), Context.root()));
}
return contexts;
}
Expand Down
4 changes: 1 addition & 3 deletions api/src/main/java/io/opentelemetry/trace/DefaultTracer.java
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,7 @@ public Span startSpan() {
spanContext = TracingContextUtils.getCurrentSpan().getContext();
}

return spanContext != null && !SpanContext.getInvalid().equals(spanContext)
? new DefaultSpan(spanContext)
: DefaultSpan.getInvalid();
return Span.getPropagated(spanContext);
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a nice cleanup. much tighter and clearer.

}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,39 +13,16 @@
* The {@code DefaultSpan} is the default {@link Span} that is used when no {@code Span}
* implementation is available. All operations are no-op except context propagation.
*
* <p>Used also to stop tracing, see {@link Tracer#withSpan}.
*
* @since 0.1.0
*/
@Immutable
public final class DefaultSpan implements Span {

private static final DefaultSpan INVALID = new DefaultSpan(SpanContext.getInvalid());

/**
* Returns a {@link DefaultSpan} with an invalid {@link SpanContext}.
*
* @return a {@code DefaultSpan} with an invalid {@code SpanContext}.
* @since 0.1.0
*/
public static Span getInvalid() {
return INVALID;
}
final class PropagatedSpan implements Span {

/**
* Creates an instance of this class with the {@link SpanContext}.
*
* @param spanContext the {@code SpanContext}.
* @return a {@link DefaultSpan}.
* @since 0.1.0
*/
public static Span create(SpanContext spanContext) {
return new DefaultSpan(spanContext);
}
static final PropagatedSpan INVALID = new PropagatedSpan(SpanContext.getInvalid());

private final SpanContext spanContext;

DefaultSpan(SpanContext spanContext) {
PropagatedSpan(SpanContext spanContext) {
this.spanContext = spanContext;
}

Expand Down
29 changes: 29 additions & 0 deletions api/src/main/java/io/opentelemetry/trace/Span.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,26 @@
@ThreadSafe
public interface Span {

/**
* Returns an invalid {@link Span}. An invalid {@link Span} is used when tracing is disabled,
* usually because there is no OpenTelemetry SDK installed.
*/
static Span getInvalid() {
return PropagatedSpan.INVALID;
}

/**
* Returns a non-recording {@link Span} that holds the provided {@link SpanContext} but has no
* functionality. It will not be exported and all tracing operations are no-op, but it can be used
* to propagate a valid {@link SpanContext} downstream.
*/
static Span getPropagated(SpanContext spanContext) {
Copy link
Member

Choose a reason for hiding this comment

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

s/getPropagated/toPropagated?

Copy link
Member

Choose a reason for hiding this comment

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

I think this should be called Span.wrap(SpanContext). Then we are also safe against renamings of propagated/noop/etc

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we are safe against behavior change, this needs to always propagate the SC. But +1 for the name

if (spanContext == null || !spanContext.isValid()) {
return getInvalid();
}
return new PropagatedSpan(spanContext);
}

/**
* Type of span. Can be used to specify additional relationships between spans in addition to a
* parent/child relationship.
Expand Down Expand Up @@ -304,6 +324,15 @@ default void setAttribute(AttributeKey<Long> key, int value) {
*/
boolean isRecording();

/**
* Returns whether this {@link Span} is valid.
*
* @see Span#getInvalid()
*/
default boolean isValid() {
return getContext().isValid();
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this change is unrelated with the PR description and purpose, consider to not add "secondary" changes to a PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure makes sense


/**
* {@link Builder} is used to construct {@link Span} instances which define arbitrary scopes of
* code that are sampled for distributed tracing as a single atomic unit.
Expand Down
3 changes: 2 additions & 1 deletion api/src/main/java/io/opentelemetry/trace/Tracer.java
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,8 @@ public interface Tracer {
*
* <p>Supports try-with-resource idiom.
*
* <p>Can be called with {@link DefaultSpan} to enter a scope of code where tracing is stopped.
* <p>Can be called with {@code Span.getPropagated(span.getContext())} to enter a scope of code
* where tracing is stopped.
*
* <p>Example of usage:
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public static Span getCurrentSpan() {
*/
public static Span getSpan(Context context) {
Span span = context.getValue(CONTEXT_SPAN_KEY);
return span == null ? DefaultSpan.getInvalid() : span;
return span == null ? Span.getInvalid() : span;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
import io.opentelemetry.context.Context;
import io.opentelemetry.context.propagation.TextMapPropagator;
import io.opentelemetry.internal.TemporaryBuffers;
import io.opentelemetry.trace.DefaultSpan;
import io.opentelemetry.trace.Span;
import io.opentelemetry.trace.SpanContext;
import io.opentelemetry.trace.SpanId;
import io.opentelemetry.trace.TraceFlags;
Expand Down Expand Up @@ -149,7 +149,7 @@ public <C> void inject(Context context, C carrier, Setter<C> setter) {
return context;
}

return TracingContextUtils.withSpan(DefaultSpan.create(spanContext), context);
return TracingContextUtils.withSpan(Span.getPropagated(spanContext), context);
}

private static <C> SpanContext extractImpl(C carrier, Getter<C> getter) {
Expand Down
31 changes: 17 additions & 14 deletions api/src/test/java/io/opentelemetry/trace/DefaultTracerTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -31,16 +31,16 @@ class DefaultTracerTest {

@Test
void defaultGetCurrentSpan() {
assertThat(defaultTracer.getCurrentSpan()).isInstanceOf(DefaultSpan.class);
assertThat(defaultTracer.getCurrentSpan().isValid()).isFalse();
}

@Test
void getCurrentSpan_WithSpan() {
assertThat(defaultTracer.getCurrentSpan()).isInstanceOf(DefaultSpan.class);
try (Scope ws = defaultTracer.withSpan(DefaultSpan.getInvalid())) {
assertThat(defaultTracer.getCurrentSpan()).isInstanceOf(DefaultSpan.class);
assertThat(defaultTracer.getCurrentSpan().isValid()).isFalse();
try (Scope ws = defaultTracer.withSpan(Span.getInvalid())) {
assertThat(defaultTracer.getCurrentSpan().isValid()).isFalse();
}
assertThat(defaultTracer.getCurrentSpan()).isInstanceOf(DefaultSpan.class);
assertThat(defaultTracer.getCurrentSpan().isValid()).isFalse();
}

@Test
Expand All @@ -50,7 +50,7 @@ void spanBuilderWithName_NullName() {

@Test
void defaultSpanBuilderWithName() {
assertThat(defaultTracer.spanBuilder(SPAN_NAME).startSpan()).isInstanceOf(DefaultSpan.class);
assertThat(defaultTracer.spanBuilder(SPAN_NAME).startSpan().isValid()).isFalse();
}

@Test
Expand All @@ -65,7 +65,7 @@ void testInProcessContext() {
assertThat(defaultTracer.getCurrentSpan()).isEqualTo(span);
}
}
assertThat(defaultTracer.getCurrentSpan()).isInstanceOf(DefaultSpan.class);
assertThat(defaultTracer.getCurrentSpan().isValid()).isFalse();
}

@Test
Expand All @@ -74,14 +74,14 @@ void testSpanContextPropagationExplicitParent() {
defaultTracer
.spanBuilder(SPAN_NAME)
.setParent(
TracingContextUtils.withSpan(DefaultSpan.create(spanContext), Context.root()))
TracingContextUtils.withSpan(Span.getPropagated(spanContext), Context.root()))
.startSpan();
assertThat(span.getContext()).isSameAs(spanContext);
}

@Test
void testSpanContextPropagation() {
DefaultSpan parent = new DefaultSpan(spanContext);
Span parent = Span.getPropagated(spanContext);

Span span =
defaultTracer
Expand All @@ -106,31 +106,34 @@ void testSpanContextPropagation_nullContext() {

@Test
void testSpanContextPropagation_fromContext() {
Context context = TracingContextUtils.withSpan(new DefaultSpan(spanContext), Context.current());
Context context =
TracingContextUtils.withSpan(Span.getPropagated(spanContext), Context.current());

Span span = defaultTracer.spanBuilder(SPAN_NAME).setParent(context).startSpan();
assertThat(span.getContext()).isSameAs(spanContext);
}

@Test
void testSpanContextPropagation_fromContextAfterNoParent() {
Context context = TracingContextUtils.withSpan(new DefaultSpan(spanContext), Context.current());
Context context =
TracingContextUtils.withSpan(Span.getPropagated(spanContext), Context.current());

Span span = defaultTracer.spanBuilder(SPAN_NAME).setNoParent().setParent(context).startSpan();
assertThat(span.getContext()).isSameAs(spanContext);
}

@Test
void testSpanContextPropagation_fromContextThenNoParent() {
Context context = TracingContextUtils.withSpan(new DefaultSpan(spanContext), Context.current());
Context context =
TracingContextUtils.withSpan(Span.getPropagated(spanContext), Context.current());

Span span = defaultTracer.spanBuilder(SPAN_NAME).setParent(context).setNoParent().startSpan();
assertThat(span.getContext()).isEqualTo(SpanContext.getInvalid());
}

@Test
void testSpanContextPropagationCurrentSpan() {
DefaultSpan parent = new DefaultSpan(spanContext);
Span parent = Span.getPropagated(spanContext);
try (Scope scope = defaultTracer.withSpan(parent)) {
Span span = defaultTracer.spanBuilder(SPAN_NAME).startSpan();
assertThat(span.getContext()).isSameAs(spanContext);
Expand All @@ -140,7 +143,7 @@ void testSpanContextPropagationCurrentSpan() {
@Test
void testSpanContextPropagationCurrentSpanContext() {
Context context =
TracingContextUtils.withSpan(DefaultSpan.create(spanContext), Context.current());
TracingContextUtils.withSpan(Span.getPropagated(spanContext), Context.current());
try (Scope scope = context.makeCurrent()) {
Span span = defaultTracer.spanBuilder(SPAN_NAME).startSpan();
assertThat(span.getContext()).isSameAs(spanContext);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,19 +17,18 @@
import io.opentelemetry.common.Attributes;
import org.junit.jupiter.api.Test;

/** Unit tests for {@link DefaultSpan}. */
class DefaultSpanTest {
class PropagatedSpanTest {

@Test
void hasInvalidContextAndDefaultSpanOptions() {
SpanContext context = DefaultSpan.getInvalid().getContext();
SpanContext context = Span.getInvalid().getContext();
assertThat(context.getTraceFlags()).isEqualTo(TraceFlags.getDefault());
assertThat(context.getTraceState()).isEqualTo(TraceState.getDefault());
}

@Test
void doNotCrash() {
Span span = DefaultSpan.getInvalid();
Span span = Span.getInvalid();
span.setAttribute(stringKey("MyStringAttributeKey"), "MyStringAttributeValue");
span.setAttribute(booleanKey("MyBooleanAttributeKey"), true);
span.setAttribute(longKey("MyLongAttributeKey"), 123L);
Expand All @@ -56,7 +55,7 @@ void doNotCrash() {

@Test
void defaultSpan_ToString() {
Span span = DefaultSpan.getInvalid();
Span span = Span.getInvalid();
assertThat(span.toString()).isEqualTo("DefaultSpan");
}
}
8 changes: 4 additions & 4 deletions api/src/test/java/io/opentelemetry/trace/SpanBuilderTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -22,18 +22,18 @@ class SpanBuilderTest {
void doNotCrash_NoopImplementation() {
Span.Builder spanBuilder = tracer.spanBuilder("MySpanName");
spanBuilder.setSpanKind(Kind.SERVER);
spanBuilder.setParent(TracingContextUtils.withSpan(DefaultSpan.create(null), Context.root()));
spanBuilder.setParent(TracingContextUtils.withSpan(Span.getPropagated(null), Context.root()));
spanBuilder.setParent(Context.root());
spanBuilder.setNoParent();
spanBuilder.addLink(DefaultSpan.getInvalid().getContext());
spanBuilder.addLink(DefaultSpan.getInvalid().getContext(), Attributes.empty());
spanBuilder.addLink(Span.getInvalid().getContext());
spanBuilder.addLink(Span.getInvalid().getContext(), Attributes.empty());
spanBuilder.setAttribute("key", "value");
spanBuilder.setAttribute("key", 12345L);
spanBuilder.setAttribute("key", .12345);
spanBuilder.setAttribute("key", true);
spanBuilder.setAttribute(stringKey("key"), "value");
spanBuilder.setStartTimestamp(12345L);
assertThat(spanBuilder.startSpan()).isInstanceOf(DefaultSpan.class);
assertThat(spanBuilder.startSpan().isValid()).isFalse();
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,12 @@ class TracingContextUtilsTest {
@Test
void testGetCurrentSpan_Default() {
Span span = TracingContextUtils.getCurrentSpan();
assertThat(span).isSameAs(DefaultSpan.getInvalid());
assertThat(span).isSameAs(Span.getInvalid());
}

@Test
void testGetCurrentSpan_SetSpan() {
Span span = DefaultSpan.create(SpanContext.getInvalid());
Span span = Span.getPropagated(SpanContext.getInvalid());
try (Scope ignored = TracingContextUtils.withSpan(span, Context.current()).makeCurrent()) {
assertThat(TracingContextUtils.getCurrentSpan()).isSameAs(span);
}
Expand All @@ -30,12 +30,12 @@ void testGetCurrentSpan_SetSpan() {
@Test
void testGetSpan_DefaultContext() {
Span span = TracingContextUtils.getSpan(Context.current());
assertThat(span).isSameAs(DefaultSpan.getInvalid());
assertThat(span).isSameAs(Span.getInvalid());
}

@Test
void testGetSpan_ExplicitContext() {
Span span = DefaultSpan.create(SpanContext.getInvalid());
Span span = Span.getPropagated(SpanContext.getInvalid());
Context context = TracingContextUtils.withSpan(span, Context.current());
assertThat(TracingContextUtils.getSpan(context)).isSameAs(span);
}
Expand All @@ -48,7 +48,7 @@ void testGetSpanWithoutDefault_DefaultContext() {

@Test
void testGetSpanWithoutDefault_ExplicitContext() {
Span span = DefaultSpan.create(SpanContext.getInvalid());
Span span = Span.getPropagated(SpanContext.getInvalid());
Context context = TracingContextUtils.withSpan(span, Context.current());
assertThat(TracingContextUtils.getSpanWithoutDefault(context)).isSameAs(span);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
import io.opentelemetry.context.Context;
import io.opentelemetry.context.propagation.TextMapPropagator.Getter;
import io.opentelemetry.context.propagation.TextMapPropagator.Setter;
import io.opentelemetry.trace.DefaultSpan;
import io.opentelemetry.trace.Span;
import io.opentelemetry.trace.SpanContext;
import io.opentelemetry.trace.SpanId;
import io.opentelemetry.trace.TraceFlags;
Expand Down Expand Up @@ -52,7 +52,7 @@ private static SpanContext getSpanContext(Context context) {
}

private static Context withSpanContext(SpanContext spanContext, Context context) {
return TracingContextUtils.withSpan(DefaultSpan.create(spanContext), context);
return TracingContextUtils.withSpan(Span.getPropagated(spanContext), context);
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

import io.opentelemetry.context.Context;
import io.opentelemetry.context.propagation.TextMapPropagator;
import io.opentelemetry.trace.DefaultSpan;
import io.opentelemetry.trace.Span;
import io.opentelemetry.trace.SpanContext;
import io.opentelemetry.trace.TraceFlags;
import io.opentelemetry.trace.TraceState;
Expand Down Expand Up @@ -63,7 +63,7 @@ public abstract static class AbstractContextInjectBenchmark {
@Fork(1)
public Map<String, String> measureInject() {
Context context =
TracingContextUtils.withSpan(DefaultSpan.create(contextToTest), Context.current());
TracingContextUtils.withSpan(Span.getPropagated(contextToTest), Context.current());
doInject(context, carrier);
return carrier;
}
Expand Down
Loading