Bump Microsoft.Extensions.Logging to net8 - part 2

[reyang]

Fixed #3205.

Follow up #4920.
Related to #4902.

Changes

1. Added Microsoft.Extensions.Logging dependency to OpenTelemetry SDK.
2. Removed Microsoft.Extension.Logging dependency from other projects since they are now getting the correct version from the SDK dependency.
3. Cleaned up build warnings that were introduced by the version bump (since Microsoft.Extensions.Logging version 8 and the transient dependencies raised bar for static analysis).
4. @CodeBlanch made some updates to the instrumentation build/package flow - these were bugs introduced earlier but not surfaced until now.

[codecov]

Codecov Report

Merging #4933 (6658fe9) into main (b091af1) will increase coverage by 83.48%.
The diff coverage is 75.00%.

@@            Coverage Diff            @@
##           main    #4933       +/-   ##
=========================================
+ Coverage      0   83.48%   +83.48%     
=========================================
  Files         0      295      +295     
  Lines         0    12325    +12325     
=========================================
+ Hits          0    10290    +10290     
- Misses        0     2035     +2035     

Flag	Coverage Δ
unittests	83.48% <75.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...lder/ProviderBuilderServiceCollectionExtensions.cs	88.23% <100.00%> (ø)
src/Shared/Options/ConfigurationExtensions.cs	96.00% <100.00%> (ø)
.../OpenTelemetry/Logs/ILogger/OpenTelemetryLogger.cs	86.79% <0.00%> (ø)

... and 292 files with indirect coverage changes

[Kielek]

How does auto-instrumention solve the System.Diagnostics.DiagnosticSource version challenge, can the same approach be applied to Microsoft.Extensions.Logging? Is there something that folks from .NET runtime can help?

@reyang, AutoInstrumentation suffers for each dependency bump in OTel SDK/APi/Instrumentation/Contrib packages. It includes System.Diagnostics.DiagnosticSource.

For .NET Framework, using profiler api, we have pretty good solution for redirecting used library versions to the latest from the release time. While library loading we check if it is on special list. If so, our version is loaded: List of libraries is avilable under https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/blob/v1.0.2/src/OpenTelemetry.AutoInstrumentation.Native/netfx_assembly_redirection.h.

This option is not available for .NET6+.

I have found realistic scenario to illustrate the issue. Please check TestApplication.GrpcNetClient referencing Grpc.Net.Client 2.56.0

2.56.0 and older versions of Grpc.Net.Client references Microsoft.Extensions.Logging.Abstractions (>= 3.0.3).

When you want to use it with Automatic Instrumentation it leads to

Unhandled exception. System.IO.FileNotFoundException: Could not load file or assembly 'Microsoft.Extensions.Logging.Abstractions, Version=3.1.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60'. The system cannot find the file specified.
File name: 'Microsoft.Extensions.Logging.Abstractions, Version=3.1.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60'
   at OpenTelemetry.Batch`1.Dispose()
   at OpenTelemetry.BatchExportProcessor`1.ExporterProc()
   at System.Threading.Thread.StartCallback()

The older version library is loaded from the application directory instead of additional store provided by automatic instrumentation distribution.

If you decide to make a bump of the Microsoft.Extensions.Logging the broader set of applications/libraries will be affected by the issue (any application referencing library with older version than the newest).

In previous PR, you have asked if runtime team can help here, probably yes, but it requires changes in the runtime probably. I am not sure if .NET team has a plan to extend additional store functionalities.

Important references:
AutomaticInstrumentation structure: https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/blob/v1.0.2/test/IntegrationTests/BuildTests.DistributionStructure_windows.verified.txt
Packages to download to verify the content: https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/releases/tag/v1.0.2

.NET related env. configured for this scenario:

Environment variable	.NET version	Value
COR_ENABLE_PROFILING	.NET Framework	1
COR_PROFILER	.NET Framework	{918728DD-259F-4A6A-AC2B-B85E1B658318}
COR_PROFILER_PATH_32	.NET Framework	$INSTALL_DIR/win-x86/OpenTelemetry.AutoInstrumentation.Native.dll
COR_PROFILER_PATH_64	.NET Framework	$INSTALL_DIR/win-x64/OpenTelemetry.AutoInstrumentation.Native.dll
CORECLR_ENABLE_PROFILING	.NET	1
CORECLR_PROFILER	.NET	{918728DD-259F-4A6A-AC2B-B85E1B658318}
CORECLR_PROFILER_PATH	.NET on Linux glibc	$INSTALL_DIR/linux-x64/OpenTelemetry.AutoInstrumentation.Native.so
CORECLR_PROFILER_PATH	.NET on Linux musl	$INSTALL_DIR/linux-musl-x64/OpenTelemetry.AutoInstrumentation.Native.so
CORECLR_PROFILER_PATH	.NET on macOS	$INSTALL_DIR/osx-x64/OpenTelemetry.AutoInstrumentation.Native.dylib
CORECLR_PROFILER_PATH_32	.NET on Windows	$INSTALL_DIR/win-x86/OpenTelemetry.AutoInstrumentation.Native.dll
CORECLR_PROFILER_PATH_64	.NET on Windows	$INSTALL_DIR/win-x64/OpenTelemetry.AutoInstrumentation.Native.dll
DOTNET_ADDITIONAL_DEPS	.NET	$INSTALL_DIR/AdditionalDeps
DOTNET_SHARED_STORE	.NET	$INSTALL_DIR/store
DOTNET_STARTUP_HOOKS	.NET	$INSTALL_DIR/net/OpenTelemetry.AutoInstrumentation.StartupHook.dll
OTEL_DOTNET_AUTO_HOME	All versions	$INSTALL_DIR

I remember that we have a automatic instrumentation nuget package to mitigate the issue, but it is not always feasible.

[reyang]

AutoInstrumentation suffers for each dependency bump in OTel SDK/APi/Instrumentation/Contrib packages. It includes System.Diagnostics.DiagnosticSource.

If you decide to make a bump of the Microsoft.Extensions.Logging the broader set of applications/libraries will be affected by the issue (any application referencing library with older version than the newest).

What's the direction of OpenTelemetry .NET auto-instrumentation? Is there a proposed solution, or the SIG feels stuck and doesn't have a clue how to address this type of issue?

In #4920 (review), @Kielek you mentioned:

Based on this, it should be safe to update dependency to 6.0.0 (I assume that it includes all critical fixes).

The way I look at these problems:

1. We should always be prepared for CVEs, and if CVE indeed happened in the near future, we'll certainly need to bump versions. OpenTelemetry .NET and auto-instrumentation SIGs should both make sure that we have a solution instead of holding back or delaying the conversation.
2. If OpenTelemetry .NET auto-instrumentation SIG doesn't have a solution to version bumps, I suggest treating it as a priority and come up with a proposal, if we need time to fix the problem, we can wait as long as the wait time is reasonable.
3. If we don't have a solution, and we don't work on a solution, the next CVE will give us bigger trouble since we'll have to make changes and break things. In other words, customers of auto-instrumentation are using a technology that might stop working at anytime.

[Kielek]

In #4920 (review), @Kielek you mentioned:

Based on this, it should be safe to update dependency to 6.0.0 (I assume that it includes all critical fixes).

I have written from top of my head. and I have made a mistake. Sorry. I will rise the topic on .NET Auto SIG in few minutes.

[rajkumar-rangaraj]

Update from the .NET Auto SIG: We've decided not to block any important package updates that occur in the SDK space. Auto-Instrumentation will implement a workaround to address the assembly load issue. Furthermore, we are exploring the possibility of a new design that might allow auto-instrumentation to overcome assembly load issues caused by package updates from the SDK.

[CodeBlanch]

This PR LGTM and it is fine to merge.

One thing we talked about on the SIG IIRC was that we wanted to release a 1.7.0 alpha which contained the OTLP changes to reintroduce log category name, eventId, & exception details before we took dependencies on anything from .NET 8. I don't know if we had really strong reasons why we wanted to do that other than it just seemed like a nice friendly thing to do for consumers 🤷 Just bringing it up here as a reminder.

[reyang]

One thing we talked about on the SIG IIRC was that we wanted to release a 1.7.0 alpha which contained the OTLP changes to reintroduce log category name, eventId, & exception details before we took dependencies on anything from .NET 8. I don't know if we had really strong reasons why we wanted to do that other than it just seemed like a nice friendly thing to do for consumers 🤷 Just bringing it up here as a reminder.

Would you elaborate what's the thinking behind this (not taking any .NET 8 dependencies in 1.7.0-alpha)?

[alanwest]

Would you elaborate what's the thinking behind this (not taking any .NET 8 dependencies in 1.7.0-alpha)?

I believe we were considering doing one more stable release prior to .net 8 releasing. However at this point with .net 8 only about a month away, I doubt that makes sense anymore. I think it's safe to commit to releasing our next stable after .net 8.

[reyang]

Would you elaborate what's the thinking behind this (not taking any .NET 8 dependencies in 1.7.0-alpha)?

I believe we were considering doing one more stable release prior to .net 8 releasing. However at this point with .net 8 only about a month away, I doubt that makes sense anymore. I think it's safe to commit to releasing our next stable after .net 8.

I want to double click on the thinking behind holding changes for alpha releases - here is my understanding:

1. Alpha releases are meant to carry risks so we can get feedback as early as possible.
2. .NET8 target (including AOT, ILogger, and TFM changes) is a fairly significant change, we're already too close to the .NET8 GA time - IMHO the first alpha release which takes .NET 8 RC dependency should be shipped on Day 1 when .NET 8 RC1 was released.
3. Having a "more stable" alpha release sounds like the wrong direction. If there is a need to quickly ship something stable, a minor release is the way to go (e.g. 1.6.1 instead of 1.7.0-alpha.1).