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
Small clean up for Propagators. #577
Small clean up for Propagators. #577
Changes from 7 commits
971168e
445c105
0dce0df
33673c5
93e44dc
9bd2f0d
a2639e3
02764e4
f99a88f
809fa6c
65eebac
8b31eb5
250967c
b44e83c
46fd93c
4c6dd1f
58af0eb
bdb834f
8d97361
48f3b06
a390037
dc659c9
1297d08
d4022d7
b69ef73
c4d4227
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.
I would remove this sentence. There's nothing difficult in calling
Propagator.inject(Context.current())
instead of expecting propagator to callContext.current()
, and passing the context explicitly allows to decouple the propagator from the implementation details of the context, i.e. it would work with explicitly propagated context just as well, without depending on thread-local model.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.
On the other hand, you would then couple all the instrumentations to the "implementation details" of the context layer which seems way worse.
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 don't think it's worse. From what we've seen in OpenTracing in Python world, which had a lot of different async frameworks, or even in Java when looking at more Rx-like space, it is pretty difficult to disentangle the instrumentation from specific context propagation framework being used. Thread-local is only one option among many, which would be forced onto everyone if the API allows not passing in the Context. Also, we can always build an additional abstraction above the context layer if we want instrumentation to be really agnostic, without coupling Propagators to thread-locals.
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.
When reading the next paragraph, what does this one add? It seems completely redundant, or I don't understand it's intended meaning.
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.
Let's remove it for now - we can add it later with a proper, longer explanation if needed.
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 paragraph seems useless. If HttpTextPropagator is a propagator, that seems obvious.
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 find this Fields section very confusing and not well-worded (e.g. what does "Returns..." refer to?) Let's revisit it in another PR. #712