Workaround dotnet CLI issue

[pjanotti]

Why

To improve the UX with dotnet CLI.

Helps with #1744

What

Manipulate the DOTNET_* env vars in the StartupHook according if the process is excluded or not. This allows a better UX with dotnet CLI and effective process exclusion in such cases.

The documentation should be updated too, but, only at the time of a new release. Prior to a release with this workaround #2128 should be visible to users.

Tests

Added automated tests for dotnet CLI.

Checklist

• CHANGELOG.md is updated.
• Documentation is updated. To be done with or just prior to the next release.
• New features are covered by tests.

[RassK]

dotnet,dotnet.exe,powershell.exe,pwsh,pwsh.exe seems like an excellent base, we should make a next step issue, where we can work out more complete list.

[pellared]

I do not think we should ignore dotnet. See #2131 (comment)

[pjanotti]

I agree with that sentiment @pellared: ideally, we would instrument dotnet <dll>, but, not the other usages of dotnet.

[lachmatt]

There is problem with tests, because of bytecode instrumentations not being initialized.
For dotnet core test applications are started with dotnet testapp.dll. In such case:

• in the native code, dotnet is observed and compared with exclude list, so initialization exits early
• in managed code, testapp is observed as application name (comes from AppDomain.FriendlyName) and compared with exclude list, so initialization continues

I think the way test applications are started is a common one.

[pjanotti]

Good point @lachmatt - 🤔 how to better handle that...

[pjanotti]

At first, the options coming to my mind are:

1. Unify the criteria: make the StartupHook use the process main module name. The advantage is the consistency between the CLR profiler and the StartupHook when excluding processes. The downside is that we won't support dotnet <DLL>, which is relatively common.
2. Give dotnet <DLL> a special treatment on the CLR profiler and ignore the exclusion in this case.

I'm leaning towards option 1 for consistency and to avoid dealing with some unknown special cases with dotnet <DLL>. Notice that currently dotnet <DLL> is not supported anyway.

[pjanotti]

Converted PR to draft until I try a few things.

[pellared]

we won't support dotnet <DLL>, which is relatively common.

I think it is a huge downside. Even the Microsoft samples use it e.g. https://github.com/dotnet/dotnet-docker/blob/main/samples/dotnetapp/README.md#build-a-net-image

It would be great to somehow just disable the auto-instrumentation for self-contained executables...

[pjanotti]

I was under the impression that we were not using dotnet CLI at all in our integration tests since it fails when we set the environment variables in a shell and try to use it. However, we are in fact using dotnet <DLL> to run net6.0 and net7.0. However, dotnet <DLL> works due to the presence of a set of Microsoft.Extensions.* DLLs added to the test applications. This makes clear that with the proper dependencies added at build it, we should be able to correctly support dotnet <DLL>. However, the presence of such DLLs is not enough to fix the normal usage of dotnet CLI.

While I want to improve the general dotnet CLI usage in the short run I don't want to go back and forth on the defaults for the excluded process list either. I would rather take a bit of more time under the current situation until we are clear with any changes in this regard.

For reference, list of Microsoft.Extensions.* DLLs that allow dotnet <DLL> to work for the smoke test app:

-a---          10/18/2022  9:19 AM          45200 Microsoft.Extensions.DependencyInjection.Abstractions.dll
-a---          10/18/2022  9:19 AM          85152 Microsoft.Extensions.DependencyInjection.dll
-a---          10/18/2022  9:19 AM          64120 Microsoft.Extensions.Logging.Abstractions.dll
-a---          10/18/2022  9:20 AM          48248 Microsoft.Extensions.Logging.dll
-a---          10/18/2022  9:19 AM          61560 Microsoft.Extensions.Options.dll
-a---          10/18/2022  9:19 AM          42624 Microsoft.Extensions.Primitives.dll

[rajkumar-rangaraj]

Did some research and understood dotnet.exe respects both AdditionalDeps and Runtime store. The issue we are facing could happen with any other apps which has package reference like dotnet.exe.

Looks like Microsoft.Extensions.Configuration.EnvironmentVariables package used in OpenTelemetry SDK is causing an issue. I see this package reference is removed in the recently released OpenTelemetry RC version. I will wait for our repo to take a reference to newly released package and continue the investigation.

[pjanotti]

@rajkumar-rangaraj One of the issues that we are having with the dependencies is that the OpenTelemetry.Instrumentation.AspNetCore package has a frameworkReference to Microsoft.AspNetCore.App which causes our build to drop all packages that are part of that framework.

I tested some self-contained apps and we didn't have a problem with those. Granted these were simple demo apps. Anyway, as you said there seem to be more issues with our additional deps that still need to be understood besides the frameworkReference.

[pjanotti]

I got it to work, but, I had to downgrade some assemblies to 6.0.0. I still have to polish this change, but, just so I get a vision of other potential problems with CI I'm going to push the changes. Anyway, for making the actual change I will wait for the package upgrade.

I do expect some tests that depend on AspNetCore to break since I'm taking a shortcut to deal with the frameworkReference issue.

[rajkumar-rangaraj]

My susception is correct. Took the artifact from #2157 and confirmed the issue is resolved.

Merging #2157 will solve dotnet CLI issue.

[pellared]

My susception is correct. Took the artifact from #2157 and confirmed the issue is resolved.

Merging #2157 will solve dotnet CLI issue.

It is better... But there are still some issues which I will describe under #1744