Add `SkipEnabledCheck` on `LoggerMessageAttribute` #54305

maryamariyan · 2021-06-17T02:48:59Z

Fixes:

Logging-Generator should allow to skip the IsEnabled check #51927 - Adds SkipEnabledCheck
Logging Source Generator fails with a NullReferenceException #52277 - Properly handle null or misconfigured input
LoggerMessage - Allow Event ID to be optional #52223 - When event id is not set, it will default to -1
LoggerMessageAttribute should have a constructor that takes EventId, LogLevel, and Message #52276 - Adds LoggerMessageAttribute ctor overload

Contributes to: #52549

/cc @gfoidl

ghost · 2021-06-17T02:49:03Z

Tagging subscribers to this area: @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

/cc @gfoidl

Author:	maryamariyan
Assignees:	maryamariyan
Labels:	`area-Extensions-Logging`
Milestone:	-

dotnet-issue-labeler · 2021-06-17T02:49:05Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

gfoidl

Two nits, otherwise LGTM.

Didn't try to run this new feature in a demo-project, as I don't know how to wire things together from the PR build. But according the tests added in this PR it should work.

src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs

Co-authored-by: Günther Foidl <gue@korporal.at>

maryamariyan · 2021-06-17T15:03:13Z

I added another commit, that fixes: #52277, making the logging generator more robust against null input arguments.

maryamariyan · 2021-06-17T15:07:33Z

src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs

@@ -450,13 +455,13 @@ public IReadOnlyList<LoggerClass> GetLogClasses(IEnumerable<ClassDeclarationSynt
                    switch (a.NameEquals.Name.ToString())
                    {
                        case "EventId":
-                            eventId = (int)sm.GetConstantValue(a.Expression, _cancellationToken).Value!;
+                            eventId = (int?)sm.GetConstantValue(a.Expression, _cancellationToken).Value ?? -1;


Also fixes issue #52223

When event ID is not set, it will default to -1, and when multiple logging methods in a class don't have event ID set, they all default to -1, causing the SYSLIB1006 warning.

...ests/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/SkipEnabledCheckExtensions.cs

...ns/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorEmitterTests.cs

eerhardt · 2021-06-17T20:08:14Z

src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs

                            break;
                        case "Message":
                            message = sm.GetConstantValue(a.Expression, _cancellationToken).ToString();
                            break;
+                        case "SkipEnabledCheck":
+                            bool? flag = (bool?)sm.GetConstantValue(a.Expression, _cancellationToken).Value;


What happens to this code if someone writes code that might not compile? Does the source generator fail with an exception, adding more errors to the Error List?

[LoggerMessage(SkipEnabledCheck = 6)]

for this specific use case we get this error:

[warning CS8785: Generator 'LoggerMessageGenerator' failed to generate source. It will not contribute to the output and compilation errors may occur as a result. Exception was of type 'InvalidCastException' with message 'Unable to cast object of type 'System.Int32' to type 'System.Nullable`1[System.Boolean]'.']

Should we fix that? That may be confusing to users.

OK looking into it.

Fixed and made generator more robust against misconfigured input in the latest commit (58e1a75)

/cc @chsienki

pranavkm · 2021-06-18T22:28:37Z

src/libraries/Microsoft.Extensions.Logging.Abstractions/src/LoggerMessageAttribute.cs

+        /// <param name="eventId">The log event Id.</param>
+        /// <param name="level">The log level.</param>
+        /// <param name="message">Format string of the log message.</param>
+        public LoggerMessageAttribute(int eventId, LogLevel level, string message)


Any reason not to have EventName here?

This was the only ctor approved since we assumed it to be the most commonly used. If we think there is value in adding another ctor with EventName included (4 args) then we can take it to API review.

maryamariyan · 2021-06-22T02:38:32Z

...ons/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorParserTests.cs

+            IReadOnlyList<Diagnostic> diagnostics = await RunGenerator(@"
+                public static partial class C
+                {
+                    [LoggerMessage(SkipEnabledCheck = 6)]


@eerhardt here's the test case you asked for.

eerhardt · 2021-06-22T14:45:56Z

src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs

@@ -93,6 +94,9 @@ public IReadOnlyList<LoggerClass> GetLogClasses(IEnumerable<ClassDeclarationSynt
                            }

                            sm ??= _compilation.GetSemanticModel(classDec.SyntaxTree);
+                            IMethodSymbol logMethodSymbol = sm.GetDeclaredSymbol(method, _cancellationToken) as IMethodSymbol;
+                            Debug.Assert(logMethodSymbol != null, "log method is present.");
+                            (int eventId, int? level, string message, string? eventName, bool skipEnabledCheck) = (-1, null, string.Empty, null, false);


Does this need to use a tuple anymore? Can this just be regular variables?

no but I thought it would be neater this way, all defined in one line.

I'm not crazy about it... Especially because I need to match up the variable with the value on the right-hand side to see what its default value is.

src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs

maryamariyan · 2021-06-22T21:24:44Z

Failures are System.IO.Tests.Directory_GetFileSystemEntries_str.GetEntriesThenDelete and System.Linq.Queryable.Tests

https://dev.azure.com/dnceng/public/_build/results?buildId=1199358&view=ms.vss-test-web.build-test-results-tab&runId=35941624&resultId=169277&paneView=debug

Add SkipEnabledCheck on LoggerMessageAttribute

46af543

maryamariyan added the area-Extensions-Logging label Jun 17, 2021

maryamariyan requested review from davidfowl, eerhardt and tarekgh June 17, 2021 02:48

maryamariyan self-assigned this Jun 17, 2021

dotnet-issue-labeler bot added the new-api-needs-documentation label Jun 17, 2021

gfoidl approved these changes Jun 17, 2021

View reviewed changes

src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs Outdated Show resolved Hide resolved

src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs Outdated Show resolved Hide resolved

maryamariyan and others added 2 commits June 17, 2021 10:47

Apply suggestions from code review

221f272

Co-authored-by: Günther Foidl <gue@korporal.at>

Make logging generator more robust with null input

1fa1344

maryamariyan commented Jun 17, 2021

View reviewed changes

eerhardt reviewed Jun 17, 2021

View reviewed changes

...ests/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/SkipEnabledCheckExtensions.cs Show resolved Hide resolved

eerhardt reviewed Jun 17, 2021

View reviewed changes

...ns/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorEmitterTests.cs Outdated Show resolved Hide resolved

eerhardt reviewed Jun 17, 2021

View reviewed changes

PR feedback

fe5181c

JamesNK mentioned this pull request Jun 17, 2021

Soft enable CA1810 dotnet/aspnetcore#33619

Closed

Adds LoggerMessageAttribute ctor overload

82271d1

pranavkm reviewed Jun 18, 2021

View reviewed changes

properly identify misconfigured input

58e1a75

maryamariyan requested a review from chsienki June 22, 2021 02:36

maryamariyan commented Jun 22, 2021

View reviewed changes

eerhardt approved these changes Jun 22, 2021

View reviewed changes

Address PR feedback

73fd442

maryamariyan merged commit 7a3343f into dotnet:main Jun 22, 2021

gfoidl mentioned this pull request Jun 23, 2021

Logging-Generator should allow to skip the IsEnabled check #51927

Closed

This was referenced Jun 23, 2021

Logging Source Generator fails with a NullReferenceException #52277

Closed

LoggerMessage - Allow Event ID to be optional #52223

Closed

karelz mentioned this pull request Jun 24, 2021

Connect_DualMode_DnsConnect_RetrievedEndPoints_Success - Assert ::1 #54677

Closed

ghost locked as resolved and limited conversation to collaborators Jul 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `SkipEnabledCheck` on `LoggerMessageAttribute` #54305

Add `SkipEnabledCheck` on `LoggerMessageAttribute` #54305

maryamariyan commented Jun 17, 2021 •

edited

Loading

ghost commented Jun 17, 2021

dotnet-issue-labeler bot commented Jun 17, 2021

gfoidl left a comment

maryamariyan commented Jun 17, 2021

maryamariyan Jun 17, 2021 •

edited

Loading

eerhardt Jun 17, 2021

maryamariyan Jun 17, 2021

eerhardt Jun 17, 2021

maryamariyan Jun 18, 2021

maryamariyan Jun 22, 2021 •

edited

Loading

pranavkm Jun 18, 2021

maryamariyan Jun 22, 2021 •

edited

Loading

maryamariyan Jun 22, 2021

eerhardt Jun 22, 2021

maryamariyan Jun 22, 2021

eerhardt Jun 22, 2021

maryamariyan commented Jun 22, 2021

Add SkipEnabledCheck on LoggerMessageAttribute #54305

Add SkipEnabledCheck on LoggerMessageAttribute #54305

Conversation

maryamariyan commented Jun 17, 2021 • edited Loading

ghost commented Jun 17, 2021

dotnet-issue-labeler bot commented Jun 17, 2021

gfoidl left a comment

Choose a reason for hiding this comment

maryamariyan commented Jun 17, 2021

maryamariyan Jun 17, 2021 • edited Loading

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

maryamariyan Jun 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maryamariyan Jun 22, 2021 • edited Loading

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

maryamariyan commented Jun 22, 2021

Add `SkipEnabledCheck` on `LoggerMessageAttribute` #54305

Add `SkipEnabledCheck` on `LoggerMessageAttribute` #54305

maryamariyan commented Jun 17, 2021 •

edited

Loading

maryamariyan Jun 17, 2021 •

edited

Loading

maryamariyan Jun 22, 2021 •

edited

Loading

maryamariyan Jun 22, 2021 •

edited

Loading