Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing expected assembly when instrumenting .NET Framework apps #1646

Closed
Kielek opened this issue Nov 23, 2022 · 5 comments · Fixed by #1825
Closed

Missing expected assembly when instrumenting .NET Framework apps #1646

Kielek opened this issue Nov 23, 2022 · 5 comments · Fixed by #1825
Assignees
Milestone

Comments

@Kielek
Copy link
Contributor

Kielek commented Nov 23, 2022

The automatic instrumentation injects, into the application, assemblies from multiple packages from OTel (the SDK, API, exporters, instrumentations, etc). These different packages can depend on different versions of the same assemblies. When the "runtime folders", e.g.: tracer-home\net462, are built the NuGet version resolution will select a single version of any conflicting assembly.

On .NET Framework this behavior becomes a problem because of the strict versioning that it implements on top of strong naming. A concrete example comes from #1647. In this PR the OTLP exporter has a dependency on Google.Protobuf which has a dependency on System.Memory, Version=4.0.1.1. However, other referenced packages depend on System.Memory, Version=4.0.1.2 so after the build only version 4.0.1.2 ends up on tracer-home\net462. At runtime, when automatic instrumentation injects the assemblies, code from Google.Protobuf fails because it tries to load version 4.0.1.1 which is not present.

This is not an issue for manually instrumented applications because when building .NET Framework apps, by default, a binding redirect, mapping all versions to a single one, is added to the application configuration file. However, automatic instrumentation builds as a class library and there are no binding redirects for class libraries.

The way to fix this without requiring the .NET Framework applications to add a reference to the auto-instrumentation NuGet package is to do the "redirect" directly on the assemblies at the Framework "runtime folder". In order to do that it is necessary to update the assembly references in metadata tokens to the single version made available by the NuGet package resolution. This should be done during the build using assembly rewriting packages like Mono.Cecil. The sample listed below shows how the issue on PR #1647 can be fixed with this "redirect".

using Mono.Cecil;

const string targetModuleFile = @"C:\src\otel\dotnet\auto\bin\tracer-home\net462\Google.Protobuf.dll";

using var targetModuleDef = ModuleDefinition.ReadModule(targetModuleFile, new ReaderParameters { ReadWrite = true });
foreach (var assemblyRef in targetModuleDef.AssemblyReferences)
{
    Console.WriteLine(assemblyRef);
}

// Hard-coded the location of the assembly reference that needs to be updated.
targetModuleDef.AssemblyReferences[1].Version = new Version(4, 0, 1, 2);

targetModuleDef.Write();
Console.WriteLine("TargetModuleDef.Write()");
@Kielek
Copy link
Contributor Author

Kielek commented Nov 23, 2022

Remember to remove App.config from TestApplication.Smoke and TestApplication.Http.NetFramework project.

@pellared pellared changed the title missing expected assembly when instrumenting Fx apps Missing expected assembly when instrumenting .NET Framework apps Nov 23, 2022
@pellared pellared added this to the 0.5.2 milestone Nov 23, 2022
pellared pushed a commit that referenced this issue Nov 25, 2022
* Bump OpenTelemetry.AutoInstrumentation dependencies to 1.4.0-beta.3
and other released to the newest versions

* Adjust setting up Prometheus exporter
based on https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Exporter.Prometheus.HttpListener/CHANGELOG.md#140-alpha2

* Remove Microsoft.Extensions.Logging.Abstractions from duplicated libraries list
After upgrade all places references 7.0.0

* Reintroduce DiagnosticSource source to additional deps
it is needed by .NET 6 applications

* Bump Additional Dependencies to 7.0
as we should always bring the newest packages

* Use HttpClientInstrumentationOptions instead of HttpWebRequestInstrumentationOptions
based on open-telemetry/opentelemetry-dotnet@399fbcf

* Add Microsoft.Extensions.Configuration.EnvironmentVariables to Additional Store
introduced by open-telemetry/opentelemetry-dotnet@f191e846d37283e82272f6daa95df10aec4edd7bhttps://github.com/open-telemetry/opentelemetry-dotnet/commit/f191e846d37283e82272f6daa95df10aec4edd7b

* Fix references in Smoke tests

* Update SourceNames for Http Instrumentation
due to https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md#100-rc96

* Change expected span names for Http Instrumentation
changes due to
dotnet/runtime@d8c0170
open-telemetry/opentelemetry-dotnet#3415
open-telemetry/opentelemetry-dotnet@8add3db

* Update Sql Integration name
due to https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Instrumentation.SqlClient/CHANGELOG.md#100-rc95

* Update tests dependencies

* Binding to System.Memory 4.0.1.2 to avoid conflicts with loaded library
Workaround will be fixed in scope of #1646

* Update distributed structure

* Remove reference to HttpWebRequestInstrumentationOptions
from documentation. It was removed from instrumentation package

* Dotnet format fix

* Propagate workaround from 411e778 to TestApplication.Http.NetFramework

* Bump OpenTelemetry.Instrumentation.Runtime to 1.1.0-beta.1

* Remove todo comment
we expect that it will be needed shortly

* Remove DiagnosticSource reference from TestApplication.Http
It is not used in the application, so it is loaded from additional store

* Update documentation
@pjanotti
Copy link
Contributor

pjanotti commented Nov 28, 2022

Follow-up from the last SIG meeting:

  1. The assembly, Google.Protobuf, in which the potential fix above was tested doesn't have an "Authenticode Signature". Hence, validating if the proposed solution works with such assemblies is still necessary.
  2. It seems possible to update the AssemblyRef metadata token using the IMetaDataAssemblyEmit::SetAssemblyRefProps Method. This requires inspecting the AssemblyRef tokens when specific modules are loaded. If using build time "redirects" as initially proposed is not viable for some reason we can prototype this alternative.

@pjanotti
Copy link
Contributor

The TestApplication.GraphQL uses some very old packages that are incompatible with the SDK 1.4.0-beta.3: just by adding

 <PackageReference Include="OpenTelemetry" Version="1.4.0-beta.3" />

to the net462 the application breaks at startup with:

Now listening on: http://localhost:5000
Now listening on: https://localhost:5001
Application started. Press Ctrl+C to shut down.
fail: Microsoft.AspNetCore.Server.Kestrel[0]
      Heartbeat.OnHeartbeat
System.TypeLoadException: Could not load type 'Microsoft.Extensions.Primitives.InplaceStringBuilder' from assembly 'Microsoft.Extensions.Primitives, Version=5.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60'.
   at Microsoft.Net.Http.Headers.DateTimeFormatter.ToRfc1123String(DateTimeOffset dateTime, Boolean quoted)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.DateHeaderValueManager.SetDateValues(DateTimeOffset value)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.DateHeaderValueManager.OnHeartbeat(DateTimeOffset now)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.Heartbeat.OnHeartbeat()

we may have to refactor the test app to not use the unmaintained Microsoft.AspNetCore package.

@pjanotti
Copy link
Contributor

pjanotti commented Dec 13, 2022

It also seems a good idea to move the transitive dependencies to the highest version supported by our direct dependencies. The reason is that if we use the lower version from our direct dependencies (the default choice for the version range of NuGet packages) we may lower the version being loaded by the application. This can happen if the first reference to the assembly comes from code injected by the instrumentation, in this case, we may end up with a version lower than what the app was built against and cause runtime errors - to be fair there is no guarantee the going to higher version won't cause problems either but the chances are much smaller.

Unfortunately, NuGet doesn't seem to have an easy way to update the transitive dependencies to the latest versions. We probably would have to rely on some tool like dotnet-outdated or implement our own dependency management via a custom build step/target.

[EDIT: tracking this via #1824]

@pjanotti
Copy link
Contributor

Since we are dealing with the potential of using versions that differ from the ones used by the application, see previous comment, we need to have a friendly way to install the assemblies in the netfx to the GAC. Following up via #1823

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants