Behavior changes between .NET logging generator and .NET extensions

[dariusclay]

          To add to this: I'm having a similar issue with the visibility of an inherited `ILogger` property.

We're using the Azure Mobile Apps SDK from Microsoft. This SDK has a base class for a controller which you can inherit from, containing a public property of type ILogger:

namespace Microsoft.AspNetCore.Datasync
{
    ...
    public class TableController<TEntity> : ControllerBase where TEntity : class, ITableData
    {
        ...
        public ILogger Logger { get; set; } = null;
        ...
    }
}

I inherited from that class as intended by the Azure Mobile Apps SDK and in my class have a more specific ILogger<T> property as follows:

public partial class InspectionSyncController : TableController<FarmInspectionSyncModel>
{
    private readonly ILogger<InspectionSyncController> logger;

    public InspectionSyncController(
        ...
        ILogger<InspectionSyncController> logger)
        : base(...)
    {
        ...
        this.logger = logger;
    }
    ...
    [LoggerMessage(
    EventId = 0,
    Level = LogLevel.Warning,
    Message = "Not authorized to perform operation `{Operation}` on entity `{EntityId}`")]
    private partial void LogNotAuthorizedToPerformOperationOnEntity(TableOperation operation, string? entityId);
    ...
}

This works without Microsoft.Extensions.Http.Resilience.

Actual behavior

After adding Microsoft.Extensions.Http.Resilience, I get this warning:

Found multiple fields or properties of type "Microsoft.Extensions.Logging.ILogger" in type "InspectionSyncController" (https://aka.ms/dotnet-extensions-warnings/LOGGEN016).

Expected behavior

It still works after adding Microsoft.Extensions.Http.Resilience

Originally posted by @hansmbakker in https://github.com/dotnet/extensions/issues/5173#issuecomment-2260871645

[dariusclay]

@hansmbakker I've moved the thread here

[dariusclay]

@hansmbakker your case is a little different than the one in the original post. When including the Microsoft.Extensions.Http.Resilience package, it replaces the .NET logging generator with the .NET extensions logging generator. This has slightly different behavior as seen with diagnostic warning. You can get around this by providing an ILogger parameter to the method and making it static:

public partial class InspectionSyncController : TableController<FarmInspectionSyncModel>
{
    private readonly ILogger<InspectionSyncController> logger;

    public InspectionSyncController(
        ...
        ILogger<InspectionSyncController> logger)
        : base(...)
    {
        ...
        this.logger = logger;
    }
    ...
    [LoggerMessage(
    EventId = 0,
    Level = LogLevel.Warning,
    Message = "Not authorized to perform operation `{Operation}` on entity `{EntityId}`")]
    private static partial void LogNotAuthorizedToPerformOperationOnEntity(ILogger logger, TableOperation operation, string? entityId);
    ...
}

@geeknoid should we consider this a bug and bring parity to the way the .NET generator works? To only use the most derived logger member?

[dariusclay]

@joperezr do you have any opinions on this?

[dariusclay]

Related to #5336

[joperezr]

Ideally we'd want the dotnet/extensions logger be a superset of features on top of the inbox one, to facilitate onboarding and upgrade scenarios, so whenever possible, we should really try to have parity. Is there a scenario in the extensions logger space that would break if we change this particular behavior to match the inbox logger?

[hansmbakker]

it replaces the .NET logging generator with the .NET extensions logging generator

Question without knowing the context - what causes the need for a replacement of the default .NET logger source generator? Could the extensions packages not simply take a dependency on that default .NET logger source generator?

[joperezr]

The logger in this repository has some additional features on top of the built-in one (like for example it allows you do to redaction or enrichment on your log messages and it also allows logging full object's state) but the way source generator works, doesn't really allow for them to compose very well. For logging, the generator typically fills in the implementation of a logging method, but doesn't really have extensibility points for an additional generator to enhance it, so for now we instead replace the generator so that we can fill in the implementation of the logger method.

[dariusclay]

@joperezr I don't think that there would be anything necessarily breaking if we align behavior, but will deep-dive to see.

[geeknoid]

@dariusclay Yeah, it's probably reasonable to follow the behavior from the original logger. I suspect this behavior was added to dotnet/runtime at some point and I missed replicating that into the dotnet/extensions code generator.

[dariusclay]

Thanks @geeknoid! I'm working on tacking this one piece at a time