Repository navigation
AddStandardResilienceHandler messes up DI container, requiring strict ordering which is bad practice #5222
Description
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
- Create a Worker Service (.net 8)
- Add latest
Microsoft.ApplicationInsights.WorkerService - 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.
Activity
dmelinosky commentedon Jun 18, 2024
I also ran into this issue. The workaround does work, but it is unfortunate that order matters in DI.
joperezr commentedon Jun 19, 2024
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 commentedon Jun 19, 2024
ApplicationInsights has a
AddSingletonIfNotExistsmethod 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
AddSingletonIfNotExistsmethod, 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 callingAddApplicationInsightsTelemetryWorkerService.joperezr commentedon Jun 19, 2024
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 commentedon Jun 19, 2024
@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 commentedon Jun 20, 2024
Thanks! I had already opened an issue but it needs to be updated with these (fascinating) details!
microsoft/ApplicationInsights-dotnet#2879
RussKie commentedon Feb 5, 2025
microsoft/ApplicationInsights-dotnet#2879 (comment):
iliar-turdushev commentedon Feb 7, 2025
@RussKie Thank you for the heads up. I've created an issue to update the documentation accordingly #5856.