Fix Yarp reparenting if OpenTelemetry.Instrumentation.Http is present#8321
Fix Yarp reparenting if OpenTelemetry.Instrumentation.Http is present#8321
Conversation
Snapshots difference summaryThe following differences have been observed in committed snapshots. It is meant to help the reviewer. 1 occurrences of : - http-client-handler-type: System.Net.Http.HttpClientHandler,
+ http-client-handler-type: System.Net.Http.SocketsHttpHandler,
|
BenchmarksBenchmark execution time: 2026-03-27 19:37:20 Comparing candidate commit 9643c5c in PR branch Found 8 performance improvements and 12 performance regressions! Performance is the same for 253 metrics, 15 unstable metrics.
|
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8321) and master. ✅ No regressions detected - check the details below Full Metrics ComparisonFakeDbCommand
HttpMessageHandler
Comparison explanationExecution-time benchmarks measure the whole time it takes to execute a program, and are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are highlighted in **red**. The following thresholds were used for comparing the execution times:
Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard. Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph). Duration chartsFakeDbCommand (.NET Framework 4.8)gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8321) - mean (73ms) : 70, 75
master - mean (72ms) : 68, 76
section Bailout
This PR (8321) - mean (76ms) : 75, 78
master - mean (76ms) : 74, 78
section CallTarget+Inlining+NGEN
This PR (8321) - mean (1,082ms) : 1025, 1138
master - mean (1,077ms) : 1036, 1118
FakeDbCommand (.NET Core 3.1)gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8321) - mean (113ms) : 109, 117
master - mean (113ms) : 109, 116
section Bailout
This PR (8321) - mean (115ms) : 112, 117
master - mean (113ms) : 110, 117
section CallTarget+Inlining+NGEN
This PR (8321) - mean (801ms) : 781, 821
master - mean (794ms) : 773, 815
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8321) - mean (99ms) : 96, 103
master - mean (100ms) : 97, 103
section Bailout
This PR (8321) - mean (101ms) : 98, 103
master - mean (100ms) : 97, 103
section CallTarget+Inlining+NGEN
This PR (8321) - mean (952ms) : 911, 993
master - mean (948ms) : 912, 984
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8321) - mean (99ms) : 96, 102
master - mean (98ms) : 95, 101
section Bailout
This PR (8321) - mean (100ms) : 98, 102
master - mean (100ms) : 97, 102
section CallTarget+Inlining+NGEN
This PR (8321) - mean (831ms) : 797, 865
master - mean (834ms) : 796, 871
HttpMessageHandler (.NET Framework 4.8)gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8321) - mean (222ms) : 209, 234
master - mean (226ms) : 210, 241
section Bailout
This PR (8321) - mean (225ms) : 211, 238
master - mean (230ms) : 215, 245
section CallTarget+Inlining+NGEN
This PR (8321) - mean (1,253ms) : 1192, 1314
master - mean (1,257ms) : 1210, 1305
HttpMessageHandler (.NET Core 3.1)gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8321) - mean (309ms) : 295, 324
master - mean (321ms) : 300, 342
section Bailout
This PR (8321) - mean (309ms) : 296, 322
master - mean (324ms) : 303, 345
section CallTarget+Inlining+NGEN
This PR (8321) - mean (1,021ms) : 985, 1056
master - mean (1,030ms) : 1000, 1060
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8321) - mean (304ms) : 294, 315
master - mean (306ms) : 292, 321
section Bailout
This PR (8321) - mean (305ms) : 293, 317
master - mean (306ms) : 296, 316
section CallTarget+Inlining+NGEN
This PR (8321) - mean (1,211ms) : 1145, 1276
master - mean (1,206ms) : 1155, 1257
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8321) - mean (304ms) : 289, 319
master - mean (311ms) : 292, 329
section Bailout
This PR (8321) - mean (302ms) : 291, 314
master - mean (308ms) : 295, 321
section CallTarget+Inlining+NGEN
This PR (8321) - mean (1,155ms) : 1049, 1261
master - mean (1,169ms) : 1076, 1261
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
This happens when we have OpenTelemetry.Instrumentation.Http installed the Instrumentation for Yarp no longer resolves the reparenting issue like normal. Snapshot is updated with the "broken" state, will be updated in the next commit, this is just to show that it is broken. Note that we get span links added that highlight the terminated_context
NoOutputPropagator still processes incoming headers which interferes when OpenTelemetry HTTP instrumentation replaces the global DistributedContextPropagator. Setting to null fully disables it. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ator - Add AddAspNetCoreInstrumentation, AddOtlpExporter (non-routable endpoint) to match the Ocelot sample's OTel configuration - Suppress ActivityHeadersPropagator on the Worker's SocketsHttpHandler to prevent the in-process OTel SDK injecting a foreign traceparent on the simulated external request, matching the Ocelot sample workaround Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
fa2bcce to
91d944f
Compare
Summary of changes
This fixes a reparenting issue with Yarp when
OpenTelemetry.Instrumentation.Httpis installed as theCreateNoOutputPropagator()would cause some reparenting.Reason for change
Noticed in #8128 that DistributedContextPropagator.CreateNoOutputPropagator() still causes broken reparenting only when
OpenTelemetry.Instrumentation.Http.Figured the same issue affected this and it does somewhat it is a bit different though in the sense that it adds
Implementation details
Changed handler.ActivityHeadersPropagator = DistributedContextPropagator.CreateNoOutputPropagator() to handler.ActivityHeadersPropagator = null
Test coverage
Update the sample application to have the
OpenTelemetry.Instrumentation.HttpReran the tests and snapshotted the new snapshots that showed some span links with the terminated context
Applied the fix, reran the tests and applied the new "fixed" snapshot
Other details
Interestingly there were some span links here with
terminated_context- I didn't see this with the Ocelot reproduction - you should be able to see these in the commits. I did the same change here that I did in the Ocelot sample where I null out the propagator for the client used in the sample app.For context:
NoOutputPropagatoraspnet_core.requestspansaspnet_core.requestnull)aspnet_core.requestSetting the
handler.ActivityHeadersPropagatortonullresolves the Yarp one; however we still would get the one form the HttpClient so nulling that one out does the same to remove that span link.I'm thinking we may want to consider attempting to resolve this more correctly in the future instead of one off (if possible), but we also haven't really seen this specific issue in the wild (not saying that it hasn't happened though).
Regardless I think this is a safe change, it was initially going to be
nullbut I recommended to change it to the no-opt 😅