Skip to content

Identity metrics clean up #62671

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Identity metrics clean up #62671

wants to merge 5 commits into from

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Jul 11, 2025

@JamesNK JamesNK added the area-identity Includes: Identity and providers label Jul 11, 2025
Copy link
Contributor

@Copilot Copilot AI left a 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 default metric value for unknown cases from _UNKNOWN to _OTHER across identity-related switch expressions, aligning with the correct metric nomenclature.

  • Replaces _UNKNOWN with _OTHER in GetTokenPurpose
  • Replaces _UNKNOWN with _OTHER in GetPasswordResult
  • Replaces _UNKNOWN with _OTHER in GetUpdateType
Comments suppressed due to low confidence (3)

src/Identity/Extensions.Core/src/UserManagerMetrics.cs:172

  • Add a unit test to verify that GetTokenPurpose returns "_OTHER" for unrecognized purposes.
            _ => "_OTHER"

src/Identity/Extensions.Core/src/UserManagerMetrics.cs:207

  • Add a unit test to verify that GetPasswordResult returns "_OTHER" for cases falling through the specified patterns.
            _ => "_OTHER"

src/Identity/Extensions.Core/src/UserManagerMetrics.cs:247

  • Add a unit test to verify that GetUpdateType returns "_OTHER" for unhandled UserUpdateType values.
            _ => "_OTHER"

@lewing
Copy link
Member

lewing commented Jul 11, 2025

Is _UNKNOWN correct here

@JamesNK JamesNK changed the title Replace _UNKNOWN with _OTHER in identity metrics Identity metrics clean up Jul 15, 2025
@JamesNK JamesNK requested a review from lmolkova July 15, 2025 06:13
_checkPasswordCounter = _meter.CreateCounter<long>(CheckPasswordCounterName, "count", "The number of check password attempts. Only checks whether the password is valid and not whether the user account is in a state that can log in.");
_verifyTokenCounter = _meter.CreateCounter<long>(VerifyTokenCounterName, "count", "The number of token verification attempts.");
_generateTokenCounter = _meter.CreateCounter<long>(GenerateTokenCounterName, "count", "The number of token generation attempts.");
_createCounter = _meter.CreateCounter<long>(CreateCounterName, "{create}", "The number of users created.");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it counts users, so should it be

Suggested change
_createCounter = _meter.CreateCounter<long>(CreateCounterName, "{create}", "The number of users created.");
_createCounter = _meter.CreateCounter<long>(CreateCounterName, "{user}", "The number of users created.");

?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'd like create, update and delete metrics to have a unit of {user}?

}
}

private static void AddExceptionTags(ref TagList tags, Exception? exception)
private static void AddExceptionTags(ref TagList tags, Exception? exception, IdentityResult? result = null)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

Suggested change
private static void AddExceptionTags(ref TagList tags, Exception? exception, IdentityResult? result = null)
private static void AddErrrorTags(ref TagList tags, Exception? exception, IdentityResult? result = null)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-identity Includes: Identity and providers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants