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

Minimal implementation is unclear on context propagation #183

Closed
tsloughter opened this issue Jul 16, 2019 · 7 comments
Closed

Minimal implementation is unclear on context propagation #183

tsloughter opened this issue Jul 16, 2019 · 7 comments
Labels
spec:context Related to the specification/context directory
Milestone

Comments

@tsloughter
Copy link
Member

I'm starting on the Erlang version of the API and minimal implementation and I find it unclear what exactly it should and shouldn't be doing.

On Gitter Tigran explained that it is not the same as the noop tracer because it is expected to do context propagation. And the spec makes clear it should not return a null span but a valid non-null span object.

What isn't clear is does having context propagation and non-null spans imply that a startSpan will create a new child span id as the current context? Or is it expected that the same context as was started with will be what is propagated? If the latter, what about when there is no parent span, then a new span id and trace id must be created?

@iredelmeier iredelmeier added spec:context Related to the specification/context directory api vs sdk bounds labels Jul 30, 2019
@bg451
Copy link
Member

bg451 commented Jul 30, 2019

I was wondering the same thing. If you have a noop span, should you still pass through a span context that was received out of band? e.g. service A and C have tracing enabled and service B has tracing disabled. A makes a call to B which makes a call to C. If the minimal noop span in B doesn't pass the context through to C the trace will break at A.

I guess it's a trade off of reducing allocations when tracing is disabled vs not breaking traces. I lean towards the latter being the case for a noop implementation.

when there is no parent span, then a new span id and trace id must be created.

My gut says the answer is no, if you start a span with no parent and it's a no op implementation, you can leave the span context empty until you reach code that has tracing enabled, which can then determine the sampled flag and all the jazz.

@mtwo
Copy link
Member

mtwo commented Jul 30, 2019

@tsloughter
Copy link
Member Author

My gut says the answer is no, if you start a span with no parent and it's a no op implementation, you can leave the span context empty until you reach code that has tracing enabled, which can then determine the sampled flag and all the jazz.

True, but the "minimal" implementation that is part of the API is not the same as the noop implementation.

@z1c0
Copy link
Contributor

z1c0 commented Aug 20, 2019

The "minimal implementation" that is referenced here is described as being "part of the API" and "incurring as little performance penalty as possible". This sounds a lot like a no-op implementation to me.

Is a there even a section in the specification that makes a clear distinction between:

  • No-Op Implementation
  • Minimal Implementation

This is a prerequisite for having this discussion about context propagation at all, I think.

@tsloughter
Copy link
Member Author

@z1c0 I don't think there is a section that makes a clear distinction. I asked in gitter before creating this issue and that is where I learned it was distinct.

@carlosalberto
Copy link
Contributor

This is a prerequisite for having this discussion about context propagation at all, I think.

I suggest discussing this as part of #208 where I hope we can come to a resolution about this point ;)

@tedsuo
Copy link
Contributor

tedsuo commented Oct 4, 2019

Hi all, I'm closing this in an attempt to resolve this conversation in #208. Please reopen if you feel there is additional scope to this issue.

@tedsuo tedsuo closed this as completed Oct 4, 2019
TuckTuckFloof pushed a commit to TuckTuckFloof/opentelemetry-specification that referenced this issue Oct 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:context Related to the specification/context directory
Projects
None yet
Development

No branches or pull requests

8 participants