Cleaner API to declare strongly-typed ILogger delegates

[lodejard]

Adds LogMessage struct for strongly typed logging

• Hides LoggerMessage.Define and Action details
• Exposes LogLevel, EventId, FormatString properties
• Existing calls to LoggerMessage.Define changed to LogMessage

Adds LogScope struct for strongly typed scopes

• Hides LoggerMessage.DefineScope and Func details
• Exposes FormatString property
• Existing call to LoggerMessage.DefineScope changed to LogScope

Name-only EventIds

• Additional EventId(string name) derives Id from Name
• Adds unit tests for name-only event ids
• Same Id derived for same name
• Derived Id values are 6-digits in decimal
  Large enough to avoid collisions within a category name
  Small enough to be human readable

Addresses #2619

[lodejard]

@davidfowl @anurse @Pilchie any comments? I'm a bit on the fence if the exception parameter should come before or after the variable value parameters...

It was on the end of the action originally assuming logging extension methods would have it as a final, optional argument.

In practice I think it's turned out that the places that call the extension method either passes an exception or they don't - so I'm thinking you'd lean towards putting the T1...Tn values last... Seems to fit the pattern of a params style function, and also matches the argument order untyped ILogger.LogXxxx(...) extension methods.

[Pilchie]

Added the api-ready-for-review label so that others can review on Monday. It looks fine to me, but I shouldn't be considered an authoritative source here 😉

[lodejard]

Fundamentally, this adds an allocation-free API wrapping the inconvenient Action<...> x = LoggerMessage.Define<...>(logLevel, eventId, formatString)

public struct LogMessage<T1, ..., Tn>
{
  public LogMessage(LogLevel logLevel, EventId eventId, string formatString);
  public LogMessage(LogLevel logLevel, int eventId, string formatString);
  public LogMessage(LogLevel logLevel, string eventName, string formatString);

  public LogLevel LogLevel {get;}
  public EventId EventId {get;}
  public string FormatString {get;}

  public void Log(ILogger logger, T1 value1, ..., Tn valuen);
  public void Log(ILogger logger, Exception exception, T1 value1, ..., Tn valuen);

  public static implicit operator LogMessage<T1, ..., Tn>((LogLevel logLevel, EventId eventId, string formatString) parameters);
  public static implicit operator LogMessage<T1, ..., Tn>((LogLevel logLevel, int eventId, string formatString) parameters);
  public static implicit operator LogMessage<T1, ..., Tn>((LogLevel logLevel, string eventName, string formatString) parameters);
}

which is used like:

internal static class MyAppLoggerExtensions
{
  private readonly static LogMessage<string, int> _logSayHello = (
    LogLevel.Debug, 
    nameof(SayHello), 
    "The program named {ProgramName} is saying hello {HelloCount} times");

  /// <summary>
  /// The program named {ProgramName} is saying hello {HelloCount} times
  /// </summary>
  /// <param name="logger">The logger to write to</param>
  /// <param name="programName">The {ProgramName} message property</param>
  /// <param name="helloCount">The {HelloCount} message property</param>
  public static void SayHello(this ILogger<MyApp> logger, string programName, int helloCount)
  { 
    _logSayHello.Log(logger, programName, helloCount);
  }
}

[halter73]

I suppose makes sense for LogMessage to allow more generic parameters than LogScope, but I think a max of 2 for LogScope is pretty small. What do people thing about allowing up to 4 generic parameters?

[lodejard]

Oops! That should have been Range(1, 3) - the methods to define delegates only goes up to LoggerMessage.DefineScope<T1, T2, T3>... Should this match T3 or should we add T4 or more to LoggerMessage.DefineScope?

[halter73]

I'm a fan of the exception parameter coming before the variable value parameters. I think the params list analogy is a valid one, and having an early null argument isn't a concern since there are overloads that don't take the exception.

[halter73]

I don't like crossing our fingers and hoping Id's don't collide. If developers are willing to accept the risk of collisions, we might as well use 0 as the Id, so that developers are forced to deal with the colliding Id's early.

[lodejard]

Although that's true, a lot of teams have devs who are happy to provide reasonable event names (especially if the string eventName isn't optional) but will balk at the the suggestion they do manual book-keeping needed to assign a unique ids to every event name in a category. Providing a reasonably unique EventId.Id for free is a way to get most of the benefits of the Id field that doesn't burden the dev teams.

I had the same concern about collisions at first also, but ended up thinking about it like this... Just like you implied even if the EventId.Id has a 100% collision rate the EventId.Name property are still different and message types can still be distinguished. Sinks like Serilog will already be storing EventId.Id and EventId.Name. It would probably also be a good idea for console and debug logger to display EventId.Name when provided instead of (or in addition to) the EventId.Id.

To quantify the odds of collision just did some simulations. Random two-part event names made from a 7700-word dictionary. Using a 9-digit Id, and assuming 10 event names each class category names, got 6 collisions over 100 million class category names.

https://gist.github.com/lodejard/725e849fa9ff57b01a10e07996d8dc16

[analogrelay]

API Review Notes:

We started looking at this. A couple concerns come up:

• The magic hashing of EventIds feels like the wrong approach. General-purpose libraries shouldn't really rely on the hashing and should set the IDs to avoid collision risk. Apps that don't care about numeric IDs could just use 0 as the numeric ID.

• This feels like something we want to wait for target-type new for rather than using implicit operators.

• Our goal here is basically to land at a high-performance logging API we can enthusiastically recommend to customers. With target-type new this would feel pretty close to that but with the operator we're a little less enthusiastic.

• We need to sync with corefx folks about this as the future of Extensions shifts around a bit.

cc @rynowak @davidfowl @bradygaster

[lodejard]

Hi, @anurse - Yea, I replied to an earlier code comment from @halter73. As you observed leaving a default Id 0 works as well. Though TBH it still leaves me curious why a 100% collision rate is better if a 6*10^-8 collision rate is considered a problem. :)

The remark about new operator did come up before as well. I think the comment made at the time was that, once the new operator is available, the implicit operator can be marked [Obsolete("Please use new new keyword instead of implicit tuple operator to initialize LogMessage")]. Not ideal, but sounds like it's unacceptable?

I can understand the concerns, just unfortunate for my scenario that it seems to push improvements further into the future and behind a c# language upgrade.

[JunTaoLuo]

Note that some sources edited in this PR has migrated to dotnet/aspnetcore. A new PR with the same changes for the affected sources should be opened in dotnet/aspnetcore. See #2974 for details.

[ghost]

As per aspnet/Announcements#411, we are currently migrating components from this repository to other repositories. This PR targets components that have been moved to dotnet/aspnetcore. If you're still interested in contributing this change, please retarget your PR to dotnet/aspnetcore and reference the original issue discussing the change or bug fix. If you have questions, or need guidance on how to port your change, please tag @dotnet/extensions-migration in a comment and we'll try to help.

[davidfowl]

Lets revisit this now that we have the repo moved.