Feedback from review

Author: JamesNK

src/Identity/Core/src/SignInManagerMetrics.cs

@@ -14,8 +14,8 @@ internal sealed class SignInManagerMetrics : IDisposable
     public const string RememberTwoFactorCounterName = "aspnetcore.identity.sign_in.remember_two_factor";
     public const string ForgetTwoFactorCounterName = "aspnetcore.identity.sign_in.forget_two_factor";
     public const string CheckPasswordCounterName = "aspnetcore.identity.sign_in.check_password";
-    public const string SignInUserPrincipalCounterName = "aspnetcore.identity.sign_in.sign_in_principal";
-    public const string SignOutUserPrincipalCounterName = "aspnetcore.identity.sign_in.sign_out_principal";
+    public const string SignInUserPrincipalCounterName = "aspnetcore.identity.sign_in.sign_in";
+    public const string SignOutUserPrincipalCounterName = "aspnetcore.identity.sign_in.sign_out";
 
     private readonly Meter _meter;
     private readonly Counter<long> _authenticateCounter;
@@ -29,12 +29,12 @@ public SignInManagerMetrics(IMeterFactory meterFactory)
     {
         _meter = meterFactory.Create(MeterName);
 
-        _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.");
-        _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 calls to sign in user principals.");
-        _signOutUserPrincipalCounter = _meter.CreateCounter<long>(SignOutUserPrincipalCounterName, "count", "The number of calls to sign out user principals.");
+        _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.");
+        _checkPasswordCounter = _meter.CreateCounter<long>(CheckPasswordCounterName, "{check}", "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, "{sign_in}", "The number of calls to sign in user principals.");
+        _signOutUserPrincipalCounter = _meter.CreateCounter<long>(SignOutUserPrincipalCounterName, "{sign_out}", "The number of calls to sign out user principals.");
     }
 
     internal void CheckPasswordSignIn(string userType, SignInResult? result, Exception? exception = null)

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

@@ -32,12 +32,12 @@ internal sealed class UserManagerMetrics : IDisposable
     public UserManagerMetrics(IMeterFactory meterFactory)
     {
         _meter = meterFactory.Create(MeterName);
-        _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. 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.");
+        _updateCounter = _meter.CreateCounter<long>(UpdateCounterName, "{update}", "The number of user updates.");
+        _deleteCounter = _meter.CreateCounter<long>(DeleteCounterName, "{delete}", "The number of users deleted.");
+        _checkPasswordCounter = _meter.CreateCounter<long>(CheckPasswordCounterName, "{check}", "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.");
     }
 
     internal void CreateUser(string userType, IdentityResult? result, Exception? exception = null)
@@ -52,7 +52,7 @@ internal void CreateUser(string userType, IdentityResult? result, Exception? exc
             { "aspnetcore.identity.user_type", userType }
         };
         AddIdentityResultTags(ref tags, result);
-        AddExceptionTags(ref tags, exception);
+        AddExceptionTags(ref tags, exception, result: result);
 
         _createCounter.Add(1, tags);
     }
@@ -70,7 +70,7 @@ internal void UpdateUser(string userType, IdentityResult? result, UserUpdateType
             { "aspnetcore.identity.user.update_type", GetUpdateType(updateType) },
         };
         AddIdentityResultTags(ref tags, result);
-        AddExceptionTags(ref tags, exception);
+        AddExceptionTags(ref tags, exception, result: result);
 
         _updateCounter.Add(1, tags);
     }
@@ -87,7 +87,7 @@ internal void DeleteUser(string userType, IdentityResult? result, Exception? exc
             { "aspnetcore.identity.user_type", userType }
         };
         AddIdentityResultTags(ref tags, result);
-        AddExceptionTags(ref tags, exception);
+        AddExceptionTags(ref tags, exception, result: result);
 
         _deleteCounter.Add(1, tags);
     }
@@ -105,7 +105,7 @@ internal void CheckPassword(string userType, bool? userMissing, PasswordVerifica
         };
         if (userMissing != null || result != null)
         {
-            tags.Add("aspnetcore.identity.user.password_result", GetPasswordResult(result, passwordMissing: null, userMissing));
+            tags.Add("aspnetcore.identity.password_check_result", GetPasswordResult(result, passwordMissing: null, userMissing));
         }
         AddExceptionTags(ref tags, exception);
 
@@ -183,15 +183,16 @@ private static void AddIdentityResultTags(ref TagList tags, IdentityResult? resu
         tags.Add("aspnetcore.identity.result", result.Succeeded ? "success" : "failure");
         if (!result.Succeeded && result.Errors.FirstOrDefault()?.Code is { Length: > 0 } code)
         {
-            tags.Add("aspnetcore.identity.result_error_code", code);
+            tags.Add("aspnetcore.identity.error_code", code);
         }
     }
 
-    private static void AddExceptionTags(ref TagList tags, Exception? exception)
+    private static void AddExceptionTags(ref TagList tags, Exception? exception, IdentityResult? result = null)
     {
-        if (exception != null)
+        var value = exception?.GetType().FullName ?? result?.Errors.FirstOrDefault()?.Code;
+        if (value != null)
         {
-            tags.Add("error.type", exception.GetType().FullName!);
+            tags.Add("error.type", value);
         }
     }
 

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

@@ -174,7 +174,7 @@ public async Task DeleteCallsStore_Failure()
             [
                 KeyValuePair.Create<string, object>("aspnetcore.identity.user_type", "Microsoft.AspNetCore.Identity.Test.PocoUser"),
                 KeyValuePair.Create<string, object>("aspnetcore.identity.result", "failure"),
-                KeyValuePair.Create<string, object>("aspnetcore.identity.result_error_code", "ConcurrencyFailure")
+                KeyValuePair.Create<string, object>("aspnetcore.identity.error_code", "ConcurrencyFailure")
             ]));
     }
 
@@ -664,7 +664,7 @@ public async Task CheckPasswordWillRehashPasswordWhenNeeded()
         Assert.Collection(checkPassword.GetMeasurementSnapshot(),
             m => MetricsHelpers.AssertContainsTags(m.Tags,
             [
-                KeyValuePair.Create<string, object>("aspnetcore.identity.user.password_result", "success_rehash_needed")
+                KeyValuePair.Create<string, object>("aspnetcore.identity.password_check_result", "success_rehash_needed")
             ]));
     }
 
@@ -871,7 +871,7 @@ public async Task CheckPasswordWithNullUserReturnsFalse()
         Assert.Collection(checkPassword.GetMeasurementSnapshot(),
             m => MetricsHelpers.AssertContainsTags(m.Tags,
             [
-                KeyValuePair.Create<string, object>("aspnetcore.identity.user.password_result", "user_missing")
+                KeyValuePair.Create<string, object>("aspnetcore.identity.password_check_result", "user_missing")
             ]));
     }