AddStandardResilienceHandler messes up DI container, requiring strict ordering which is bad practice

[sander1095]

Description

Using AddStandardResilienceHandler() does something with the service collection, causing Application Insights' TelemtryClient to go missing from the DI container.

I'm pretty sure (or I hope that) this bug is not related to Application Insights; most likely it has something to do with the way this package changes the ServiceCollection. However, I will demonstrate it using Application Insights because that's how I bumped into this issue.

Reproduction Steps

1. Create a Worker Service (.net 8)
2. Add latest Microsoft.ApplicationInsights.WorkerService
3. Add latest Microsoft.Extensions.Http.Resilience

Write the following in Program.cs:

using Microsoft.ApplicationInsights;
using Microsoft.Extensions.Http.Resilience;

var builder = Host.CreateApplicationBuilder(args);

// Turn these 2 around to make it work!
builder.Services.AddHttpClient("TEST").AddStandardResilienceHandler();
builder.Services.AddApplicationInsightsTelemetryWorkerService(x => 
    x.ConnectionString = "This does not get triggered when set up after the resilience handler");

var host = builder.Build();

// This will throw an exception about TelemetryClient missing in the DI contaner
// To fix this, turn the 2 lines around in the builder.Services code.
_ = host.Services.GetRequiredService<TelemetryClient>();
host.Run();

Expected behavior

I would expect that IServiceCollection does NOT require the caller to configure it in a specific order. This is considered a bad practice; the http pipeline in ASP.NET Core is ordered, for example ,but the DI is not.

Actual behavior

Using AddStandardResilienceHandler() modifies the service collection somehow, causing some services to become unresolvable.

Regression?

No response

Known Workarounds

Configure Resilience last in your service collection

Configuration

.NET 8
Windows 11

Other information

The docs mention the following:

Creates a xref:Microsoft.Extensions.DependencyInjection.ServiceCollection instance.

Which changed my mind on guessing this was a resilience <-> app insights issue, and more likely related to how this resilience package modifies the service collection in an illegal/bad practice kind of way.

https://learn.microsoft.com/en-us/dotnet/core/extensions/dependency-injection

Service registration is generally order-independent except when registering multiple implementations of the same type.

[dmelinosky]

I also ran into this issue. The workaround does work, but it is unfortunate that order matters in DI.

[joperezr]

Thanks for the bug report @sander1095, this is indeed interesting and we should take a peek. By just skimming the code I'm not sure what may be going on here as the standard resilience handler should only be adding services to the container and not removing any. cc @iliar-turdushev this might require some debugging to figure out.

[kevincathcart-cas]

ApplicationInsights has a AddSingletonIfNotExists method it uses internally that does not work correctly if there are any keyed services registered. And AddStandardResilienceHandler does register at least one keyed service.

ApplicationInsights is generally designed as best effort observability, which means that if it encounters errors it is not supposed to take down your service, and this is taken to the point where it mostly silently catches exceptions that occur inside .AddApplicationInsightsTelemetryWorkerService() (although it does call WorkerServiceEventSource.Instance.LogError(...) with the exception).

(This also means that doing .GetRequiredService<TelemetryClient>() is dangerous, as it is very deliberately the case that the registration could be missing if there were problems when trying to register the service.)

All that said, this is fundamentally just a bug with Application Insight's AddSingletonIfNotExists method, which really should be updated able to handle a ServiceCollection that contains keyed services, since that is a standard DI feature now, and users should not need to defer registering such services until after calling AddApplicationInsightsTelemetryWorkerService.

[joperezr]

Thanks for the info @kevincathcart-cas, I actually didn't know that so today I learned 😃. I was able to confirm this by swapping the call to AddStandardResilienceHandler in your repro to a simple builder.Services.AddKeyedSingleton<MyService>("myService"); and that still repros if called before adding AppInsights. Given this isn't really an issue with the our resilience library, I'll go ahead and close this issue, and I'd suggest you opening one in the App Insights repo so that they can fix this issue.

[joperezr]

@sander1095 FWIW, in case you plan to open an issue this is the method in question:

https://github.com/microsoft/ApplicationInsights-dotnet/blob/c929c85e957b36e614299c5ef6682f8bafef3532/NETCORE/src/Shared/Extensions/ApplicationInsightsExtensions.cs#L283-L301

[sander1095]

Thanks! I had already opened an issue but it needs to be updated with these (fascinating) details!

microsoft/ApplicationInsights-dotnet#2879

[RussKie]

microsoft/ApplicationInsights-dotnet#2879 (comment):

Just released 2.23.0-beta1 which contains the fix for this issue nuget.org/packages/Microsoft.ApplicationInsights.AspNetCore/2.23.0-beta1