This division / overload that Nuke has is really cool

I suspected that this would not work when 1) the 64-bit installer is installed and 2) a 32-bit process is being instrumented, and I was able to verify this issue locally. When this happens the loader emits the following log lines:

[2022-08-08 14:01:16.683 | debug | PId: 62804 | TId: 56360] Load: C:\Program Files\Datadog\.NET Tracer\win-x64\Datadog.Tracer.Native.dll
[2022-08-08 14:01:16.683 | warning | PId: 62804 | TId: 56360] Load: Error loading dynamic library 'C:\Program Files\Datadog\.NET Tracer\win-x64\Datadog.Tracer.Native.dll': 02F66CA0

This is only an issue with the MSI and specifically the 64-bit installer because we have to globally set DD_NATIVELOADER_CONFIGFILE for all applications, but the config loader must be able to select the native bits that match the process bitness. To handle this correctly, I think we'll need to do the following:

1. Install the loader.conf at the [INSTALLFOLDER] location
2. This loader.conf will need to include the win-x64 and win-x86 subdirectories in its lookup paths
3. Always set DD_NATIVELOADER_CONFIGFILE to point to the [INSTALLFOLDER]loader.conf location

Compared to the BEFORE state, the only change should be the loader.conf file. The new loader.conf should look like the following:

#Profiler
PROFILER;{BD1A650D-AC5D-4896-B64F-D6FA25D6B26A};win-x64;.\win-x64\Datadog.Profiler.Native.dll
PROFILER;{BD1A650D-AC5D-4896-B64F-D6FA25D6B26A};win-x86;.\win-x86\Datadog.Profiler.Native.dll

#Tracer
TRACER;{50DA5EED-F1ED-B00B-1055-5AFE55A1ADE5};win-x64;.\win-x64\Datadog.Tracer.Native.dll
TRACER;{50DA5EED-F1ED-B00B-1055-5AFE55A1ADE5};win-x86;.\win-x86\Datadog.Tracer.Native.dll

I started a thread here where I suggested we could move all of the 32-bit assets into the Program Files (x86) folder just like Datadog.Trace.ClrProfiler.Native.dll, since that's required to be placed there. If we did that AND we did not set DD_NATIVELOADER_CONFIGFILE globally, then what would happen is the loader DLL would look in its directory for a loader.conf file. Since all of the 64-bit files (and the loader) would be in the same directory as the 64-bit Datadog.Trace.ClrProfiler.Native.dll loader and all of the 32-bit files (and the loader) would be in the same directory as the 32-bit Datadog.Trace.ClrProfiler.Native.dll loader, this should fix the issue I surfaced. This seems to me like the cleanest solution that addresses both issues. What do you think?

I think having everything 32-bit in the 32-bit program folder makes a lot of sense. My only question is whether having all the managed dlls in a separate folder away from the native loaders becomes an issue then.. It breaks the "clean, unified layout" we currently have (but the MSI breaks that anyway by having two install locations).

Essentially it's a question of whether it's better to have

• x86 assets all in the same location
• simple loader conf
• tracer home cannot be inferred from native dll locations, and is in a separate location

or

• native loader separate from x86 native dlls + loader conf
• tweaked loader conf
• tracer home can be inferred from native tracer etc

I was initially thinking the first approach, but now I'm leaning to the second approach...

Also... I didn't realise we don't have any 32-bit on x64 tests in CI 😬 We should have one at least! I'll try adding a smoke test separately...

Have we considered installing all the files into a single base location, either Program Files or Program Files (x86) or other, instead of splitting them up? I think we can do that now that we have the win-x64 and win-x86 folders, and it could make things simpler by keeping everything contained in a single path (like all the other packages). On the other hand, this may require some MSI trickery, since MSI can be finicky about what is x64 vs x86.

Related: there is a long-standing bug where we allow users to change the tracer installation path, but that only applies to the files in Program Files. Users cannot change where the Program Files (x86) files are installed. We've had support cases of users not wanting to install anything into C:\, for example, but that is currently not possible.

Have we considered installing all the files into a single base location, either Program Files or Program Files (x86) or other, instead of splitting them up?

MSIs aren't my strong point, so I don't know why we split them up at all (I thought we always had the win-x64 and winx-86 folders). I assumed it was because we register via the ID with the MSI, not using the profiler path, so I assumed the profiler had to be in the expected folder (i.e. Program Files (X86) for 32-bit processes)

If we can avoid it, I'm strongly in favour of not splitting them up! Especially if it solves additional issues.

Awesome!

Those changes look good to me! We should also update our docs to remove the unnecessary COR_PROFILER/CORECLR_PROFILER GUID.

Question: As we now set the COR_PROFILER/CORECLR_PROFILER GUID as a machine-wide env var, should we remove it from the IIS registry keys to avoid the duplication?

I say go for it 👍🏼

I think it's always clearer for the user (or troubleshooter) to have all the config variables in the same place. If we are setting the IIS variables in the registry I find it always usefull to have them all together, althaough some of them could be declared globally.

I think it's always clearer for the user (or troubleshooter) to have all the config variables in the same place.

The trouble is we need to support IIS, windows services and ad-hoc apps. With the approach we've taken here, users only need to set COR_ENABLE_PROFILING for the windows services or for the standalone apps, nothing else needs setting.

If we don't take this approach then either:

• We require customers to set the env var paths to the profiler correctly when running windows services etc. That's potentially 4/8 env vars... I think it's better to only require the COR_ENABLE_PROFILING/CORECLR_ENABLE_PROFILING variables, as they're harder to screw up
• Duplicate config variables in multiple places, i.e. globally and in IIS. There's more chances for things to go out sync then

Updated docs: DataDog/documentation#14980

Removing the bitness info in the path might break here. Since x86 and x64 binaries have now the same name, they can be copied into the same folder: I don't know which one will erase the other.

Am I missing something?

Removing the bitness info in the path might break here. Since x86 and x64 binaries have now the same name, they can be copied into the same folder: I don't know which one will erase the other.

I mean, yes, they can be overwritten, but if they are, then we have a build problem 😄 They are currently (before and after this change), built to separate folders, e.g. Release-x86 and Release-x64.

When they're copied to DDProf-Deploy they're deployed to the win-x64 and win-x86 folders, so there's again no issue with overwritting AFAICT?

There's a bigger question here for me - why do we need the DDProf-Deploy directory at all? For me, it adds confusion and complexity? It's not where things are built to, so we could just "deploy" them to the Monitoring Home directory, where they end up eventually anyway?

I was a bit torn on the "dropping the bitness from the file names" question, but we ultimately opted for it for consistency, and to reduce the duplication in the arch-specific folders. That, coupled with the fact we have to drop it in some cases for back compat reasons led us to this state... I think we can safely add it back in later though if it's an issue, it's only the Datadog.Trace.ClrProfiler.Native file we can't ever rename 🙂

When they're copied to DDProf-Deploy they're deployed to the win-x64 and win-x86 folders, so there's again no issue with overwritting AFAICT?
Where is this done?
The $(BaseOutputPath)$(ConfigBasedRelativeOutputPath) does not seem to contain the bitness

Where is this done?
The (ConfigBasedRelativeOutputPath) does not seem to contain the bitness

ConfigBasedRelativeOutputPath contains the bitness 🙂 It's (e.g.) Release-x86 or Release-x64

Same comment

Also, the simple answer is that NativeWindowsDeployFilesSrcDir is different for different architectures 🙂

I have to admit that I don't see NativeWindowsDeployFilesSrcDir set.
However, is it the "source" from which the files will be copied (could be different because generated during the build)?
Here, I'm talking about the destination in DDProf-Deploy

I have to admit that I don't see NativeWindowsDeployFilesSrcDir set.
It's set on line 46, on your previous comment, and includes Release-x64 etc in the path.

Here, I'm talking about the destination in DDProf-Deploy

The destination is given by DeployDirectoryBase, which is DDProf-Deploy/win-x64 and DDProf-Deploy/win-x86 respectively.

On linux we currently still build to the root, as we only build one architecture. We would need to update the CMakefile etc to account for that if we start building for multiple arches at once on Linux. But that's a current issue too, so there's no change in that

Same comment

Same comment

What are the files that will be copied in this root folder for the profiler?
I'm not sure we have any since we got rid of Datadog.AutoInstrumentation.Profiler.Managed.dll

You're right, there aren't any! The Tracer has this too, I'm not sure exactly why it's there, not sure if it's required or not. I'll try cleaning it up in a subsequent PR

Won't this cause some confusion? In other places in this PR, it seemed like we are deprecating the concept of a "TracerHome", but maybe I misinterpreted?

Won't this cause some confusion? In other places in this PR, it seemed like we are deprecating the concept of a "TracerHome", but maybe I misinterpreted?

Yep, we're deprecating the use of TracerHome, there's still some cleanup to do to remove all traces of it though 🙂 I'll push that to a later PR for simplicity