Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
sander1095 opened this issue Jun 12, 2024 · 6 comments
Labels
area-resilience bug This issue describes a behavior which is not expected - a bug. bug-external

Comments

@sander1095
Copy link

sander1095 commented Jun 12, 2024

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
Copy link

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

@joperezr
Copy link
Member

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
Copy link

kevincathcart-cas commented Jun 19, 2024

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
Copy link
Member

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
Copy link
Member

@sander1095
Copy link
Author

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

microsoft/ApplicationInsights-dotnet#2879

@github-actions github-actions bot locked and limited conversation to collaborators Jul 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-resilience bug This issue describes a behavior which is not expected - a bug. bug-external
Projects
None yet
Development

No branches or pull requests

5 participants