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 17 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
14 changes: 0 additions & 14 deletions api/src/main/java/io/opentelemetry/trace/DefaultTracer.java
Original file line number Diff line number Diff line change
Expand Up @@ -79,20 +79,6 @@ public Span startSpan() {
: 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;
}

@Override
public NoopSpanBuilder setParent(Context context) {
Utils.checkNotNull(context, "context");
Expand Down
45 changes: 0 additions & 45 deletions api/src/main/java/io/opentelemetry/trace/Span.java
Original file line number Diff line number Diff line change
Expand Up @@ -395,51 +395,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
12 changes: 10 additions & 2 deletions api/src/test/java/io/opentelemetry/trace/DefaultTracerTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -82,15 +82,23 @@ void testInProcessContext() {

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

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

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 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 @@ -20,6 +20,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.Attributes;
import io.opentelemetry.trace.Span.Kind;
import org.junit.jupiter.api.Test;
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(TracingContextUtils.withSpan(DefaultSpan.create(null), Context.ROOT));
Copy link
Contributor

Choose a reason for hiding this comment

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

youch. this API sure is ugly! Remind me again how this is going to make things easier/better for our users?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a unit test. Normally you would just call setNoParent.

Copy link
Contributor

Choose a reason for hiding this comment

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

fair enough, but I think having to do parenting via the TCU is going to be pretty counter-intuitive to most users. :(

Copy link
Member Author

Choose a reason for hiding this comment

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

I fully agree, but at the same time, I don't think users should normally do that. Usually you would just use the implicit current parent, or sometimes you need to capture a context with Context.current() and forward that to somewhere else. If you start a span at one thread and need to continue it at another thread while not setting it as active in the current thread is the only place you would need TracingContextUtils. I also think there should be some may to make that more easy. I could imagine span.inCurrentContext() (Java 8 default methods?), or TracingContextUtils.spanInCurrent(span) to make this easier.

Remind me again how this is going to make things easier/better for our users?

As you said, using TracingContextUtils is rather unintuitive, and thus I think most users did not do it, they just passed around spans without the remaining context. However, a custom propagator should be able to set a value in the context in extract and rely on it still being available in inject.

BTW, using inject is a place where you already have to often muck around with TracingContextUtils currently.

Copy link
Member

Choose a reason for hiding this comment

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

Is it safe to assume the implicit current parent is accurate?

For instance, when dealing with asynchronous methods the thread being executed on is usually not the same thread that might have initiated/received the original request. In that situation, the implicit current parent from ThreadLocal is more than likely not the parent you want

Copy link
Member Author

@Oberon00 Oberon00 Sep 24, 2020

Choose a reason for hiding this comment

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

Maybe we can provide some convenience function (default interface implementation?), so one can do Context capturedParent = span.withCurrentContext(). But it would be interesting how common the cases where you would need this even are.

Copy link
Member

Choose a reason for hiding this comment

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

Personally, it's a lot messier, but more importantly way less obvious that that's what is needed to achieve the desired result.

With respect to a default interface implementation, that wouldn't be possible with the JDK 7 requirement, correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

(FWIW, we need more convenience functions in TracingContextUtils, or in whatever new name it ends up having ;) )

Copy link
Member Author

Choose a reason for hiding this comment

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

@kenfinnigan Java 7 was dropped recently, Java 8 is now required.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @Oberon00, don't follow things for a few weeks, and everything changes!

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_NullContext() {
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 @@ -21,10 +21,13 @@
import static io.opentelemetry.common.AttributesKeys.longKey;
import static io.opentelemetry.common.AttributesKeys.stringKey;

import io.grpc.Context;
import io.opentelemetry.common.AttributeKey;
import io.opentelemetry.correlationcontext.CorrelationContext;
import io.opentelemetry.trace.DefaultSpan;
import io.opentelemetry.trace.Span.Kind;
import io.opentelemetry.trace.Status;
import io.opentelemetry.trace.TracingContextUtils;
import io.opentracing.Span;
import io.opentracing.SpanContext;
import io.opentracing.Tracer.SpanBuilder;
Expand Down Expand Up @@ -192,11 +195,13 @@ public Span start() {
if (ignoreActiveSpan && parentSpan == null && parentSpanContext == null) {
builder.setNoParent();
} else if (parentSpan != null) {
builder.setParent(parentSpan.getSpan());
builder.setParent(TracingContextUtils.withSpan(parentSpan.getSpan(), Context.ROOT));
SpanContextShim contextShim = spanContextTable().get(parentSpan);
distContext = contextShim == null ? null : contextShim.getCorrelationContext();
} else if (parentSpanContext != null) {
builder.setParent(parentSpanContext.getSpanContext());
builder.setParent(
TracingContextUtils.withSpan(
DefaultSpan.create(parentSpanContext.getSpanContext()), Context.ROOT));
distContext = parentSpanContext.getCorrelationContext();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,14 +58,13 @@ final class SpanBuilderSdk implements Span.Builder {
private final IdsGenerator idsGenerator;
private final Clock clock;

@Nullable private Span parent;
@Nullable private SpanContext remoteParent;
@Nullable private Context parent;
private Kind spanKind = Kind.INTERNAL;
@Nullable private AttributesMap attributes;
@Nullable private List<io.opentelemetry.trace.Link> links;
private int totalNumberOfLinksAdded = 0;
private ParentType parentType = ParentType.CURRENT_CONTEXT;
private long startEpochNanos = 0;
private boolean isRootSpan;

SpanBuilderSdk(
String spanName,
Expand All @@ -84,34 +83,18 @@ final class SpanBuilderSdk implements Span.Builder {
this.clock = clock;
}

@Override
public Span.Builder setParent(Span parent) {
this.parent = Objects.requireNonNull(parent, "parent");
this.remoteParent = null;
this.parentType = ParentType.EXPLICIT_PARENT;
return this;
}

@Override
public Span.Builder setParent(SpanContext remoteParent) {
this.remoteParent = Objects.requireNonNull(remoteParent, "remoteParent");
this.parent = null;
this.parentType = ParentType.EXPLICIT_REMOTE_PARENT;
return this;
}

@Override
public Span.Builder setParent(Context context) {
Objects.requireNonNull(context, "context");
setParent(TracingContextUtils.getSpan(context));
this.isRootSpan = false;
this.parent = context;
return this;
}

@Override
public Span.Builder setNoParent() {
this.parentType = ParentType.NO_PARENT;
this.isRootSpan = true;
this.parent = null;
this.remoteParent = null;
return this;
}

Expand Down Expand Up @@ -206,17 +189,20 @@ public Span.Builder setStartTimestamp(long startTimestamp) {

@Override
public Span startSpan() {
SpanContext parentContext = parent(parentType, parent, remoteParent);
final Context parentContext =
isRootSpan ? Context.ROOT : parent == null ? Context.current() : parent;
final Span parentSpan = TracingContextUtils.getSpan(parentContext);
final SpanContext parentSpanContext = parentSpan.getContext();
String traceId;
String spanId = idsGenerator.generateSpanId();
TraceState traceState = TraceState.getDefault();
if (!parentContext.isValid()) {
if (!parentSpanContext.isValid()) {
// New root span.
traceId = idsGenerator.generateTraceId();
} else {
// New child span.
traceId = parentContext.getTraceIdAsHexString();
traceState = parentContext.getTraceState();
traceId = parentSpanContext.getTraceIdAsHexString();
traceState = parentSpanContext.getTraceState();
}
List<io.opentelemetry.trace.Link> immutableLinks =
links == null
Expand All @@ -230,7 +216,12 @@ public Span startSpan() {
traceConfig
.getSampler()
.shouldSample(
parentContext, traceId, spanName, spanKind, immutableAttributes, immutableLinks);
parentSpanContext,
traceId,
spanName,
spanKind,
immutableAttributes,
immutableLinks);
Sampler.Decision samplingDecision = samplingResult.getDecision();

SpanContext spanContext =
Expand Down Expand Up @@ -263,11 +254,11 @@ public <T> void consume(AttributeKey<T> key, T value) {
spanName,
instrumentationLibraryInfo,
spanKind,
parentContext.getSpanIdAsHexString(),
parentContext.isRemote(),
parentSpanContext.getSpanIdAsHexString(),
parentSpanContext.isRemote(),
traceConfig,
spanProcessor,
getClock(parentSpan(parentType, parent), clock),
getClock(parentSpan, clock),
resource,
recordedAttributes,
immutableLinks,
Expand All @@ -289,38 +280,4 @@ private static Clock getClock(Span parent, Clock clock) {
return MonotonicClock.create(clock);
}
}

private static SpanContext parent(
ParentType parentType, Span explicitParent, SpanContext remoteParent) {
switch (parentType) {
case NO_PARENT:
return SpanContext.getInvalid();
case CURRENT_CONTEXT:
return TracingContextUtils.getCurrentSpan().getContext();
case EXPLICIT_PARENT:
return explicitParent.getContext();
case EXPLICIT_REMOTE_PARENT:
return remoteParent;
}
throw new IllegalStateException("Unknown parent type");
}

@Nullable
private static Span parentSpan(ParentType parentType, Span explicitParent) {
switch (parentType) {
case CURRENT_CONTEXT:
return TracingContextUtils.getSpanWithoutDefault(Context.current());
case EXPLICIT_PARENT:
return explicitParent;
default:
return null;
}
}

private enum ParentType {
CURRENT_CONTEXT,
EXPLICIT_PARENT,
EXPLICIT_REMOTE_PARENT,
NO_PARENT,
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -74,14 +74,7 @@ void setSpanKind_null() {
@Test
void setParent_null() {
assertThrows(
NullPointerException.class, () -> tracerSdk.spanBuilder(SPAN_NAME).setParent((Span) null));
}

@Test
void setRemoteParent_null() {
assertThrows(
NullPointerException.class,
() -> tracerSdk.spanBuilder(SPAN_NAME).setParent((SpanContext) null));
NullPointerException.class, () -> tracerSdk.spanBuilder(SPAN_NAME).setParent(null));
}

@Test
Expand Down Expand Up @@ -600,7 +593,7 @@ void noParent() {
tracerSdk
.spanBuilder(SPAN_NAME)
.setNoParent()
.setParent(parent)
.setParent(Context.current())
.setNoParent()
.startSpan();
try {
Expand All @@ -623,7 +616,11 @@ void noParent_override() {
try {
RecordEventsReadableSpan span =
(RecordEventsReadableSpan)
tracerSdk.spanBuilder(SPAN_NAME).setNoParent().setParent(parent).startSpan();
tracerSdk
.spanBuilder(SPAN_NAME)
.setNoParent()
.setParent(TracingContextUtils.withSpan(parent, Context.current()))
.startSpan();
try {
assertThat(span.getContext().getTraceIdAsHexString())
.isEqualTo(parent.getContext().getTraceIdAsHexString());
Expand All @@ -635,7 +632,7 @@ void noParent_override() {
tracerSdk
.spanBuilder(SPAN_NAME)
.setNoParent()
.setParent(parent.getContext())
.setParent(TracingContextUtils.withSpan(parent, Context.current()))
.startSpan();
try {
assertThat(span2.getContext().getTraceIdAsHexString())
Expand All @@ -661,7 +658,7 @@ void overrideNoParent_remoteParent() {
tracerSdk
.spanBuilder(SPAN_NAME)
.setNoParent()
.setParent(parent.getContext())
.setParent(TracingContextUtils.withSpan(parent, Context.current()))
.startSpan();
try {
assertThat(span.getContext().getTraceIdAsHexString())
Expand Down Expand Up @@ -747,7 +744,10 @@ void parent_invalidContext() {

RecordEventsReadableSpan span =
(RecordEventsReadableSpan)
tracerSdk.spanBuilder(SPAN_NAME).setParent(parent.getContext()).startSpan();
tracerSdk
.spanBuilder(SPAN_NAME)
.setParent(TracingContextUtils.withSpan(parent, Context.current()))
.startSpan();
try {
assertThat(span.getContext().getTraceIdAsHexString())
.isNotEqualTo(parent.getContext().getTraceIdAsHexString());
Expand All @@ -768,9 +768,9 @@ void startTimestamp_null() {
@Test
void parent_clockIsSame() {
Span parent = tracerSdk.spanBuilder(SPAN_NAME).startSpan();
try {
try (Scope scope = tracerSdk.withSpan(parent)) {
RecordEventsReadableSpan span =
(RecordEventsReadableSpan) tracerSdk.spanBuilder(SPAN_NAME).setParent(parent).startSpan();
(RecordEventsReadableSpan) tracerSdk.spanBuilder(SPAN_NAME).startSpan();

assertThat(span.getClock()).isSameAs(((RecordEventsReadableSpan) parent).getClock());
} finally {
Expand Down
Loading