SentryHttpMessageHandler not being added when AddHttpClient is before UseSentry

[mattjohnsonpint]

Package

Sentry.AspNetCore

.NET Flavor

.NET

.NET Version

7.0.5

OS

Any (not platform specific)

SDK Version

3.31.0

Self-Hosted Sentry Version

No response

Steps to Reproduce

An an ASP.NET Core app, such as Sentry.Samples.AspNetCore.Basic use IHttpClientFactory per ms docs to make an outbound request from within a web controller.

Expected Result

The SentryHttpMessageHandler should automatically be connected by use of the UseSentry call from Sentry.AspNetCore. The order that it is added in relation to AddHttpClient should not matter.

Actual Result

It only works if AddHttpClient is after UseSentry. If it's before UseSentry, then the message handler is not added, and HTTP Client spans are not auto-instrumented.

This works. HttpClient requests will be auto-instrumented in Sentry.

builder.WebHost.UseSentry();
builder.Services.AddHttpClient();

This does not, but should. HttpClient requests succeed, but are not auto-instremented in Sentry.

builder.Services.AddHttpClient();
builder.WebHost.UseSentry();

Further info

SentryHttpMessageHandler gets wired up for outgoing requests in SentryWebHostBuilderExtensions. Specifically, this is done when configuring logging via:

var sentryBuilder = logging.Services.AddSentry();

Eventually that calls Sentry.Extensions.Logging.ServiceCollectionExtensions.AddSentry which contains this call that refers to #785:

// Custom handler for HttpClientFactory.
// Must be singleton: https://github.com/getsentry/sentry-dotnet/issues/785
services.TryAddSingleton<IHttpMessageHandlerBuilderFilter, SentryHttpMessageHandlerBuilderFilter>();

[jamescrosswell]

@mattjohnsonpint is this not by design?

Reading through the docs for ASP.NET Core Middleware:

The order that middleware components are added in the Program.cs file defines the order in which the middleware components are invoked on requests and the reverse order for the response. The order is critical for security, performance, and functionality.

[mattjohnsonpint]

Middleware order matters, but service registration order should not.

I don't think any ASP.NET middleware is involved here. Its just registering SentryHttpMessageHandlerBuilderFilter with the service provider.

[jamescrosswell]

Created a minimal ASP.NET Core webapp with a custom DelegatingHandler that gets registered via a custom IHttpMessageHandlerBuilderFilter and with that app the order of service registration has no effect (the custom message handler gets invoked either way).

There must be something funky about how we're wiring things up then.

[jamescrosswell]

On a whim, I tried changing this line in our code:

        services.TryAddSingleton<IHttpMessageHandlerBuilderFilter, SentryHttpMessageHandlerBuilderFilter>();

To this instead...

        services.AddSingleton<IHttpMessageHandlerBuilderFilter, SentryHttpMessageHandlerBuilderFilter>();

And it "fixes" the issue. It's not what I was expecting. Without tracing into what the TryAddSingleton method is doing (and why it's not registering our SentryHttpMessageHandlerBuilderFilter when the AddSingleton method is), it's hard to puzzle this one out.

What are the dangers of using AddSingleton here vs TryAddSingleton? If it's about having our filter run multiple times on outbound requests, we could potentially mitigate with some code in the SentryHttpMessageHandlerBuilderFilter. For example (haven't tried this... just hypothetically):

    public Action<HttpMessageHandlerBuilder> Configure(Action<HttpMessageHandlerBuilder> next) =>
        handlerBuilder =>
        {
            var hub = _getHub();
            if (!handlerBuilder.AdditionalHandlers.Any(h => h is SentryHttpMessageHandler))
            {
                handlerBuilder.AdditionalHandlers.Add(
                    new SentryHttpMessageHandler(hub)
                );
            }

            next(handlerBuilder);
        };

[mattjohnsonpint]

That sounds like a good approach. We should just add the filter (not try-add), and the filter builder can ensure the message handler is only added once.

Nice find!