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.
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
Implement spec change to only accept Context as span parent. #1611
Changes from 4 commits
8154b32
a7fccf4
e102def
5500b81
0eb9829
e152a50
a6308f5
66b6130
2221c2a
c24deaf
30fbaf7
255cd9c
2253f8d
2e275b2
e8dc898
9f618e0
6eea962
332ce25
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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!