Load the managed profiler code as early as possible #462
Conversation
zacharycmontoya
added
status:work-in-progress
zacharycmontoya
force-pushed
the
zach/security-grant-fix
branch
from
f3c0f87 to
2192e98
Compare
zacharycmontoya
removed
the
status:work-in-progress
|
This passes the Linux integration tests 👍 One last thing I'd like to do before merging (and addressing PR-related feedback of course) is to enable the Smoke tests for Windows, since this feature only hits Windows right now. |
| { | ||
| try | ||
| { | ||
| Assembly.Load(new AssemblyName("Datadog.Trace.ClrProfiler.Managed, Version=1.6.0.0, Culture=neutral, PublicKeyToken=def86d061d0d2eeb")); |
There was a problem hiding this comment.
We may want to tie this to some sort of constant if possible. Is the current idea to update this manually for every release? I'm okay with that for now.
There was a problem hiding this comment.
Oh yeah, that was an item I wanted to revisit. I hard-coded it to start but I would much prefer making it more automatic if we can figure out a good way to do that.
There was a problem hiding this comment.
At worst, tests will fail if we miss it currently.
There was a problem hiding this comment.
The scarier part is if we miss this on the version update...maybe we should also enable running the smoke tests for the packaging step?
There was a problem hiding this comment.
Won't the tests fail on the version update PR if we miss this piece?
There was a problem hiding this comment.
Looks like our integration tests don't catch this because the changes only apply to Windows and our smoke tests (Category="Smoke") don't count spans. I'll try enabling one Category="EndToEnd" (I think the HttpMessageHandler) that doesn't have external dependencies
There was a problem hiding this comment.
Heh yeah I got the Datadog.Trace.ClrProfiler.IntegrationTests.HttpClientTests.HttpClient to pass with the correct assembly name but fail with the incorrect assembly name. I'll get this into this PR 👍
There was a problem hiding this comment.
Currently blocked on adding this until the following issue is fixed with the .NET Core SDK installer task
There was a problem hiding this comment.
Okay I just pushed 00c0090 which runs the HttpClientTests E2E tests on Windows. I was able to show on a separate branch that this fails when the string in the Loader fails. That's because we expect at least one span in the integration tests and we don't end up sending any in this scenario
There was a problem hiding this comment.
I'll leave this comment chain as unresolved to indicate that there's still some work left here to do, but I don't think I'll be changing this on this PR, if that's fine with you guys.
|
I'm going to bulk resolve all of Colin's |
Run StackExchangeRedisStackOverflowExceptionSmokeTest only on non-Windows platforms
… of time for smoke tests.
…y can be found by the smoke test suite.
…ate.CoreLib, and save the module id's for the corelib module and the managed profiler.
…tegration test suite
…dows integration test failures. In MockTracerAgent, save the request headers instead of the requests themselves. The requests get disposed when the response is closed, causing ObjectDisposedExceptions to be thrown.
zacharycmontoya
force-pushed
the
zach/security-grant-fix
branch
from
38dff6c to
4fd78ba
Compare
| // proper string for CreateInstance to successfully call | ||
| #ifdef _WIN32 | ||
| LPCWSTR load_helper_str = | ||
| L"Datadog.Trace.ClrProfiler.Managed.Loader.Startup"; |
There was a problem hiding this comment.
Did you try this with "some_string"_W? That should work on both platforms. See pal.h.
(If this works, no need to fix in this PR. We can look into this when working on the Linux implementation.)
There was a problem hiding this comment.
I had issues using "string"_W working on both Windows and Linux. Let me follow up on this when I get the Linux implementation up and running.
The main mechanism for loading the managed profiler code was introduced in #462 (commit 4ce932f). This PR turns on the feature for Linux. The main components are embedding the resources into the shared library, retrieving the embedded resources in the native profiler, and adding the correct PInvoke from the injected assembly load.
Implementation Changes (Enabled only on Windows for now): * Creates a new library called Datadog.Trace.ClrProfiler.Managed.Loader that is embedded in the native profiler image. When the type is loaded, it has a static constructor to call Assembly.Load on our managed profiler code in a try-catch block. * On first JITCompilationStarted, dynamically emit IL to load the Datadog.Trace.ClrProfiler.Managed.Loader assembly and attempt to load the managed profiler code. * Change the native profiler so no edits are made in the JITCompilationStarted event unless the managed profiler code is already loaded. * Keep track of the app domain of mscorlib, the managed profiler assembly, and the method caller's assembly and do not edit the IL of a method if referencing the managed profiler assembly would invalidate the caller's domain-neutral assembly reference closure. Tests and Results: * Enable a subset of integration tests on Windows. Smoke tests and the E2E HttpClientTests are now run on Windows. * Fixes System.IO.FileNotFoundException when trying to load (Assembly.Load) the managed profiler but it cannot be found. Adds a smoke test called AssemblyLoad.FileNotFoundException to verify the behavior (Windows-only for now). * Fixes System.IO.FileLoadException: Loading this assembly would produce a different grant set from other instances. Adds a smoke test called SecurityGrant.FileNotFoundException to verify the behavior (only runs on .NET Framework).
Changes proposed in this pull request:
AppDomain.Load(byte[], byte[])-- the second byte array reads the symbolsFixes:
@DataDog/apm-dotnet