open-telemetry / opentelemetry-dotnet-instrumentation

OpenTelemetry .NET Automatic Instrumentation
https://opentelemetry.io
Apache License 2.0
368 stars 91 forks source link

Perform bytecode instrumentation for .NET Core applications hosted in a .NET Framework IIS application pool #3005

Closed zacharycmontoya closed 2 weeks ago

zacharycmontoya commented 1 year ago

After the large native code update that is in effect for published versions v0.7.0-v1.0.2, .NET Core applications hosted in IIS will only be instrumented when their application pool is configured with No Managed Code.

Update the native code to allow bytecode instrumentation to function when the application pool is configured to run with the .NET Framework.

The main proposal discussed in the SIG meeting was to convert the is_desktop_iis into a sort of app_domain_id mapping, which we have done in several places within the native code. This should allow us to perform a is_desktop_iis at the app-domain level, so it would be safe to instrument the .NET Core application even when the .NET Framework is loaded.

muhaook commented 1 year ago

Hi @zacharycmontoya , please let me know when the fix is available. I can verify it on my envs.

Looks like the fix targets 1.2.0. Do you know the ETA for 1.2.0 release?

muhaook commented 10 months ago

My understanding is the root cause is both CoreCLR (for .NET core) and CLR (for .NET framework) were brought up in an application pool.

Looked into the native code, I found only 1 profiler has been saved which is used for callbacks from managed code (C#) to native code: extern CorProfiler* profiler; // global reference to callback object

According to its comment, First CLR to try to load us into this process wins

// Note: Generally you should not have a single, global callback implementation, // as that prevents your profiler from analyzing multiply loaded in-process // side-by-side CLRs. However, this profiler implements the "profile-first" // alternative of dealing with multiple in-process side-by-side CLR instances. // First CLR to try to load us into this process wins; so there can only be one // callback implementation created. (See ProfilerCallback::CreateObject.)

However, in CorProfiler::Initialize(), looks like the last profiler wins, not the first: profiler = this;

Is it a problem?

If multiple profilers can be initailized, is it ideal to save them in an array of pointers instead of a pointer for future callback?

muhaook commented 10 months ago

Hi team, I made a quick fix to honor the first profiler: image seems it fixed the bytecode instrumentation issue on my env (need thorough tests though)

I wonder the reason why you decide First CLR to try to load us into this process wins? could you share details so that I can fully understand it please?

nrcventura commented 2 weeks ago

Closing per SIG discussion.