Apply feedback

Author: JamesNK

src/Identity/Core/src/SignInManagerMetrics.cs

@@ -31,11 +31,11 @@ public SignInManagerMetrics(IMeterFactory meterFactory)
     {
         _meter = meterFactory.Create(MeterName);
 
-        _authenticateCounter = _meter.CreateCounter<long>(AuthenticateCounterName, "count", "The number of authenticate and sign in attempts.");
+        _authenticateCounter = _meter.CreateCounter<long>(AuthenticateCounterName, "count", "The number of authenticate attempts. The authenticate counter is incremented by sign in methods such as PasswordSignInAsync and TwoFactorSignInAsync.");
         _rememberTwoFactorClientCounter = _meter.CreateCounter<long>(RememberTwoFactorCounterName, "count", "The number of two factor clients remembered.");
         _forgetTwoFactorCounter = _meter.CreateCounter<long>(ForgetTwoFactorCounterName, "count", "The number of two factor clients forgotten.");
         _refreshCounter = _meter.CreateCounter<long>(RefreshCounterName, "count", "The number of refresh sign-in attempts.");
-        _checkPasswordCounter = _meter.CreateCounter<long>(CheckPasswordCounterName, "count", "The number of check password attempts.");
+        _checkPasswordCounter = _meter.CreateCounter<long>(CheckPasswordCounterName, "count", "The number of check password attempts. Checks that the account is in a state that can log in and that the password is valid using the UserManager.CheckPasswordAsync method.");
         _signInUserPrincipalCounter = _meter.CreateCounter<long>(SignInUserPrincipalCounterName, "count", "The number of user principals signed in.");
         _signOutUserPrincipalCounter = _meter.CreateCounter<long>(SignOutUserPrincipalCounterName, "count", "The number of user principals signed out.");
     }
@@ -61,10 +61,7 @@ internal void AuthenticateSignIn(string userType, string authenticationScheme, S
             { "aspnetcore.identity.sign_in.type", GetSignInType(signInType) },
             { "aspnetcore.identity.sign_in.is_persistent", isPersistent },
         };
-        if (result != null)
-        {
-            tags.Add("aspnetcore.identity.sign_in.result", GetSignInResult(result));
-        }
+        AddSignInResult(ref tags, result);
         AddExceptionTags(ref tags, exception);
 
         _authenticateCounter.Add(1, tags);

src/Identity/Extensions.Core/src/UserManager.cs

@@ -936,11 +936,9 @@ protected virtual async Task<PasswordVerificationResult> VerifyPasswordAsync(IUs
         var hash = await store.GetPasswordHashAsync(user, CancellationToken).ConfigureAwait(false);
         if (hash == null)
         {
-            _metrics?.VerifyPassword(typeof(TUser).FullName!, passwordMissing: true, result: null);
             return PasswordVerificationResult.Failed;
         }
         var result = PasswordHasher.VerifyHashedPassword(user, hash, password);
-        _metrics?.VerifyPassword(typeof(TUser).FullName!, passwordMissing: false, result);
 
         return result;
     }

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

@@ -18,7 +18,6 @@ internal sealed class UserManagerMetrics : IDisposable
     public const string UpdateCounterName = "aspnetcore.identity.user.update";
     public const string DeleteCounterName = "aspnetcore.identity.user.delete";
     public const string CheckPasswordCounterName = "aspnetcore.identity.user.check_password";
-    public const string VerifyPasswordCounterName = "aspnetcore.identity.user.verify_password";
     public const string VerifyTokenCounterName = "aspnetcore.identity.user.verify_token";
     public const string GenerateTokenCounterName = "aspnetcore.identity.user.generate_token";
 
@@ -27,7 +26,6 @@ internal sealed class UserManagerMetrics : IDisposable
     private readonly Counter<long> _updateCounter;
     private readonly Counter<long> _deleteCounter;
     private readonly Counter<long> _checkPasswordCounter;
-    private readonly Counter<long> _verifyPasswordCounter;
     private readonly Counter<long> _verifyTokenCounter;
     private readonly Counter<long> _generateTokenCounter;
 
@@ -37,8 +35,7 @@ public UserManagerMetrics(IMeterFactory meterFactory)
         _createCounter = _meter.CreateCounter<long>(CreateCounterName, "count", "The number of users created.");
         _updateCounter = _meter.CreateCounter<long>(UpdateCounterName, "count", "The number of user updates.");
         _deleteCounter = _meter.CreateCounter<long>(DeleteCounterName, "count", "The number of users deleted.");
-        _checkPasswordCounter = _meter.CreateCounter<long>(CheckPasswordCounterName, "count", "The number of check password attempts.");
-        _verifyPasswordCounter = _meter.CreateCounter<long>(VerifyPasswordCounterName, "count", "The number of password verification attempts.");
+        _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.");
     }
@@ -115,20 +112,6 @@ internal void CheckPassword(string userType, bool? userMissing, PasswordVerifica
         }
     }
 
-    internal void VerifyPassword(string userType, bool passwordMissing, PasswordVerificationResult? result)
-    {
-        if (_verifyPasswordCounter.Enabled)
-        {
-            var tags = new TagList
-            {
-                { "aspnetcore.identity.user_type", userType },
-                { "aspnetcore.identity.user.password_result", GetPasswordResult(result, passwordMissing, userMissing: null) },
-            };
-
-            _verifyPasswordCounter.Add(1, tags);
-        }
-    }
-
     internal void VerifyToken(string userType, bool? result, string purpose, Exception? exception = null)
     {
         var tags = new TagList

src/Identity/test/Identity.Test/UserManagerTest.cs

@@ -629,7 +629,6 @@ public async Task CheckPasswordWillRehashPasswordWhenNeeded()
         var testMeterFactory = new TestMeterFactory();
         using var updateUser = new MetricCollector<long>(testMeterFactory, "Microsoft.AspNetCore.Identity", UserManagerMetrics.UpdateCounterName);
         using var checkPassword = new MetricCollector<long>(testMeterFactory, "Microsoft.AspNetCore.Identity", UserManagerMetrics.CheckPasswordCounterName);
-        using var verifyPassword = new MetricCollector<long>(testMeterFactory, "Microsoft.AspNetCore.Identity", UserManagerMetrics.VerifyPasswordCounterName);
 
         var store = new Mock<IUserPasswordStore<PocoUser>>();
         var hasher = new Mock<IPasswordHasher<PocoUser>>();
@@ -667,11 +666,6 @@ public async Task CheckPasswordWillRehashPasswordWhenNeeded()
             [
                 KeyValuePair.Create<string, object>("aspnetcore.identity.user.password_result", "success_rehash_needed")
             ]));
-        Assert.Collection(verifyPassword.GetMeasurementSnapshot(),
-            m => MetricsHelpers.AssertContainsTags(m.Tags,
-            [
-                KeyValuePair.Create<string, object>("aspnetcore.identity.user.password_result", "success_rehash_needed")
-            ]));
     }
 
     [Fact]