opentelemetry-dotnet-contrib icon indicating copy to clipboard operation
opentelemetry-dotnet-contrib copied to clipboard

AddHttpClientInstrumentation is not using the baggage header in azure functions

Open Rick-van-Dam opened this issue 1 year ago • 9 comments

Component

OpenTelemetry.Instrumentation.Http

Package Version

Package Name Version
OpenTelemetry.Instrumentation.Http 1.8.1
OpenTelemetry.Extensions.Hosting 1.8.1
OpenTelemetry.Exporter.OpenTelemetryProtocol 1.8.1

Runtime Version

net8.0, azure functions

Description

When instrumenting httpclient in azure functions and adding baggage the request send by httpclient does not contain the baggage header. Instead the older Correlation-Context is being used.

Steps to Reproduce

  1. Instrument httpclient with .AddHttpClientInstrumentation
  2. Add baggage, for instance Activity.Current?.SetBaggage("foobar2", "fus43gf3g3");
  3. Do a http call using httpclient
  4. Notice the correlation-context header contain the baggage values and there is no baggage header

Expected Result

baggage header is used instead of the correlation-context header

Actual Result

correlation-context header is used

Additional Context

No response

Rick-van-Dam avatar May 29 '24 09:05 Rick-van-Dam

@Barsonax - Do you have a repro?

vishweshbankwar avatar May 29 '24 18:05 vishweshbankwar

Iam able to reproduce it in our codebase with the steps above if that's what you mean.

Rick-van-Dam avatar May 29 '24 18:05 Rick-van-Dam

The header Correlation-Context is not added by the OTel SDK but by the runtime in HttpHandlerDiagnosticListener.

https://github.com/dotnet/runtime/blob/81adb342a98566244207106700678e838ea23645/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/HttpHandlerDiagnosticListener.cs#L646

https://github.com/dotnet/runtime/issues/45496 is tracking the replacement of this header by baggage. But for the moment the baggage spec is still in draft, so the issue is on standby.

joegoldman2 avatar May 29 '24 18:05 joegoldman2

Thanks for your answer! So if I understand this correctly its not just azure functions but the whole .net ecosystem that uses the old header then?

EDIT: I do see something was changed in aspnet: https://github.com/dotnet/aspnetcore/pull/28328/files

Rick-van-Dam avatar May 29 '24 19:05 Rick-van-Dam

By default, ASP.NET Core uses LegacyPropagator which supports the extraction from baggage header with a fallback to Correlation-Context.

https://github.com/dotnet/runtime/blob/49c10ed5d96375d563d5202c71ca39a0fab3618e/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/LegacyPropagator.cs#L63

joegoldman2 avatar May 29 '24 19:05 joegoldman2

Ok that atleast clarifies alot. I wrote my own azure functions middleware to workaround the context not propagating. I think I will stick with that for now till this gets fixed in dotnet. Again thanks for the answers so I don't have to keep digging.

Rick-van-Dam avatar May 29 '24 19:05 Rick-van-Dam

Hi @Barsonax , can you share more details about your Functions App? Is it InProc or Isolated?

RohitRanjanMS avatar May 29 '24 19:05 RohitRanjanMS

Its a .net 8.0 isolated function app but I think @joegoldman2 already clarified that its a issue in dotnet itself. Only aspnet has a propagator that supports both headers.

Rick-van-Dam avatar May 29 '24 19:05 Rick-van-Dam

@Barsonax - Just adding some clarification here:

HttpClientInstrumentation does not propagate the baggage that is set via Activity.Current?.SetBaggage("foobar2", "fus43gf3g3")

You would need to Use Baggage API for that. https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/src/OpenTelemetry.Api#baggage-api

vishweshbankwar avatar May 29 '24 20:05 vishweshbankwar

This is actually a runtime bug, not an OpenTelemetry bug. If you use the Baggage API from OpenTelemetry, this should work fine. We could do better at documenting it in the OpenTelemetry docs though.

I'm suggesting that the runtime team look into this, and we close the issue in our repo.

martinjt avatar Jun 01 '24 18:06 martinjt

@Barsonax - Just adding some clarification here:

HttpClientInstrumentation does not propagate the baggage that is set via Activity.Current?.SetBaggage("foobar2", "fus43gf3g3")

You would need to Use Baggage API for that. https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/src/OpenTelemetry.Api#baggage-api

That's super confusing in the api. Will try this out and report back.

Rick-van-Dam avatar Jun 03 '24 06:06 Rick-van-Dam

@Barsonax - Just adding some clarification here: HttpClientInstrumentation does not propagate the baggage that is set via Activity.Current?.SetBaggage("foobar2", "fus43gf3g3") You would need to Use Baggage API for that. https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/src/OpenTelemetry.Api#baggage-api

That's super confusing in the api. Will try this out and report back.

"The recommended way to add Baggage is to use the Baggage.SetBaggage() API. OpenTelemetry users should not use the Activity.AddBaggage method."

There is this wording in the docs to warn about this already. It is not an ideal situation, and something that needs to be fixed. See https://github.com/open-telemetry/opentelemetry-dotnet/issues/5667 for some more details on this same topic.

cijothomas avatar Jun 03 '24 17:06 cijothomas

The header Correlation-Context is not added by the OTel SDK but by the runtime in HttpHandlerDiagnosticListener.

https://github.com/dotnet/runtime/blob/81adb342a98566244207106700678e838ea23645/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/HttpHandlerDiagnosticListener.cs#L646

dotnet/runtime#45496 is tracking the replacement of this header by baggage. But for the moment the baggage spec is still in draft, so the issue is on standby.

The baggage propagation spec from W3C just moved to candidate recommendation : https://www.w3.org/TR/baggage/

cijothomas avatar Jun 03 '24 17:06 cijothomas

Closing as this is working as expected. https://github.com/open-telemetry/opentelemetry-dotnet-contrib/issues/1848#issuecomment-2143545720

cijothomas avatar Jun 03 '24 17:06 cijothomas

So tested this to be sure and for anyone that stumbles on this:

  • activity.SetBaggage will use the Correlation-Context header.
  • Baggage.SetBaggage will use the baggage header (if you called AddHttpClientInstrumentation while adding tracing, else there will be no header)

Rick-van-Dam avatar Jun 03 '24 19:06 Rick-van-Dam

Hi, I have another issue around this. I'm trying hybrid scenario, where some services are instrumented via OTEL SDK, and some services are instrumented with dotnet autoinstrumentation (using kubernetes operator). I have simple flow, only HTTP communication in following order: Service 1 (OTEL SDK), Service 2 (dotnet autoinstrumentation), Service 3 (OTEL SDK). I'm setting Baggage in Service 1 (OTEL baggage). If I look at headers in Service 2, I see correct "baggage" header. If I look at headers in Service 3, it changed to "Correlation-Context". Any idea why is this happening?

fiha8 avatar Jul 09 '24 06:07 fiha8