Add additional LoggerMessageAttribute constructors

[noahfalk]

By @geeknoid

• Event id is not an effective concept; customers don't want to use it and don't have anything to do with it. So, we want overloads that don't require that.
• We are trying to shift customers to not use message strings and stick with structured logging. So, we want overloads that don't include a message.
• We routinely find situations where log levels are specified as a logging method parameter rather than in the attribute definition, so we want overloads that don't include a level.

Bottom line is that the lack of these constructors leads to considerably more ceremony for the customers. More stuff to type with no upside to them.

Proposal

public sealed class LoggerMessageAttribute : Attribute
{
    public LoggerMessageAttribute()
    public LoggerMessageAttribute(int eventId, LogLevel level, string message)

+   public LoggerMessageAttribute(LogLevel level, string message)
+   public LoggerMessageAttribute(LogLevel level)
+   public LoggerMessageAttribute(string message)
}

Original description

dotnet/extensions supports a variety of shortcut constructors on LogMethodAttribute. In the past these shortcuts were deemed undesirable. While I don't know the details behind the original decisions, now that we have more history of internal usage and a better track record we should reconsider.

@geeknoid - can you enumerate exactly which APIs you hope to bring over?

[geeknoid]

Here's the set:

public sealed class LoggerMessageAttribute : Attribute
{
    // existing
    public LoggerMessageAttribute()
    public LoggerMessageAttribute(int eventId, LogLevel level, string message)

    // proposed additions
    public LoggerMessageAttribute(int eventId, LogLevel level)
    public LoggerMessageAttribute(LogLevel level, string message)
    public LoggerMessageAttribute(LogLevel level)
    public LoggerMessageAttribute(string message)
    public LoggerMessageAttribute(int eventId, string message)
    public LoggerMessageAttribute(int eventId)

    // existing
    public int EventId { get; set; } = -1;
    public string? EventName { get; set; }
    public LogLevel Level { get; set; } = LogLevel.None;
    public string Message { get; set; } = "";
    public bool SkipEnabledCheck { get; set; }
}

So:

• Event id is not an effective concept, customers don't want to use it and don't have anything to do with it. So we want overloads that don't require that.

• We are trying to shift customers to not use message strings and stick with structured logging. So we want overloads that don't include a message.

• We routinely find situations where log levels are specified as a logging method parameter rather than in the attribute definition, so we want overloads that don't include a level.

Bottom line is that the lack of these contrusctors leads to considerably more ceremony for the customers. More stuff to type with no upside to them.

[ghost]

Tagging subscribers to this area: @dotnet/area-extensions-logging
See info in area-owners.md if you want to be subscribed.

Issue Details

---

dotnet/extensions supports a variety of shortcut constructors on LogMethodAttribute. In the past these shortcuts were deemed undesirable. While I don't know the details behind the original decisions, now that we have more history of internal usage and a better track record we should reconsider.

@geeknoid - can you enumerate exactly which APIs you hope to bring over?

Author:	noahfalk
Assignees:	geeknoid
Labels:	area-Extensions-Logging
Milestone:	8.0.0

[tarekgh]

Event id is not an effective concept, customers don't want to use it and don't have anything to do with it. So we want overloads that don't require that.

If this is the case, why are we proposing the following constructors? would be better reducing a more helper constructors in such scenario.

 public LoggerMessageAttribute(int eventId, LogLevel level)
 public LoggerMessageAttribute(int eventId, string message)
 public LoggerMessageAttribute(int eventide)

[geeknoid]

@tarekgh I hate it when people point out logical flaws in my stuff :-)

Yeah, I suppose it would be reasonable to not include those.

[noahfalk]

Just a heads up, I suspect without changes to static analysis users that attempt to use the new attribute constructors will get a warning that multiple events use the same -1 id, and warnings that the message string doesn't include their parameter names. Presumably we'd want to remove those warnings.

[tarekgh]

@noahfalk what you are referring to is the source gen diagnostic SYSLIB1006 which we already changed it to be just info instead of warning

#72263
#85581

runtime/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/DiagnosticDescriptors.cs

Line 48 in 7de4e1b

DiagnosticSeverity.Info,

We may consider changing the code to ignore the default event id -1 and not produce any warning or info for that Id.

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

Line 269 in 7de4e1b

if (!eventIds.Add(lm.EventId))

If there is not any other concern, I'll modify the proposal and post it in the description, and we can proceed with the design review.

[terrajobst]

Video

• They can already do this today because the properties are settable
  • We generally try to avoid explosion of constructor overloads
• The bullet points, especially that we consider event id a mistake, is somewhat surprising
  • @noahfalk @geeknoid
  • Adding those constructors is going against our existing Roslyn analyzers
• What does the console logger do for cases where there is no message?
• It would help to see code samples that highlight the reduction of ceremony

namespace Microsoft.Extensions.Logging;

public sealed partial class LoggerMessageAttribute : Attribute
{
    // Existing:
    // public LoggerMessageAttribute();
    // public LoggerMessageAttribute(int eventId, LogLevel level, string message);

    public LoggerMessageAttribute(LogLevel level, string message);
    public LoggerMessageAttribute(LogLevel level);
    public LoggerMessageAttribute(string message);
}

[geeknoid]

A few follow-ups:

• Regarding event id, we found internally that our telemetry backend tools have nothing to do with event id/event name and those just get dropped on the floor. As such, application teams don't really care about the event ids and would rather avoid having to deal with them.

• Regarding the LogLevel, the code generator recognizes that there isn't any LogLevel specified in the attribute and then insists that a LogLevel value be provided as an argument to the logging method. This makes it possible to have dynamically configurable log level which is desirable in several situations.

• In terms of ceremony, here's some examples:

internal static class Logging
{
    [LoggerMessage(LogLevel.Information)]
    void CreateRecord(this ILogger logger, string recordId);

    [LoggerMessage(LogLevel.Debug)]
    void DeleteRecord(this ILogger logger, string recordId);

    [LoggerMessage("A record is being skipped: {recordId}")]
    void SkipRecord(this ILogger logger, string recordId);

    [LoggerMessage(LogLevel.Warning, "I am a broken record: {recordId}")]
    void BrokenRecord(this ILogger logger, string recordId);
}

As opposed to the more verbose:

internal static class Logging
{
    [LoggerMessage(LogLevel = LogLevel.Information)]
    void CreateRecord(this ILogger logger, string recordId);

    [LoggerMessage(LogLevel = LogLevel.Debug)]
    void DeleteRecord(this ILogger logger, string recordId);

    [LoggerMessage(Messgae = "A record is being skipped: {recordId}")]
    void SkipRecord(this ILogger logger, string recordId);

    [LoggerMessage(LogLevel = LogLevel.Warning, Message = "I am a broken record: {recordId}")]
    void BrokenRecord(this ILogger logger, string recordId);
}

There is simply no value in forcing the developer to enter this extra boilerplate throughout their code rather than adding a set of constructors that address the real-world mainstream use of the attribute.

[noahfalk]

The bullet points, especially that we consider event id a mistake, is somewhat surprising

To add a little history, I think EventId got added to the original logging design to increase compat with ETW, and the id got added to the LoggerMessage attribute in turn because it was part of ILogger.Log(). I don't consider EventId existing to be a mistake, but I do agree with Martin that currently customers aren't getting much value from specifying it. Having it be optional feels better than having it be required. In the future some new scenarios like client-side EventId based filtering might provide customers more value around it, but other customers will continue having no reason to care about it.

[tarekgh]

Marked as ready for review as @geeknoid want to discuss it one more time after he added more info to the issue.

[bartonjs]

Video

Given that we've already gone against guidelines by having the (int, LogLevel, string) secondary constructor, we feel that this is a reasonable enhancement to usability of people creating these logger functions.

namespace Microsoft.Extensions.Logging;

public sealed partial class LoggerMessageAttribute : Attribute
{
    // Existing:
    // public LoggerMessageAttribute();
    // public LoggerMessageAttribute(int eventId, LogLevel level, string message);

    public LoggerMessageAttribute(LogLevel level, string message);
    public LoggerMessageAttribute(LogLevel level);
    public LoggerMessageAttribute(string message);
}