Change default propagator in unconfigured API #930
Conversation
|
|
||
| OpenTelemetry API MUST use no-op propagators when used unconfigured. Context | ||
| propagation may be used for various telemetry signals - traces, metrics, logging | ||
| and more. Platforms such as ASP.NET may pre-configure out-of-the-box |
There was a problem hiding this comment.
Platforms such as ASP.NET
So .NET without ASP will work differently?
Co-authored-by: John Watson <jkwatson@gmail.com>
Co-authored-by: John Watson <jkwatson@gmail.com>
Co-authored-by: John Watson <jkwatson@gmail.com>
Co-authored-by: John Watson <jkwatson@gmail.com>
Co-authored-by: John Watson <jkwatson@gmail.com>
Co-authored-by: John Watson <jkwatson@gmail.com>
Co-authored-by: John Watson <jkwatson@gmail.com>
Co-authored-by: John Watson <jkwatson@gmail.com>
Co-authored-by: John Watson <jkwatson@gmail.com>
Co-authored-by: John Watson <jkwatson@gmail.com>
|
Overall LGTM. @open-telemetry/specs-approvers please review. |
| specified in [api-baggage.md](../baggage/api.md#serialization), | ||
| in order to provide propagation even in the presence of no-op | ||
| OpenTelemetry implementations. | ||
| The OpenTelemetry API should provide a way to obtain a propagator for each supported |
There was a problem hiding this comment.
Actually I'd prefer to have this as a MAY - we still need to decide whether TraceContext and Baggage Propagators will be in the API or in a separate package (the latter seemed to have slightly more approvals).
There was a problem hiding this comment.
I'd go with MUST. Not much use of API if there is no way to propagate context. @carlosalberto are you thinking of the long term vision where context just floats without the need for any API? Is it why you suggest "MAY"?
There was a problem hiding this comment.
Fine with going with MUST, as this is what we have right now. Observe that we already specified that, at least for B3 and Jaeger, the Propagators ought to live in extension packages - so you may need to be concise and mention Baggage + TraceContext here.
There was a problem hiding this comment.
this is about API surface. API must provide a means to help with extracting and injecting the context without the need to import special propagation package.
|
Ping @SergeyKanzhelev |
Is this ping for the comment from @Oberon00? What else is blocking? I see it's mostly blocked on number of reviews
|
|
@SergeyKanzhelev Mostly on @Oberon00's feedback ;) That being said, having it all resolved will hopefully motivate people to review this (and with @Oberon00's approval, we can technically merge this). |
|
Ping @Oberon00 Please review again ;) |
|
@SergeyKanzhelev Please rename the PR to "Change default propagator in unconfigured API" or something like that as the current title makes it sounds like this would just add a non-normative side note 🙂 |
| supported `Propagator` type. Instrumentation libraries SHOULD call propagators | ||
| to extract and inject the context on all remote calls. Propagators, depending on | ||
| the language, MAY be set up using various dependency injection techniques or | ||
| available as global accessors. |
There was a problem hiding this comment.
What's meant by "global accessors"? Via global getters?
We could also more generically say "available as global properties on the API".
There was a problem hiding this comment.
I think what's written is also generic enough. Let me know if this is blocking
There was a problem hiding this comment.
Not blocking (I already approved the PR) but a bit unclear to me.
Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
SergeyKanzhelev
changed the title
|
We got enough approvals and in general there was agreement in a pair of Spec SIG calls. Will wait till early tomorrow to merge this ;) |
Fixes #428
Alternative to #721
Changes
Default propagators in un-configured API must be no-op