-
Notifications
You must be signed in to change notification settings - Fork 287
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
App Insights breaks when a keyed service is added #2879
Comments
Just spent a good few days trying to figure out why App Insights was not working until I finally discovered it is because I added some keyed services. |
We worked around it by registering application insights BEFORE any other (keyed) services. In our case that was Orleans 8 |
The root cause is at this line which fails if any keyed service is added to the service collection at the time. What needs to be done is to first check if the service is a keyed service. Anyway, there is another place that might be discussed. It effectively swallows any exception and makes debugging incredibly difficult. Could we possibly change the function to throw and let its caller to decide if it's safe to swallow the exception or not? |
We have had the same issue. App Insights just silently stopped working due to a new dependency being used. See: #5421 |
Same here, spent two days trying to figure out what the issue was. |
Thanks! This workaround does work for me. |
I had the same issue and it was only surfaced because of Scrutor uses KeyedServices for the decorator pattern starting with version 5 (released September 25). |
For my own reproes of this, the culprit appears to be this method: ApplicationInsights-dotnet/NETCORE/src/Shared/Extensions/ApplicationInsightsExtensions.cs Lines 293 to 301 in e34286b
internal static void AddSingletonIfNotExists<TService, TImplementation>(this IServiceCollection services)
where TService : class
where TImplementation : class, TService
{
- if (!services.Any(o => o.ImplementationFactory == null && typeof(TImplementation).IsAssignableFrom(o.ImplementationType ?? o.ImplementationInstance.GetType())))
+ if (!services.Any(o => o.ImplementationFactory == null && typeof(TImplementation).IsAssignableFrom(o.ImplementationType ?? (o.ImplementationInstance is not null ? o.ImplementationInstance.GetType() : null))))
{
services.AddSingleton<TService, TImplementation>();
}
} That's probably not the cleanest way, but I just wanted to share the general idea. |
It worked, or at least I thought. It stopped throwing the exception, but nothing was being sent to my app insights |
Any update on this. Just spent hours to find out this was the issue. Any chance there's a fix in a preview version? |
Just spent my afternoon tracking down what was going on and traced it down to this line as well, you are on the right track. |
Guys, this is not funny. Why this is still not fixed? I just spend a day trying to understand why my library isn't compatible with Application Insights (and it will fail randomly), just to find this issue. It even has already approved PR which fixes it. Why it isn't merged and released already?! It's unacceptable. |
Also, it seems like PR #2908 should resolve the issue, but still not merged and not released. |
Do you have any plans/date to release this fix? |
See the following bug report:
dotnet/extensions#5222
When a keyed service is added, app insights breaks silently (no logging is sent to app insights) or breaks with an exception when the user tries to retrieve a telemetryclient.
Comments that lead to this issue being related to App Insights:
@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() 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"); 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.
The text was updated successfully, but these errors were encountered: