Add log event name for Identity #16879

Kahbazi · 2019-11-06T21:01:55Z

Added event id and name for logs.
change Some of event ids that were 0.
make logs structured in Identity/ApiAuthorization.IdentityServer

Addresses #6381

HaoK · 2020-03-10T05:33:59Z

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

@@ -714,7 +714,7 @@ public virtual async Task<bool> CheckPasswordAsync(TUser user, string password)
            var success = result != PasswordVerificationResult.Failed;
            if (!success)
            {
-                Logger.LogWarning(0, "Invalid password for user {userId}.", await GetUserIdAsync(user));
+                Logger.LogWarning(LoggerEventIds.InvalidPassword, "Invalid password for user {userId}.", await GetUserIdAsync(user));


I don't think we can just change log ids like this can we? @ajcvickers @blowdart is there some kind of guarantee with event ids being stable generally in logs?

Yes, they are expected to remain stable.

@Kahbazi so I guess if you are going to move to a variable for these, you are going to have to ensure that the order/values match.

HaoK · 2020-06-29T23:30:44Z

Closing since this PR changes event ids

Kahbazi · 2020-06-30T09:39:58Z

@HaoK I forgot about this PR! I will change back the event ids. Can I continue on this PR or should I create a new one?

HaoK · 2020-06-30T10:02:41Z

You can continue using this one sure!

HaoK · 2020-07-15T21:46:51Z

src/Identity/Core/src/SignInManager.cs

@@ -291,7 +291,7 @@ public virtual async Task<TUser> ValidateTwoFactorSecurityStampAsync(ClaimsPrinc
            {
                return user;
            }
-            Logger.LogDebug(5, "Failed to validate a security stamp.");


Looks like we have different event ids for the same message, 4 and 5 due to different methods

@Kahbazi I think its fine to introduce the logging extensions to the API which wasn't currently using event ids, but it doesn't look like its worth the potential regression in modifying the event ids for the sign in manager/existing apis. Unless you want to introduce a line by line method, so maybe lets just revert/delete LogginExtensions, but keep the LoggerEventIds changes

I wanted to be consentience for each log. Should I delete LogginExtensions completely and add event id to each logs?
Or add another method to LogginExtensions for invalid security stamp?

Right maybe the safest way to do it is https://github.com/dotnet/aspnetcore/pull/16879/files#diff-7d0bb5dd924f2d3e3de48da228cfbd19 so introduce a new file that's just the ids,, so no LoggingExtensions class like you said

HaoK · 2020-11-02T08:30:58Z

src/Identity/UI/src/Areas/Identity/Pages/V4/Account/Register.cshtml.cs

@@ -138,7 +138,7 @@ public override async Task<IActionResult> OnPostAsync(string returnUrl = null)

                if (result.Succeeded)
                {
-                    _logger.LogInformation("User created a new account with password.");
+                    _logger.LogInformation(LoggerEventIds.UserCreated, "User created a new account with password.");


@blowdart any concerns with logging using an event id in the library and scaffolding still logging like before?

Yes actually. If once you scaffold your log event ids change that's a bad thing. They need to match.

So are you saying you prefer the UI just doesn't specify event ids? It seems bad to scaffold out a new file just to add event ids

@Kahbazi I chatted with @blowdart offline and looks like this is pretty much ready to go except for one minor tweak, can you make the LoggerEventIds class public that the Identity UI code uses, since we want to be able to reference that in the scaffolded code. When we port these changes to the scaffolded pages

HaoK · 2020-11-20T23:13:00Z

Thanks for making it public @Kahbazi unfortunately you will need to add doccomments for these now as well :/

Kahbazi · 2020-11-21T16:17:36Z

Thanks for making it public @Kahbazi unfortunately you will need to add doccomments for these now as well :/

Do I need to add documents for just the class or include each EventId? If so could you please provide a sample so I can follow the sentence structure.

HaoK · 2020-12-10T23:21:02Z

Sorry for the delay getting back to you @Kahbazi you will need to document public/protected just like everything else for apis, I don't think you need to go into much detail, for example: "Static class that exposes logging event ids." for the class summary, then something like "Event id when a user is created by an external provider." for UserCreatedByExternalProvider as an example.

HaoK · 2021-01-11T22:33:00Z

Hey @Kahbazi thanks for grinding through this, do you think you can try applying the relevant steps here to fix the issues with the public surface area? #24347 (comment)

With step 7 being the only thing you really need to do, fixing the new api stuff I think, since you don't need to add the baseline files

Choose “Add Blah to public API” / “Fix all occurrences in … Solution”
Click Apply

Let me know if that doesn't work, I can take over the PR and merge it into one of my branches for the final step if the public api baselines don't update for you correctly

HaoK · 2021-01-26T02:10:32Z

Thanks for sticking with this and getting this done @Kahbazi !

Pilchie added the area-identity Includes: Identity and providers label Nov 7, 2019

Kahbazi requested a review from HaoK February 20, 2020 12:46

Tratcher assigned HaoK Feb 20, 2020

HaoK reviewed Mar 10, 2020

View reviewed changes

HaoK closed this Jun 29, 2020

HaoK reopened this Jun 30, 2020

Kahbazi force-pushed the kahbazi/identityEventLog branch from 3f067ee to c099cd1 Compare July 2, 2020 08:47

jaredpar mentioned this pull request Jul 2, 2020

OSX machines are de-provisioned during CI / PR runs leading to failures dotnet/runtime#34472

Closed

HaoK reviewed Jul 15, 2020

View reviewed changes

Kahbazi force-pushed the kahbazi/identityEventLog branch from c099cd1 to fd4714c Compare October 8, 2020 12:22

HaoK reviewed Nov 2, 2020

View reviewed changes

Tratcher added the community-contribution Indicates that the PR has been added by a community member label Nov 19, 2020

Kahbazi added 4 commits January 14, 2021 12:27

Add log event name for Identity

e63b59b

make the EventId for UI public

5e5778b

Add api doc

ffaf3ec

Add public apis

cd53b2e

Kahbazi force-pushed the kahbazi/identityEventLog branch from 6063ad1 to cd53b2e Compare January 15, 2021 21:57

Base automatically changed from master to main January 22, 2021 01:32

HaoK approved these changes Jan 25, 2021

View reviewed changes

HaoK merged commit 8822621 into dotnet:main Jan 26, 2021

Kahbazi deleted the kahbazi/identityEventLog branch January 26, 2021 05:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add log event name for Identity #16879

Add log event name for Identity #16879

Kahbazi commented Nov 6, 2019

HaoK Mar 10, 2020

ajcvickers Mar 10, 2020

HaoK Mar 10, 2020

HaoK commented Jun 29, 2020

Kahbazi commented Jun 30, 2020

HaoK commented Jun 30, 2020

HaoK Jul 15, 2020

HaoK Jul 15, 2020

Kahbazi Jul 23, 2020

HaoK Aug 5, 2020

HaoK Nov 2, 2020

blowdart Nov 2, 2020

HaoK Nov 2, 2020

HaoK Nov 19, 2020

HaoK commented Nov 20, 2020

Kahbazi commented Nov 21, 2020

HaoK commented Dec 10, 2020 •

edited

Loading

HaoK commented Jan 11, 2021 •

edited

Loading

HaoK commented Jan 26, 2021

Add log event name for Identity #16879

Add log event name for Identity #16879

Conversation

Kahbazi commented Nov 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoK commented Jun 29, 2020

Kahbazi commented Jun 30, 2020

HaoK commented Jun 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoK commented Nov 20, 2020

Kahbazi commented Nov 21, 2020

HaoK commented Dec 10, 2020 • edited Loading

HaoK commented Jan 11, 2021 • edited Loading

HaoK commented Jan 26, 2021

HaoK commented Dec 10, 2020 •

edited

Loading

HaoK commented Jan 11, 2021 •

edited

Loading