Propagators Support

[tarekgh]

Fixes #50658

[dotnet-issue-labeler]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Fixes #50658

Author:	tarekgh
Assignees:	-
Labels:	area-System.Diagnostics.Tracing, new-api-needs-documentation
Milestone:	-

[tarekgh]

@noahfalk @MihaZupan @shirhatti

[shirhatti]

Yaay!!! 🥳

[MihaZupan]

A few comments, looks good otherwise

[stephentoub]

Is "Output" the right naming to use? Would it be better to keep with the naming used elsewhere in the API, e.g. "NoInjection" or something like that?

[tarekgh]

This was discussed before. Actually, it was originally proposed with the Inject term instead of Output.

The reason was the terms Inject and Extract are coming from the specs and the APIs has names including these terms will be used in the low-level libraries (e.g. Http Client and asp.net). It is not expected many people call such APIs. While the APIs like CreateNoOutputPropagator will be called for configuration which be done by users who really don't care how the propagators work and even not exposed to Inject/Extract operations. That is why it was preferred to use Output for simplicity for such users.

[noahfalk]

Looks good to me, its mostly suggestions on the comment text + one corner case where Activity parent/child aren't required to match formats.

[stephentoub]

For what it's worth, I still think we're making a mistake with the naming here. We design .NET APIs to fit well in the .NET ecosystem; the fact that some of these names map to what's used by OpenTelemetry is going to be lost on the vast majority of folks that use this, and they'll be stuck with this "TextMapPropagator"-named thing that doesn't connect in any named way to the rest of the Activity-based system it's meant to work with. This is only going to get worse as we start considering proposals like @MihaZupan's where there's talk of adding a SocketsHttpHandler.TextMapPropagator, where it is actually in developer's faces. We should really reconsider the naming here.

cc: @bartonjs, @terrajobst

[tarekgh]

I am still thinking sticking with the spec naming is better here for the following reasons:

• The propagator APIs even if it is used in http client wouldn't be used much and I expect very few people would use it. These people would be familiar with the propagator's concepts. So, the name wouldn't make a significant difference for such users.
• I understand the .NET ecosystem point but we should also consider the bigger ecosystem across languages/technologies. Any dev migrating from other languages to .NET would find this easier instead of trying to map the specs to the type which wouldn't be obvious.
• TextMapPropagator is not bad name at the end as it is defined in the spec TextMapPropagator performs the injection and extraction of a cross-cutting concern value as string key/values pairs into carriers that travel in-band across process boundaries.. So, it is about strings (which TextMap make sense). In the future, it is expected to get a different propagator types. e.g. binary propagators.

CC @reyang

[stephentoub]

the name wouldn't make a significant difference for such users

If it shows up in the public surface area on SocketsHttpHandler, it'll be there in IntelliSense for everyone to see and have to comprehend, regardless of whether they actually need to use it, as they'll need to decide whether or not they need to use it.

Any dev migrating from other languages to .NET would find this easier instead of trying to map the specs to the type which wouldn't be obvious.

Any dev migrating from other languages will need to understand a vast number of differences, in and out of tracing. This is a drop in the bucket, and having one thing that's named based on a different ecosystem helps only minimally for a vast minority of developers while harming the experience of the vast majority.

TextMapPropagator is not bad name at the end as it is defined in the spec

In .NET, we no longer use "map" for such things; the only artifact of that is Hashmap, with everything newer being Dictionary in collections, Select in LINQ, etc. And if it's about headers, it should say so. If it's about Activity, it should say so. ActivityHeaderPropagator or some such thing is way more relatable than TextMapPropagator, which, to me at least, is meaningless.

[tarekgh]

If it shows up in the public surface area on SocketsHttpHandler, it'll be there in IntelliSense for everyone to see and have to comprehend, regardless of whether they actually need to use it, as they'll need to decide whether or not they need to use it.

That is right but I am not expecting anyone will use it just because it show-up there. Whatever name used there will not make a lot of difference if the user of the API has no prior knowledge what propagators are in general.

Any dev migrating from other languages will need to understand a vast number of differences, in and out of tracing. This is a drop in the bucket, and having one thing that's named based on a different ecosystem helps only minimally for a vast minority of developers while harming the experience of the vast majority.

That is right but it doesn't mean we make the naming differences bigger. I am seeing it is better to reduce the name differences if we can. We are trying to converge with the specs.

In .NET, we no longer use "map" for such things; the only artifact of that is Hashmap, with everything newer being Dictionary in collections, Select in LINQ, etc. And if it's about headers, it should say so. If it's about Activity, it should say so. ActivityHeaderPropagator or some such thing is way more relatable than TextMapPropagator, which, to me at least, is meaningless.

ActivityHeaderPropagator is not the right name. Although we have one API takes Activity, but we are trying to not restrict it to Activity usage. It is expected in the future to add more APIs which not related to Activity. Propagators can be used in other scenarios not tied with Activity at all (for example can be used with Metrics if needed to propagators key-value labels). The name should be generic enough and not restrict it to specific narrow scenario.

[noahfalk]

I spent a little time looking around online for the top search results relating to the OpenTelemetry concept:

• More than half the pages I found refer to 'Propagator' or 'Http Propagator' and never mention 'TextMapPropagator'
• Rust docs talk about TextMapFormatter
• A minority of docs ( <25% ) refered to 'TextMapPropagator' exactly.
• I didn't notice any language where the user needs to write "TextMapPropagator" in their code in order to configure it. They might be creating an instance of a type that derives from TextMapPropagator, but that is an implementation detail they don't see unless they are deriving a custom propagator. In the common case they write something like (java example):

OpenTelemetrySdk sdk =
        OpenTelemetrySdk.builder()
            .setTracerProvider(sdkTracerProvider)
            .setPropagators(ContextPropagators.create(W3CTraceContextPropagator.getInstance()))
            .build();

My take is that as long as the word "Propagator" shows up in the name users are likely to understand how the concepts align. If the type was named something like "DistributedContextPropagator" devs would be more likely to understand its intent when writing DistributedContextPropagator.Current = bla or socketsHandler.DistributedContextPropagator = bla.

[bartonjs]

For what it's worth, I still think we're making a mistake with the naming here. We design .NET APIs to fit well in the .NET ecosystem; the fact that some of these names map to what's used by OpenTelemetry is going to be lost on the vast majority of folks that use this, and they'll be stuck with this "TextMapPropagator"-named thing that doesn't connect in any named way to the rest of the Activity-based system it's meant to work with.

I agree, and that's been my stance for everything to do with OpenTelemetry. My personal feeling is we should make sure all the concepts exist, make them work in a .NET-ty way, and then just write up a doc that maps the OpenTelemetry concepts to the .NET names.

I'd feel different if all of this was down in the weeds as just protocol/interop, but it's showing up in a lot of places.

[terrajobst]

There isn't a single .NET API where aligning with an outside spec has resulted in a better API. My position has always been that API specs are largely not that useful for the consumer. They only help the team that has to implement the API across multiple languages, which in my opinion isn't the right group of people to optimize for.

For an API to be successful, it is far more important to build an API that feels natural for the given platform they are provided for.

"But what about people who read the spec?" newsflash: most of our users don't read the documentation as a first step. They only consult them when stuff doesn't work. And very, very few people read actual specs.

If the type was named something like "DistributedContextPropagator" devs would be more likely to understand its intent when writing DistributedContextPropagator.Current = bla or socketsHandler.DistributedContextPropagator = bla.

DistributedContextPropagator works for me as the name.