Added name for EventId in logs #11379

mikeo52 · 2019-06-19T17:16:31Z

Added name for EventId in Hosting, DataProtection, EntityFrameworkCore. #6381

dnfclas · 2019-06-19T17:16:45Z

All CLA requirements met.

analogrelay

Looks good, but we should remove the nameofs. Even though they reference LoggerEventIds, we still want to reduce the possibility of unintentionally changing these names without realizing.

analogrelay · 2019-06-19T18:24:43Z

src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs

@@ -172,7 +172,7 @@ private void LogRequestStarting(HttpContext httpContext)
            // IsEnabled is checked in the caller, so if we are here just log
            _logger.Log(
                logLevel: LogLevel.Information,
-                eventId: LoggerEventIds.RequestStarting,
+                eventId: new EventId(LoggerEventIds.RequestStarting, nameof(LoggerEventIds.RequestStarting)),


I would avoid nameof here. Even though we don't intent to change this, if we did ever change the name of the property it would break consumers. We started using nameof originally but decided it was better to just use string constants. The goal of nameof is to ensure that when you rename the target of the nameof expression, your reference is updated which is actually not what we want here.

I hadn't thought about it that way at first, but I can see your point. This should be corrected in the latest commit.

analogrelay

Looks good. It's unfortunate that some of these have 0 as the numeric event ID, but changing that is a breaking change and I don't think we want to do that.

analogrelay · 2019-06-19T20:08:55Z

Build failures look unrelated. I'll restart.

analogrelay · 2019-06-19T20:09:04Z

@aspnet-hello triage https://dev.azure.com/dnceng/public/_build/results?buildId=230970

aspnet-hello · 2019-06-19T20:12:28Z

This comment was made automatically. If there is a problem contact [email protected].

I've triaged the above build. I've created/commented on the following issue(s)
https://github.com/aspnet/AspNetCore-Internal/issues/2220
https://github.com/aspnet/AspNetCore-Internal/issues/2410
https://github.com/aspnet/AspNetCore-Internal/issues/2623
https://github.com/aspnet/AspNetCore-Internal/issues/2489
https://github.com/aspnet/AspNetCore-Internal/issues/2415
https://github.com/aspnet/AspNetCore-Internal/issues/2674

Kahbazi · 2019-08-11T06:03:55Z

@anurse is there any chance that this could merge into asp.net core 3?

Tratcher · 2019-08-11T15:43:33Z

/azp run

azure-pipelines · 2019-08-11T15:43:45Z

Azure Pipelines successfully started running 2 pipeline(s).

Kahbazi · 2019-08-25T16:51:19Z

@mikeo52 Can you resolve the conflict?
I'm hoping this would reach the 3.0 version.

Tratcher · 2019-08-25T21:03:33Z

FYI this is targeting the wrong branch for v3.x. It would need to be rebased and retargeted to release/3.0.

@Pilchie bar check for 3.x? Incremental diagnostic improvements.

Pilchie · 2019-08-26T20:04:14Z

It's very late for 3.0, but does fit into a diagnosability theme for 3.1.

analogrelay · 2019-10-03T16:36:28Z

Oof, this got lost in some of the push to get 3.0 out the door. I'm going to respin the builds and see where we land. There is also a conflict to resolve.

analogrelay · 2019-10-03T16:37:04Z

@mikeo52 are you up to try rebasing this on master and resolving the conflict so we can try to take this for 5.0 at least?

mikeo52 · 2019-10-21T04:29:28Z

@anurse yeah I updated it locally since there are just two spots where it's conflicting, but I'm having a lovely fun time merging because of the same error someone else reported below.

#15125

mikeo52 · 2019-10-21T04:36:33Z

Okay that makes absolutely no sense to me, but whatever. On this PR's merge conflicts screen I can edit the merge to my repo via the browser and resolve the conflicts, but if I use the git command line I constantly run into the other issue.

analogrelay

A few small things. Looking good though. Thanks for resolving the conflict!

src/DataProtection/DataProtection/src/LoggingExtensions.cs

analogrelay · 2019-10-21T15:49:19Z

src/DataProtection/DataProtection/src/LoggingExtensions.cs

                logLevel: LogLevel.Warning,
                formatString: Resources.FileSystem_EphemeralKeysLocationInContainer);

            _usingRegistryAsKeyRepositoryWithDPAPI = LoggerMessage.Define<string>(
-                eventId: 0,
+                eventId: new EventId(0, "UsingRegistryAsKeyRepositoryWithDPAPI"),


Given that this is 5.0, I feel OK putting proper EventIds in place here.

For simplicity, I think it's OK to just use the same EventId sequence as the previous ones, even if these messages are used with different loggers. Thoughts @davidfowl ? It makes it easy to keep them consistent and EventIds are arbitrary anyway.

@mikeo52 can you continue the EventId number sequence with these last few events? (61, 62, 63...)

@anurse should be good to go with the latest commit.

analogrelay · 2019-10-21T15:51:11Z

src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs

@@ -175,7 +175,7 @@ private void LogRequestStarting(HostingApplication.Context context)

            _logger.Log(
                logLevel: LogLevel.Information,
-                eventId: LoggerEventIds.RequestStarting,
+                eventId: new EventId(LoggerEventIds.RequestStarting, "RequestStarting"),


I think you could go to LoggerEventIds and change them from ints to EventId structs (you'd also have to change from const to static readonly fields but it's an internal type so I think that's OK. Could you give that a look @mikeo52 ?

@anurse I think I've got the change you were after made in this regard. Take a look and let me know. I'm having trouble building locally since my local setup's a little borked, so my apologies if the extra commits are making noise.

analogrelay · 2019-10-21T15:51:28Z

src/Hosting/Hosting/src/Internal/HostingLoggerExtensions.cs

@@ -22,15 +22,15 @@ public static IDisposable RequestScope(this ILogger logger, HttpContext httpCont
        public static void ApplicationError(this ILogger logger, Exception exception)
        {
            logger.ApplicationError(
-                eventId: LoggerEventIds.ApplicationStartupException,
+                eventId: new EventId(LoggerEventIds.ApplicationStartupException, "ApplicationStartupException"),


Same here, we should try to just replace the LoggerEventIds constants themselves.

…ggingExtensions.

Co-Authored-By: Andrew Stanton-Nurse <[email protected]>

Tratcher · 2019-11-04T16:51:48Z

Thanks

Kahbazi · 2019-11-04T18:13:30Z

@Tratcher Is it possible to also merge this to 3.1?

Tratcher · 2019-11-04T20:57:03Z

No, 3.1 is only taking critical patches now.

Kahbazi · 2019-11-04T21:00:42Z

I see, thanks.

mikeo52 added 2 commits June 18, 2019 14:38

Added log event names to Hosting.

d060c3d

Added log event names to DataProtection and EntityFrameworkCore

746d332

mikeo52 requested review from analogrelay and Tratcher as code owners June 19, 2019 17:16

Eilon added area-dataprotection Includes: DataProtection area-hosting labels Jun 19, 2019

analogrelay reviewed Jun 19, 2019

View reviewed changes

Replacing nameof with string constants.

712e480

analogrelay approved these changes Jun 19, 2019

View reviewed changes

Merge branch 'master' into master

11a5202

analogrelay suggested changes Oct 21, 2019

View reviewed changes

Kahbazi mentioned this pull request Oct 25, 2019

Provide new EventId implicitly with tuple syntax dotnet/extensions#2543

Closed

Mike Oliver and others added 4 commits November 4, 2019 04:28

Changing LoggerEventIds to be EventIds and continuing numbering in Lo…

169e000

…ggingExtensions.

Update src/DataProtection/DataProtection/src/LoggingExtensions.cs

66510ce

Co-Authored-By: Andrew Stanton-Nurse <[email protected]>

That's what I get for forgetting a namespace.

11a478b

Merge branch 'master' of https://github.com/mikeo52/AspNetCore

fa6368f

Tratcher approved these changes Nov 4, 2019

View reviewed changes

Tratcher merged commit 774f8db into dotnet:master Nov 4, 2019

Kahbazi mentioned this pull request Jul 9, 2020

Moientajik/EventIds refactor dotnet/yarp#318

Closed

campersau mentioned this pull request May 5, 2021

Convert HealthChecks logging to use new Logging Source Generator #32414

Merged

amcasey added the area-hosting Includes Hosting label Jun 1, 2023

Added name for EventId in logs #11379

Added name for EventId in logs #11379

Uh oh!

Conversation

mikeo52 commented Jun 19, 2019

Uh oh!

dnfclas commented Jun 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

analogrelay left a comment

Choose a reason for hiding this comment

Uh oh!

analogrelay Jun 19, 2019

Choose a reason for hiding this comment

Uh oh!

mikeo52 Jun 19, 2019

Choose a reason for hiding this comment

Uh oh!

analogrelay left a comment

Choose a reason for hiding this comment

Uh oh!

analogrelay commented Jun 19, 2019

Uh oh!

analogrelay commented Jun 19, 2019

Uh oh!

aspnet-hello commented Jun 19, 2019

Uh oh!

Kahbazi commented Aug 11, 2019

Uh oh!

Tratcher commented Aug 11, 2019

Uh oh!

azure-pipelines bot commented Aug 11, 2019

Uh oh!

Kahbazi commented Aug 25, 2019

Uh oh!

Tratcher commented Aug 25, 2019

Uh oh!

Pilchie commented Aug 26, 2019

Uh oh!

analogrelay commented Oct 3, 2019

Uh oh!

analogrelay commented Oct 3, 2019

Uh oh!

mikeo52 commented Oct 21, 2019

Uh oh!

mikeo52 commented Oct 21, 2019

Uh oh!

analogrelay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

analogrelay Oct 21, 2019

Choose a reason for hiding this comment

Uh oh!

mikeo52 Nov 4, 2019

Choose a reason for hiding this comment

Uh oh!

analogrelay Oct 21, 2019

Choose a reason for hiding this comment

Uh oh!

mikeo52 Nov 4, 2019

Choose a reason for hiding this comment

Uh oh!

analogrelay Oct 21, 2019

Choose a reason for hiding this comment

Uh oh!

Tratcher commented Nov 4, 2019

Uh oh!

Kahbazi commented Nov 4, 2019

Uh oh!

Tratcher commented Nov 4, 2019

Uh oh!

Kahbazi commented Nov 4, 2019

Uh oh!

Uh oh!

dnfclas commented Jun 19, 2019 •

edited

Loading