open-telemetry / opentelemetry-dotnet-contrib

This repository contains set of components extending functionality of the OpenTelemetry .NET SDK. Instrumentation libraries, exporters, and other components can find their home here.
https://opentelemetry.io
Apache License 2.0
421 stars 251 forks source link

AddHttpClientInstrumentation is not using the baggage header in azure functions #1848

Closed Barsonax closed 3 weeks ago

Barsonax commented 4 weeks ago

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

vishweshbankwar commented 3 weeks ago

@Barsonax - Do you have a repro?

Barsonax commented 3 weeks ago

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

joegoldman2 commented 3 weeks ago

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.

Barsonax commented 3 weeks ago

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

joegoldman2 commented 3 weeks ago

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

Barsonax commented 3 weeks ago

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.

RohitRanjanMS commented 3 weeks ago

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

Barsonax commented 3 weeks ago

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.

vishweshbankwar commented 3 weeks ago

@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

martinjt commented 3 weeks ago

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.

Barsonax commented 3 weeks ago

@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.

cijothomas commented 3 weeks ago

@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 commented 3 weeks ago

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 commented 3 weeks ago

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

Barsonax commented 3 weeks ago

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