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

Fix issue where AppInsights SDK doesn't load/configure correctly due to keyed services being registered in DI #594

Merged
merged 4 commits into from
Apr 16, 2024

Conversation

martinothamar
Copy link
Contributor

@martinothamar martinothamar commented Apr 16, 2024

Description

A bug report came in, where logs/telemetry wasn't being shipped to AppInsights.
Upon inspecting the IServiceCollection registrations after calling AddApplicationInsightsTelemetry, we discovered that everything until (and including) these lines of registrations succeeded:
https://github.com/microsoft/ApplicationInsights-dotnet/blob/4626e3632323b741a2bd56d3d529c283cf7626c9/NETCORE/src/Microsoft.ApplicationInsights.AspNetCore/Extensions/ApplicationInsightsExtensions.cs#L134

However the TelemetryClient itself was never registered in DI. In the code linked above, if there is an exception, it is (relatively silently) swallowed and registered on the event source for AppInsights.

We traced the event source in a local test app:

dotnet trace collect --providers Microsoft-ApplicationInsights-AspNetCore -- ./bin/Debug/net8.0/Altinn.App

Which could be loaded into PerfView

image

We see this error:

HasStack="True" ThreadID="2,558,902" ProcessorNumber="0" errorMessage="System.InvalidOperationException: This service descriptor is keyed. Your service provider may not support keyed services. at Microsoft.Extensions.DependencyInjection.ServiceDescriptor.ThrowKeyedDescriptor() 
at Microsoft.Extensions.DependencyInjection.ServiceDescriptor.get_ImplementationFactory() 
at Microsoft.Extensions.DependencyInjection.ApplicationInsightsExtensions.<>c__28`2.<AddSingletonIfNotExists>b__28_0(ServiceDescriptor o) 
at System.Linq.Enumerable.Any[TSource](IEnumerable`1 source, Func`2 predicate) 
at Microsoft.Extensions.DependencyInjection.ApplicationInsightsExtensions.AddSingletonIfNotExists[TService,TImplementation](IServiceCollection services) 
at Microsoft.Extensions.DependencyInjection.ApplicationInsightsExtensions.AddCommonTelemetryModules(IServiceCollection services) at Microsoft.Extensions.DependencyInjection.ApplicationInsightsExtensions.AddApplicationInsightsTelemetry(IServiceCollection services)" appDomainName="Altinn.App" 

Which means that AppInsights thinks that our DI container (Scrutor) doesn't support keyed services. I think that's right? Since there is an open issue on that: khellang/Scrutor#209

This PR replaces keyed services by just injecting multiple services, and introducing a second marker interface to distinguish between them instead.

Other options:

  • Use a different DI container (for example the built in one), which supports keyed services right now?
  • ?

Related Issue(s)

  • N/A

Verification

  • Your code builds clean without any errors or warnings
  • Manual testing done (required)
  • Relevant automated test added (if you find this hard, leave it and we'll help out)
  • All tests run green

Documentation

  • User documentation is updated with a separate linked PR in altinn-studio-docs. (if applicable)

Copy link

sonarcloud bot commented Apr 16, 2024

@martinothamar
Copy link
Contributor Author

We can probably also just register AppInsights earlier in the DI container, since the issue is provoked when AppInsights scans the registry.. https://github.com/microsoft/ApplicationInsights-dotnet/blob/4626e3632323b741a2bd56d3d529c283cf7626c9/NETCORE/src/Shared/Extensions/ApplicationInsightsExtensions.cs#L297
But that would feel less solid

@martinothamar martinothamar added bugfix Label Pull requests with bugfix. Used when generation releasenotes kind/bug Something isn't working labels Apr 16, 2024
@martinothamar martinothamar changed the title Fix issue where AppInsights SDK doesn't load/configure correctly Fix issue where AppInsights SDK doesn't load/configure correctly due to keyed services being registered in DI Apr 16, 2024
@martinothamar martinothamar merged commit dc62cf8 into main Apr 16, 2024
10 checks passed
@martinothamar martinothamar deleted the fix/replace-keyed-registration-with-normal branch April 16, 2024 13:01
@martinothamar martinothamar mentioned this pull request May 6, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Label Pull requests with bugfix. Used when generation releasenotes kind/bug Something isn't working
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants