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

[carlosalberto]

Fixes #428

Changes

Have the global Propagators default to a no-op instance. The change itself is small but the side effects are important. This change is proposed as:

• Some APIs may not be able to provide propagation without a SDK.
• Keep it simple ("least surprise property").
• Avoid dealing with any unexpected, non-obvious corner case.

Also see this comment

Note: There was initial, overall agreement on also having the TraceContext propagator moved out of the API, but will do that in a separate PR as a follow up.

cc @dyladan

[dyladan]

Observe that API/SDK will still be expected to contain a default implementation of TraceContext, B3 and Zipkin

Does this mean it will be contained in the API or the SDK?

[Oberon00]

I think either the SDK or a separate propagator pacakage that only depends on API+context layer? A propagator implementation is no API, so I see no reason for it to be there.

[carlosalberto]

A propagator implementation is no API, so I see no reason for it to be there.

Observe that TraceContext propagators are usually in API, based on what I observed from a few languages. Other propagators will probably land in either the SDK or a contrib/extension package.

[Oberon00]

Did these languages also have it as the default API-only propagator?

Anyway, this reminds me that we should really have a document explaining the goals of the API / SDK split (I'm not sure there is an agreed set of these currently) and guidelines to decide what to put where.

[dyladan]

A propagator implementation is no API, so I see no reason for it to be there.

Observe that TraceContext propagators are usually in API, based on what I observed from a few languages. Other propagators will probably land in either the SDK or a contrib/extension package.

It's odd to me that a propagator would be the one piece of actual functionality in an otherwise totally no-op api.

[carlosalberto]

It's odd to me that a propagator would be the one piece of actual functionality in an otherwise totally no-op api.

The idea, IIRC, is that users can do inter-process code propagation with the API alone if they want.

[Oberon00]

This would also be possible with the propagator being in a separate artifact. But I agree that would be more complicated, although it varies by language how much boilerplate, etc. it is to create a separate artifact.

[carlosalberto]

Did these languages also have it as the default API-only propagator?

In Java/Python they do, for a start.

[SergeyKanzhelev]

In .NET propagation is enabled when either logging or tracing is enabled. At any verbosity. There may be no tracing exporter configured, but IDs will be propagated. So error logs can be correlated with the originating request on front end service. This also simplifies monitoring using service mesh as critical-only verbosity logs are configured on many .NET apps out of the box.

I would consider the .NET implementation to be a legit implementation of OpenTelemetry so I would suggest to add a language to proposed change that either default propagator is OK.

Furthermore, I would advocate to include propagators to API so longer term both scenarios - logs correlation and service mesh-based monitoring would work out of the box for any application. I believe we can get to the level of expectations when not propagated headers would be a surprising behavior.

That's said if there is a strong pushback - it's OK to migrate propagators to API in later versions. But please, include the language in specification that allows default propagator to be W3C one.

[Oberon00]

Default if SDK is included is another thing, but the API must be completely no-op by default IMHO.

[yurishkuro]

I think baggage propagation should be on by default, even in the absence of any SDK.

[Oberon00]

I don't think we have baggage (in the OpenTracing sense) even specified yet.

[yurishkuro]

baggage/correlation context. They are independent of any telemetry reporting.

[Oberon00]

I was under the impression that Correlation context was write-only for the application, and only to be used to associate labels with metrics and attributes with spans? But maybe I got it exactly wrong?

[anuraaga]

Can someone describe what it means exactly to do propagation without any SDK? Generally the actual propagation is implemented in instrumentation which relies on having SDK, for example the servlet instrumentation extracts from headers, OkHttp client instrumentation writes to headers. I can only imagine that without an SDK, a user would be calling the API methods directly to read / write baggage with their (presumably non-instrumented) server/client. Is there even a concept of "on by default" for this case?

[carlosalberto]

Generally the actual propagation is implemented in instrumentation which relies on having SDK

Instrumentation only needs an API to work, and actually it would be a bad idea to have it rely on any specific SDK implementation.

[anuraaga]

Instrumentation only needs an API to work

Ah sorry, I keep on forgetting this - I think it's because I just can't shake the weird feeling of depending on, e.g., opentelemetry-instrumentation-grpc but it only doing propagation without actually instrumenting unless the SDK is also brought in. Anyways, it's not related to this PR.

[carlosalberto]

But please, include the language in specification that allows default propagator to be W3C one.

What about having the TraceContext propagator in the API without being installed, and have either the user manually or some configuration/util library 'automatically' install it? Not enough for your requirements?

I personally don't like the idea of having different defaults in different languages, as it may create confusion, so I'd try to avoid that if possible 😉

[carlosalberto]

Ping @SergeyKanzhelev ;)

[SergeyKanzhelev]

@carlosalberto I don't think we can eliminate different defaults at this stage. In .NET many components are pre-instrumented. In other languages, user needs to install additional libraries to instrument components.

What the text was saying is that once something is instrumented - context have to be propagated. So if some library is pre-instrumented, it is automatically a part of a distributed tracing. So it will send Trace Context to the backend that can be used to correlate things. If language/component is not instrumented, there is an explicit step to enable API. I have doubts anybody will enable API without the desire to get Trace Context (even without SDK). Since it was an explicit step - there shouldn't be much concern.

Blocking PR for now.

WDYT?

[carlosalberto]

@SergeyKanzhelev Ping ;)

[SergeyKanzhelev]

The feedback specifically for this PR is:

• from @Oberon00: expand the definition of the user.
• from me: we should allow languages to decide on global propagators. There is no need to limit what languages will decide to do since we already have examples of this working.
• from me, @bogdandrutu and @yurishkuro: Why no-op shouldn't proppogate? What breaks if this is required?

@carlosalberto @Oberon00 and @jkwatson seems to be voting for this PR.

@carlosalberto if you want to move this PR forward, I'd suggest to re-word it into:

• libraries instrumented with OpenTelemetry API MUST call into propagation APIs to inject and extract context even when no SDK present or monitoring disabled. Default propagators implementation may be either no-op or propagation using TraceContext.
• there should be a way for application owner disable propagation of context. Languages must define either global switch or per-library.

I believe this will satisfy all feedback.

[Oberon00]

Default propagators implementation may be either no-op or propagation using TraceContext.

I really think we have to decide on either one or the other (per situation), and I think at least without an SDK this must be no-op.

[SergeyKanzhelev]

Default propagators implementation may be either no-op or propagation using TraceContext.

I really think we have to decide on either one or the other (per situation), and I think at least without an SDK this must be no-op.

Can you please elaborate on a reasoning to make a definitive decision here? I see languages and instrumentation patterns as being too different. Also, with the introduction of logging we will have more information on this as we might (?) want to use context for logging even without tracing enabled. But would it even be possible in different languages as logging APIs/consumers are very language specific - based on existing loggers.

So I think postponing this decision for now is OK. It can always be adjusted in future versions.

[carlosalberto]

@Oberon00 Added a note on the fact that any component (other than the API) is allowed to call Set Global Propagator (this makes much more sense than restricting it to the application developer IMHO).

@SergeyKanzhelev Added a note on the fact that .Net is an exception and will provide a set of default Propagators. Wondering if you know exactly what Propagators will be used (could add that as documentation); but more importantly, was wondering if you want me to add an explanation on why this is needed (if YES, please propose a sentence or two on the matter).

cc @reyang

[SergeyKanzhelev]

I don't think that suggested phrasing reflects the feedback given. Let me add more comments

[SergeyKanzhelev]

@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.

[carlosalberto]

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

[carlosalberto]

Ping @SergeyKanzhelev

[carlosalberto]

@open-telemetry/specs-approvers Please review/approve this PR. It has been standing idle for a week now, and although I'm waiting for feedback from @SergeyKanzhelev before moving on, general approval needs to happen as well.

[arminru]

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

[carlosalberto]

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

[carlosalberto]

Ping @SergeyKanzhelev @reyang

[reyang]

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

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[arminru]

@carlosalberto Can we close this as there seems to be consensus on #930?

[carlosalberto]

Let's go.