`Microsoft.Extensions.Http.Resilience` breaks `LoggerMessage` code generation.

Question

`Microsoft.Extensions.Http.Resilience` breaks `LoggerMessage` code generation.

hugoqribeiro opened this issue 4 months ago · 20 comments

Description

We have a .NET 8 project where we add a reference to a package that includes a dependency on Microsoft.Extensions.Http.Resilience. That project also uses [LoggerMessage] to take advantage of the source generator.

That project also includes a reference to a package (.NET Standard 2.0) that is using il-repack, which results in all the types under Microsoft.Extensions.Logging (including Microsoft.Extensions.Logging.LoggerMessageAttribute) being included in the assembly as internal types.

In this scenario, the source generator does not work.

[LoggerMessage(Level = LogLevel.Information, Message = "Something")]
private static partial void LogSomething(ILogger logger);

The error is:

CS8795	Partial method 'Program.LogSomething(ILogger)' must have an implementation part because it has accessibility modifiers.

Investigating this, I believe that the problem is related with how Microsoft.Gen.Logging.LoggingGenerator.LoggingGenerator is looking up for Microsoft.Extensions.Logging.LoggerMessageAttribute.

I think that Microsoft.Extensions.Logging.Generators.LoggerMessageGenerator.Parser did that lookup considering the visibility of the types:

INamedTypeSymbol bestTypeByMetadataName = _compilation.GetBestTypeByMetadataName("Microsoft.Extensions.Logging.LoggerMessageAttribute");

I'm not 100% sure, but it looks like Microsoft.Gen.Logging.LoggingGenerator.LoggingGenerator isn't considering visibility. Which results in the code not being generated.

Reproduction Steps

Here's the csproj definition:

<Project Sdk="Microsoft.NET.Sdk">

    <PropertyGroup>
        <OutputType>Exe</OutputType>
        <TargetFramework>net8.0</TargetFramework>
        <ImplicitUsings>enable</ImplicitUsings>
        <Nullable>enable</Nullable>
    </PropertyGroup>

    <ItemGroup>
        <PackageReference Include="Cegid.Hydrogen.Hosting" Version="8.0.5" />
        <PackageReference Include="Cegid.Lithium.Settings.Client.Rest.SingleAssembly" Version="6.0.0" />
    </ItemGroup>

</Project>

Cegid.Hydrogen.Hosting is the package that depends on Microsoft.Extensions.Http.Resilience.
Cegid.Lithium.Settings.Client.Rest.SingleAssembly is the package that uses il-repack and has LoggerMessageAttribute as an internal type.

Expected behavior

The source generator should work as expected, because there is only one public type accessible named Microsoft.Extensions.Logging.LoggerMessageAttribute.

Actual behavior

The source generator does not kick-in.

Regression?

No.

Known Workarounds

Adding this in the csproj, after the package reference, makes the old source generator work:

<PropertyGroup>
   <DisableMicrosoftExtensionsLoggingSourceGenerator>false</DisableMicrosoftExtensionsLoggingSourceGenerator>
</PropertyGroup>

Obviously, this does not work if we remove the reference to the single assembly, as BOTH source generators kick-in and we end up with 2 implementations of the logging method.

Configuration

We're using .NET 8.0.4 and Microsoft.Extensions.Http.Resilience 8.4.0.
Nothing else seams relevant for this issue.

Other information

No response

Answer 1 · 2024-05-22T12:32:21.000Z

Can you provide a minimal reproduction of this issue using il-repack? It would help with investigations.

Answer 2 · 2024-05-22T13:58:54.000Z

You mean a simplified package after being processed with il-repack?

I believe that I can reproduce this without il-repack, if I add an internal type named Microsoft.Extensions.Logging.LoggerMessageAttribute in the original project...

Will that help?

Answer 3 · 2024-05-22T14:09:57.000Z

With il-repack, it can be configured to not internalize specific types. Do you know if this is possible to use in your case?

 - /internalize[:<excludefile>]  sets all types but the ones from the first assembly 'internal'. <excludefile> contains one regex per
                                 line to compare against FullName of types NOT to internalize.

Answer 4 · 2024-05-22T14:15:13.000Z

With il-repack, it can be configured to not internalize specific types. Do you know if this is possible to use in your case?
 - /internalize[:<excludefile>]  sets all types but the ones from the first assembly 'internal'. <excludefile> contains one regex per
                                 line to compare against FullName of types NOT to internalize.

I'm unsure that having 2 public types with the same name will solve the issue.
What I've tried was renaming Microsoft.Extensions.Logging to Cegid.Microsoft.Extensions.Logging, but that breaks the package at runtime (because il-repack is unable to do the replaces everywhere, for some reason that I didn't understand (yet).

Let me try reproducing the issue without il-repack...

Answer 5 · 2024-05-22T14:40:07.000Z

Can you provide a minimal reproduction of this issue using il-repack? It would help with investigations.

OK. This is reproducible by referencing a simple client library.

Here's a solution that reproduces the problem:

Repo: https://github.com/hugoqribeiro/issues
Solution: https://github.com/hugoqribeiro/issues/tree/main/src/2024.05.Microsoft.Extensions.Http.Resilience

Answer 6 · 2024-05-22T15:30:22.000Z

Thanks for the repro

Answer 7 · 2024-05-22T15:32:01.000Z

I'm unsure that having 2 public types with the same name will solve the issue.

There's also this /union but not sure how that will look either without actually trying it.

Answer 8 · 2024-07-31T16:05:53.000Z

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

Answer 9 · 2024-08-01T13:27:39.000Z

Hi @dariusclay? No news on this issue?

It keeps coming back as more and more of our projects that the dependency that references Microsoft.Extensions.Http.Resilience.

Answer 10 · 2024-08-02T12:05:23.000Z

@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?

Answer 11 · 2024-08-02T12:10:18.000Z

@hugoqribeiro we can alter the logic to search for a public attribute. Although, I am curious if using the il-repack options internalize + union works for you?

Answer 12 · 2024-08-02T12:58:02.000Z

@dariusclay thank you for the explanation and the workaround!
Even though there may be a good reason for it, I was surprised to read

it replaces the .NET logging generator with the .NET extensions logging generator. This has slightly different behavior as seen with diagnostic warning.

A lot of people will be getting the Microsoft.Extensions.Http.Resilience package as part of adding Aspire.NET orchestration to their solution, and are not notified of this behavior change.

Can you please move our comments to a new issue? I can only recreate my own comments but not move yours (I did not intend to hijack @hugoqribeiro's issue, I thought they our issues were symptoms of the same cause).

Answer 13 · 2024-08-02T15:42:39.000Z

To the original discussion -- I've narrowed down the issue and created a unit test that captures the problem during symbol loading. Will publish a PR with the changes soon, and we can discuss on the review. @geeknoid

Answer 14 · 2024-08-02T16:02:13.000Z

@hugoqribeiro note that this will occur if any of the following types are internalized by il-repack

"Microsoft.Extensions.Logging.LoggerMessageAttribute"
"Microsoft.Extensions.Logging.LogPropertiesAttribute"
"Microsoft.Extensions.Logging.TagProviderAttribute"
"Microsoft.Extensions.Logging.TagNameAttribute"
"Microsoft.Extensions.Logging.LogPropertyIgnoreAttribute"
"Microsoft.Extensions.Logging.ITagCollector"
"Microsoft.Extensions.Logging.ILogger"
"Microsoft.Extensions.Logging.LogLevel"
"Microsoft.Extensions.Compliance.Classification.DataClassificationAttribute"
"Microsoft.Extensions.Compliance.Classification.NoDataClassificationAttribute"

Answer 15 · 2024-08-04T16:51:42.000Z

@dariusclay, thanks. I'm waiting for the guy that implemented the il-repack thing to come back from vacations. :)

Answer 16 · 2024-08-13T16:27:39.000Z

Hi @dariusclay, we tested union and it doesn't solve the issue. There is still an internal ILogger that causes the issue.

I see you merged a fix into main. Does that mean that it will be released in 8.x anytime soon?

Thanks.

Answer 17 · 2024-08-13T20:29:09.000Z

@hugoqribeiro Yes, this will be part of the next monthly snapshot.

Answer 18 · 2024-08-14T09:26:22.000Z

@hugoqribeiro can you test the fix using our nightlies? I did a test run using my local repro but it will be good to know if it works in the wild.

Update your nuget package sources to include our public nightly feed:
https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet8/nuget/v3/index.json

Add a reference to this package:
<PackageReference Include="Microsoft.Extensions.Telemetry.Abstractions" Version="8.8.0-preview.24413.3" />