[API Proposal]: Add the ability to disable code generation, that overrides default code generators from Microsoft.Extensions.Logging

[vanbukin]

Background and motivation

As a developer, I want to add different policies to my http clients that ensure resilience: retry, bulkhead, timeout, etc. I am connecting the nuget package Microsoft.Extensions.Http.Resilience, which contains all the necessary classes for this. My project includes treat warnings as errors, as well as all analyzers. My logging is breaking down.

This happens because the Microsoft.Extensions.Http.Resilience package depends on the Microsoft.Extensions.Resilience package, which in turn depends on Microsoft.Extensions.Telemetry.Abstractions, which contains custom code generation for logging and its own set of analyzers, which also override code generation for logs from Microsoft.Extensions.Logging. As a developer, I want to retries for HTTP clients, please do not touch my logs.

I have the following code in my project that works with the Microsoft.Extensions.Logging code generator, which understands the MessageTemplates format

public static partial class DefaultModelStateErrorLoggerLoggingExtensions
{
    [LoggerMessage(Level = LogLevel.Warning, Message = "Invalid model state: {@ModelState}")]
    public static partial void InvalidModelState(this ILogger logger, LoggingModelState modelState);
}

then my logs are transferred to Serilog, which also understands the MessageTemplates format and when writing to sink via the formatter, it decomposes the transferred object into separate properties.

And I want it to stay as it is, but instead I get a warning

DefaultInvalidModelStateLogger.cs(31,89): Warning LOGGEN036 : The type "Infrastructure.Mvc.Extensions.Models.LoggingModelState" doesn't implement ToString(), IConvertible, or IFormattable (did you forget to apply [LogProperties] or [TagProvider] to "modelState"?) (https://aka.ms/dotnet-extensions-warnings/LOGGEN036)

Due to treat warnings as errors, this becomes a compilation error. I could suppress it or implement what is required from me, but the problem is not in the warning as such, but in the fact that instead of the code generator from Microsoft.Extensions.Logging, I now use the code generator from Microsoft.Extensions.Telemetry.Abstractions.

Therefore, I suggest adding an MSBuild property that allows to disable this logic globally (via Directory.Build.props at the level .sln) or locally (at the level of a single project, in its .csproj file).

API Proposal

MSBuild property that disables the replacement of the log generator from Microsoft.Extensions.Logging with the log generator from Microsoft.Extensions.Telemetry.Abstractions

<Project>
    <PropertyGroup>
        <DisableMicrosoftExtensionsTelemetrySourceGeneration>true</DisableMicrosoftExtensionsTelemetrySourceGeneration>
    </PropertyGroup>
</Project>

API Usage

Set the required value to true in the csproj file or in the Directory.Build.props

Workaround

Directory.Build.targets

<Project>

    <Target Name="_Microsoft_Extensions_Telemetry_AbstractionsRemoveAnalyzers" BeforeTargets="ResolveReferences">
        <PropertyGroup>
            <DisableMicrosoftExtensionsLoggingSourceGenerator>false</DisableMicrosoftExtensionsLoggingSourceGenerator>
        </PropertyGroup>
        <ItemGroup>
            <_Microsoft_Extensions_Telemetry_AbstractionsAnalyzer
                Include="@(Analyzer)"
                Condition="'%(Analyzer.NuGetPackageId)' == 'Microsoft.Extensions.Telemetry.Abstractions'" />
        </ItemGroup>
        <ItemGroup>
            <Analyzer Remove="@(_Microsoft_Extensions_Telemetry_AbstractionsAnalyzer)" />
        </ItemGroup>
    </Target>

</Project>

Risks

When referencing any NuGet package that has a dependency on Microsoft.Extensions.Telemetry.Abstractions, another code generator for logs begins to be used.

[iliar-turdushev]

@vanbukin Could you, please, try to use the following property?

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

Setting it to false should disable replacing of M.E.Logging with M.E.Telemetry.Abstractions.

[vanbukin]

@iliar-turdushev

I checked and it doesn't work. I also made a dotnet build -bl and looked at the MSBuild logs using https://msbuildlog.com and I found the following there:

Property reassignment: $(DisableMicrosoftExtensionsLoggingSourceGenerator)="true" (previous value: "false") at C:\Users\vanbukin\.nuget\packages\microsoft.extensions.telemetry.abstractions\8.9.1\buildTransitive\net6.0\Microsoft.Extensions.Telemetry.Abstractions.props (4,5)

This happens because the NuGet package contains a buildTransitive with a Microsoft.Extensions.Telemetry.Abstractions.props file that explicitly overrides the value of DisableMicrosoftExtensionsLoggingSourceGenerator to true.

[iliar-turdushev]

This happens because the NuGet package contains a buildTransitive with a Microsoft.Extensions.Telemetry.Abstractions.props file that explicitly overrides the value of DisableMicrosoftExtensionsLoggingSourceGenerator to true.

That sounds like a bug, unless I'm missing something.

@RussKie @dariusclay How do you think if we should remove the props file, and users will be able to decide whether to enable/disable M.E.Telemetry.Abstractions logging?

[dariusclay]

I'm less familiar with the Resilience package. Does it depend on anything in particular from the Telemetry.Abstractions package? I'll need to confirm.

The thing is, if redaction or enrichment is required anywhere then ExtendedLogger should be used.

[dariusclay]

The other comment here, is that the dotnet/extensions logging generator SHOULD be compatible with the one provided by dotnet/runtime, and thus should not break users when this one is used. If breaks users then it's a bug and we can work to fix the problem instead.

[RussKie]

@iliar-turdushev

I checked and it doesn't work. I also made a dotnet build -bl and looked at the MSBuild logs using msbuildlog.com and I found the following there:

Property reassignment: $(DisableMicrosoftExtensionsLoggingSourceGenerator)="true" (previous value: "false") at C:\Users\vanbukin\.nuget\packages\microsoft.extensions.telemetry.abstractions\8.9.1\buildTransitive\net6.0\Microsoft.Extensions.Telemetry.Abstractions.props (4,5)

This happens because the NuGet package contains a buildTransitive with a Microsoft.Extensions.Telemetry.Abstractions.props file that explicitly overrides the value of DisableMicrosoftExtensionsLoggingSourceGenerator to true.

It means you set it too early. Try setting it in a targets file.

[RussKie]

It would be great if you could provide a simple standalone repro so we could investigate it further. Without a repro we'd be speculating a lot.

[vanbukin]

I made a repository that reproduces the problem. Initially, the repository is in a state where everything is working. To reproduce the problem, uncomment the installation of Microsoft.Extensions.Http.Resilience in ReproExtensions5476.csproj There are also added Directory.Build.props and Directory.Build.targets. You are free to experiment with them.

[vanbukin]

@RussKie

Try setting it in a targets file.

Uncomment the installation of Microsoft.Extensions.Http.Resilience:

0>Program.cs(135,91): Error LOGGEN036 : The type "ReproExtensions5476.WeatherForecast" doesn't implement ToString(), IConvertible, or IFormattable (did you forget to apply [LogProperties] or [TagProvider] to "first"?) (https://aka.ms/dotnet-extensions-warnings/LOGGEN036)

Next, I try to fix these errors by setting the DisableMicrosoftExtensionsLoggingSourceGenerator property to false.

1. Directory.Build.targets with the following contents:

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

leads me to the following errors:

0>Program.cs(135,91): Error LOGGEN036 : The type "ReproExtensions5476.WeatherForecast" doesn't implement ToString(), IConvertible, or IFormattable (did you forget to apply [LogProperties] or [TagProvider] to "first"?) (https://aka.ms/dotnet-extensions-warnings/LOGGEN036)
0>LoggerMessage.g.cs(13,36): Error CS0757 : A partial method may not have multiple implementing declarations
0>LoggerMessage.g.cs(25,36): Error CS0757 : A partial method may not have multiple implementing declarations

2. Try with Directory.Build.props:

<Project>

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

</Project>

and we get the following error:

0>Program.cs(135,91): Error LOGGEN036 : The type "ReproExtensions5476.WeatherForecast" doesn't implement ToString(), IConvertible, or IFormattable (did you forget to apply [LogProperties] or [TagProvider] to "first"?) (https://aka.ms/dotnet-extensions-warnings/LOGGEN036)

3. ReproExtensions5476.csproj:

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

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

</Project>

and we get:

0>Program.cs(135,91): Error LOGGEN036 : The type "ReproExtensions5476.WeatherForecast" doesn't implement ToString(), IConvertible, or IFormattable (did you forget to apply [LogProperties] or [TagProvider] to "first"?) (https://aka.ms/dotnet-extensions-warnings/LOGGEN036)
0>LoggerMessage.g.cs(13,36): Error CS0757 : A partial method may not have multiple implementing declarations
0>LoggerMessage.g.cs(25,36): Error CS0757 : A partial method may not have multiple implementing declarations

4. Now let's try to combine my workaround with the proposed solution and edit the Directory.Build.targets as follows:

<Project>

    <Target Name="_Microsoft_Extensions_Telemetry_AbstractionsRemoveAnalyzers" BeforeTargets="ResolveReferences">
        <PropertyGroup>
            <DisableMicrosoftExtensionsLoggingSourceGenerator>false</DisableMicrosoftExtensionsLoggingSourceGenerator>
        </PropertyGroup>
    </Target>

</Project>

no luck:

0>Program.cs(135,91): Error LOGGEN036 : The type "ReproExtensions5476.WeatherForecast" doesn't implement ToString(), IConvertible, or IFormattable (did you forget to apply [LogProperties] or [TagProvider] to "first"?) (https://aka.ms/dotnet-extensions-warnings/LOGGEN036)
0>LoggerMessage.g.cs(13,36): Error CS0757 : A partial method may not have multiple implementing declarations
0>LoggerMessage.g.cs(25,36): Error CS0757 : A partial method may not have multiple implementing declarations

[iliar-turdushev]

@vanbukin Thank you for sharing the repro steps. I played with the project and wasn't able to fix the issue too. As @dariusclay mentioned, our source generator should be compatible with the one provided by Microsoft.Extensions.Logging.Abstractions and shouldn't break customers, e.g. triggering warnings or errors. What if we update our source generator not to trigger LOGGEN036 : The type "WeatherForecast" doesn't implement ToString(), IConvertible... warning/error?

[RussKie]

@geeknoid any thoughts on this?

[vanbukin]

@iliar-turdushev If the logging behavior does not change, then this also looks like an acceptable option.

[vanbukin]

Any updates?

[dariusclay]

Closing this issue so that we can track it properly here #5672