Comments (13)
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 sunsettingTextMapPropagators
- 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
- don't break
- 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 ofcorrelation-context
) - I will check
- when user sets
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!
-
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. -
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.
Related Issues (20)
- Derived method 'TryCreateLogger' in type 'OpenTelemetry.Logs.LoggerProviderSdk' from assembly 'OpenTelemetry, Version=1.0.0.0, Culture=neutral, PublicKeyToken=7bd6737fe5b67e3c' cannot reduce access. HOT 12
- Milliseconds bucket HOT 1
- [docs] [metrics] Return the old exemplars sample
- Manipulate Scope Storage
- OpenTelemetry-dotnet: Support for true Delta UpDownCounters & ObservableGauges
- [repo] Investigate enabling NuGetAudit
- Proposal: OTel SDK should expose a metric to inform about sampling decisions made HOT 3
- Compiling code HOT 5
- CLOMonitor - verify recommendations
- Allow Exporter to export a protobuf service request directly
- [feature request] Implement http/json in OtlpExporter HOT 1
- Log level per exporter? HOT 2
- Testing BaseProcessor based filters HOT 5
- Log message is not formatted HOT 2
- Invalid Characters in "{OriginalFormat}" when using OTLPExporter HOT 1
- [zipkin\otlp] Call SendAsync on mobile
- Proposal for Stale issue automation HOT 6
- [metrics] Optimize lookup costs using .NET 9 alternative lookup
- Async BatchExportProcessor HOT 1
- Poor Histogram Support for the Prometheus Exporter HOT 3
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
π Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. πππ
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google β€οΈ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from opentelemetry-dotnet.