Request for Logging Internals to be public again

[JakenVeina]

Use case: I'd like to have a LoggerMessage.Define() method that accepts more than 5 type args. I'm perfectly happy to implement this myself, by just copying how the others are done, in my own project, and adding more type args. However, the existing implementations rely on the internal LogValuesFormatter class, which used to be public within a .Internals namespace.

For now, my solution is simply to copy/paste the entire LogValuesFormatter class into my own project, which just seems like a waste.

Alternatively, could we simply get LoggerMessage.Define() methods that take more type args? Perhaps up to 8, like Action<T>, Func<T>, ValueTuple<T> etc? Is there some particular performance reason that more than 5 type args would be disadvantageous?

I'm happy to PR this myself, so long as no one is opposed to the change.

[davidfowl]

Alternatively, could we simply get LoggerMessage.Define() methods that take more type args? Perhaps up to 8, like Action, Func, ValueTuple etc? Is there some particular performance reason that more than 5 type args would be disadvantageous?

This sounds reasonable.Would you like to send a PR?

[JakenVeina]

Certainly.

[maryamariyan]

@JakenVeina please note you can now find the Logging source code in the dotnet/runtime repository.

You might want to check these instruction links below:

• How to build runtime repo
• How to update the reference assembly
• Microsoft.Extensions libraries code are now under src/libraries in runtime repo. e.g. LoggerMessage is here and its reference assembly is here.

I'll transfer this issue to runtime repo now.

[maryamariyan]

I just noticed your PR :)

[maryamariyan]

@JakenVeina, the runtime repo guidelines require us to go through an API review process.
https://github.com/dotnet/runtime/blob/b2bfe9c6cdeb9d6d658ff7e12da85105687281b6/docs/project/api-review-process.md

Looking at the existing APIs on LoggerMessage we see Define has more overloads than DefineScope.

    public static partial class LoggerMessage
    {
        public static System.Func<Microsoft.Extensions.Logging.ILogger, System.IDisposable> DefineScope(string formatString) { throw null; }
        public static System.Func<Microsoft.Extensions.Logging.ILogger, T1, System.IDisposable> DefineScope<T1>(string formatString) { throw null; }
        public static System.Func<Microsoft.Extensions.Logging.ILogger, T1, T2, System.IDisposable> DefineScope<T1, T2>(string formatString) { throw null; }
        public static System.Func<Microsoft.Extensions.Logging.ILogger, T1, T2, T3, System.IDisposable> DefineScope<T1, T2, T3>(string formatString) { throw null; }
        public static System.Action<Microsoft.Extensions.Logging.ILogger, System.Exception> Define(Microsoft.Extensions.Logging.LogLevel logLevel, Microsoft.Extensions.Logging.EventId eventId, string formatString) { throw null; }
        public static System.Action<Microsoft.Extensions.Logging.ILogger, T1, System.Exception> Define<T1>(Microsoft.Extensions.Logging.LogLevel logLevel, Microsoft.Extensions.Logging.EventId eventId, string formatString) { throw null; }
        public static System.Action<Microsoft.Extensions.Logging.ILogger, T1, T2, System.Exception> Define<T1, T2>(Microsoft.Extensions.Logging.LogLevel logLevel, Microsoft.Extensions.Logging.EventId eventId, string formatString) { throw null; }
        public static System.Action<Microsoft.Extensions.Logging.ILogger, T1, T2, T3, System.Exception> Define<T1, T2, T3>(Microsoft.Extensions.Logging.LogLevel logLevel, Microsoft.Extensions.Logging.EventId eventId, string formatString) { throw null; }
        public static System.Action<Microsoft.Extensions.Logging.ILogger, T1, T2, T3, T4, System.Exception> Define<T1, T2, T3, T4>(Microsoft.Extensions.Logging.LogLevel logLevel, Microsoft.Extensions.Logging.EventId eventId, string formatString) { throw null; }
        public static System.Action<Microsoft.Extensions.Logging.ILogger, T1, T2, T3, T4, T5, System.Exception> Define<T1, T2, T3, T4, T5>(Microsoft.Extensions.Logging.LogLevel logLevel, Microsoft.Extensions.Logging.EventId eventId, string formatString) { throw null; }
        public static System.Action<Microsoft.Extensions.Logging.ILogger, T1, T2, T3, T4, T5, T6, System.Exception> Define<T1, T2, T3, T4, T5, T6>(Microsoft.Extensions.Logging.LogLevel logLevel, Microsoft.Extensions.Logging.EventId eventId, string formatString) { throw null; }
    }

The missing ones you are proposing are:

...
        public static System.Func<Microsoft.Extensions.Logging.ILogger, T1, T2, T3, T4, System.IDisposable> DefineScope<T1, T2, T3, T4>(string formatString) { throw null; }
        public static System.Func<Microsoft.Extensions.Logging.ILogger, T1, T2, T3, T4, T5, System.IDisposable> DefineScope<T1, T2, T3, T4, T5>(string formatString) { throw null; }
        public static System.Func<Microsoft.Extensions.Logging.ILogger, T1, T2, T3, T4, T5, T6, System.IDisposable> DefineScope<T1, T2, T3, T4, T5, T6>(string formatString) { throw null; }
...

The above look reasonable. But looking at your PR, I see you are also proposing to added more overloads, to take up to 8 arg types. Regarding these other APIs, why the magic number 8? Is there a strong reason to have 8 and not just stick to existing 6?

Referring to:

        public static System.Func<Microsoft.Extensions.Logging.ILogger, T1, T2, T3, T4, T5, T6, T7, System.IDisposable> DefineScope<T1, T2, T3, T4, T5, T6, T7>(string formatString) { throw null; }
        public static System.Func<Microsoft.Extensions.Logging.ILogger, T1, T2, T3, T4, T5, T6, T7, T8, System.IDisposable> DefineScope<T1, T2, T3, T4, T5, T6, T7, T8>(string formatString) { throw null; }
  
        public static System.Action<Microsoft.Extensions.Logging.ILogger, T1, T2, T3, T4, T5, T6, T7, System.Exception> Define<T1, T2, T3, T4, T5, T6, T7>(Microsoft.Extensions.Logging.LogLevel logLevel, Microsoft.Extensions.Logging.EventId eventId, string formatString) { throw null; }
        public static System.Action<Microsoft.Extensions.Logging.ILogger, T1, T2, T3, T4, T5, T6, T7, T8, System.Exception> Define<T1, T2, T3, T4, T5, T6, T7, T8>(Microsoft.Extensions.Logging.LogLevel logLevel, Microsoft.Extensions.Logging.EventId eventId, string formatString) { throw null; }
 

[JakenVeina]

No particular reason for 8, other than that's the "magic number" used elsewhere in the framework. E.G. Action<T>, Func<T>, and ValueTuple<T>.

[bartonjs]

Video

Adding the DefineScope T4-T6 for parity is approved. If you want to add further ones, that's also approved, but in the review the comments were "why stop at 8?" and "do we need 7 and 8?". If there's need for this, then they're approved. If not, then maybe hold off.

    public static partial class LoggerMessage
    {
        public static System.Func<Microsoft.Extensions.Logging.ILogger, T1, T2, T3, T4, System.IDisposable> DefineScope<T1, T2, T3, T4>(string formatString) { throw null; }
        public static System.Func<Microsoft.Extensions.Logging.ILogger, T1, T2, T3, T4, T5, System.IDisposable> DefineScope<T1, T2, T3, T4, T5>(string formatString) { throw null; }
        public static System.Func<Microsoft.Extensions.Logging.ILogger, T1, T2, T3, T4, T5, T6, System.IDisposable> DefineScope<T1, T2, T3, T4, T5, T6>(string formatString) { throw null; }
    }

[JakenVeina]

That's a fair analysis. I can trim the T7 and T8 overloads out of the PR.

After watching the video, I will go ahead and clarify the original request: LoggerMessage.Create() and LoggerMessage.BeginScope() are not the methods that used to be internal. There actually used to be a namespace Microsoft.Extensions.Logging.Abstractions.Internal that contained a few public types. Most significantly, they contained the LogMessageFormatter type, which is actually where a lot of the logic is that powers the LoggerMessage.Create() and LoggerMessage.BeginScope() methods. My original thought was to just make these types public again, so that someone who needs more type args than what's available has an easier time just implementing their own methods.

The current internal version of LogMessageFormatter can be seen here.

Here's the commit that changed these from public to internal.

[maryamariyan]

@JakenVeina I see you originally had a PR prepared for this.
Were you thinking of reviving your PR with the final approved APIs?

[JakenVeina]

Yes, definitely. I'll see if I can get it re-worked this weekend.

[maryamariyan]

Thanks for contributing @JakenVeina. If we wanted to get this in for 5.0 the deadline would be August 18 I believe. It would be good if we could get this merged in before that time.

[danmoseley]

We branch on Aug 17th - I'm guessing midday, maybe later though.

[JakenVeina]

I've force-pushed an update to my fork. You should be able to re-open #34741.

[maryamariyan]

@JakenVeina I doubt reopening would work and it wouldn't get the updated changes. Could you please open a new PR instead?

[JakenVeina]

Normally, that would work for me. I guess since it's already closed, github stops listening for changes on the forked branch.

Regardless. New PR submitted.