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

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

Closed
carlosalberto opened this issue Mar 10, 2020 · 9 comments · Fixed by #875
Closed

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

carlosalberto opened this issue Mar 10, 2020 · 9 comments · Fixed by #875
Assignees
Labels
area:api Cross language API specification issue priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:baggage Related to the specification/baggage directory spec:context Related to the specification/context directory spec:trace Related to the specification/trace directory
Milestone

Comments

@carlosalberto
Copy link
Contributor

carlosalberto commented Mar 10, 2020

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);
@carlosalberto carlosalberto added this to the v0.5 milestone Mar 10, 2020
@carlosalberto carlosalberto changed the title Consider only having start* operations taking Context Consider using Context as the unique way to specify parenthood Mar 10, 2020
@bogdandrutu
Copy link
Member

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
Copy link
Contributor

jmacd commented Mar 18, 2020

I support this motion.

@Oberon00
Copy link
Member

Oberon00 commented Mar 18, 2020

I think we should at least remove the SpanContext option, mainly because of the issues outlined in open-telemetry/oteps#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 open-telemetry/oteps#73, open-telemetry/oteps#68).

@carlosalberto carlosalberto modified the milestones: v0.5, v0.6 Jun 9, 2020
@bogdandrutu bogdandrutu added spec:context Related to the specification/context directory spec:trace Related to the specification/trace directory labels Jun 12, 2020
@carlosalberto carlosalberto added the spec:baggage Related to the specification/baggage directory label Jun 26, 2020
@reyang reyang added the release:required-for-ga Must be resolved before GA release, or nice to have before GA label Jul 1, 2020
@mtwo mtwo added this to Required for GA, needs action. Add label:release:required-for-ga to issues and PRs when moving them to this column. in GA Spec Burndown Jul 6, 2020
@andrewhsu andrewhsu added the priority:p2 Medium priority level label Jul 17, 2020
@bogdandrutu
Copy link
Member

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

@Oberon00
Copy link
Member

Oberon00 commented Aug 24, 2020

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 #759 (comment)

@Oberon00
Copy link
Member

Oberon00 commented Sep 7, 2020

Inspired by @anuraaga's comment #875 (comment), 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
Copy link
Member

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

@andrewhsu andrewhsu added priority:p1 Highest priority level and removed priority:p2 Medium priority level labels Sep 14, 2020
@mwear
Copy link
Member

mwear commented Sep 14, 2020

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
Copy link
Member

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

@andrewhsu andrewhsu assigned Oberon00 and unassigned tsloughter Sep 21, 2020
@andrewhsu andrewhsu moved this from Required for GA, needs action. Add label:release:required-for-ga to issues and PRs when moving them to this column. to Required for GA, in progress in GA Spec Burndown Sep 22, 2020
GA Spec Burndown automation moved this from Required for GA, in progress to Required for GA, done Sep 23, 2020
Oberon00 added a commit to dynatrace-oss-contrib/opentelemetry-specification that referenced this issue Oct 9, 2020
arminru pushed a commit that referenced this issue Oct 13, 2020
…#510) (#1079)

* Update compliance matrix for #510.

* Java implemented this already.

* Add: SpanProcessor.OnStart receives parent Context
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Cross language API specification issue priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:baggage Related to the specification/baggage directory spec:context Related to the specification/context directory spec:trace Related to the specification/trace directory
Projects
No open projects
GA Spec Burndown
  
Required/Allowed for GA, resolved.
8 participants