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.

Sign up for GitHub

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

Jump to bottom

Add LoggerMessage.Define overloads accepting up to 14 arguments. #50913

Closed
maryamariyan opened this issue Apr 8, 2021 · 10 comments
Closed

Add LoggerMessage.Define overloads accepting up to 14 arguments. #50913

maryamariyan opened this issue Apr 8, 2021 · 10 comments
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-Extensions-Logging
Milestone
6.0.0

Comments

@maryamariyan
Copy link
Member

maryamariyan commented Apr 8, 2021 •
edited
Loading

We would like to have a LoggerMessage.Define() method that accepts up to 14 type args.

New API Proposal

public static partial class LoggerMessage
{
    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, Microsoft.Extensions.Logging.LogOptions options) {​ 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, Microsoft.Extensions.Logging.LogOptions options) {​ throw null; }​
    public static System.Action<Microsoft.Extensions.Logging.ILogger, T1, T2, T3, T4, T5, T6, T7, T8, T9, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9>(Microsoft.Extensions.Logging.LogLevel logLevel, Microsoft.Extensions.Logging.EventId eventId, string formatString, Microsoft.Extensions.Logging.LogOptions options) {​ throw null; }​
    public static System.Action<Microsoft.Extensions.Logging.ILogger, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10>(Microsoft.Extensions.Logging.LogLevel logLevel, Microsoft.Extensions.Logging.EventId eventId, string formatString, Microsoft.Extensions.Logging.LogOptions options) {​ throw null; }​
    public static System.Action<Microsoft.Extensions.Logging.ILogger, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11>(Microsoft.Extensions.Logging.LogLevel logLevel, Microsoft.Extensions.Logging.EventId eventId, string formatString, Microsoft.Extensions.Logging.LogOptions options) {​ throw null; }​
    public static System.Action<Microsoft.Extensions.Logging.ILogger, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12>(Microsoft.Extensions.Logging.LogLevel logLevel, Microsoft.Extensions.Logging.EventId eventId, string formatString, Microsoft.Extensions.Logging.LogOptions options) {​ throw null; }​
    public static System.Action<Microsoft.Extensions.Logging.ILogger, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13>(Microsoft.Extensions.Logging.LogLevel logLevel, Microsoft.Extensions.Logging.EventId eventId, string formatString, Microsoft.Extensions.Logging.LogOptions options) {​ throw null; }​
    public static System.Action<Microsoft.Extensions.Logging.ILogger, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14>(Microsoft.Extensions.Logging.LogLevel logLevel, Microsoft.Extensions.Logging.EventId eventId, string formatString, Microsoft.Extensions.Logging.LogOptions options) {​ throw null; }​

where Microsoft.Extensions.Logging.LogOptions only has the one bool flag SkipEnabledCheck below:

namespace Microsoft.Extensions.Logging
{ 
    public class LogOptions
    {
        public bool SkipEnabledCheck { get; set; }
    }
}

The 6 new APIs below that were added in .NET 6 timeframe need to get updated as well.
It now makes sense to update to taking LogOptions instead of boolean skipEnabledCheck flag:

- 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, bool skipEnabledCheck) { 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, Microsoft.Extensions.Logging.LogOptions options) {​ 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, bool skipEnabledCheck) { 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, Microsoft.Extensions.Logging.LogOptions options) {​ 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, bool skipEnabledCheck) { 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, Microsoft.Extensions.Logging.LogOptions options) {​ 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, bool skipEnabledCheck) { 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, Microsoft.Extensions.Logging.LogOptions options) {​ 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, bool skipEnabledCheck) { 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, Microsoft.Extensions.Logging.LogOptions options) {​ 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, bool skipEnabledCheck) { 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, Microsoft.Extensions.Logging.LogOptions options) {​ throw null; }​

More info for motive: #50334 (comment)

New proposal adds 10 new overloads. To support up to 16 arguments because that is what Action allows as well.

Previous API Proposal 
public static partial class LoggerMessage
{
        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, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7>(Microsoft.Extensions.Logging.LogLevel logLevel, Microsoft.Extensions.Logging.EventId eventId, string formatString, bool skipEnabledCheck) { 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; }
        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, bool skipEnabledCheck) { throw null; }
        public static System.Action<Microsoft.Extensions.Logging.ILogger, T1, T2, T3, T4, T5, T6, T7, T8, T9, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9>(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, T9, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9>(Microsoft.Extensions.Logging.LogLevel logLevel, Microsoft.Extensions.Logging.EventId eventId, string formatString, bool skipEnabledCheck) { throw null; }
        public static System.Action<Microsoft.Extensions.Logging.ILogger, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10>(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, T9, T10, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10>(Microsoft.Extensions.Logging.LogLevel logLevel, Microsoft.Extensions.Logging.EventId eventId, string formatString, bool skipEnabledCheck) { throw null; }
        public static System.Action<Microsoft.Extensions.Logging.ILogger, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11>(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, T9, T10, T11, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11>(Microsoft.Extensions.Logging.LogLevel logLevel, Microsoft.Extensions.Logging.EventId eventId, string formatString, bool skipEnabledCheck) { throw null; }
        public static System.Action<Microsoft.Extensions.Logging.ILogger, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12>(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, T9, T10, T11, T12, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12>(Microsoft.Extensions.Logging.LogLevel logLevel, Microsoft.Extensions.Logging.EventId eventId, string formatString, bool skipEnabledCheck) { throw null; }
        public static System.Action<Microsoft.Extensions.Logging.ILogger, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13>(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, T9, T10, T11, T12, T13, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13>(Microsoft.Extensions.Logging.LogLevel logLevel, Microsoft.Extensions.Logging.EventId eventId, string formatString, bool skipEnabledCheck) { throw null; }
        public static System.Action<Microsoft.Extensions.Logging.ILogger, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14>(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, T9, T10, T11, T12, T13, T14, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14>(Microsoft.Extensions.Logging.LogLevel logLevel, Microsoft.Extensions.Logging.EventId eventId, string formatString, bool skipEnabledCheck) { throw null; }
        public static System.Action<Microsoft.Extensions.Logging.ILogger, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15>(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, T9, T10, T11, T12, T13, T14, T15, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15>(Microsoft.Extensions.Logging.LogLevel logLevel, Microsoft.Extensions.Logging.EventId eventId, string formatString, bool skipEnabledCheck) { throw null; }
        public static System.Action<Microsoft.Extensions.Logging.ILogger, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15, T16, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15, T16>(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, T9, T10, T11, T12, T13, T14, T15, T16, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15, T16>(Microsoft.Extensions.Logging.LogLevel logLevel, Microsoft.Extensions.Logging.EventId eventId, string formatString, bool skipEnabledCheck) { throw null; }
}

Older issue (#35060) added up to 6 overlods for Define, this issue adds up to 16.

The reason to add up to 16 is because Action itself takes up to 16 parameters: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/Action.cs#L24

Motivation

We've had requests to add more overloads in the past as well (e.g. #35060) so there is community interest in adding these APIs.

Also, the compile-time source generation for logging, allows support arbitrary number of parameters in a message template. But the runtime capabilities provided by LoggerMessage.Define APIs today is limited to only 6 parameters.

Adding the above APIs helps keep a more coherent range of logging feature support between compile-time and runtime in the framework library. This way for the most frequently hit user scenarios (up to 16 arguments) the compile time implementation can remain intact with runtime as well.

Additional Information

  • Link to logging generator: here
  • Link to generated baseline code samples using the logging generator: here

Follow up proposal - Support Dynamic Log Levels

As a follow up to the API proposal above, we could consider the APIs below as well to allow supporting dynamic log levels using LoggerMessage.Define.

Note: The compile-time source generator supports dynamic log levels as a feature.

public static partial class LoggerMessage
{
    public static System.Action<Microsoft.Extensions.Logging.ILogger, Microsoft.Extensions.Logging.LogLevel logLevel, System.Exception?> Define(Microsoft.Extensions.Logging.EventId eventId, string formatString, Microsoft.Extensions.Logging.LogOptions options) {​ throw null; }​
    public static System.Action<Microsoft.Extensions.Logging.ILogger, Microsoft.Extensions.Logging.LogLevel logLevel, T1, System.Exception?> Define<T1>(Microsoft.Extensions.Logging.EventId eventId, string formatString, Microsoft.Extensions.Logging.LogOptions options) {​ throw null; }​
    public static System.Action<Microsoft.Extensions.Logging.ILogger, Microsoft.Extensions.Logging.LogLevel logLevel, T1, T2, System.Exception?> Define<T1, T2>(Microsoft.Extensions.Logging.EventId eventId, string formatString, Microsoft.Extensions.Logging.LogOptions options) {​ throw null; }​
    public static System.Action<Microsoft.Extensions.Logging.ILogger, Microsoft.Extensions.Logging.LogLevel logLevel, T1, T2, T3, System.Exception?> Define<T1, T2, T3>(Microsoft.Extensions.Logging.EventId eventId, string formatString, Microsoft.Extensions.Logging.LogOptions options) {​ throw null; }​
    public static System.Action<Microsoft.Extensions.Logging.ILogger, Microsoft.Extensions.Logging.LogLevel logLevel, T1, T2, T3, T4, System.Exception?> Define<T1, T2, T3, T4>(Microsoft.Extensions.Logging.EventId eventId, string formatString, Microsoft.Extensions.Logging.LogOptions options) {​ throw null; }​
    public static System.Action<Microsoft.Extensions.Logging.ILogger, Microsoft.Extensions.Logging.LogLevel logLevel, T1, T2, T3, T4, T5, System.Exception?> Define<T1, T2, T3, T4, T5>(Microsoft.Extensions.Logging.EventId eventId, string formatString, Microsoft.Extensions.Logging.LogOptions options) {​ throw null; }​
    public static System.Action<Microsoft.Extensions.Logging.ILogger, Microsoft.Extensions.Logging.LogLevel logLevel, T1, T2, T3, T4, T5, T6, System.Exception?> Define<T1, T2, T3, T4, T5, T6>(Microsoft.Extensions.Logging.EventId eventId, string formatString, Microsoft.Extensions.Logging.LogOptions options) {​ throw null; }​
    public static System.Action<Microsoft.Extensions.Logging.ILogger, Microsoft.Extensions.Logging.LogLevel logLevel, T1, T2, T3, T4, T5, T6, T7, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7>(Microsoft.Extensions.Logging.EventId eventId, string formatString, Microsoft.Extensions.Logging.LogOptions options) {​ throw null; }​
    public static System.Action<Microsoft.Extensions.Logging.ILogger, Microsoft.Extensions.Logging.LogLevel logLevel, T1, T2, T3, T4, T5, T6, T7, T8, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8>(Microsoft.Extensions.Logging.EventId eventId, string formatString, Microsoft.Extensions.Logging.LogOptions options) {​ throw null; }​
    public static System.Action<Microsoft.Extensions.Logging.ILogger, Microsoft.Extensions.Logging.LogLevel logLevel, T1, T2, T3, T4, T5, T6, T7, T8, T9, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9>(Microsoft.Extensions.Logging.EventId eventId, string formatString, Microsoft.Extensions.Logging.LogOptions options) {​ throw null; }​
    public static System.Action<Microsoft.Extensions.Logging.ILogger, Microsoft.Extensions.Logging.LogLevel logLevel, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10>(Microsoft.Extensions.Logging.EventId eventId, string formatString, Microsoft.Extensions.Logging.LogOptions options) {​ throw null; }​
    public static System.Action<Microsoft.Extensions.Logging.ILogger, Microsoft.Extensions.Logging.LogLevel logLevel, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11>(Microsoft.Extensions.Logging.EventId eventId, string formatString, Microsoft.Extensions.Logging.LogOptions options) {​ throw null; }​
    public static System.Action<Microsoft.Extensions.Logging.ILogger, Microsoft.Extensions.Logging.LogLevel logLevel, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12>(Microsoft.Extensions.Logging.EventId eventId, string formatString, Microsoft.Extensions.Logging.LogOptions options) {​ throw null; }​
    public static System.Action<Microsoft.Extensions.Logging.ILogger, Microsoft.Extensions.Logging.LogLevel logLevel, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13>(Microsoft.Extensions.Logging.EventId eventId, string formatString, Microsoft.Extensions.Logging.LogOptions options) {​ throw null; }​
}

Dynamic Log Levels - Usage in dotnet/aspnetcore:

https://github.com/dotnet/aspnetcore/blob/bbe65b8eaadcb6021f863a293fb57c46085db883/src/HealthChecks/HealthChecks/src/DefaultHealthCheckService.cs#L211-L227
@maryamariyan maryamariyan added area-Extensions-Logging api-ready-for-review API is ready for review, it is NOT ready for implementation labels Apr 8, 2021
@ghost
Copy link

ghost commented Apr 8, 2021

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

Issue Details

we would like to have a LoggerMessage.Define() method that accepts up to 16 type args.

        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, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7>(Microsoft.Extensions.Logging.LogLevel logLevel, Microsoft.Extensions.Logging.EventId eventId, string formatString, bool skipEnabledCheck) { 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; }
        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, bool skipEnabledCheck) { throw null; }
        public static System.Action<Microsoft.Extensions.Logging.ILogger, T1, T2, T3, T4, T5, T6, T7, T8, T9, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9>(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, T9, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9>(Microsoft.Extensions.Logging.LogLevel logLevel, Microsoft.Extensions.Logging.EventId eventId, string formatString, bool skipEnabledCheck) { throw null; }
        public static System.Action<Microsoft.Extensions.Logging.ILogger, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10>(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, T9, T10, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10>(Microsoft.Extensions.Logging.LogLevel logLevel, Microsoft.Extensions.Logging.EventId eventId, string formatString, bool skipEnabledCheck) { throw null; }
        public static System.Action<Microsoft.Extensions.Logging.ILogger, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11>(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, T9, T10, T11, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11>(Microsoft.Extensions.Logging.LogLevel logLevel, Microsoft.Extensions.Logging.EventId eventId, string formatString, bool skipEnabledCheck) { throw null; }
        public static System.Action<Microsoft.Extensions.Logging.ILogger, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12>(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, T9, T10, T11, T12, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12>(Microsoft.Extensions.Logging.LogLevel logLevel, Microsoft.Extensions.Logging.EventId eventId, string formatString, bool skipEnabledCheck) { throw null; }
        public static System.Action<Microsoft.Extensions.Logging.ILogger, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13>(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, T9, T10, T11, T12, T13, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13>(Microsoft.Extensions.Logging.LogLevel logLevel, Microsoft.Extensions.Logging.EventId eventId, string formatString, bool skipEnabledCheck) { throw null; }
        public static System.Action<Microsoft.Extensions.Logging.ILogger, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14>(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, T9, T10, T11, T12, T13, T14, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14>(Microsoft.Extensions.Logging.LogLevel logLevel, Microsoft.Extensions.Logging.EventId eventId, string formatString, bool skipEnabledCheck) { throw null; }
        public static System.Action<Microsoft.Extensions.Logging.ILogger, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15>(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, T9, T10, T11, T12, T13, T14, T15, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15>(Microsoft.Extensions.Logging.LogLevel logLevel, Microsoft.Extensions.Logging.EventId eventId, string formatString, bool skipEnabledCheck) { throw null; }
        public static System.Action<Microsoft.Extensions.Logging.ILogger, T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15, T16, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15, T16>(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, T9, T10, T11, T12, T13, T14, T15, T16, System.Exception?> Define<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15, T16>(Microsoft.Extensions.Logging.LogLevel logLevel, Microsoft.Extensions.Logging.EventId eventId, string formatString, bool skipEnabledCheck) { throw null; }

Older issue (#35060) added up to 6 overlods for Define, this issue adds up to 16.

Author: maryamariyan
Assignees: -
Labels:

api-ready-for-review, 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 Apr 8, 2021
@ghost ghost added this to Untriaged in ML, Extensions, Globalization, etc, POD. Apr 8, 2021
@maryamariyan maryamariyan mentioned this issue Apr 8, 2021
Closed
@maryamariyan maryamariyan added this to the 6.0.0 milestone Apr 8, 2021
@maryamariyan maryamariyan removed the untriaged New issue has not been triaged by the area owner label Apr 8, 2021
@ghost ghost moved this from Untriaged to 6.0.0 in ML, Extensions, Globalization, etc, POD. Apr 8, 2021
@davidfowl davidfowl added the blocking Marks issues that we want to fast track in order to unblock other important work label Apr 8, 2021
@bartonjs
Copy link
Member

bartonjs commented Apr 9, 2021 •
edited by dotnet-api-review bot
Loading

Video

We think there's not a scenario for this, and we shouldn't add it until there is one.

@bartonjs bartonjs removed the api-ready-for-review API is ready for review, it is NOT ready for implementation label Apr 9, 2021
@bartonjs bartonjs closed this as completed Apr 9, 2021
ML, Extensions, Globalization, etc, POD. automation moved this from 6.0.0 to Done Apr 9, 2021
@davidfowl
Copy link
Member

Don't we want to add more than 6 though? Like 10?

@maryamariyan
Copy link
Member Author

maryamariyan commented Apr 29, 2021 •
edited
Loading

Reopening this issue, with an update.

Updated the issue description with the new proposal.
The new proposal adds 10 new overloads.

@maryamariyan maryamariyan reopened this Apr 29, 2021
@maryamariyan maryamariyan added the api-ready-for-review API is ready for review, it is NOT ready for implementation label Apr 29, 2021
@GrabYourPitchforks
Copy link
Member

This issue now covers two topics: changing the signature of the previously-approved 6.0 APIs; and adding new overloads to cover additional generic parameters. I suspect the first one will sail through fairly smoothly.

For the additional generic parameters, check Jeremy's comment at #50913 (comment). Is there new information that shows a need for 16 arguments? Something we wouldn't be able to get by adding, say, better low-allocation string interpolation overloads to the existing logging functions, which would allow them to scale up to arbitrary-arity inputs and would allow for short-circuiting when the given warning level is not enabled?

Also keep in mind that if you're using Action<...>, you can't go beyond T14. You're already using two parameters for your ILogger and Exception args.

@maryamariyan maryamariyan changed the title Add LoggerMessage.Define overloads accepting up to 16 arguments. Add LoggerMessage.Define overloads accepting up to 14 arguments. Apr 29, 2021
@maryamariyan
Copy link
Member Author

Also keep in mind that if you're using Action<...>, you can't go beyond T14. You're already using two parameters for your ILogger and Exception args.

updated

@maryamariyan
Copy link
Member Author

maryamariyan commented Apr 29, 2021 •
edited
Loading

For the additional generic parameters, check Jeremy's comment at #50913 (comment). Is there new information that shows a need for 16 arguments?

One of the comments were about why we are adding two overloads per number of arguments (one with the skipEnabledCheck and one without). Using the new proposal we only add one overload per message template argument count.

cc @geeknoid since he also pointed out that they have cases using more than 6 arguments.

@maryamariyan
Copy link
Member Author

maryamariyan commented Apr 29, 2021 •
edited
Loading

Something we wouldn't be able to get by adding, say, better low-allocation string interpolation overloads to the existing logging functions, which would allow them to scale up to arbitrary-arity inputs and would allow for short-circuiting when the given warning level is not enabled?

The string interpolation feature does not yet have a completed design for structured logging (scheduled within the .NET 6 timeframe). For Microsoft.Extensions.Logging we prototyped it partially in this design doc. There is also some discussion with Serilog owner on this too here.

The goal here is to make sure the runtime capability keeps up with what we support with code gen. the string interpolation builder would still be code generated. So it probably would still make sense to add the above overloads regardless of where we end up with string interpolation feature.

This was referenced May 10, 2021
Closed
Closed
@bartonjs
Copy link
Member

The new overloads need more work, @davidfowl has concerns about the return type, and some explosion.

The new LogOptions class should be LogDefineOptions

namespace Microsoft.Extensions.Logging
{ 
    public class LogDefineOptions
    {
        public bool SkipEnabledCheck { get; set; }
    }
}

Updating the not-yet-released API:

namespace Microsoft.Extensions.Logging
{ 
    public static partial class LoggerMessage
    {
-        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, bool skipEnabledCheck) { 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, Microsoft.Extensions.Logging.LogOptions options) {​ 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, bool skipEnabledCheck) { 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, Microsoft.Extensions.Logging.LogOptions options) {​ 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, bool skipEnabledCheck) { 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, Microsoft.Extensions.Logging.LogOptions options) {​ 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, bool skipEnabledCheck) { 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, Microsoft.Extensions.Logging.LogOptions options) {​ 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, bool skipEnabledCheck) { 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, Microsoft.Extensions.Logging.LogOptions options) {​ 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, bool skipEnabledCheck) { 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, Microsoft.Extensions.Logging.LogOptions options) {​ throw null; }​
    }
}

@bartonjs bartonjs added api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels May 11, 2021
@maryamariyan maryamariyan removed the blocking Marks issues that we want to fast track in order to unblock other important work label Jun 8, 2021
maryamariyan added a commit to maryamariyan/runtime that referenced this issue Jun 22, 2021
@maryamariyan
Change preview overloads, take LogDefineOptions
ca9bbc4 
Related to dotnet#50913
@maryamariyan maryamariyan mentioned this issue Jun 22, 2021
Merged
maryamariyan added a commit that referenced this issue Jul 6, 2021
@maryamariyan
Update new LoggerMessage.Define APIs, to take LogDefineOptions (#…
be5a950 
…54581)

* Change preview overloads, take LogDefineOptions

Related to #50913

* Apply PR feedback

* Fix compile issue
@maryamariyan maryamariyan mentioned this issue Jul 12, 2021
Open
1 task
@maryamariyan
Copy link
Member Author

Closing this issue in favor of #55525. We already covered updating new .NET 6.0 overloads to use LogDefineOptions. The remainder of the work (support for more arguments) is scoped for .NET 7.0

ML, Extensions, Globalization, etc, POD. automation moved this from 6.0.0 to Done Jul 12, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-Extensions-Logging
Projects
None yet
Milestone
6.0.0
Development

No branches or pull requests
4 participants
@davidfowl @GrabYourPitchforks @maryamariyan @bartonjs