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

Having both local and remote span parents? #949

Closed
rakyll opened this issue Sep 14, 2020 · 7 comments · Fixed by #994
Closed

Having both local and remote span parents? #949

rakyll opened this issue Sep 14, 2020 · 7 comments · Fixed by #994
Assignees
Labels
area:api Cross language API specification issue area:span-relationships Related to span relationships priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:context Related to the specification/context directory spec:trace Related to the specification/trace directory

Comments

@rakyll
Copy link
Contributor

rakyll commented Sep 14, 2020

In the Go trace package, the following functions uses different keys to store the local and remote span contexts and allow a context to have both a local and remote parent:
https://pkg.go.dev/go.opentelemetry.io/otel@v0.11.0/api/trace?tab=doc#ContextWithRemoteSpanContext
https://pkg.go.dev/go.opentelemetry.io/otel@v0.11.0/api/trace?tab=doc#ContextWithSpan

I'm not sure if this is permitted by the spec and if so, it's not clear in the spec what use cases it enables. What's the expected behavior It'd be good to document it at https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#determining-the-parent-span-from-a-context.

@rakyll rakyll added the spec:trace Related to the specification/trace directory label Sep 14, 2020
@iNikem
Copy link
Contributor

iNikem commented Sep 15, 2020

From the spec:

Each span has zero or one parent span

A Span is said to have a remote parent if it is the child of a Span created in another process

SERVER [SpanKind] ... is the child of a remote CLIENT span that was expected to wait for a response

Based on that I would say that Span can have either remote parent or local parent but not both.

@Oberon00
Copy link
Member

@Oberon00 Oberon00 added area:api Cross language API specification issue area:span-relationships Related to span relationships question Question for discussion spec:context Related to the specification/context directory labels Sep 15, 2020
@Oberon00
Copy link
Member

Hmm, actually that spec is quite strange. It basically means that if you extract into a Context that already has a Span in it, the extracted SpanContext will be ignored. At least that is my reading. And Java would then mis-implement the spec by storing a Span instead of a SpanContext in the Context. CC @carlosalberto @bogdandrutu

The parent should be selected in the following order of precedence:

  • Use the current Span, if available.
  • Use the extracted SpanContext, if available.
  • There is no parent. Create a root Span.

(bold added by me)

@Oberon00 Oberon00 removed the question Question for discussion label Sep 15, 2020
@andrewhsu andrewhsu added priority:p2 Medium priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA priority:p1 Highest priority level and removed priority:p2 Medium priority level labels Sep 15, 2020
@andrewhsu
Copy link
Member

from the spec sig mtg today, set priority to p1. @bogdandrutu to describe how this works in java. can then find an assignee.

@bogdandrutu
Copy link
Member

I completely agree with @rakyll, and that's why in Java I pushed to not have 2 different entries in the Context:

  1. Complicated logic for extracting the current span or span context to be used as parent when a new Span is created.
  2. Error prone, users may update one of the value and expect that to be the parent. E.g. for a Context with a Span if a new remote span context is added, that is not going to be used by the start span logic.
  3. Multiple things to care about for our users.

The implementation in Java is much simpler, we have only one entry in the Context for a Span that is always used as parent. For propagators we have a "propagation only" Span that can wrap any SpanContext (usually remote) and drops any other trace event added (attributes, events, etc).

This way we simplify the logic about things that are present in the Context and simplify the usage of our API.

@rakyll
Copy link
Contributor Author

rakyll commented Sep 17, 2020

I agree with @bogdandrutu. We should do what Java has done in the Go package. It's error prone and creates a lot of confusion what it means to have two entries especially given users can access to both via public APIs.

@tsloughter
Copy link
Member

I don't see how this can be done without having 2 entries in the context?

But I also don't see how it is confusing to users to have a remote extracted context, they shouldn't be touching it. Maybe that is the issue with Go's implementation?

In the Erlang impl we use a separate entry for the "external span context". The propagators extract the trace context, if it exists, to this entry and then it is looked at as the parent if there is no local parent.

The spec is clear that if no local or explicit parent is given then the remote parent is checked for.

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 area:span-relationships Related to span relationships priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:context Related to the specification/context directory spec:trace Related to the specification/trace directory
Projects
None yet
7 participants