-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Update identity metrics with feedback #62982
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
Conversation
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 updates the ASP.NET Core Identity metrics with significant naming and type changes based on user feedback. The changes focus on converting timing operations from counters to histograms with duration suffixes, and renaming several metrics for clarity and consistency.
Key changes:
- Convert create/update/delete user operations from counters to histograms tracking duration
- Rename metrics to be more descriptive (e.g., adding "attempts" suffix for password checks)
- Update SignInManager metrics with better naming conventions
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/Shared/ValueStopwatch/ValueStopwatch.cs |
Refactored timing logic to support duration measurements |
src/Identity/test/Shared/MetricsHelpers.cs |
Added helper method for validating duration metrics |
src/Identity/test/Identity.Test/UserManagerTest.cs |
Updated test assertions to validate histogram duration metrics |
src/Identity/test/Identity.Test/SignInManagerTest.cs |
Updated test metric collectors for renamed SignInManager metrics |
src/Identity/Extensions.Core/src/UserUpdateType.cs |
Renamed UserName enum value to SetUserName |
src/Identity/Extensions.Core/src/UserManagerMetrics.cs |
Converted counters to histograms and updated metric names |
src/Identity/Extensions.Core/src/UserManager.cs |
Added timestamp tracking for duration measurements |
src/Identity/Extensions.Core/src/Microsoft.Extensions.Identity.Core.csproj |
Added ValueStopwatch shared source reference |
src/Identity/Core/src/SignInManagerMetrics.cs |
Updated SignInManager metric names and types |
src/Identity/Core/src/SignInManager.cs |
Added timestamp tracking for authenticate duration |
src/Identity/Core/src/Microsoft.AspNetCore.Identity.csproj |
Added ValueStopwatch shared source reference |
MackinnonBuck
left a comment
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.
Looks great!
Based on feedback at open-telemetry/semantic-conventions#2508
Also pluralizes the counter names which makes them more consistent with other ASP.NET Core counters.
User metrics:
aspnetcore.identity.user.create,aspnetcore.identity.user.update,aspnetcore.identity.user.deletenames to have.durationsuffix. Change from counters to histogramsaspnetcore.identity.user.check_passwordtoaspnetcore.identity.user.check_password_attemptsaspnetcore.identity.user.verify_tokentoaspnetcore.identity.user.verify_token_attemptsaspnetcore.identity.user.generate_tokentoaspnetcore.identity.user.generated_tokensSign in metrics:
aspnetcore.identity.sign_in.authenticatetoaspnetcore.identity.sign_in.authenticate.durationand from counter to histogramaspnetcore.identity.sign_in.remember_two_factortoaspnetcore.identity.sign_in.two_factor_clients_rememberedaspnetcore.identity.sign_in.forget_two_factortoaspnetcore.identity.sign_in.two_factor_clients_forgottenaspnetcore.identity.sign_in.check_passwordtoaspnetcore.identity.sign_in.check_password_attemptsaspnetcore.identity.sign_in.sign_intoaspnetcore.identity.sign_in.sign_insaspnetcore.identity.sign_in.sign_outtoaspnetcore.identity.sign_in.sign_outsMetadata:
aspnetcore.identity.authentication_schemetoaspnetcore.authentication.scheme(shared with AuthN metrics)aspnetcore.identity.sign_in.is_persistenttoaspnetcore.authentication.is_persistent(in case AuthN metrics also records it)