Taking the example from #45290 and bring it to the logging-generator:
public class Connection
{
private string? _connectionId;
public string ConnectionId => _connectionId ??= GenerateConnectionId();
private string GenerateConnectionId() => Guid.NewGuid().ToString(); // or something more expensive
}
public static partial class Log
{
public static void ConnectionStarted(this ILogger logger, Connection connection)
{
if (logger.IsEnabled(LogLevel.Debug))
{
ConnectionStarted(logger, connection.ConnectionId);
}
}
[LoggerMessage(EventId = 1, EventName = "ConnectionStarted", Level = LogLevel.Debug, Message = "Connection {ConnectionId} started")]
private static partial void ConnectionStarted(this ILogger logger, string connectionId);
}
The key-point of the logger.IsEnabled check is to avoid the creation of a expensive resource if not needed.
The generated code looks like (simplifyed some names, to make it easiert to read):
partial class Log
{
private static readonly Action<ILogger, string, Exception?> __ConnectionStartedCallback =
LoggerMessage.Define<string>(LogLevel.Debug, new EventId(1, "ConnectionStarted"), "Connection {ConnectionId} started", true);
private static partial void ConnectionStarted(this global::Microsoft.Extensions.Logging.ILogger logger, global::System.String connectionId)
{
if (logger.IsEnabled(global::Microsoft.Extensions.Logging.LogLevel.Debug))
{
__ConnectionStartedCallback(logger, connectionId, null);
}
}
}
So in LoggerMessage.Define true is passed in to skip the IsEnabled-check, which is good.
But in the "logging method" IsEnabled is checked, regardless if our code already checked it.
Note: the IsEnabled check isn't free either, I'll defer to @davidfowl's comment: #50334 (comment)
Thus I propose to extend
[AttributeUsage(AttributeTargets.Method)]
public sealed class LoggerMessageAttribute : Attribute
{
public LoggerMessageAttribute() { }
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; } = false;
}
so the above example could be written as:
public static partial class Log
{
// ...
[LoggerMessage(EventId = 1, EventName = "ConnectionStarted", Level = LogLevel.Debug, Message = "Connection {ConnectionId} started", SkipEnabledCheck = true)]
private static partial void ConnectionStarted(this ILogger logger, string connectionId);
}
and the generated code to be like:
partial class Log
{
private static readonly Action<ILogger, string, Exception?> __ConnectionStartedCallback =
LoggerMessage.Define<string>(LogLevel.Debug, new EventId(1, "ConnectionStarted"), "Connection {ConnectionId} started", true);
private static partial void ConnectionStarted(this global::Microsoft.Extensions.Logging.ILogger logger, global::System.String connectionId)
{
__ConnectionStartedCallback(logger, connectionId, null);
}
}
That way IsEnabled is checked only once, if the user is aware of that pattern and opts-into this behaviour.
/cc: @maryamariyan
PS: should this be a formal api proposal? I didn't chose that template, as this api isn't shipped public
RetroSearch is an open source project built by @garambo | Open a GitHub Issue
Search and Browse the WWW like it's 1997 | Search results from DuckDuckGo
HTML:
3.2
| Encoding:
UTF-8
| Version:
0.7.4