Support W3C Baggage proposed standard in HttpHandlerDiagnosticListener

[jbogard]

The W3C Correlation-Context proposed standard was renamed to "baggage". As part of that rename, the header was renamed from Correlation-Context to baggage.

The details of the spec are in public working draft, which remain the same as Correlation-Context except with the header name change.

Since this is a potentially breaking change (server may be expecting the legacy name Correlation-Context, either this can be a clean break (remove/rename Correlation-Context to baggage), or it can be done in phases:

1. Populate both headers with same value
2. Remove legacy header name/value

Related: dotnet/aspnetcore#28319

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[jbogard]

@tommcdon I think this is probably yours, I didn't get the label magic to work. It's System.Diagnostics.DiagnosticSource.

[ghost]

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

Issue Details

---

The W3C Correlation-Context proposed standard was renamed to "baggage". As part of that rename, the header was renamed from Correlation-Context to baggage.

The details of the spec are in public working draft, which remain the same as Correlation-Context except with the header name change.

Since this is a potentially breaking change (server may be expecting the legacy name Correlation-Context, either this can be a clean break (remove/rename Correlation-Context to baggage), or it can be done in phases:

1. Populate both headers with same value
2. Remove legacy header name/value

Related: dotnet/aspnetcore#28319

Author:	jbogard
Assignees:	-
Labels:	area-System.Diagnostics.Tracing, untriaged
Milestone:	-

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

The W3C Correlation-Context proposed standard was renamed to "baggage". As part of that rename, the header was renamed from Correlation-Context to baggage.

The details of the spec are in public working draft, which remain the same as Correlation-Context except with the header name change.

Since this is a potentially breaking change (server may be expecting the legacy name Correlation-Context, either this can be a clean break (remove/rename Correlation-Context to baggage), or it can be done in phases:

1. Populate both headers with same value
2. Remove legacy header name/value

Related: dotnet/aspnetcore#28319

Author:	jbogard
Assignees:	-
Labels:	area-System.Net.Http, untriaged
Milestone:	-

[karelz]

Triage: We should do it once the spec is final. Is that the case already @jbogard?
cc @MihaZupan

[jbogard]

No, it is not. It moved into the first public working draft status back on October 20th, 2020. However, the Correlation-Context header behavior for Baggage was implemented before this standard was in public working draft status.

It's in an odd place - the name of the standard/header was renamed, and now in public working draft, but it was the "old" name, never standardized, that was implemented for .NET Core 3.0.

[karelz]

Oh, does it mean that we implemented a draft, that won't be part of the final spec?
We can either act right away, or wait for the spec to mature a bit more ... @MihaZupan what would you recommend?
Either way, we will have to deal with handling potential breaking change for customers -- are there mitigations we can put in place? (e.g. generating both headers)

[jbogard]

Yes - it was a draft implemented. The simplest way would be to generate both headers. It's different than the transition from Request-Id to traceparent, as that was the adoption of the W3C standard, and you could check the ActivityIdFormat. There's no similar check to be made here.

[nnnvvvcv]

I've reached this page after finding that Correlation-Context is being populated by default (I believe) in my Azure Function code based on baggage within the current Activity.

While I can see the advantage of this for systems where services are closely-coupled/in-house/public-domain I'm not so sure about defaulting it on for every HTTP request. Some requests will be going to 3rd parties where we definitely DON'T want details stored in Activity.Baggage going over the wire.

Maybe I'm just missing the configuration setting which allows me to enabled/disable this feature on a request-by-request or HttpClient basis.

[dominikjeske]

Lack of this functionality makes inconsistent behavior because Activity.Baggage is send via 'Correlation-Context' header and in ASP.NET Core this header is ignored (because change in this PR dotnet/aspnetcore@281679e#diff-f4b6e9bc19da99141d4427d88673ed12fdf3474082f571b027a583cb6959ca99 by @jbogard ) so baggage is lost.
In other hand when we use OpenTelemetry Baggage.SetBaggage (which sets 'baggage' header in instrumentation code) then it is read by ASP.NET core into Activity.Baggage - so on server side we have same data in Activity.Baggage and Baggage.Current and when we send another request then we will have Correlation-Context (from Activity.Baggage) and baggage (from Baggage.Current when OpenTelemetry instrumentation enabled).
Generally baggage implementation and usage is now very confusing.

[tarekgh]

@dominikjeske can't you use DistributedContextPropagator.Inject to inject the baggage with the desired header name?

[dominikjeske]

@tarekgh It is not a problem for me to send something with correct header - I can use OpenTelemetry to include Baggage and read it or I can do it manually with delegating handlers and custom middleware but this is not a point. I described two problems with default behavior which I find confusing:

1. One header used to send and other used to read
2. Header duplication when OpenTelemetry is used

ps. Is there a way do disable propagation of Activity.Baggage?

[mxwlf]

As of .NET 8, it is still the behavior to send the Correlation-Context header instead of baggage, even when Activity.DefaultIdFormat is set to W3C.
It is a good thing that ASP.NET supports both headers but this leaves other systems with potentially broken traceability. Have you come to a decision offline? Maybe supporting configuration to send both or another Format option for the legacy Correlation-Context header?

[dominikjeske]

Unfortunately my tests on .NET 8 confirms that situation is still broken. Maybe I will put my results here to show how much broken it is especially when using with Open Telemetry and I think many people are using OTel now. My test environment

PowerShell A -> WebApi B -> WebApi C.

1. B is filling Activity.Current?.Baggage -> C has Activity.Current?.Baggage propagated by 'Correlation-Context' header and Baggage.Current is not set
2. B is filling OTel Baggage.Current -> C has Activity.Current?.Baggage propagated by 'Correlation-Context' header and Baggage.Current is set by 'baggage' header
3. B is filling both Activity.Current?.Baggage and Baggage.Current -> C has propagated Activity.Current?.Baggage and Baggage.Current but only from values of B Baggage.Current (data from Activity is missing)
4. A is filling baggage -> B receive that and both both Activity.Current?.Baggage and Baggage.Current -> C receive data in header 'baggage' and duplicated in 'Correlation-Context' because first header is from OTel and second from Activity.

Problem is that ASP.NET Core supports both headers and http client support one and none of this is configurable. Additionally we have two Baggages and logic from OTel. Maybe it is time to clean this?

[tarekgh]

CC @noahfalk @samsp-msft @reyang @cijothomas

[dominikjeske]

For those who want to change this behavior - you should override DistributedContextPropagator.Current with own implementation instead LegacyPropagator. It is weird that default propagator is named LegacyPropagator :)
The other confusing point is that when we set something in DistributedContextPropagator it could be ignored because TraceContextPropagator already set this value - I have this when setting traceparent.

[lmolkova]

The baggage has reached "W3C Candidate Recommendation" state!

But I don't think it can be implemented without some API changes.
One of the important features of the W3C baggage is that it does not require distributed tracing (W3C Trace Context) so it SHOULD NOT be part of the Activity.

I.e. I should be able to use baggage with disabled/dropped activities.

Leaving aside the baggage API, the propagator needs to be able to inject baggage. It currently has the following methods:

• IEnumerable<KeyValuePair<string, string?>>? ExtractBaggage(object? carrier, PropagatorGetterCallback? getter)
• void ExtractTraceIdAndState(object? carrier, PropagatorGetterCallback? getter, out string? traceIf, out string? traceState)
• void Inject(Activity? activity, object? carrier, PropagatorSetterCallback? setter)

and should get a new method like

• void InjectBaggage(IEnumerable<KeyValuePair<string, string?>>, object? carrier, PropagatorSetterCallback? setter)

Slightly off-topic - the propagator API is quite hard to use:

• I might not have the full activity to inject - ActivityContext should be enough
• it should be able to extract ActivityContext and {FutureBaggage}
• the traceId should really be traceparent

So along with W3C baggage support, it'd be great to consider some other usability improvements.

[jbogard]

Ah great news!

[cijothomas]

I think, we can probably workaround as shown below with the existing APIs itself:

public abstract void Inject(Activity? activity, object? carrier, PropagatorSetterCallback? setter)
{
	// ignore the activity passed to it, as our baggage is not in this activity

	// Obtain OTel’s Baggage using Current!
                 var baggage =	Baggage.Current;

	// inject baggage into carrier using the provided setter.
}

(Assuming its too late for .NET 9 changes! The proposed additional method is already there, just marked internal.. not sure if that was intentional....)

[lmolkova]

I think, we can probably workaround as shown below with the existing APIs itself:

We can definitely workaround in OTel, but not in the .NET implementation of the baggage which should eventually come.

void InjectBaggage(IEnumerable<KeyValuePair<string, string?>>, object? carrier, PropagatorSetterCallback? setter)

is probably non-controversial API addition which would still be enough to implement basic W3CPropagator (Trace Context + Baggage) - would be great to have it in .NET 9

[tarekgh]

I think the comment is mostly regarding W3C and ignoring the legacy cases.

I might not have the full activity to inject - ActivityContext should be enough

The Activity was needed in the injection to inject the Activity.Baggage which not part of the ActivityContext.

the traceId should really be traceparent

This is true only for W3C. for legacy activities will be RequestId. I think this is why named traceId covers both cases.

should get a new method like void InjectBaggage(IEnumerable<KeyValuePair<string, string?>>, object? carrier, PropagatorSetterCallback? setter)

This makes sense.

[fiha8]

Hi, I have slightly different scenario. Hope there is somebody able to explain, what is happening in this case:
I have hybrid instrumentation approach - some services instrumented via OTEL SDK and some service instrumented via dotnet auto-instrumentation (using Kubernetes operator). We can simplify it only into 3 services:

• Service 1 (instrumented via OTEL SDK)

• Service 2 (instrumented via dotnet auto-instrumentation).

• Service 3 (instrumented via OTEL SDK)

• Service 1 sets OTEL Baggage.

• I see only correct "baggage" header in Service 2

• I see only header "Correlation-Context" in Service 3.

Note: There is no custom logic in Service 2 handling anything around Baggage or Activities - purely auto-instrumented. My expectation is that "baggage" header is simply propagated further as it is.

Anyone able to explain this behaviour?

[dominikjeske]

@fiha8 See my analyse above from 28 March. .NET reads baggage and Correlation-Context but http client LegacyPropagator only understands Correlation-Context so Service2 is sending only this. To fix this you should use your own propagator or use OTel.