Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Have the global Propagators default to a no-op instance. #721

Closed
wants to merge 9 commits into from
Closed

Have the global Propagators default to a no-op instance. #721

Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5702786
Have the global Propagators default to a no-op instance.
carlosalberto Jul 20, 2020
d2c7bc2
Merge branch 'master' into global_propagators_initialvalue
carlosalberto Aug 22, 2020
aa69021
Clarify that any component can override the Global Propagators.
carlosalberto Aug 22, 2020
380d1ca
Add a note on the OTel .Net exception.
carlosalberto Aug 22, 2020
9049246
Merge branch 'master' into global_propagators_initialvalue
carlosalberto Aug 23, 2020
ce7047d
Update specification/context/api-propagators.md
carlosalberto Sep 1, 2020
b236789
Update specification/context/api-propagators.md
carlosalberto Sep 1, 2020
af5c4f1
Merge branch 'master' into global_propagators_initialvalue
carlosalberto Sep 1, 2020
c829b9e
Merge branch 'master' into global_propagators_initialvalue
carlosalberto Sep 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions specification/context/api-propagators.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,11 +249,13 @@ Required arguments:
Implementations MAY provide global `Propagator`s for
each supported `Propagator` type.

If offered, the global `Propagator`s should default to a composite `Propagator`
containing the W3C Trace Context Propagator and the Correlation Context `Propagator`
specified in [api-correlationcontext.md](../correlationcontext/api.md#serialization),
in order to provide propagation even in the presence of no-op
OpenTelemetry implementations.
If offered, the global `Propagator`s MUST default to a no-op instance.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@carlosalberto in this PR are you advocating for code cleanliness or scenario? This may be the root of the misunderstanding. I think myself, @yurishkuro and @bogdandrutu all advocating for a long term goal of having context ambiently propagated everywhere. It may be that the extensions you mentioned below would be the mechanism of this enablement, but I believe this context should be everywhere.

For example, to explain better what happens in .NET. - there is no context propagation when nothing is enabled. But even smaller level of logging enablement (which is default) will enable context propagation. We clearly recognize some people want to turn it off, But default is ON.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the lack of context. A week ago, in the Specification SIG call (at least @bogdandrutu was there), we came to an agreement on having the API with no-op Propagators by default (.Net will keep its current behavior, given the specific needs of how it's used in the .Net framework and how things are instrumented by default).

But even smaller level of logging enablement (which is default) will enable context propagation

Oh, actually this is what I tried to convey when I mentioned OpenTelemetry extensions (same in spirit, although not quite the same).

cc @bogdandrutu @yurishkuro

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping @SergeyKanzhelev

In order to override the default value, an call to
[Set Global Propagator](#set-global-propagator) MUST be done
by either SDK implementations, OpenTelemetry extensions or final application code.
carlosalberto marked this conversation as resolved.
Show resolved Hide resolved
carlosalberto marked this conversation as resolved.
Show resolved Hide resolved

Note: OpenTelemetry .NET is an exception to this rule as his global
carlosalberto marked this conversation as resolved.
Show resolved Hide resolved
`Propagator`s default to a set of specific `Propagator`s, including `TraceContext`.
Comment on lines +277 to +278
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to reference the .NET documentation giving more details on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@reyang @SergeyKanzhelev Any 'official' documentation we could mention here for the .NET case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping @SergeyKanzhelev @reyang

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is here https://docs.microsoft.com/dotnet/api/system.diagnostics.activitycontext.


### Get Global Propagator

Expand Down