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

Make SpanContext optional #999

Closed
anuraaga opened this issue Sep 24, 2020 · 9 comments
Closed

Make SpanContext optional #999

anuraaga opened this issue Sep 24, 2020 · 9 comments
Assignees
Labels
area:api Cross language API specification issue release:after-ga Not required before GA release, and not going to work on before GA spec:trace Related to the specification/trace directory

Comments

@anuraaga
Copy link
Contributor

Currently, all propagation is done using Context, both remote and in-process. This leaves the SpanContext as just a simple wrapper of the trace ID, span ID, flags, and state. It could make sense to remove SpanContext and inline into the Span.

When users come to tracing for the first time, I think they understand the concept of a span. And they notice that a span is supposed to have a trace id and span id, they see this in tracing UIs. The presence of SpanContext seems to just add indirection that complicates the model for new users. It's especially confusing with a similarly named but very different Context.

While I would propose removing it completely, I suspect it's not realistic at this stage. Making it optional seems safer for a last minute change though.

@anuraaga anuraaga added the spec:trace Related to the specification/trace directory label Sep 24, 2020
@anuraaga
Copy link
Contributor Author

/cc @Oberon00

@Oberon00 Oberon00 added the area:api Cross language API specification issue label Sep 24, 2020
@Oberon00
Copy link
Member

We can even make it optional after ga, as I suppose all the SIGs have it already implemented.

@anuraaga
Copy link
Contributor Author

Well, Java Instrumentation is always happy to reduce wrappers due to bridging concerns so we'd be happy to rip it out ASAP if it's allowed :)

@trask
Copy link
Member

trask commented Sep 25, 2020

Big 👍 to reducing the number of different objects we need to bridge in Java Instrumentation before GA.

But even more, I totally agree with this:

When users come to tracing for the first time, I think they understand the concept of a span. And they notice that a span is supposed to have a trace id and span id, they see this in tracing UIs. The presence of SpanContext seems to just add indirection that complicates the model for new users. It's especially confusing with a similarly named but very different Context.

I think inlining trace ID, span ID, flags, and state into Span would address this confusion nicely.

@andrewhsu andrewhsu added the release:after-ga Not required before GA release, and not going to work on before GA label Sep 25, 2020
@Oberon00
Copy link
Member

Note, this originated from #969 (comment)

@andrewhsu andrewhsu added priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA and removed release:after-ga Not required before GA release, and not going to work on before GA labels Sep 29, 2020
@andrewhsu
Copy link
Member

from the spec sig mtg today, discussed this issue and identified that we need a decision on this issue if it makes a change to the trace api so bumping priority. need prototypes by next sig meeting to arrive at decision.

@yurishkuro
Copy link
Member

yurishkuro commented Oct 5, 2020

When users come to tracing for the first time, I think they understand the concept of a span. And they notice that a span is supposed to have a trace id and span id, they see this in tracing UIs. The presence of SpanContext seems to just add indirection that complicates the model for new users. It's especially confusing with a similarly named but very different Context.

I would disagree with this, mainly because of the Extract() function: if Span is an active, writeable object, then Extract() fundamentally cannot return it, since it belongs to the caller process. The recent addition of PropagatedOnlySpan is quite inelegant, because it relies on runtime behavior of the span to ignore write methods, rather than on preventing them at the API level. Pretending that a Span is what's being propagated over the wire might confuse users even more (I actually met some users at conferences and chat rooms thinking that distributed tracing works by passing all the captured data with the requests).

If I had to choose between keeping only one of Span and SpanContext, I would remove the Span.

@carlosalberto
Copy link
Contributor

As the related PRs were closed and we did #1075 in hopes of helping the part regarding the name, I will mark this as "After-GA", so we can continue discussing the alternatives by then.

@carlosalberto carlosalberto added release:after-ga Not required before GA release, and not going to work on before GA and removed priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA labels Oct 14, 2020
@jmacd
Copy link
Contributor

jmacd commented Oct 20, 2020

From the Spec SIG, we agreed that this issue is "too late" to change in the specification.

@jmacd jmacd closed this as completed Oct 20, 2020
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 release:after-ga Not required before GA release, and not going to work on before GA spec:trace Related to the specification/trace directory
Projects
None yet
7 participants