Fix ITelemetryModule singleton registration to support presence of keyed services #2908

CodeBlanch · 2024-10-22T19:24:05Z

Fixes #2879
Relates to dotnet/extensions#5222

Changes

Remove AddSingletonIfNotExists custom implementation for detection of registered services and call TryAddEnumerable instead because it understands keyed services added in 8.0.0

…hen keyed services are registered.

CodeBlanch · 2024-10-22T19:27:06Z

...Insights.AspNetCore.Tests/Extensions/ApplicationInsightsExtensionsTests/AddSingletonTests.cs

-    /// <summary>
-    /// When working with IServiceCossection, it can store three types of Implementations:
-    /// ImplementationFactory, ImplementationInstance, and ImplementationType.
-    /// We want to be able to add a Singleton but only if a user hasn't already done so.
-    /// This class is to test all the various edge cases.
-    /// </summary>


The issue is as of 8.0.0 this summary is false.

There are new "keyed" forms of the possible registration types:

AddSingletonIfNotExists could be made smarter to understand the new shape. But that would require an 8.0.0 bump for M.E.DI/.A in Microsoft.ApplicationInsights.AspNetCore.

Easier IMO to just call into TryAddEnumerable and let it sort out what to do!

CodeBlanch · 2024-10-22T19:28:53Z

Sorry about the whitespace on diff! Blame VS 🤣

TimothyMothra · 2024-10-22T20:13:37Z

....Tests/Extensions/ApplicationInsightsExtensionsTests/AddApplicationInsightsTelemetryTests.cs

@@ -37,6 +37,25 @@ namespace Microsoft.Extensions.DependencyInjection.Test

    public class AddApplicationInsightsTelemetryTests : BaseTestClass
    {
+        [Fact]
+        public static void TelemetryModuleResolvableWhenKeyedServiceRegistered()


This test appears to pass even without your change.
I copied it locally and ran it in main branch.

Interesting. It looks like without the fix, worker service can't find any registered ITelemetryHttpModule and the test fails. AspNetCore though finds RequestTrackingTelemetryModule registered so the test passes.

With the fix in place there are actually 7 ITelemetryHttpModules registered. I updated the tests to validate all 7 are found when keyed service is added to the mix.

Just pushed updates to the test to improve them further. Added validation that a) DiagnosticsTelemetryModule is present in the final list and b) if the user supplied one we don't end up with 2.

xiang17 · 2024-10-22T20:29:45Z

The bug report is for .NET 8, but does it apply for other .NET versions and .NET Framework?

CodeBlanch · 2024-10-22T20:34:07Z

The bug report is for .NET 8, but does it apply for other .NET versions and .NET Framework?

Could impact all runtimes. All user needs to do is take a dependency on something which a) brings in M.E.DI 8.0.0+ and b) registers a keyed service. In the case of the bug report, it is actually Polly registering the keyed service.

TimothyMothra

LGTM

rajkumar-rangaraj

LGTM, Thanks for fixing this quicker. Could you please add a changelog entry.

CodeBlanch · 2024-10-23T19:02:28Z

@TimothyMothra @rajkumar-rangaraj I pushed a change to call TryAddEnumerable instead of TryAddSingleton. Please take another look.

I was looking through the history and trying to figure out why we had AddSingletonIfNotExists at all when there are other spots calling TryAddSingleton. What I came to realize is we want multiple ITelemetryModules to be registered. But we only want 1 DiagnosticsTelemetryModule. TryAddSingleton<ITelemetryModule, DiagnosticsTelemetryModule>() won't add DiagnosticsTelemetryModule if some other ITelemetryModule is registered. That is incorrect! TryAddEnumerable is what we want here. What it will do is only add the DiagnosticsTelemetryModule registration if it hasn't already been registered.

So my guess is we had AddSingletonIfNotExists because TryAddSingleton didn't work and we didn't realize TryAddEnumerable was the thing to use 😄

CodeBlanch · 2024-10-23T19:10:02Z

NETCORE/src/Shared/Extensions/ApplicationInsightsExtensions.cs

@@ -332,7 +312,7 @@ private static void AddCommonInitializers(IServiceCollection services)
        private static void AddCommonTelemetryModules(IServiceCollection services)
        {
            // Previously users were encouraged to manually add the DiagnosticsTelemetryModule.
-            services.AddSingletonIfNotExists<ITelemetryModule, DiagnosticsTelemetryModule>();
+            services.TryAddEnumerable(ServiceDescriptor.Singleton<ITelemetryModule, DiagnosticsTelemetryModule>());


@TimothyMothra FYI I didn't make any other changes here but really all of the services.AddSingleton<ITelemetryModule, ...>(); calls should be TryAddEnumerable calls. If user only calls registration once everything will be fine. But if they call one of the "Add" extensions more than once, they'll get duplicates of all of these modules. Typically "Add" extensions should be safe to be called more than once which is accomplished by calling TryAddSingleton \ TryAddEnumerable 🤷

I don't remember the full history, but I reviewed all my previous commit comments.

It looks like I was trying to add safety checks for every possible way that a user could have added the DiagnosticsTelemetryModule (link 1)(link 2). The TryAdd is substantially better :)

Thank you

TimothyMothra · 2024-10-23T23:12:47Z

I pinged @cijothomas offline to also take a look. He was involved in the previous PR #1886

pcn-michaelulloa · 2024-11-04T14:24:38Z

Any updates on this? 👀👀

RussKie · 2024-12-16T01:48:29Z

...icationInsights.WorkerService.Tests/Microsoft.ApplicationInsights.WorkerService.Tests.csproj

    <PackageReference Include="Microsoft.Extensions.Configuration.Json" Version="2.1.0" />
+    <PackageReference Include="Microsoft.Extensions.DependencyInjection" Version="8.0.0" />
    <PackageReference Include="Microsoft.Extensions.Hosting" Version="2.1.0" />
    <PackageReference Include="Microsoft.Extensions.Configuration.EnvironmentVariables" Version="2.1.0" />


This looks as though it introduces version-mismatch. It's worth considering either using 2.1.0 across the board or bump all to 8.0.

Remove AddSingletonIfNotExists and call TryAddSingleton to fix bugs w…

55b8e03

…hen keyed services are registered.

CodeBlanch commented Oct 22, 2024

View reviewed changes

TimothyMothra reviewed Oct 22, 2024

View reviewed changes

Test tweaks.

d5f7d8b

TimothyMothra approved these changes Oct 22, 2024

View reviewed changes

rajkumar-rangaraj approved these changes Oct 22, 2024

View reviewed changes

CodeBlanch marked this pull request as draft October 23, 2024 18:41

Tweaks and CHANGELOG update.

42cd7e3

CodeBlanch marked this pull request as ready for review October 23, 2024 19:02

CodeBlanch commented Oct 23, 2024

View reviewed changes

captainsafia mentioned this pull request Oct 23, 2024

[WebToolsE2E][Aspire] Running with new Azure Function + Aspire, the error appears: "Application Insights SDK has not been added. Please add and configure the Application Insights SDK..." dotnet/aspire#6452

Closed

TimothyMothra approved these changes Oct 23, 2024

View reviewed changes

TimothyMothra added this to the 2.23 milestone Nov 6, 2024

claudiogodoy99 mentioned this pull request Dec 13, 2024

Error on handling AddSingletonIfNotExists (AddApplicationInsightsTelemetryWorkerService) #2921

Open

asvishnyakov mentioned this pull request Dec 13, 2024

App Insights breaks when a keyed service is added #2879

Open

RussKie reviewed Dec 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ITelemetryModule singleton registration to support presence of keyed services #2908

Fix ITelemetryModule singleton registration to support presence of keyed services #2908

CodeBlanch commented Oct 22, 2024 •

edited

Loading

CodeBlanch Oct 22, 2024 •

edited

Loading

CodeBlanch commented Oct 22, 2024

TimothyMothra Oct 22, 2024

CodeBlanch Oct 22, 2024

CodeBlanch Oct 23, 2024

xiang17 commented Oct 22, 2024

CodeBlanch commented Oct 22, 2024

TimothyMothra left a comment

rajkumar-rangaraj left a comment

CodeBlanch commented Oct 23, 2024

CodeBlanch Oct 23, 2024

TimothyMothra Oct 23, 2024

TimothyMothra commented Oct 23, 2024

pcn-michaelulloa commented Nov 4, 2024

RussKie Dec 16, 2024

Fix ITelemetryModule singleton registration to support presence of keyed services #2908

Are you sure you want to change the base?

Fix ITelemetryModule singleton registration to support presence of keyed services #2908

Conversation

CodeBlanch commented Oct 22, 2024 • edited Loading

Changes

CodeBlanch Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

CodeBlanch commented Oct 22, 2024

TimothyMothra Oct 22, 2024

Choose a reason for hiding this comment

CodeBlanch Oct 22, 2024

Choose a reason for hiding this comment

CodeBlanch Oct 23, 2024

Choose a reason for hiding this comment

xiang17 commented Oct 22, 2024

CodeBlanch commented Oct 22, 2024

TimothyMothra left a comment

Choose a reason for hiding this comment

rajkumar-rangaraj left a comment

Choose a reason for hiding this comment

CodeBlanch commented Oct 23, 2024

CodeBlanch Oct 23, 2024

Choose a reason for hiding this comment

TimothyMothra Oct 23, 2024

Choose a reason for hiding this comment

TimothyMothra commented Oct 23, 2024

pcn-michaelulloa commented Nov 4, 2024

RussKie Dec 16, 2024

Choose a reason for hiding this comment

CodeBlanch commented Oct 22, 2024 •

edited

Loading

CodeBlanch Oct 22, 2024 •

edited

Loading