Baggage propagation is not possible for instrumentations without depending on OpenTelemetry.Api package about opentelemetry-dotnet HOT 13 OPEN

It seems we're in agreement that we need to use DistributedContextPropagator and sunset OTel propagators. I can join .NET SIG tomorrow to discuss it further.

from opentelemetry-dotnet.

It seems we're in agreement that we need to use DistributedContextPropagator and sunset OTel propagators. I can join .NET SIG tomorrow to discuss it further.

Yes💯! It was always the plan, but never had the bandwidth to make it happen. @vishweshbankwar did some exploration, and should be able to assist as well.

I don't have strong preference over keeping OTel propagators as a thin wrapper around DistributedContextPropagator, similar to how TelemetrySpan API acts as a thin wrapper around Activity, on an opt-in basis. We will need to be fully back-compatible, as we are not planning major version bump to 2.0!

Anyway, these are details, and can be addressed in implementation phase.

from opentelemetry-dotnet.

sure, the summary:

• this is the problem for any instrumentation, including native ones coming in .NET 9
• there is a consensus on relying purely on DistributedContextPropagator and sunsetting TextMapPropagators
• the Otel-bridge implementation of DistributedContextPropagator needs to do some tricks
  • don't break Activity.Baggage + Correlation-Context (i.e. if someone uses it, it should keep working)
  • it has to populate Baggage.Current in extract methods
• ASP.NET Core caches the value of DistributedContextPropagator.Current
  • when user sets DistributedContextPropagator.Current, they control when it happens
  • when otel is built it might be too late and won't update the instance ASP.NET Core uses
    • I will check what can be done and reach out to ASP.NET Core to change things if necessary
    • Perhaps it's not too late to change how ASP.NET Core uses Activity.Baggage (maybe they can let users opt-out or configure baggage instead of correlation-context) - I will check

Assuming we can make things above work, there are no objections from going ahead with DistributedContextPropagator implementations, it was the plan, SIG just didn't get to it.
I should be able to work on it (but can't make time commitments beyond ASP.NET Core investigation in v9 timeframe).

from opentelemetry-dotnet.

DistributedContextPropagator.Current = new OTelBridgePropagator(otelPropagator); // this should be done by OTel SDK

@lmolkova FYI - This could run into some sequencing issues and may not work in all cases if set within Sdk. See https://github.com/open-telemetry/opentelemetry-dotnet/pull/3769/files#r1003730495.

Overall I think this intermediate step will also be needed for supporting OTel Baggage with native instrumentation coming in .NET9.0. (Assuming we don't get baggage API in .NET)

from opentelemetry-dotnet.

This could run into some sequencing issues and may not work in all cases if set within Sdk. See https://github.com/open-telemetry/opentelemetry-dotnet/pull/3769/files#r1003730495.

Good catch! This is not a new problem though. The Sdk.SetDefaultTextMapPropagator should also be called at the right time by the user.

Overall I think this intermediate step will also be needed for supporting OTel Baggage with native instrumentation coming in .NET9.0.

Not just baggage, but any non W3C context propagation - if users configured B3 on otel, native instrumentations need to be aware of this.

As an alternative, OTel may make OTel propagators implement DistributedContextPropagator, deprecate Sdk.SetDefaultTextMapPropagator and ask users to set DistributedContextPropagator.Current first thing after app startup (however ugly it is)

from opentelemetry-dotnet.

Overall I think this intermediate step will also be needed for supporting OTel Baggage with native instrumentation coming in .NET9.0.

Not just baggage, but any non W3C context propagation - if users configured B3 on otel, native instrumentations need to be aware of this.

This is already broken even today. The proposed native instrumentation in .NET 9 is about populating attributes (tags). Asp.Net Core/HttpClient are already natively doing the context propagation part, and they don't respect OTel Propagators. (this is compensated for by our instrumentation libraries, by sometimes doing the extreme of throwing away the Activity created from the asp.net core, and creating a fresh one following otel propagators, leading to poor perf!)

As an alternative, OTel may make OTel propagators implement DistributedContextPropagator, deprecate Sdk.SetDefaultTextMapPropagator and ask users to set DistributedContextPropagator.Current first thing after app startup (however ugly it is)

This makes sense. OTel's propagators must deprecate itself, and fully embrace DistributedContextPropagator from runtime. (just like OTel .NET decided to sacrifice its own API in favor of Activity API from runtime)

from opentelemetry-dotnet.

I'd be interested as to what came out of the discussion at the SIG, can someone detail that here?

from opentelemetry-dotnet.

this is the problem for any instrumentation, including native ones coming in .NET 9

They are no longer part of .NET 9 :(
dotnet/aspnetcore#52439 (comment)

from opentelemetry-dotnet.

Happy to contribute the change if there are no objections.

from opentelemetry-dotnet.

However there are two baggages:

System.Diagnostics.Acitivity.Baggage - legacy(?) API that uses Correlation-Context header (by default via default implementation of DistributedContextPropagator)
OpenTelemetry.Baggage.Current - OTel baggage available in OpenTelemetry.Api package.

Yes. A problem we've been facing from day1, thanks for looking into this!

1. Now that W3CBaggage spec has moved to be stable, this issue needs higher priority. We need to see if .NET Runtime is willing to offer a Baggage API (independent of Activity). Not likely to happen in .NET 9 timeframe, but .NET 10 can be targeted.

2. Also, OTel's own propagator API should be deprecated in favor of the ones coming from runtime. There were some investigations done on how to achieve this, but nothing solid yet.

Is the above proposal meant to short-term unblock libraries (like Azure SDK for example), to benefit from Baggage, without taking a dependency on OTel.API? While I don't object to that, I think we should solve the above 2 fundamental issues at the root, so there won't be a need of any workarounds. 2 is solvable just within OTel repo, but 1 needs runtime support (i don't think there is a tracking issue, so need to create one).

from opentelemetry-dotnet.

Since the problem in not going to be fixed in at least 1.5 years, I think workaround that does not introduce any public API and will disappear without the trace along with OTel propagators is quite reasonable.

It's not going to solve p1, but it will:

• make .NET native HTTP instrumentations inject the context using the propagator configured by users on OTel. I don't know how otherwise we can have native .NET instrumentations
• support baggage in non-HTTP instrumentations without dependency on OTel.Api

The core part of the above proposal is to do DistributedContextPropagator.Current = new OTelBridgePropagator(otelPropagator); in the OTel SDK whenever someone calls Sdk.SetDefaultTextMapPropagator (or when tracer provider is built with a default one).
So users can keep using either: Baggage.Current or Activity.Baggage (if that's what they did), there is no need to unify those APIs to make the propagator bridge work.

I'm also fine not adding any support for Activity.Baggage in the bridge propagator.

from opentelemetry-dotnet.

So users can keep using either: Baggage.Current or Activity.Baggage (if that's what they did), there is no need to unify those APIs to make the propagator bridge work.

mm.. Not so sure given there is nothing taking care of in-proc propagation. The propagator API tweak can only fix part of the problem by propagating baggages set via both Activity.Baggage and Baggage via "baggage" header(or whatever configured header). But within process, neither Activity.Current.Baggage nor Baggage.Current is aware of each other....

from opentelemetry-dotnet.

mm.. Not so sure given there is nothing taking care of in-proc propagation. The propagator API tweak can only fix part of the problem by propagating baggages set via both Activity.Baggage and Baggage via "baggage" header(or whatever configured header). But within process, neither Activity.Current.Baggage nor Baggage.Current is aware of each other....

Sure, but it seems the baggage API and propagation are two independent concepts, right?
So we can break this problem down in two separate things:

• how to add baggage, which API to use, etc - this is runtime issue to solve. Not sure if there an existing one already, happy to create.
• propagator implementation compatible with OTel and .NET - e.g. B3 DistributedContextPropagator should probably exist in OTel, but not necessarily .NET

from opentelemetry-dotnet.