-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Enable Identity telemetry by instantiating metrics classes directly #64265
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
base: main
Are you sure you want to change the base?
Conversation
|
Greetings human! You've submitted a PR that modifies code that is shared with https://github.com/dotnet/runtime . Please make sure you synchronize this code with the changes in that repo! |
Co-authored-by: captainsafia <1857993+captainsafia@users.noreply.github.com>
…able UserManagerMetrics registration Co-authored-by: captainsafia <1857993+captainsafia@users.noreply.github.com>
| </ItemGroup> | ||
|
|
||
| <ItemGroup> | ||
| <InternalsVisibleTo Include="Microsoft.AspNetCore.Identity" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Is this required? We want to avoid adding InternalsVisibleTo between product assemblies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed in 749b76d. Now only SignInManagerMetrics (which is in Core) is registered in Core's AddIdentity. UserManagerMetrics (which is in Extensions.Core) is only registered in Extensions.Core's AddIdentityCore.
Co-authored-by: JamesNK <303201+JamesNK@users.noreply.github.com>
|
@copilot Change UserManagerMetrics and SignInManagerMetrics to not be registered in DI. Instead, create types using their constructor and just DI to resolve their |
Co-authored-by: JamesNK <303201+JamesNK@users.noreply.github.com>
Updated in bb2a007. Metrics classes are now instantiated directly in the manager constructors, resolving only |
|
@copilot Called |
Co-authored-by: JamesNK <303201+JamesNK@users.noreply.github.com>
Added in 7b2ae04. |
|
@copilot Add a test to MapIdentityApiTests in Microsoft.AspNetCore.Identity.FunctionalTests that asserts that metrics are recorded for UserManager and SignInManager. |
…ager Co-authored-by: JamesNK <303201+JamesNK@users.noreply.github.com>
Added in 756e4f0. The test
The test uses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors how metrics are initialized in the Identity system by transitioning from singleton-registered metrics classes to on-demand instantiation via IMeterFactory. The change simplifies the dependency injection setup while maintaining the same metrics functionality.
- Removed explicit singleton registrations of
UserManagerMetricsandSignInManagerMetricsfrom service collection configuration - Modified
UserManagerandSignInManagerto instantiate metrics classes directly fromIMeterFactorywhen available - Added
AddMetrics()calls to Identity service registration methods to ensureIMeterFactoryis available - Added functional test to verify metrics are properly recorded in real scenarios
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Identity/test/Shared/MockHelpers.cs | Removed manual singleton registrations of metrics classes in test helper methods |
| src/Identity/Extensions.Core/src/UserManager.cs | Changed to instantiate UserManagerMetrics directly from IMeterFactory instead of retrieving from DI |
| src/Identity/Core/src/SignInManager.cs | Changed to instantiate SignInManagerMetrics directly from IMeterFactory instead of retrieving from DI |
| src/Identity/Extensions.Core/src/IdentityServiceCollectionExtensions.cs | Added AddMetrics() call to ensure IMeterFactory is registered |
| src/Identity/Core/src/IdentityServiceCollectionExtensions.cs | Added AddMetrics() call to ensure IMeterFactory is registered |
| src/Identity/Extensions.Core/src/Microsoft.Extensions.Identity.Core.csproj | Added reference to Microsoft.Extensions.Diagnostics package |
| src/Identity/Core/src/Microsoft.AspNetCore.Identity.csproj | Added reference to Microsoft.Extensions.Diagnostics package |
| src/Identity/test/Identity.FunctionalTests/Microsoft.AspNetCore.Identity.FunctionalTests.csproj | Added shared TestMeterFactory and Microsoft.Extensions.Diagnostics.Testing reference for testing |
| src/Identity/test/Identity.FunctionalTests/MapIdentityApiTests.cs | Added functional test to verify metrics are recorded for both UserManager and SignInManager operations |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Plan: Enable Identity telemetry by instantiating metrics classes directly
SignInManagerMetricsandUserManagerMetricsare not registered in DI, causing metrics to not surfaceSummary
Successfully enabled Identity metrics to surface in applications by instantiating metrics classes directly in their respective manager constructors and ensuring the metrics infrastructure is properly initialized.
Final Approach
The metrics classes are instantiated directly in the constructors of
UserManagerandSignInManager:IMeterFactoryfrom DI and createsSignInManagerMetricsinstanceIMeterFactoryfrom DI and createsUserManagerMetricsinstanceAddIdentityCoreandAddIdentityto ensure metrics infrastructure is initializedThis approach:
IMeterFactoryand metrics infrastructure are properly registeredIMeterFactoryto be available in DIInternalsVisibleToneeded)Changes Made
SignInManagerMetricsdirectly usingIMeterFactoryfrom DIUserManagerMetricsdirectly usingIMeterFactoryfrom DIservices.AddMetrics()call inAddIdentityservices.AddMetrics()call inAddIdentityCoreMicrosoft.Extensions.DiagnosticsMicrosoft.Extensions.DiagnosticsIMeterFactory, not the metrics classesMetricsAreRecordedForUserManagerAndSignInManagertest to verify metrics are recordedTestMeterFactoryand reference toMicrosoft.Extensions.Diagnostics.TestingTesting
Fixes #64264
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.