open-telemetry / opentelemetry-specification

Specifications for OpenTelemetry
https://opentelemetry.io
Apache License 2.0
3.65k stars 873 forks source link

Consider using Context as the unique way to specify parenthood #510

Closed carlosalberto closed 3 years ago

carlosalberto commented 4 years ago

At this moment, CorrelationContext and Span instances being created can explicitly specify their parent:

spanBuilder.setParent(anotherSpan);
spanBuilder.setParent(someSpanContext);

As part of OTEP 66, we could remove these and have a single one specifying Context:

// Decide the parent of whatever it is in the specified context.
spanBuilder.setParent(context);
bogdandrutu commented 4 years ago

For the same reasons we have a Span object (mostly for manual propagation - users that don't want to use Context we need to support the version that receives as parent a Span as well).

jmacd commented 4 years ago

I support this motion.

Oberon00 commented 4 years ago

I think we should at least remove the SpanContext option, mainly because of the issues outlined in https://github.com/open-telemetry/oteps/pull/58.

If we want to go further, after that, we can actually remove SpanContext completely from any public facing API (methods to get span+trace ID move to Span, sampled flag and tracestate is better hidden away in the context anyway). And then it's a small step to remove Span and replace it with tracer operations that take a context argument (see https://github.com/open-telemetry/oteps/issues/73, https://github.com/open-telemetry/oteps/issues/68).

bogdandrutu commented 3 years ago

Supporting passing a Span is important for user that don't want to use Context. So we need to support that anyway.

Oberon00 commented 3 years ago

I don't think that's a good argument. Why should we support users who don't want to use Context? Why would users not want to use Context? But it might be necessary to store a parent Context in the span anyway, see https://github.com/open-telemetry/opentelemetry-specification/issues/759#issuecomment-678765521

Oberon00 commented 3 years ago

Inspired by @anuraaga's comment https://github.com/open-telemetry/opentelemetry-specification/pull/875#issuecomment-687999941, an alternative which I think is less elegant but certainly more compatible, is to store a (parent) Context in the SpanContext. Then we can continue passing just SpanContext and we will still be able to pass information from extract to inject. Of course, if the user wants to add more info to the Context in-between, it would still be better to use Context.

andrewhsu commented 3 years ago

from the maintainers mtg discussion today, talked about this issue and the related PR #875 and it was decided to bump this to P1 to concentrate on the decision if this is necessary for spec trace freeze. cc @carlosalberto @tedsuo

mwear commented 3 years ago

I would be in the support of this as well. I think removing parenting via span context is the easiest and least controversial parts of this proposal. In a post OTEP-66 world, having a handle on a span context, but not the span, or a context is going to be very uncommon.

Removing the ability to parent by a span is slightly more controversial, but likely necessary if correlations, or anything else from the context need to be accessed at span start or span finish time. This will impact use cases where context is managed explicitly and has implications for the languages that support Tracer.withSpan() or similar functionality. I think overall, this will prevent bugs and expected surprises when trying to parent using a span alone.

andrewhsu commented 3 years ago

from the maintainers mtg today, assigning to @Oberon00 since he has an open pr on this