Fixing logging generator not getting removed in web projects

[joperezr]

Fixes #4286

PR #4238 was meant to automatically remove the platform's logging generator whenever the Microsoft.Extensions.Telemetry package was referenced, which worked just fine for console applications. For web projects though, the issue is that the generator can also be coming from the aspnetcore targeting pack, so we need special logic to be able to remove it from there as well. This PR should address that problem.

Microsoft Reviewers: Open in CodeFlow

[joperezr]

Saw a failure in CI in the Http.Telemetry tests which I'm almost sure is unrelated, @geeknoid @martintmk did we expect the following test to be flaky?

[xUnit.net 00:00:05.69]     Microsoft.Extensions.Http.Telemetry.Metering.Test.HttpMeteringHandlerTests.When_DiagSourceAndDelegatingHandler_BothConfigured_MetricsOnlyEmittedOnce [FAIL]
[xUnit.net 00:00:05.70]       Assert.Equal() Failure
[xUnit.net 00:00:05.70]       Expected: 1
[xUnit.net 00:00:05.70]       Actual:   2
[xUnit.net 00:00:05.70]       Stack Trace:
[xUnit.net 00:00:05.70]         /_/test/Libraries/Microsoft.Extensions.Http.Telemetry.Tests/Metering/HttpMeteringHandlerTests.cs(1089,0): at Microsoft.Extensions.Http.Telemetry.Metering.Test.HttpMeteringHandlerTests.When_DiagSourceAndDelegatingHandler_BothConfigured_MetricsOnlyEmittedOnce()
[xUnit.net 00:00:05.70]         --- End of stack trace from previous location ---
  Passed Microsoft.Extensions.Http.Telemetry.Metering.Test.HttpMeteringHandlerTests.AddHttpClientMeteringForAllHttpClients_EmitMetrics_OnException [2 s]
  Passed Microsoft.Extensions.Http.Telemetry.Tracing.Test.HttpClientTracingExtensionsTests.AddHttpClientTracing_GetRequestMetadataFromDownstreamDependencyMetadataManager_ActivityEnrichedWithMetadata [1 s]
  Failed Microsoft.Extensions.Http.Telemetry.Metering.Test.HttpMeteringHandlerTests.When_DiagSourceAndDelegatingHandler_BothConfigured_MetricsOnlyEmittedOnce [135 ms]
  Error Message:
   Assert.Equal() Failure
Expected: 1
Actual:   2
  Stack Trace:
     at Microsoft.Extensions.Http.Telemetry.Metering.Test.HttpMeteringHandlerTests.When_DiagSourceAndDelegatingHandler_BothConfigured_MetricsOnlyEmittedOnce() in /_/test/Libraries/Microsoft.Extensions.Http.Telemetry.Tests/Metering/HttpMeteringHandlerTests.cs:line 1089
--- End of stack trace from previous location ---

If it's just a one-off failure I guess it's fine ignoring, but if we see that again or have seen it before we should quarantine the test so we can investigate.

[xakep139]

@joperezr you can safely ignore that failure as we decided to remove metering bits from the repo anyway: #4214

[tarekgh]

Other than the comment I left, LGTM!

[RussKie]

Please re-target to release/8.0, if this is necessary for .NET 8.

[joperezr]

/backport to release/8.0-rc1

[github-actions]

Started backporting to release/8.0-rc1: https://github.com/dotnet/extensions/actions/runs/5906347957

[RussKie]

I'm curious, why did we choose to reuse the target name and didn't create a new one (e.g., _Microsoft_Extensions_Logging_AbstractionsRemoveAnalyzersForSure)? I wonder if MSBuild handles targets with the same name correctly.

[ViktorHofer]

Targets with the same name are overwritten. The last one wins.

[joperezr]

Exactly, and IIRC in this particular case it was by design and intentional to replace the other target as we wanted that the same property would trigger this target without the need of hooking it up again.

Overwriting targets is pretty common in MSBuild, that is for instance how cross-compiling works when doing the outer-builds (By re-defining the Build, Test, Restore, etc targets for outer builds.)

[ViktorHofer]

Hmm, what guarantees that this target is imported after the Microsoft.Extensions.Logging.Abstractions target when referencing the packages?

[joperezr]

Great question, I don't think that is deterministic. I did this a while back so don't recall exactly (and clearly I should've added better docs for it :P) but perhaps the issue then was that there were few cases where this target wouldn't be defined, so for those cases I'm just ensuring it's defined.

[joperezr]

Ohh ohh I think I remember now 😄. The logging generator is part of the aspnetcore targeting pack, so aspnetcore projects wouldn't need reference to the logging.abstractions package, and therefore wouldn't import the buildTransitive targets that disable the package. For these cases, I'm providing the target in our package as well, in order to make sure that the generator is also removed for aspnetcore apps.

We could have of course picked a different target name, but then we would have to add logic to detect if we are NOT an aspnetcore app and getting the reference from the package instead, so that we don't try to run the same operation twice. By just re-defining the target, we have the same mechanism for removing the generator whether your application references the logging.generator package, or the aspnetcore targeting pack, and we are guaranteed to only run once.

[joperezr]

Yup, that seems to be the reason why we picked this approach. If you read into the comments of the issue that this PR resolved, you can see how we realized about this case and picked this design to fix it: #4286 (comment)