microsoft / ApplicationInsights-dotnet

ApplicationInsights-dotnet
MIT License
567 stars 285 forks source link

Service registration fails #2852

Open vijuhe opened 6 months ago

vijuhe commented 6 months ago

ASP.NET Core API service registration of Application Insights classes is not completed correctly and the API requests fail with the following message: Unable to resolve service for type 'Microsoft.ApplicationInsights.TelemetryClient'. TelemetryClient is a dependency of a class in the API. Application Insights was registered with the IServiceCollection extension method AddApplicationInsightsTelemetry. Version of package Microsoft.ApplicationInsights.AspNetCore is 2.22.0.

The error started occurring after updating another API dependency Microsoft.Extensions.Http.Resilience to version 8.2.0. I'm guessing that the error has something to do with this change in the service collection https://github.com/dotnet/runtime/issues/64427. There have been service collection usage errors in other Microsoft packages as well after the change (e.g. Microsoft.Identity.Web: https://github.com/AzureAD/microsoft-identity-web/pull/2676).

onionhammer commented 6 months ago

I'm also seeing this same issue after upgrading these two dependencies (within an Aspire project)

Maybe related:

4865783a5d commented 4 months ago

We're also seeing this after we migrated from Polly to Microsoft.Extensions.Http.Resilience 8.3.0

rajkumar-rangaraj commented 4 months ago

Could one of you assist with a demo app that could replicate this issue?

vijuhe commented 4 months ago

The issue can be replicated with this API https://github.com/vijuhe/ApplicationInsightsIssue. Just start the API and you get the error.

kevincathcart-cas commented 3 months ago

This is fundamentally just a bug with AddSingletonIfNotExists, which cannot handle iterating through keyed services. It is not trivial to directly fix this because one needs to check the new in 8.0 ServiceKey property before touching the implementation related properties on ServiceDescriptor, which would mean needing to reference Microsoft.Extensions.DependencyInjection.Abstractions 8.0.

Generally, the safe way to handle this short of logic is with .TryAddSingleton (if you want to skip registration if any implementation of the service type is present), or the much lesser known .TryAddEnumerable (to skip registration only if there is a registration of the given implementation type for the given service type). The name of the latter hinting at the fact that this is often desirable if you expect to resolve this service type as an IEnumerable, while often not as useful if you expect to resolve the service type directly.

I think .TryAddEnumerable is what is wanted here. I see only three relevant differences vs the existing method beyond tolerating keyed services. The first is that AddSingletonIfNotExists will skip registering if the implementation type is registered for literally any service type, not just the service type we are trying to register (that seems like a bug). The second difference is that AddSingletonIfNotExists also looks for subtypes of the implementation type, rather than just an exact type match (which could be a feature), but since the only usage of that method is for the sealed implementation type of DiagnosticsTelemetryModule, that difference really doesn't matter.

So simplest fix is probably as simple as replacing the body of AddSingletonIfNotExists with:

services.TryAddEnumerable(ServiceDescriptor.Singleton<TService, TImplementation>());

That simple fix also has the benefit of working with Microsoft.Extensions.DependencyInjection.Abstractions going back all the way to 2.0.