LoggerMessageAttribute should have a constructor that takes EventId, LogLevel, and Message

[eerhardt]

When using the logging source generator, it is too much clutter to have to put EventId =, Level =. and Message = in my code:

        [LoggerMessage(EventId = EventIds.HealthCheckEndId, Level = LogLevel.Debug, Message = HealthCheckEndText)]
        private partial void HealthCheckEndHealthy(string HealthCheckName, HealthStatus HealthStatus, double ElapsedMilliseconds, string? HealthCheckDescription);

        [LoggerMessage(EventId = EventIds.HealthCheckEndId, Level = LogLevel.Warning, Message = HealthCheckEndText)]
        private partial void HealthCheckEndDegraded(string HealthCheckName, HealthStatus HealthStatus, double ElapsedMilliseconds, string? HealthCheckDescription);

        [LoggerMessage(EventId = EventIds.HealthCheckEndId, Level = LogLevel.Error, Message = HealthCheckEndText)]
        private partial void HealthCheckEndUnhealthy(string HealthCheckName, HealthStatus HealthStatus, double ElapsedMilliseconds, string? HealthCheckDescription, Exception? exception);

vs

        [LoggerMessage(EventIds.HealthCheckEndId, LogLevel.Debug, HealthCheckEndText)]
        private partial void HealthCheckEndHealthy(string HealthCheckName, HealthStatus HealthStatus, double ElapsedMilliseconds, string? HealthCheckDescription);

        [LoggerMessage(EventIds.HealthCheckEndId, LogLevel.Warning, HealthCheckEndText)]
        private partial void HealthCheckEndDegraded(string HealthCheckName, HealthStatus HealthStatus, double ElapsedMilliseconds, string? HealthCheckDescription);

        [LoggerMessage(EventIds.HealthCheckEndId, LogLevel.Error, HealthCheckEndText)]
        private partial void HealthCheckEndUnhealthy(string HealthCheckName, HealthStatus HealthStatus, double ElapsedMilliseconds, string? HealthCheckDescription, Exception? exception);

We should add a constructor to LoggerMessage that takes these 3 parameters, so users don't have to specify the properties on every LoggerMessage attribute.

[System.AttributeUsageAttribute(System.AttributeTargets.Method)]
public sealed partial class LoggerMessageAttribute : System.Attribute
{
    public LoggerMessageAttribute() { }
+    public LoggerMessageAttribute(string message)
+    public LoggerMessageAttribute(LogLevel level, string message) { }
+    public LoggerMessageAttribute(int eventId, string message)
+    public LoggerMessageAttribute(int eventId, LogLevel level, string message) { }
    public int EventId { get { throw null; } set { } }
    public string? EventName { get { throw null; } set { } }
    public Microsoft.Extensions.Logging.LogLevel Level { get { throw null; } set { } }
    public string Message { get { throw null; } set { } }
}

cc @maryamariyan @davidfowl @shirhatti

[ghost]

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

Issue Details

---

When using the logging source generator, it is too much clutter to have to put EventId =, Level =. and Message = in my code:

        [LoggerMessage(EventId = EventIds.HealthCheckEndId, Level = LogLevel.Debug, Message = HealthCheckEndText)]
        private partial void HealthCheckEndHealthy(string HealthCheckName, HealthStatus HealthStatus, double ElapsedMilliseconds, string? HealthCheckDescription);

        [LoggerMessage(EventId = EventIds.HealthCheckEndId, Level = LogLevel.Warning, Message = HealthCheckEndText)]
        private partial void HealthCheckEndDegraded(string HealthCheckName, HealthStatus HealthStatus, double ElapsedMilliseconds, string? HealthCheckDescription);

        [LoggerMessage(EventId = EventIds.HealthCheckEndId, Level = LogLevel.Error, Message = HealthCheckEndText)]
        private partial void HealthCheckEndUnhealthy(string HealthCheckName, HealthStatus HealthStatus, double ElapsedMilliseconds, string? HealthCheckDescription, Exception? exception);

vs

        [LoggerMessage(EventIds.HealthCheckEndId, LogLevel.Debug, HealthCheckEndText)]
        private partial void HealthCheckEndHealthy(string HealthCheckName, HealthStatus HealthStatus, double ElapsedMilliseconds, string? HealthCheckDescription);

        [LoggerMessage(EventIds.HealthCheckEndId, LogLevel.Warning, HealthCheckEndText)]
        private partial void HealthCheckEndDegraded(string HealthCheckName, HealthStatus HealthStatus, double ElapsedMilliseconds, string? HealthCheckDescription);

        [LoggerMessage(EventIds.HealthCheckEndId, LogLevel.Error, HealthCheckEndText)]
        private partial void HealthCheckEndUnhealthy(string HealthCheckName, HealthStatus HealthStatus, double ElapsedMilliseconds, string? HealthCheckDescription, Exception? exception);

We should add a constructor to LoggerMessage that takes these 3 parameters, so users don't have to specify the properties on every LoggerMessage attribute.

cc @maryamariyan @davidfowl @shirhatti

Author:	eerhardt
Assignees:	-
Labels:	area-Extensions-Logging
Milestone:	-

[shirhatti]

We had this exact conversation during API review. The point I raised is that we may want to additional ctors (for scenarios like #52223) and the determination was made to move all validation to the source generator and only keep the parameter-less ctor.

I'm not opposed to it adding them back, but just want to point out that we've flip-flopped on this already

[davidfowl]

Seems like we're saying we'd have 2 constructors, default and this one?

[shirhatti]

What about the variant without LogLevel? And one/two more for #52223?

[eerhardt]

We had this exact conversation during API review.
just want to point out that we've flip-flopped on this already

I've been using this generator as a user would this week (I'm app building for preview4 - see dotnet/aspnetcore#32414). It feels like too much clutter in my code, and makes it hard to read. Notice the difference in the top post. I imagine most devs would feel similarly that this is too much clutter.

[bartonjs]

Video

We think that only one simplified constructor is necessary. The generator needs to account for the fact that the EventId/LogLevel/String can now be specified both as the ctor and as the property set -- in the same attribute instance. ([LoggerMessage(1, ..., ..., EventId = 2)] is actually event ID 2)

partial class LoggerMessageAttribute
{
    public LoggerMessageAttribute(int eventId, LogLevel level, string message) { }
}

[davidfowl]

cc @geeknoid

[geeknoid]

@davidfowl Hmmm, I can't seem to find the I Told You So emoji... :-)

[pranavkm]

I started some investigation on using the source generator as part of ASP.NET Core. Currently all of the ASP.NET Core logger messages include an explicit EventName parameter.

My guess is that this API suggestion relied on inferring the event name from the method name something we want to avoid because it risks breaking logging filters if you refactor the method name. Would we want to consider adding an overload that accepts an event name:

+ public LoggerMessageAttribute(LogLevel level, int eventId, string eventName, string message)

Edit: Tweaked it a bit to more closely resemble the shape of LoggerMessage.Define: https://docs.microsoft.com/en-us/dotnet/api/microsoft.extensions.logging.loggermessage.define?view=dotnet-plat-ext-5.0#Microsoft_Extensions_Logging_LoggerMessage_Define_Microsoft_Extensions_Logging_LogLevel_Microsoft_Extensions_Logging_EventId_System_String_

[maryamariyan]

My guess is that this API suggestion relied on inferring the event name from the method name something we want to avoid because it risks breaking logging filters if you refactor the method name. Would we want to consider adding an overload that accepts an event name:

That's a good point, I didn't realize the danger with refactoring method and it being a breaking change until I checked your ASP.NET Core PR. I'll mark this to be added for API review.

[terrajobst]

• We don't want to add another overload because the design of attributes is that optional parameters are settable properties and this one is optional for some consumers
• It seems for library developers it would be beneficial to have an analyzer that flags code that doesn't initialize EventName to a literal string. And with that, it wouldn't matter if it's a constructor or a property.
• Aesthetically, we can see the argument for wanting a constructor argument but we feel the added complexity isn't worth it.