Add fallback logic for DD_INTEGRATIONS and DD_DOTNET_TRACER_HOME #579

zacharycmontoya · 2019-12-06T20:37:49Z

Goal: Use DD_DOTNET_TRACER_HOME to locate both definition json file and the managed assemblies for the CLR automatic instrumentation. This eliminates the need for the separate DD_INTEGRATIONS environment variable and reduce the number of non-profiler environment variables to 1.

This PR does the following:

Remove DD_INTEGRATIONS from the Windows MSI.
If DD_INTEGRATIONS is undefined -- the new default case -- load the integrations file specified at $DD_DOTNET_TRACER_HOME/integrations.json
If DD_DOTNET_TRACER_HOME is undefined -- which may occur when upgrading the profiler to 1.8.0+ -- infer the location of the managed assemblies on disk from the DD_INTEGRATIONS variable.

@DataDog/apm-dotnet

zacharycmontoya · 2019-12-06T21:21:01Z

Opening for review to get automatic CI. Will remove the status:work-in-progress label when it's truly ready. Currently missing is my good regression test and the actual changes in cor_profiler.cpp to do the fallback for missing DD_INTEGRATIONS variable.

…nstaller.

… is set but not DD_DOTNET_TRACER_HOME. No crash, but no instrumentation.

…uild variable. For every test project, copy the managed profiler assets into the profiler directory, to simulate the customer scenario.

…ts. This will help later with this PR. Fix integration_loader_test

…nd wix installer." This reverts commit fe58b6d.

…st that we receive traces.

…we pass DD_DOTNET_TRACER_HOME in CI. Will modify this...

…ied by the test case, and edit MissingDotnetTracerHomeEnvTest to remove DD_DOTNET_TRACER_HOME

…nd the managed assemblies on-disk when DD_DOTNET_TRACER_HOME is not set but DD_INTEGRATIONS is still set.

…S) when the CI returns the empty string for DD_DOTNET_TRACER_HOME instead of null.

setup and wix installer.

…k all the tests. They'll pass again when I fix the profiler to fill in the DD_INTEGRATIONS path from DD_DOTNET_TRACER_HOME.

…OME if DD_INTEGRATIONS is undefined.

…ionsFromEnvironment, in various environment variable scenarios.

zacharycmontoya · 2019-12-07T01:51:18Z

Test.Common.props

  </ItemGroup>

-  <Target Name="AfterBuildCopyManagedProfiler" AfterTargets="AfterBuild" Condition=" '$(LoadManagedProfilerFromProfilerDirectory)' == 'true'">
+  <Target Name="AfterBuildCopyManagedProfiler" AfterTargets="AfterBuild">


I'm removing $(LoadManagedProfilerFromProfilerDirectory) because it's not intuitive when to use it. For now, while wasteful, we'll always copy all managed binaries to a sample app's profiler-lib directory.

zacharycmontoya · 2019-12-07T01:52:19Z

deploy/Datadog.Trace.ClrProfiler.WindowsInstaller/Product.wxs


    <ComponentGroup Id="EnvironmentVariables.Machine" Directory="INSTALLFOLDER">
      <Component Id="EnvironmentVariablesShared" Guid="{C314A305-9C24-4E46-9ECF-E5EEA703BDEA}" Win64="$(var.Win64)">
        <CreateFolder/>


I'm removing DD_INTEGRATIONS from our MSI since we can key off DD_DOTNET_TRACER_HOME. If for some reason the user has a different integrations.json file they want to use, they can manually set DD_INTEGRATIONS to prioritize that one.

zacharycmontoya · 2019-12-07T01:54:08Z

samples/Samples.HttpClient.MissingDotnetTracerHomeEnv/Properties/launchSettings.json

+          "CORECLR_PROFILER": "{846F5F1C-F9AE-4B07-969E-05C26BC060D8}",
+          "CORECLR_PROFILER_PATH": "$(ProjectDir)$(OutputPath)profiler-lib\\Datadog.Trace.ClrProfiler.Native.dll",
+
+          "DD_INTEGRATIONS": "$(ProjectDir)$(OutputPath)profiler-lib\\integrations.json"


This very simple test program uses DD_INTEGRATIONS and doesn't use DD_DOTNET_TRACER_HOME to simulate a user who has upgraded the profiler without updating the environment variables.

zacharycmontoya · 2019-12-07T01:55:11Z

src/Datadog.Trace.ClrProfiler.Managed.Loader/Startup.NetCore.cs

-            var tracerHomeDirectory = Environment.GetEnvironmentVariable("DD_DOTNET_TRACER_HOME") ?? string.Empty;
+
+            var tracerHomeDirectory = Environment.GetEnvironmentVariable("DD_DOTNET_TRACER_HOME");
+            if (string.IsNullOrWhiteSpace(tracerHomeDirectory))


.NET Core: Fallback to DD_INTEGRATIONS if DD_DOTNET_TRACER_HOME was not set.

zacharycmontoya · 2019-12-07T01:55:18Z

src/Datadog.Trace.ClrProfiler.Managed.Loader/Startup.NetFramework.cs


-            var tracerHomeDirectory = Environment.GetEnvironmentVariable("DD_DOTNET_TRACER_HOME") ?? string.Empty;
+            var tracerHomeDirectory = Environment.GetEnvironmentVariable("DD_DOTNET_TRACER_HOME");
+            if (string.IsNullOrWhiteSpace(tracerHomeDirectory))


.NET Framework: Fallback to DD_INTEGRATIONS if DD_DOTNET_TRACER_HOME was not set.

zacharycmontoya · 2019-12-07T01:56:46Z

src/Datadog.Trace.ClrProfiler.Native/cor_profiler.cpp

+    auto profiler_home_path =
+        GetEnvironmentValue(environment::profiler_home_path);
+
+    if (!profiler_home_path.empty()) {


Simple fallback in the profiler: if DD_INTEGRATIONS was not set (as a result, it's the empty string), try DD_DOTNET_TRACER_HOME and if it is not empty, create an integrations.json path.

We duplicate this logic in integration_loader.cpp just so we can raise an immediate error immediately here if the environment variables don't look right. Also, for testing purposes, it's much easier to do all of the environment variable processing again in LoadIntegrationsFromEnvironment so we can unit test it.

lucaspimentel · 2019-12-09T15:46:20Z

src/Datadog.Trace.ClrProfiler.Native/cor_profiler.cpp

+        GetEnvironmentValue(environment::profiler_home_path);
+
+    if (!profiler_home_path.empty()) {
+      const auto fallback_integration_path = std::filesystem::path(profiler_home_path) / "integrations.json";


We can't use std::filesystem unless we update to a newer build system on Linux.

I started working on updating the Linux build a few months ago. It's on branch lpimentel/update-linux-build.

For this PR, which solution would you prefer:

Writing our own routine that does simple path appending (you can see that work in some of the recent commits that I reverted)

Updating the Linux build to take advantage of std::filesystem?

Trying to update the build system right now, starting with the changes from your branch. I'll let you know how that goes

I've spent all day trying to update the build system just to use the filesystem::append method but the result is that std::experimental::filesystem is available but not std::filesystem. I'm going to give up on this approach for the PR because I spent an entire day on this already. Unless you can help me take care of that, I will roll our own simple path function for this PR.

lucaspimentel · 2019-12-09T15:49:12Z

src/Datadog.Trace.ClrProfiler.Native/util.cpp

-std::vector<WSTRING> GetEnvironmentValues(const WSTRING &name) {
-  return GetEnvironmentValues(name, L';');
+std::vector<WSTRING> GetValues(const WSTRING &delimited_values) {
+  return GetValues(delimited_values, L';');


Unrelated to this PR, but this should really default to ; on Windows and : on Linux.

I'm hesitant to change the default behavior right now in this PR, but this would make for a good follow-up PR

src/Datadog.Trace.ClrProfiler.Native/util.cpp

…s. Add another unit test to make sure we handle the cases correctly.

…ourselves. Add another unit test to make sure we handle the cases correctly." This reverts commit a676a4f.

… a corresponding unit test

zacharycmontoya added area:native-library Automatic instrumentation native C++ code (Datadog.Trace.ClrProfiler.Native) area:installers labels Dec 6, 2019

zacharycmontoya added this to the 1.11.0 milestone Dec 6, 2019

zacharycmontoya self-assigned this Dec 6, 2019

zacharycmontoya added the status:work-in-progress Actively worked on. If this is a PR, no review needed yet. WIP. label Dec 6, 2019

zacharycmontoya marked this pull request as ready for review December 6, 2019 21:19

zacharycmontoya requested a review from a team as a code owner December 6, 2019 21:19

zacharycmontoya added 8 commits December 6, 2019 13:22

Remove DD_INTEGRATIONS environment variable from test setup and wix i…

a21a2ad

…nstaller.

Add MissingDotnetTracerHomeEnv to reproductions where DD_INTEGRATIONS…

66885e1

… is set but not DD_DOTNET_TRACER_HOME. No crash, but no instrumentation.

Remove the use of the $(LoadManagedProfilerFromProfilerDirectory) MSB…

3c09572

…uild variable. For every test project, copy the managed profiler assets into the profiler directory, to simulate the customer scenario.

Modify the integration_loader helpers to only take file_paths as inpu…

72d6160

…ts. This will help later with this PR. Fix integration_loader_test

Revert "Remove DD_INTEGRATIONS environment variable from test setup a…

4aa5cec

…nd wix installer." This reverts commit fe58b6d.

Move MissingDotNetTracerHomeEnv to the samples directory so we can te…

218bfbe

…st that we receive traces.

Rename new test project to Samples.HttpClient.MissingDotnetTracerHomeEnv

72c8239

Add MissingDotnetTracerHomeEnvTest test. It currently passes because …

28e4412

…we pass DD_DOTNET_TRACER_HOME in CI. Will modify this...

zacharycmontoya force-pushed the zach/feature/remove_dd_integrations_env branch from 00cfba1 to 28e4412 Compare December 6, 2019 21:25

zacharycmontoya added 9 commits December 6, 2019 13:46

Add Samples.HttpClient.MissingDotnetTracerHomeEnv to the Linux CI

59175a1

Edit the test infrastructure to unset environment variables as specif…

9a8e202

…ied by the test case, and edit MissingDotnetTracerHomeEnvTest to remove DD_DOTNET_TRACER_HOME

Fix the AssemblyResolve events for .NET Core and .NET Framework to fi…

d84194a

…nd the managed assemblies on-disk when DD_DOTNET_TRACER_HOME is not set but DD_INTEGRATIONS is still set.

Add debugging statements in initial profiler directory discovery

464d668

Fix the fallback so we check for Path.GetDirectoryName(DD_INTEGRATION…

692eb58

…S) when the CI returns the empty string for DD_DOTNET_TRACER_HOME instead of null.

Redo the set of changes to Remove DD_INTEGRATIONS environment from test

4661024

setup and wix installer.

Remove DD_INTEGRATION from all tests using EnvironmentHelper and wrec…

dd5f469

…k all the tests. They'll pass again when I fix the profiler to fill in the DD_INTEGRATIONS path from DD_DOTNET_TRACER_HOME.

Perform quick argument check: In the profiler, use DD_DOTNET_TRACER_H…

7cd2c2d

…OME if DD_INTEGRATIONS is undefined.

Reset the integration_loader API's so we can easily test LoadIntegrat…

66d74ec

…ionsFromEnvironment, in various environment variable scenarios.

zacharycmontoya commented Dec 7, 2019

View reviewed changes

Add <filesystem> to cor_profiler.cpp

22df6c5

zacharycmontoya removed the status:work-in-progress Actively worked on. If this is a PR, no review needed yet. WIP. label Dec 7, 2019

lucaspimentel reviewed Dec 9, 2019

View reviewed changes

src/Datadog.Trace.ClrProfiler.Native/util.cpp Outdated Show resolved Hide resolved

zacharycmontoya added 5 commits December 9, 2019 09:30

Remove the <filesystem> include, taking care of path joining ourselve…

a676a4f

…s. Add another unit test to make sure we handle the cases correctly.

Revert "Remove the <filesystem> include, taking care of path joining …

6d6e356

…ourselves. Add another unit test to make sure we handle the cases correctly." This reverts commit a676a4f.

Rename GetValues to SplitAndTrim

1e9f671

Add function AppendToPath in util.cpp to do path join for each OS and…

0469a2b

… a corresponding unit test

Remove <filesystem> include from integration_loader

432bc86

colin-higgins removed this from the 1.11.0-prerelease milestone Dec 10, 2019

lucaspimentel added this to the 1.11.1 milestone Dec 11, 2019

bobuva modified the milestones: 1.11.1, 1.11.2 Jan 13, 2020

zacharycmontoya removed this from the 1.11.2 milestone Jan 14, 2020

lucaspimentel requested a review from a team February 4, 2020 19:02

lucaspimentel added the type:enhancement Improvement to an existing feature label Feb 4, 2020

lucaspimentel marked this pull request as draft May 1, 2020 15:02

lucaspimentel added the status:work-in-progress Actively worked on. If this is a PR, no review needed yet. WIP. label May 1, 2020

lucaspimentel changed the title ~~[ClrProfiler] Add fallback logic for DD_INTEGRATIONS and DD_DOTNET_TRACER_HOME~~ Add fallback logic for DD_INTEGRATIONS and DD_DOTNET_TRACER_HOME May 1, 2020

zacharycmontoya closed this Jan 29, 2021

zacharycmontoya deleted the zach/feature/remove_dd_integrations_env branch January 29, 2021 01:28

Add fallback logic for DD_INTEGRATIONS and DD_DOTNET_TRACER_HOME #579

Add fallback logic for DD_INTEGRATIONS and DD_DOTNET_TRACER_HOME #579

Uh oh!

Conversation

zacharycmontoya commented Dec 6, 2019

Uh oh!

zacharycmontoya commented Dec 6, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zacharycmontoya Dec 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zacharycmontoya Dec 9, 2019 •

edited

Loading