Why is there a DefaultSpan?

[yurishkuro]

There is no such concept in the specification, but this class is present in the Java API.

https://github.com/open-telemetry/opentelemetry-java/blob/efc49522ac4fd01bcf94afa60789264515aa75cd/api/src/main/java/io/opentelemetry/trace/DefaultSpan.java

Can it not be hidden inside the SDK as implementation detail?

[jkwatson]

@carlosalberto Can probably answer this more completely than I can, but the purpose of the DefaultSpan is to provide an implementation when there is no SDK installed. The implementation does context propagation but nothing else.

[yurishkuro]

I don't think that's the only reason, because such default implementation can be done without exposing the DefaultSpan as a public class in the API.

[jkwatson]

Looks like the only usage that's keeping it from being made package-access is in the HttpTraceContext implementation (and other Propagators). @carlosalberto would probably have the most insight into why that leaked in there.

[bogdandrutu]

@yurishkuro I am happy to work on making the default implementations not public. Would also be good to make this call across different languages, currently looking for example at go https://godoc.org/go.opentelemetry.io/otel/api/trace there a Noop public types. I think it is good to be consistent.

[carlosalberto]

Well, having DefaultSpan accessible (or at least, allowing the user to create it) allows us to do some in-propagation stuff.

I'd like us to offer a way to keep this, if possible (by keeping a method somewhere, that returns an empty Span out of a SpanContext).

[yurishkuro]

Can you be specific? What does it allow?

[carlosalberto]

Hey @yurishkuro

So it allows users to use a SpanContext as a Span. Moreover, with the latest changes from OTEP 66, this is the only way to store an active SpanContext there:

Context context = TracingContextUtils.withSpan(DefaultSpan.create(spanContext)); // OR
Scope scope = TracingContextUtils.currentContextWithSpan(DefaultSpan.create(spanContext));

But perhaps that's the only important case we need to support. I see two alternatives:

1. Keep a method somewhere to create empty Spans out of SpanContext values (as it's currently the case with DefaultSpan.create()), or

2. Add TracingContextUtils method overloads taking a SpanContext instead of Span (which internally would use a DefaultSpan anyway):

public static Scope currentContextWithSpanContext(SpanContext spanContext) {
  return currentContextWithSpan(DefaultSpan.create(spanContext));
}

[yurishkuro]

This sounds like a private contract between the SDK Tracer and the base Propagator. It could be all encapsulated in the base propagator, with Tracer having no knowledge of how the span/spancontext are stored in the Context.

[bogdandrutu]

@yurishkuro there are two use-cases:

1. We use the same instance instead of duplicating code in the SDK if the sampling decision is to not sample when creating a Span. I would not duplicate that code, but we can do it.
2. We use it in the propagator as @carlosalberto described.

[yurishkuro]

@bogdandrutu

1. is an implementation detail that does not require this class in the API
2. is also an implementation detail of the propagator

Just recently I saw several questions in OpenTracing about "how can I create a NoopSpan", "why my NoopSpan is being rejected", etc. - having API concerts like DefaultSpan invites these questions.

[ghost]

Drive by comment, we're using this library and interoperating with it and having the following would help:

Add TracingContextUtils method overloads taking a SpanContext instead of Span (which internally would use a DefaultSpan anyway):

[carlosalberto]

Closing as now we have this as PropagatedSpan in the Specification.

[jkwatson]

This will be resolved by #1704, so yes!