Customized SocketsHttpHandler does not support diagnostics tracking

[rangp]

I want to use custom HttpConnectionSettings for my requests since I need to decrease DefaultMaxConnectionsPerServer. As far as I know HttpConnectionSettings are only exposed when creating a new SocketsHttpHandler. However if I create a new handler with custom settings and use the handler as message handler for my HttpClient I lose my dependency tracking.

Upon debugging I noticed that the default message handler is HttpClientHandler which includes a DiagnosticsHandler that is not publicly exposed.

How can I customize HttpConnectionSettings while still seeing request diagnostics?

I don't necessarily want to copy DiagnosticsHandler into my code to accomplish this.

On first glance I could see two possible solutions:

1. Expose DiagnosticsHandler publicly, so consumers can wrap their custom HttpHandlers with it.
2. Provide a way to customize HttpConnectionSettings without creating a new message handler, so consumers can use the HttpClientHandler while still being able to configure connection behavior.

[stephentoub]

cc: @lmolkova

[scalablecory]

Can you give more detail on how explicitly using a SocketsHttpHandler breaks dependency tracking?

Max connections per server can be customized via the HttpClientHandler API already, if your goal is to continue using diagnostics:

using var handler = new HttpClientHandler { MaxConnectionsPerServer = 10 };
using var client = new HttpClient(handler);

[stephentoub]

@scalablecory, DiagnosticHandler is created and wrapped around SocketsHttpHandler in HttpClientHandler, e.g.
https://github.com/dotnet/corefx/blob/a3f71bfe819463e388fa7afe77a579ebf99ac866/src/System.Net.Http/src/System/Net/Http/HttpClientHandler.Windows.cs#L31
If you construct a different handler than HttpClientHandler, DiagnosticHandler isn't created.

[scalablecory]

@stephentoub yes, that is clear.

[rangp]

@scalablecory Thanks for the info, I didn't see that HttpClientHandler is customizable this way. However I would also like to adjust MaxResponseDrainSize which I can't see as a property of HttpClientHandler.

I think that my dependency tracking is broken because DiagnosticsHandler is missing around my SocketsHttpHandler. I'm using application insights' automatic dependency tracking that seems to integrate with the diagnostics propagated by DiagnosticsHandler.

In general I would like the freedom to adjust the settings of the SocketsHttpHandler contained in my HttpClient without losing the diagnostics. The issue I'm having is that I can't access the SocketsHttpHandler that's automatically created by HttpClientHandler and as explained I can't just create a SocketsHttpHandler myself without losing diagnostic information.

I just posted this as an issue because I expected to be able to adjust the full set of HttpConnectionSettings without trade-offs like losing dependency tracking

[ctaggart]

The dotnet ApplicationInsights SDK also builds on this. If we create a custom SocketsHttpHandler, we loose http request tracking.

[karelz]

Triage: We should do something -- there is multiple options - expose it as type (not a very interesting one), static factory, or HttpClient property to enable it.
Xamarin workloads have native handlers and that's why we need to make it work outside of just SocketsHttpHandler.

We should make it happen in 5.0 as AppInsights is important scenario for custom handler -- Xamarin and SocketsHttpHandler.

[Tratcher]

Workaround: you can probably copy DiagnosticsHandler into your project. Has anyone tried that?

[JamesNK]

What about making DiagnosticsHandler public?

gRPC client will be customizing the SocketsHttpHandler and so will lose the automatic diagnostics functionality that HttpClientHandler currently offers. This isn't a big deal because the gRPC client already has its own Activity, but I'll need to add some logic to propagate the trace ID in the header. Copy and paste this I think.

[karelz]

@JamesNK it is one of the options listed above - #31261 (comment) ... we just didn't get to making a decision here :(.

[lmolkova]

I was OOF for quite a while, sorry for not commenting earlier.

I agree that exposing DiagnosticasHandler make sense as it enabled diagnostics for custom/customized innermost handler, and makes instrumentation testable.
I guess we might want to expose some configuration and perhaps different defaults (so we don't enable back-compat stuff).

@karelz assuming we make decision to expose it , did we miss .NET 5 train or there is still a chance ?

[cijothomas]

I was OOF for quite a while, sorry for not commenting earlier.

I agree that exposing DiagnosticasHandler make sense as it enabled diagnostics for custom/customized innermost handler, and makes instrumentation testable.
I guess we might want to expose some configuration and perhaps different defaults (so we don't enable back-compat stuff).

@karelz assuming we make decision to expose it , did we miss .NET 5 train or there is still a chance ?

Please consider making backports to 2.1 and 3.1 as well to benefit more customers. :) Many of the affected customers are not going to be in 5.0 anytime soon :(

[karelz]

The chance to get it into 5.0 is very low at this point. The team is busy, time is short, this is not the highest priority issue for 5.0 and the design is unclear and will require iterations - #31261 (comment)

@cijothomas .NET Core does not add new APIs in servicing -- I think it is because we don't ship new ref assemblies to use the new APIs.
Let's first focus on getting it fixed properly somewhere, before we start muddying water with backport requests, etc.

If targeting older versions is top need, then we should think about temporary alternarives like standalone package outside of .NET Core.

[avaneerd]

Workaround: you can probably copy DiagnosticsHandler into your project. Has anyone tried that?

I have tried this and this causes other issues.

Using reflection does work:

        private static Func<SocketsHttpHandler, DelegatingHandler> CreateDiagnosticsHandlerFactory()
        {
            var diagnosticsHandler = typeof(SocketsHttpHandler).Assembly.GetType("System.Net.Http.DiagnosticsHandler");

            var ctor = diagnosticsHandler!.GetConstructors().First();
            var param = Expression.Parameter(typeof(HttpMessageHandler), "innerHandler");
            var newExp = Expression.New(ctor, param);
            var lambda = Expression.Lambda<Func<SocketsHttpHandler, DelegatingHandler>>(newExp, param);

            return lambda.Compile();
        }

var createDiagnosticsHandler= CreateDiagnosticsHandlerFactory();
var diagnosticsHandler = createDiagnosticsHandler(socketsHttpHandler);

[JamesNK]

Problem again: grpc/grpc-dotnet#1210

I have fixed it in grpc-dotnet by basically duplicating all of DiagnosticsSource: grpc/grpc-dotnet#1211

[jeffreyrivor]

@karelz @JamesNK is there a plan of record for this in .NET 6.0? With a preview already out and 6.0 being the LTS version, it seems like we should make a fix or at least some forward progress on some scenarios.

Perhaps getting a DiagnosticsHandler equivalent added in SocketsHttpHandler.SetupHandlerChain() when SocketsHttpHandler is constructed directly instead of through HttpClientHandler as a stopgap improvement if we can't make DiagnosticsHandler public in 6.0? At least that would fix microsoft/ApplicationInsights-dotnet#1434 but we have to be careful to use the same private activity classes because AppInsights implicitly depends on them when extracting activity data via reflection (which is why @avaneerd's workaround is reflection-based).

[karelz]

@jeffreyrivor yes, it is on our 6.0 list to solve. Technical details how it will be solved are still TBD.

[JamesNK]

I have fixed it in grpc-dotnet by basically duplicating all of DiagnosticsSource: grpc/grpc-dotnet#1211

FYI, AppInsights doesn't like events it listens to being raised by multiple libraries: microsoft/ApplicationInsights-dotnet#2194

I submitted a PR to them.

[paulomorgado]

Regardless of Application Insights (or OpenTelemetry, for that matter) a tracing handler should exist to be injected via IHttpClientFactory.

[MihaZupan]

#55392 changed the behavior of SocketsHttphandler to match HttpClientHandler in honoring the global DistributedContextPropagator.Current.
As such, SocketsHttphandler will now inject trace headers by default.

On top of the global default, the behavior can also be customized on a per-handler basis (#55556):

using var handler = new SocketsHttpHandler
{
    ActivityHeadersPropagator = DistributedContextPropagator.CreatePassThroughPropagator()
};