Live metrics stops working when adding KeyedServices BEFORE AddApplicationInsightsTelemetry() in dotnet 8 rc2

[westmatte]

• List of NuGet packages and version that you are using:
  Microsoft.ApplicationInsights.AspNetCore
• Runtime version (e.g. net461, net48, netcoreapp2.1, netcoreapp3.1, etc. You can find this information from the *.csproj file):
  net8.0 (sdk 8.0.100-rc.2.23502.2)
• Hosting environment (e.g. Azure Web App, App Service on Linux, Windows, Ubuntu, etc.):
  When running on Windows locally, but in Azure on Azure Container Apps with chiseled image: mcr.microsoft.com/dotnet/aspnet:8.0-jammy-chiseled (Linux)

Describe the bug

When using KeyedSingleton together with AddApplicationInsightsTelemetry(), depending on the order, the Live Metrics in Azure won't work. Expect this to not matter in what order, or to get an explanation for why this happens. This might be an issue that should be reported to the team working on dotnet 8 and KeyedServices instead.

To Reproduce

Simple repo to showcase the issue: https://github.com/westmatte/keyedsingleton-livemetrics-issue/tree/main
Needs to add the instrumentation key to an existing app insights in the appsettings.json to run of course.
Alternatively, create a new dotnet 8 minimal API. Add application insights to it. See that the Live metrics work. Add KeyedSingleton for whatever BEFORE the AddApplicationInsightsTelemetry(), Live metrics no longer works. In program.cs:

builder.Services.AddSingleton<StupidService>(); // old stuff, works perfectly fine
// builder.Services.AddKeyedSingleton("StupidServiceKeyed", new StupidService()); // if adding this here, Live Metrics wont work
builder.Services.AddApplicationInsightsTelemetry();
builder.Services.AddKeyedSingleton("StupidServiceKeyed", new StupidService()); // if adding this after AddApplicationInsightsTelemetry^, Live Metrics will work perfectly fine

We will close this issue if:

• The repro project you share with us is complex. We can't investigate custom projects, so don't point us to such, please.
• If we will not be able to repro the behavior you're reporting

[amis92]

We've just hit this. Right now it prevents targeting net8.0 without workarounds, and worse - it's enough if a project references Microsoft.Extensions.DependencyInjection.Abstractions v8 (which is supported on net6 as well), so simply by a dependency update (not retargeting to new framework) it can crash. Precondition of inserting Keyed registration is still required.

Edit: I've raised dotnet/runtime#95789 since it seems to be a very problematic regression.

[wc-whiteheadd]

Just to add to this as i was about to type up a bug ticket for the same issue until i found this.

You can get past this for now by changing your DI ordering. If you do the application insights before you do the keyed service injections then it will work.

The reason this is failing is because of the internal "AddSingletonIfNotExists" extension method in the library which is looping through the already registered services.

If you take the example code below and change the order so Example2 is before Example1 you will notice it works as the extension method is not conflicting with keyed service that have already been registered. Its not a DI issue in itself, its more a bug with how they are looping over the pre-registered services when trying to register more.

I have abstracted the code below from the library which is causing the issue, to help show why its breaking, so people have an example to work from:-

using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;

HostApplicationBuilder builder = Host.CreateApplicationBuilder(args);
builder.Services.AddKeyedScoped<IExample, Example>("example");
builder.Services.AddSingletonIfNotExists<IExampleTwo, ExampleTwo>();//Breaks due to extension method

using IHost host = builder.Build();

public interface IExample { }
public class Example : IExample { }

public interface IExampleTwo { }
public class ExampleTwo : IExampleTwo { }

//Abstracted extension from Application Insights library
public static class Extensions
{
    public 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())))
        {
            services.AddSingleton<TService, TImplementation>();
        }
    }
}

[tore-hammervoll]

I just spent a day debugging some changes unrelated to Application Insights that was hit by this bug. Luckily the changes never made it to production, since the application crashed on startup due to this setup in our Serilog config:

WriteTo.ApplicationInsights(services.GetRequiredService<TelemetryConfiguration>(), TelemetryConverter.Traces)

The TelemetryConfiguration was not registered due the exception thrown in AddSingletonIfNotExists.

After hours of debugging to figure out what I changed to cause this issue, I isolated it to adding resilience to an HttpClient with AddStandardResilienceHandler() from Microsoft.Extensions.Http.Resilience.

Either AddSingletonIfNotExists has to be updated to handle keyed services, or the ServiceDescriptor implementation has to be changed to not throw exception on property access.

The workaround of having to register Application Insights (or anything else not updated to handle keyed services) first, before anything using keyed service, is not acceptable in the long run. This will not scale as keyed services usage increases in the future.

[wc-whiteheadd]

@tore-hammervoll This was not meant as a permanent fix, it was simply to allow people to progress while the Microsoft team look into it and resolve the problem. My suggestion was to save people having to go through and look at the internal code just to get their setup working.

I do agree with you that in the longer term, this issue will need to be looked at but i would assume thats why this ticket is still marked as "open"?

[tore-hammervoll]

@wc-whiteheadd My post was not intended as a response to the workaround, that was unintended. The workaround you posted has unblocked us from this bug, and is much appreciated.

I only intended to stress the importance of an actual fix, before this problem gets bigger when more third party packages (or official Microsoft packages) starts relying on keyed services. The consequences of this issue is a bug that is hard to debug, especially when it is triggered by completely unrelated changes. That coupled with the fact that it can even be hard to discover since the exception is caught in AddApplicationInsights(), and only manifests as live metrics not working in a running application (I suspect their are other potential issues caused by this, as there are several services that are not registered properly when this exception is thrown). I do not envy anyone finding out their live metrics (or whatever else) has not worked for days, weeks, or even months, and need to find out what caused it.

[wc-whiteheadd]

@tore-hammervoll that makes senses and i completely agree with you, this certainly needs fixing quite urgently. Hopefully our information will help their investigation into fixing it faster 😄

[steveoshima]

Wow, just spent hours trying to figure out why a simple application insights setup didn't work in my .net core 8 web api.
I put the application insights setup above the keyed services and its working now.
Get it fixed ASAP please.

[CommCody]

I would like to bump this as well!
It is still a problem. It manifested in our case because we upgraded a totally unrelated nuget package, but because of this is de ordering of Services.Add... methods super important. This is completely illogical.

Please prioritize this

1+

Just hit this while working with Semantic Kernel, which have Keyed services in all their examples.

[zhimin-z]

This issue is stale because it has been open 300 days with no activity. Remove stale label or this will be closed in 7 days. Commenting will instruct the bot to automatically remove the label.

[jkonecki]

It really shouldn't take 2 years for the team to triage and comment on a reported bug. Maybe too many devs were replaced with Copilot?