dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.29k stars 4.74k forks source link

CodeFix to make strongly-typed ILogger extension method from weakly-typed calls #36020

Open lodejard opened 5 years ago

lodejard commented 5 years ago

Is your feature request related to a problem? Please describe.

The weakly-typed structured logging can be done directly from an application classes, which helps my teams add the right logging at the right place without interrupting their development workflow.

I'm am trying to adopt strongly-typed logging in application domain components but adding extension methods requires all of our developers to have the discipline to switch over to a static logging extension class, declare a field and method there first, then switch back and finally add the call at the place they were at in the application class.

Describe the solution you'd like

I would like to be able to put weakly-typed logger calls in the application class and have them all converted to strongly typed extension methods with a Roslyn analyzer.

public class MyWorker
{
    private readonly ILogger<MyWorker> _logger;

    public MyWorker(ILogger<MyWorker> logger)
    {
        _logger = logger;
    }

    public void FetchKeys(string keyRingName)
    {
        _logger.LogInformation("FetchingKeys: Fetching all of the keys in the {KeyRing} key ring", keyRingName);
    }
}

Should provide a code fix ends up like:

    public void FetchKeys(string keyRingName)
    {
        _logger.FetchingKeys(keyRingName);
    }
...
public internal class MyWorkerLoggerExtensions
{
    private readonly LogMessage<string> _logFetchingKeys = (LogLevel.Information, nameof(FetchingKeys), "Fetching all of the keys in the {KeyRing} key ring");

    /// <summary>
    /// Fetching all of the keys in the {KeyRing} key ring
    /// </summary>
    /// <param name="logger">The <see cref="ILogger"/> to write to.</param>
    /// <param name="keyRing">The {KeyRing} log message property.</param>
    public void FetchingKeys(ILogger<MyWorker> logger, string keyRing) => _logFetchingKeys.Log(logger, keyRing);
}

Describe alternatives you've considered

You could put your log definitions in a data file as well, but that still leaves you with the problem that it breaks up your developer workflow. And compile-time codegen brings in a whole new set of challenges.

Additional context

Ideally there should also be a warning when a strongly-typed log extension method is unused. There should also be a simple way to assert from a unit test that the category/eventid strings have not changed, because refactoring method, class, and namespace name is pretty easy.

Dotnet-GitSync-Bot commented 4 years ago

I couldn't figure out the best area label to add to this issue. Please help me learn by adding exactly one area label.

maryamariyan commented 4 years ago

Existing Logging Analyzers today live under

https://github.com/dotnet/extensions/blob/master/src/Logging/Logging.Analyzers/src/

I marked this up-for-grabs in case someone wants to add an analyzer for this scenario as well.

Also a link to docs on LoggerMessage.Define: https://docs.microsoft.com/en-us/aspnet/core/fundamentals/logging/loggermessage?view=aspnetcore-5.0#loggermessagedefine

cc: @davidfowl

KalleOlaviNiemitalo commented 4 years ago

Could also offer a code fix to create an extension method when calling an undefined method on ILogger<TCategoryName>:

    public void FetchKeys(string keyRingName)
    {
        _logger.FetchingKeys(keyRingName);
    }

It wouldn't know the entire format string in that case, but it could use "{KeyRingName}" (with the recommended initial capital) and let the developer add the rest.