-
Notifications
You must be signed in to change notification settings - Fork 887
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
Spans's parent must be passed as Context instead of Span(Context)s. #875
Merged
carlosalberto
merged 17 commits into
open-telemetry:master
from
dynatrace-oss-contrib:contextparent
Sep 23, 2020
Merged
Changes from 13 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
03fa286
Spans must have parent Contexts instead of parent Span(Context)s.
Oberon00 d572726
Add CHANGELOG.
Oberon00 2b5182b
Fix links, TOC, add note for ReadableSpan.
Oberon00 14c9ad7
Lint.
Oberon00 5a6958f
Merge branch 'master' into contextparent
Oberon00 14210f7
Merge branch 'master' into contextparent
Oberon00 b6062b8
Merge branch 'master' into contextparent
Oberon00 a7e7065
Remove storage of Context.
Oberon00 e6e5d97
Add Context to OnStart.
Oberon00 18a66aa
Remove misplaced paragraph.
Oberon00 224531d
Wording.
Oberon00 aa67440
Whitespace for lint.
Oberon00 cf57fdd
Typo.
Oberon00 0f7d514
Merge branch 'master' into contextparent
Oberon00 f98a516
Revert no longer needed "effective span" wording.
Oberon00 ad4a72f
Revert changes to "a span encapsulates..."
Oberon00 6db9535
Merge branch 'master' into contextparent
arminru File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also allow just
Span
in case people are not using ContextThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should people not use context? It is possible to support this but it would introduce a complication. We would have to specify: "If a Span is passed as Context, the parent Context of the child span is the parent Context of the parent span (i.e., the grandparent Context) with the Span in it replaced with the parent Span".
EDIT: As noted in the sdk part of this PR, a span can be in multiple contexts, therefore it is impossible to map a Span back to a Context. Thus passing a Span instead of a Context implies data-loss.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In JS I know it is a problem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also in some cases you start an operation and directly interact with the Span.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Context is already the only way to inject/propagate a Span. So both of the problems you mention are necessarily already solved by all SIGs that implement the OTEP66 propagation spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Oberon00 believe me or not, I saw lots of cases where just
Span
could be provided (no longer work at Google to show more examples). I do believe this is a mistake to not support Span as well. Don't know how to convince besides waiting for a language to implement this and make all changes to all the instrumentations to see there will be cases where this is not possible.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In these cases, you can also not inject. Or rather, you can by using something like TracingContextUtil.currentContextWithSpan. This at least makes it more explicit that you are dropping information (and more cumbersome, which I think is OK here, as it nudges you to preserve the full Context).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree with this and think it should be only a Span/SpanContext. Making the parent a Context means there is an association between the Span and the Baggage. I may be wrong but my understanding was that they are separate concerns.
If the issue is returning to the parent but not losing the baggage (or anything else in the context) then the fix should be to not reset the entire context.
In the Erlang implementation the Context contains a list of SpanContexts. When the current span is ended it is popped from the head of the list, the rest of the Context remains unchanged.
You could call the list of SpanContexts the TraceContext, which is just one part of the total Context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding was that while they are separate concerns, we do want to be able to cross-reference and connect different concerns, and the Context should be the place where everything comes together. That was how I understood OTEP66, and also our current propagator spec. But I guess this needs a decision. If we don't want this connection (#867, #381 seem to imply we do), then I agree that this PR is not the way we should go and @anuraaga's idea of extending SpanContext is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, I will self apply the rule of "minimal api" :). We know for sure we need the "context" parent so it is great, for the "Span" parent we may or may not have good use-cases, so maybe add it later if that is the case.
So happy for me to start with only accepting Context for this because we can add later the "Span".