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

Implement spec change to only accept Context as span parent. #1611

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
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 @@ -24,7 +24,6 @@
import io.opentelemetry.trace.TraceFlags;
import io.opentelemetry.trace.TraceId;
import io.opentelemetry.trace.TraceState;
import io.opentelemetry.trace.TracingContextUtils;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
Expand Down Expand Up @@ -93,7 +92,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(DefaultSpan.createInContext(context, Context.ROOT));
}
return contexts;
}
Expand Down
27 changes: 21 additions & 6 deletions api/src/main/java/io/opentelemetry/trace/DefaultSpan.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package io.opentelemetry.trace;

import io.grpc.Context;
import io.opentelemetry.common.AttributeValue;
import io.opentelemetry.common.Attributes;
import javax.annotation.concurrent.Immutable;
Expand All @@ -31,8 +32,10 @@
@Immutable
public final class DefaultSpan implements Span {

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

// TODO: Do we need to change this to getInvalid(Context parent)?
/**
* Returns a {@link DefaultSpan} with an invalid {@link SpanContext}.
*
Expand All @@ -44,20 +47,27 @@ public static Span getInvalid() {
}

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

public static Context createInContext(SpanContext spanContext, Context parent) {
return TracingContextUtils.withSpan(create(spanContext, parent), parent);
}

private final SpanContext spanContext;
private final Context parent;

DefaultSpan(SpanContext spanContext) {
DefaultSpan(SpanContext spanContext, Context parent) {
this.spanContext = spanContext;
this.parent = parent;
}

@Override
Expand Down Expand Up @@ -116,6 +126,11 @@ public SpanContext getContext() {
return spanContext;
}

@Override
public Context getParent() {
return parent;
}

@Override
public boolean isRecording() {
return false;
Expand Down
31 changes: 10 additions & 21 deletions api/src/main/java/io/opentelemetry/trace/DefaultTracer.java
Original file line number Diff line number Diff line change
Expand Up @@ -67,43 +67,32 @@ static NoopSpanBuilder create(String spanName) {
}

private boolean isRootSpan;
@Nullable private SpanContext spanContext;
@Nullable private Context parent;

@Override
public Span startSpan() {
if (spanContext == null && !isRootSpan) {
spanContext = TracingContextUtils.getCurrentSpan().getContext();
if (parent == null) {
parent = Context.current();
}
if (isRootSpan) {
parent = TracingContextUtils.withSpan(DefaultSpan.getInvalid(), parent);
}

return spanContext != null && !SpanContext.getInvalid().equals(spanContext)
? new DefaultSpan(spanContext)
: DefaultSpan.getInvalid();
}

@Override
public NoopSpanBuilder setParent(Span parent) {
Utils.checkNotNull(parent, "parent");
spanContext = parent.getContext();
return this;
}

@Override
public NoopSpanBuilder setParent(SpanContext remoteParent) {
Utils.checkNotNull(remoteParent, "remoteParent");
spanContext = remoteParent;
return this;
return new DefaultSpan(TracingContextUtils.getSpan(parent).getContext(), parent);
}

@Override
public NoopSpanBuilder setParent(Context context) {
Utils.checkNotNull(context, "context");
spanContext = TracingContextUtils.getSpan(context).getContext();
isRootSpan = false; // TODO port this fix to mainline
Oberon00 marked this conversation as resolved.
Show resolved Hide resolved
parent = context;
return this;
}

@Override
public NoopSpanBuilder setNoParent() {
isRootSpan = true;
parent = null;
return this;
}

Expand Down
53 changes: 8 additions & 45 deletions api/src/main/java/io/opentelemetry/trace/Span.java
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,14 @@ enum Kind {
*/
SpanContext getContext();

/**
* Returns the parent {@code Context} associated with this {@code Span}.
*
* @return the parent {@code Context} associated with this {@code Span}.
* @since 0.8.0
*/
Context getParent();
Oberon00 marked this conversation as resolved.
Show resolved Hide resolved

/**
* Returns {@code true} if this {@code Span} records tracing events (e.g. {@link
* #addEvent(String)}, {@link #setAttribute(String, long)}).
Expand Down Expand Up @@ -383,51 +391,6 @@ enum Kind {
*/
interface Builder {

/**
* Sets the parent {@code Span} to use. If not set, the value of {@code Tracer.getCurrentSpan()}
* at {@link #startSpan()} time will be used as parent.
*
* <p>This <b>must</b> be used to create a {@code Span} when manual Context propagation is used
* OR when creating a root {@code Span} with a parent with an invalid {@link SpanContext}.
*
* <p>Observe this is the preferred method when the parent is a {@code Span} created within the
* process. Using its {@code SpanContext} as parent remains as a valid, albeit inefficient,
* operation.
*
* <p>If called multiple times, only the last specified value will be used. Observe that the
* state defined by a previous call to {@link #setNoParent()} will be discarded.
*
* @param parent the {@code Span} used as parent.
* @return this.
* @throws NullPointerException if {@code parent} is {@code null}.
* @see #setNoParent()
* @since 0.1.0
*/
Builder setParent(Span parent);

/**
* Sets the parent {@link SpanContext} to use. If not set, the value of {@code
* Tracer.getCurrentSpan()} at {@link #startSpan()} time will be used as parent.
*
* <p>Similar to {@link #setParent(Span parent)} but this <b>must</b> be used to create a {@code
* Span} when the parent is in a different process. This is only intended for use by RPC systems
* or similar.
*
* <p>If no {@link SpanContext} is available, users must call {@link #setNoParent()} in order to
* create a root {@code Span} for a new trace.
*
* <p>If called multiple times, only the last specified value will be used. Observe that the
* state defined by a previous call to {@link #setNoParent()} will be discarded.
*
* @param remoteParent the {@link SpanContext} used as parent.
* @return this.
* @throws NullPointerException if {@code remoteParent} is {@code null}.
* @see #setParent(Span parent)
* @see #setNoParent()
* @since 0.1.0
*/
Builder setParent(SpanContext remoteParent);

/**
* Sets the parent to use from the specified {@code Context}. If not set, the value of {@code
* Tracer.getCurrentSpan()} at {@link #startSpan()} time will be used as parent.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ private static <C> void injectImpl(SpanContext spanContext, C carrier, Setter<C>
return context;
}

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

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

@Test
void testSpanContextPropagationExplicitParent() {
Span span = defaultTracer.spanBuilder(SPAN_NAME).setParent(spanContext).startSpan();
Span span =
defaultTracer
.spanBuilder(SPAN_NAME)
.setParent(DefaultSpan.createInContext(spanContext, Context.ROOT))
.startSpan();
assertThat(span.getContext()).isSameAs(spanContext);
}

@Test
void testSpanContextPropagation() {
DefaultSpan parent = new DefaultSpan(spanContext);
DefaultSpan parent = new DefaultSpan(spanContext, Context.ROOT);

Span span = defaultTracer.spanBuilder(SPAN_NAME).setParent(parent).startSpan();
Span span =
defaultTracer
.spanBuilder(SPAN_NAME)
.setParent(TracingContextUtils.withSpan(parent, Context.ROOT))
.startSpan();
assertThat(span.getContext()).isSameAs(spanContext);
}

Expand All @@ -108,15 +116,17 @@ void testSpanContextPropagation_nullContext() {

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

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

@Test
void testSpanContextPropagationCurrentSpan() {
DefaultSpan parent = new DefaultSpan(spanContext);
DefaultSpan parent = new DefaultSpan(spanContext, Context.ROOT);
try (Scope scope = defaultTracer.withSpan(parent)) {
Span span = defaultTracer.spanBuilder(SPAN_NAME).startSpan();
assertThat(span.getContext()).isSameAs(spanContext);
Expand All @@ -125,8 +135,7 @@ void testSpanContextPropagationCurrentSpan() {

@Test
void testSpanContextPropagationCurrentSpanContext() {
Context context =
TracingContextUtils.withSpan(DefaultSpan.create(spanContext), Context.current());
Context context = DefaultSpan.createInContext(spanContext, Context.current());
try (Scope scope = ContextUtils.withScopedContext(context)) {
Span span = defaultTracer.spanBuilder(SPAN_NAME).startSpan();
assertThat(span.getContext()).isSameAs(spanContext);
Expand Down
15 changes: 5 additions & 10 deletions api/src/test/java/io/opentelemetry/trace/SpanBuilderTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertThrows;

import io.grpc.Context;
import io.opentelemetry.common.AttributeValue;
import io.opentelemetry.common.Attributes;
import io.opentelemetry.trace.Span.Kind;
Expand All @@ -32,8 +33,8 @@ class SpanBuilderTest {
void doNotCrash_NoopImplementation() {
Span.Builder spanBuilder = tracer.spanBuilder("MySpanName");
spanBuilder.setSpanKind(Kind.SERVER);
spanBuilder.setParent(DefaultSpan.getInvalid());
spanBuilder.setParent(DefaultSpan.getInvalid().getContext());
spanBuilder.setParent(DefaultSpan.createInContext(null, Context.ROOT));
spanBuilder.setParent(Context.ROOT);
spanBuilder.setNoParent();
spanBuilder.addLink(DefaultSpan.getInvalid().getContext());
spanBuilder.addLink(DefaultSpan.getInvalid().getContext(), Attributes.empty());
Expand Down Expand Up @@ -61,15 +62,9 @@ public Attributes getAttributes() {
}

@Test
void setParent_NullSpan() {
void setParent_EmptyContext() {
Span.Builder spanBuilder = tracer.spanBuilder("MySpanName");
assertThrows(NullPointerException.class, () -> spanBuilder.setParent((Span) null));
}

@Test
void setParent_NullSpanContext() {
Span.Builder spanBuilder = tracer.spanBuilder("MySpanName");
assertThrows(NullPointerException.class, () -> spanBuilder.setParent((SpanContext) null));
assertThrows(NullPointerException.class, () -> spanBuilder.setParent(null));
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ void testGetCurrentSpan_Default() {

@Test
void testGetCurrentSpan_SetSpan() {
Span span = DefaultSpan.create(SpanContext.getInvalid());
Span span = DefaultSpan.create(SpanContext.getInvalid(), Context.current());
Context orig = TracingContextUtils.withSpan(span, Context.current()).attach();
try {
assertThat(TracingContextUtils.getCurrentSpan()).isSameAs(span);
Expand All @@ -49,7 +49,7 @@ void testGetSpan_DefaultContext() {

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

@Test
void testGetSpanWithoutDefault_ExplicitContext() {
Span span = DefaultSpan.create(SpanContext.getInvalid());
Span span = DefaultSpan.create(SpanContext.getInvalid(), Context.current());
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 @@ -67,7 +67,7 @@ private static SpanContext getSpanContext(Context context) {
}

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

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,7 @@ private static SpanData getSpanData(long startMs, long endMs) {
.setHasEnded(true)
.setTraceId(TraceId.fromLowerBase16(TRACE_ID, 0))
.setSpanId(SpanId.fromLowerBase16(SPAN_ID, 0))
.setParentSpanId(SpanId.fromLowerBase16(PARENT_SPAN_ID, 0))
.setParent(SpanId.fromLowerBase16(PARENT_SPAN_ID, 0), false)
.setName("GET /api/endpoint")
.setStartEpochNanos(TimeUnit.MILLISECONDS.toNanos(startMs))
.setEndEpochNanos(TimeUnit.MILLISECONDS.toNanos(endMs))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ void toProtoSpan() {
.setHasEnded(true)
.setTraceId(TRACE_ID)
.setSpanId(SPAN_ID)
.setParentSpanId(SpanId.getInvalid())
.setParent(SpanId.getInvalid(), false)
.setName("GET /api/endpoint")
.setKind(Kind.SERVER)
.setStartEpochNanos(12345)
Expand Down
Loading