Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
eerhardt opened this issue May 4, 2021 · 11 comments
Labels
api-ready-for-review API is ready for review, it is NOT ready for implementation area-Extensions-Logging
Milestone

Comments

@eerhardt
Copy link
Member

eerhardt commented May 4, 2021

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
Copy link

ghost commented May 4, 2021

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: -

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label May 4, 2021
@maryamariyan maryamariyan removed the untriaged New issue has not been triaged by the area owner label May 4, 2021
@maryamariyan maryamariyan added this to the 6.0.0 milestone May 4, 2021
@maryamariyan maryamariyan added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label May 4, 2021
@shirhatti
Copy link
Contributor

shirhatti commented May 4, 2021

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
Copy link
Member

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

@shirhatti
Copy link
Contributor

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

@eerhardt
Copy link
Member Author

eerhardt commented May 5, 2021

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.

@maryamariyan maryamariyan added blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels May 10, 2021
@bartonjs
Copy link
Member

bartonjs commented May 13, 2021

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) { }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels May 13, 2021
@davidfowl
Copy link
Member

cc @geeknoid

@geeknoid
Copy link
Member

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

@pranavkm
Copy link
Contributor

pranavkm commented Jun 22, 2021

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
Copy link
Member

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.

@maryamariyan maryamariyan added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed blocking Marks issues that we want to fast track in order to unblock other important work api-approved API was approved in API review, it can be implemented labels Jun 22, 2021
@terrajobst
Copy link
Member

  • 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.

@ghost ghost locked as resolved and limited conversation to collaborators Jul 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-ready-for-review API is ready for review, it is NOT ready for implementation area-Extensions-Logging
Projects
None yet
Development

No branches or pull requests

8 participants