-
Notifications
You must be signed in to change notification settings - Fork 833
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
Implement spec change to only accept Context as span parent. #1611
Merged
anuraaga
merged 18 commits into
open-telemetry:master
from
dynatrace-oss-contrib:feature/contextparent
Sep 25, 2020
Merged
Changes from 15 commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
8154b32
Prototype for Context as span parent.
Oberon00 a7fccf4
Simplify ZipkinSpanExporterEndToEndHttpTest.
Oberon00 e102def
Minimize PR diff.
Oberon00 5500b81
Make Codecov happy.
Oberon00 0eb9829
Fix overcomplicated DefaultTracer.
Oberon00 e152a50
Merge remote-tracking branch 'upstream/master' into feature/contextpa…
Oberon00 a6308f5
Merge remote-tracking branch 'upstream/master' into feature/contextpa…
Oberon00 66b6130
Fix build after merge.
Oberon00 2221c2a
Merge branch 'master' into feature/contextparent
Oberon00 c24deaf
Merge branch 'master' into feature/contextparent
Oberon00 30fbaf7
Fix build after merge.
Oberon00 255cd9c
Merge remote-tracking branch 'upstream/master' into feature/contextpa…
Oberon00 2253f8d
Revert storage of full Context.
Oberon00 2e275b2
Merge branch 'master' into feature/contextparent
Oberon00 e8dc898
Lint.
Oberon00 9f618e0
Clean up OpenTracing SpanBuilderShim.
Oberon00 6eea962
Rename Context -> RequestHandlerContext in testbed.
Oberon00 332ce25
Merge remote-tracking branch 'upstream/master' into feature/contextpa…
Oberon00 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
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
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
Oops, something went wrong.
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.
youch. this API sure is ugly! Remind me again how this is going to make things easier/better for our users?
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.
This is a unit test. Normally you would just call setNoParent.
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.
fair enough, but I think having to do parenting via the TCU is going to be pretty counter-intuitive to most users. :(
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 fully agree, but at the same time, I don't think users should normally do that. Usually you would just use the implicit current parent, or sometimes you need to capture a context with
Context.current()
and forward that to somewhere else. If you start a span at one thread and need to continue it at another thread while not setting it as active in the current thread is the only place you would need TracingContextUtils. I also think there should be some may to make that more easy. I could imaginespan.inCurrentContext()
(Java 8 default methods?), orTracingContextUtils.spanInCurrent(span)
to make this easier.As you said, using TracingContextUtils is rather unintuitive, and thus I think most users did not do it, they just passed around spans without the remaining context. However, a custom propagator should be able to set a value in the context in extract and rely on it still being available in inject.
BTW, using
inject
is a place where you already have to often muck around with TracingContextUtils currently.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.
Is it safe to assume the implicit current parent is accurate?
For instance, when dealing with asynchronous methods the thread being executed on is usually not the same thread that might have initiated/received the original request. In that situation, the implicit current parent from
ThreadLocal
is more than likely not the parent you wantThere 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.
Maybe we can provide some convenience function (default interface implementation?), so one can do
Context capturedParent = span.withCurrentContext()
. But it would be interesting how common the cases where you would need this even are.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.
Personally, it's a lot messier, but more importantly way less obvious that that's what is needed to achieve the desired result.
With respect to a default interface implementation, that wouldn't be possible with the JDK 7 requirement, correct?
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.
(FWIW, we need more convenience functions in
TracingContextUtils
, or in whatever new name it ends up having ;) )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.
@kenfinnigan Java 7 was dropped recently, Java 8 is now required.
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.
Thanks @Oberon00, don't follow things for a few weeks, and everything changes!