-
Notifications
You must be signed in to change notification settings - Fork 889
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
Initial composite/global Propagators update for OTEP 66. #440
Initial composite/global Propagators update for OTEP 66. #440
Conversation
Co-Authored-By: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-Authored-By: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-Authored-By: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-Authored-By: Tyler Yahn <MrAlias@users.noreply.github.com>
@open-telemetry/specs-approvers please take a look at this one too ;) |
Co-Authored-By: Armin Ruech <armin.ruech@gmail.com>
Co-Authored-By: Armin Ruech <armin.ruech@gmail.com>
Co-Authored-By: Armin Ruech <armin.ruech@gmail.com>
in order to provide propagation even in the presence of no-op | ||
OpenTelemetry implementations. | ||
|
||
### Get Global Propagator |
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.
Do we want one global propagator (which may be composite), or do we want to add the propagator to the TracerProvider? Reason being that if you have multiple apps in a single runtime, each of which has its own tracer provider, you may want to use different propagation formats between apps.
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.
To me doing that sounds like an overkill for now - in practice, it looks like most people want a single place/component to do the entire injection/extraction.
We could add in the future an additional Propagator
component to items such as TracerProvider
and similar components (defaulting to null
) in case users want to (optionally) override the global one, though 😃
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.
How should we be calling the propagator?
api.propagator.extract(...)
tracer.extract(...)
tracer_provider.extract(...)
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.
You should be using 1) as this will be the 'default' thing - we might (but we might not) add 3) as a custom, override thing, in which case we could call that one as well.
The second option is due to be removed ;)
- The carrier that holds propagation fields. | ||
- The instance of `Getter` invoked for each propagation key to get. | ||
|
||
### Inject |
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.
Are we going to specify whether inject mutates the carrier or returns a new one?
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.
So currently the carrier
that is passed is in turn passed to the Setter
instance, which decides what to do with it along with the keys/values. In practice (99% of the time?) the carrier is indeed mutated, but not 'enforced'.
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 was going to mark this as resolved, but for some reason don't have the button available like in the spec repo. Are permissions different here?
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.
The Carrier APIs should be defined earlier in the doc. They are generally mutable by definition, although I could see how this could irk some FP-oriented people. It is rather difficult to make individual pieces immutable and composable, because the Carrier API fundamentally implies some transformation that is unknown to the propagator, like writing every key-value pair as HTTP header is a specific implementation of HTTPRequest.
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.
(well, it's actually not THAT difficult, using some pipeline like propagators.map(_.stateAsMap(context)).flatMap(carrier.accumulate)
,
but it's rather inefficient).
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.
@dyladan You need to be the author who opened the PR or have write permissions to the repo (i.e., be in spec-approvers) to resolve conversations.
@@ -62,7 +75,7 @@ Injects the value downstream. For example, as http headers. | |||
|
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 do not follow anything in the preceding Fields section. It seems very vague and random. And certainly does not belong specifically under the HTTP format.
|
||
- A `Context`. | ||
- The carrier that holds propagation fields. | ||
- The `Setter` invoked for each propagation key to add or remove. |
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.
Why do we need to separate carrier and setter?
Hey @yurishkuro thanks for the feedback. I've fixed the issues that are strictly related to this PR, and have created #453 to keep track the changes Please re-review and let me know ;) |
…try#440) * Initial Propagators update for OTEP 66. * Fix style. * Initial feedback pass. * Update specification/api-propagators.md Co-Authored-By: Tyler Yahn <MrAlias@users.noreply.github.com> * Update specification/api-propagators.md Co-Authored-By: Tyler Yahn <MrAlias@users.noreply.github.com> * Update specification/api-propagators.md Co-Authored-By: Tyler Yahn <MrAlias@users.noreply.github.com> * Update specification/api-propagators.md Co-Authored-By: Tyler Yahn <MrAlias@users.noreply.github.com> * Update specification/api-propagators.md Co-Authored-By: Armin Ruech <armin.ruech@gmail.com> * Fix the mixed up getter/setter. * Update specification/api-propagators.md Co-Authored-By: Armin Ruech <armin.ruech@gmail.com> * Update specification/api-propagators.md Co-Authored-By: Armin Ruech <armin.ruech@gmail.com> * Do not use RPC in api-propagators * Use Format instead of format. * Clarification on format in overview.md Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> Co-authored-by: Armin Ruech <armin.ruech@gmail.com>
…try#440) * Initial Propagators update for OTEP 66. * Fix style. * Initial feedback pass. * Update specification/api-propagators.md Co-Authored-By: Tyler Yahn <MrAlias@users.noreply.github.com> * Update specification/api-propagators.md Co-Authored-By: Tyler Yahn <MrAlias@users.noreply.github.com> * Update specification/api-propagators.md Co-Authored-By: Tyler Yahn <MrAlias@users.noreply.github.com> * Update specification/api-propagators.md Co-Authored-By: Tyler Yahn <MrAlias@users.noreply.github.com> * Update specification/api-propagators.md Co-Authored-By: Armin Ruech <armin.ruech@gmail.com> * Fix the mixed up getter/setter. * Update specification/api-propagators.md Co-Authored-By: Armin Ruech <armin.ruech@gmail.com> * Update specification/api-propagators.md Co-Authored-By: Armin Ruech <armin.ruech@gmail.com> * Do not use RPC in api-propagators * Use Format instead of format. * Clarification on format in overview.md Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> Co-authored-by: Armin Ruech <armin.ruech@gmail.com>
This is an initial PR focusing on updating the
Propagator
s API, and is meant to be get initial feedback, rather than getting merged right away. This is because related PRs are still not merged, which hopefully will change soon ;)Overall:
Propagator
concept.