search

DuendeSoftware / BFF

Framework for ASP.NET Core to secure SPAs using the Backend-for-Frontend (BFF) pattern
Other
326 stars 75 forks source link

BFF's SocketsHttpHandler configuration out of sync with Yarp's #194

Open ArturDorochowicz opened 1 month ago

ArturDorochowicz commented 1 month ago

Which version of Duende BFF are you using? 2.2.0

Which version of .NET are you using? net8.0

Describe the bug

At work we use BFF in an application that uses Microsoft Application Insights (AppInsights). Since introducing BFF, it's always reported telemetry nested incorrectly. What we'd expect is something like this:

Instead, what we actually get is this (i.e. incoming requests are siblings and there's no outgoing request):

This has puzzled me, because Yarp docs say that it works out of the box (https://microsoft.github.io/reverse-proxy/articles/distributed-tracing.html). Only recently have I realized that it is dependent on using Yarp's created SocketsHttpHandler (in ForwarderHttpClientFactory) and BFF doesn't do that.

So it turns out that BFF's configuration for SocketsHttpHandler is out of sync with Yarp.

In Yarp 2.1.0 we have additionally ActivityHeadersPropagator (which is the missing piece behind correct distributed tracing) and ConnectTimeout. https://github.com/microsoft/reverse-proxy/blob/v2.1.0/src/ReverseProxy/Forwarder/ForwarderHttpClientFactory.cs#L44-L54

And in Yarp 2.2.0 (currently preview) there's also EnableMultipleHttp2Connections. https://github.com/microsoft/reverse-proxy/blob/v2.2.0-preview1/src/ReverseProxy/Forwarder/ForwarderHttpClientFactory.cs#L44-L55

To Reproduce

Steps to reproduce the behavior.

Expected behavior

A clear and concise description of what you expected to happen.

Log output/exception with stacktrace

data

Additional context

In my opinion BFF should employ Yarp's classes here and not recreate this, but I will create separate issue for that.

AndersAbel commented 1 month ago

Thank you for your detailed bug report. It indeed looks like we are not propagating the activity headers correctly.

I'm transferring this issue to the BFF repository and marking it as a bug.