Implemented assembly redirect for non-default AppDomain by iskiselev · Pull Request #4728 · open-telemetry/opentelemetry-dotnet-instrumentation

iskiselev · 2025-12-24T02:03:20Z

Why

Exisitng solution #4187 force LoaderOptimization.SingleDomain for every additional app domains. It is not standard behaviour and may result in increased memory consumption and slower warm-up for process hosting multiple web-applications - and other processes that use additional AppDomains.

Implement second stage of fix for #4186.

Parts of implementation uses code generated by Codex with GPT-5.4 model

What

Added environment variable OTEL_DOTNET_AUTO_APP_DOMAIN_STRATEGY that controls strategy of fixing assembly conflicts in non-default app domains.
Possible values:

None - do not apply anything. Works with test app and OTEL 1.14, crash with Loading this assembly would produce a different grant set from other instances #4186 on OTEL 1.13. May be useful for custom host that creates AppDomains.
LoaderOptimizationSingleDomain - exisiting solution implemented in Automatic LoaderOptimization.SingleDomain #4187 - all additional AppDomains use LoaderOptimization.SingleDomain - used by default
AssemblyRedirect - new stategy - patch assemblyBindings/dependentAssembly/bindingRedirect in memory-loaded app.config. On test app docker requires app pool to loading user profile if assemblies loaded from folder (and has no issues when assemblies are registered in GAC). On my local computer works even without loading user profile for app pool.

Tests

AspNetTests.SubmitsTraces now test additional modes.
AssemblyBindingUpdaterTests - unit test coverage
Tested all strategies with OTEL assemblies in GAC and not in GAC on web sites that does/does not use newer version of dependent assemblies. For dependent assembly with newer version System.Diagnostics.DignosticsSource 11 was used.

Checklist

CHANGELOG.md is updated.
Documentation is updated.
New features are covered by tests.
~~How new setting should be integrated with file-based config? - documented as not supported~~

Introduce OTEL_APP_DOMAIN_STRATEGY to control AppDomain config update strategies for .NET Framework, supporting LoaderOptimization, assembly redirects, or no action. Add dynamic app.config modification for assembly binding redirects via AppConfigUpdater and new AssemblyCatalog. Expand IIS tests to cover all strategies.

iskiselev · 2025-12-24T02:06:42Z

~~Some special processing required for MongoDB - should be validated. Probably we should not include MongoDB dll in our distro at all, if we are not loading it?~~

SOLVED: We do not use any special processing for MongoDB anymore and do not include it in distro

iskiselev · 2025-12-24T20:24:37Z

    public static void ModifyConfig(AppDomainSetup appDomainSetup)
    {
-        appDomainSetup.LoaderOptimization = LoaderOptimization.SingleDomain;
+        var patchMode = Environment.GetEnvironmentVariable("OTEL_APP_DOMAIN_STRATEGY") ?? string.Empty;


Should we try to parse app.config when we check it? Which app.config should be used - creator domain or target domain?
How should be file-based config be integrated here?
For now, I plan to add it to the list of settings supported only by environment variables.

…AIN_STRATEGY` Added documentation and changlog information.

Handled creation of config when not provided by AppDomain

# Conflicts: # CHANGELOG.md # docs/config.md # src/OpenTelemetry.AutoInstrumentation.Loader/AssemblyResolver.Net.cs # src/OpenTelemetry.AutoInstrumentation.Loader/AssemblyResolver.cs # src/OpenTelemetry.AutoInstrumentation.Loader/Loader.cs # test/IntegrationTests/AspNetTests.cs

Copilot

Pull request overview

Adds a new .NET Framework AppDomain handling strategy to reduce assembly conflicts in non-default AppDomains without always forcing LoaderOptimization.SingleDomain, addressing the follow-up to #4186/#4187.

Changes:

Introduces OTEL_DOTNET_AUTO_APP_DOMAIN_STRATEGY with strategies (None, LoaderOptimizationSingleDomain, AssemblyRedirect) and integrates strategy selection into the loader.
Implements in-memory config patching for assembly binding redirects/codeBase via new AssemblyBindingUpdater + AssemblyCatalog, with accompanying unit tests.
Updates documentation, changelog, and ASP.NET integration tests/Docker setup to exercise the new modes.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
test/test-applications/integrations/TestApplication.AspNet.NetFramework/Dockerfile	Adjusts IIS app pool settings used by containerized ASP.NET .NETFX tests.
test/OpenTelemetry.AutoInstrumentation.Loader.Tests/AssemblyBindingUpdaterTests.cs	Adds unit tests covering assembly binding redirect/codeBase rewrite behavior.
test/IntegrationTests/AspNetTests.cs	Extends end-to-end ASP.NET tests to run with multiple AppDomain strategies.
src/OpenTelemetry.AutoInstrumentation/Util/ManagedProfilerLocationHelper.cs	Refactors `.link` resolution into a reusable helper with validation.
src/OpenTelemetry.AutoInstrumentation.Loader/AssemblyResolver.NetFramework.cs	Improves loader debug logging for `Assembly.LoadFrom`.
src/OpenTelemetry.AutoInstrumentation.Loader/AssemblyCatalog.cs	New catalog that discovers profiler/shared runtime assemblies (incl. `.link`) for redirect decisions.
src/OpenTelemetry.AutoInstrumentation.Loader/AssemblyBindingUpdater.cs	New updater that rewrites/augments `assemblyBinding` for non-default AppDomains.
src/OpenTelemetry.AutoInstrumentation.Loader/AppConfigUpdater.cs	Adds env-var driven strategy selection and hooks logger lifecycle.
docs/netfx-appdomain-strategy.md	New doc explaining the AppDomain problem space and strategy tradeoffs.
docs/config.md	Documents the new environment variable and its supported values.
docs/assembly-conflict-resolution.md	Links AppDomain strategy doc and clarifies .NETFX GAC precedence behavior.
CHANGELOG.md	Notes the addition of `OTEL_DOTNET_AUTO_APP_DOMAIN_STRATEGY`.
.cspell/dot-net.txt	Adds `appdomain` to spell-check dictionary.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

zacharycmontoya · 2026-04-28T00:00:09Z

+  the application. In that mode you accept the risk that secondary
+  `AppDomain` creation may make instrumentation incomplete or unstable.
+
+## Scope of this document


Thanks for the great detail in this markdown. I think if you move this ## Scope of this document section before the ## Why secondary AppDomains are special section, that could be helpful

zacharycmontoya · 2026-04-28T00:03:44Z

-    [InlineData(AppPoolMode.Integrated, Gac.UseGac)]
-    [InlineData(AppPoolMode.Integrated, Gac.UseLocal)]
-    public async Task SubmitsTraces(AppPoolMode appPoolMode, Gac useGac)
+    [InlineData(AppPoolMode.Integrated, Gac.UseGac, AppDomainWorkaround.LoaderOptimizationSingleDomain)]


Is there a reason to not test AppPoolMode.Integrated with all possibilities of Gac and AppDomainWorkaround?

Saving of electric eneregy :)
I do not expect that changing app pool mode between ingerated and classic will chenge anything in assembly loading. So, checking it only once will work (or checking classic only once).
I will add a comment to explain it.

zacharycmontoya · 2026-04-28T00:39:25Z

 COPY bin/${configuration}/app.publish .

 FROM integrated-base AS integrated-nogac
+RUN Start-IISCommitDelay;`


I don't understand what these changes are for, can you explain?

AssemblyRedirect - ... On test app docker requires app pool to loading user profile if assemblies loaded from folder...

So, here I enable loading of user profile for both IIS when assemlbies are not installed in GAC to make it compatible with AssemblyRedirect mode

zacharycmontoya · 2026-04-28T00:57:49Z

+    }
+
+#pragma warning disable SA1202
+    protected virtual string? GetCreatorApplicationBase()


Can we inline this where it's called?

Oh I see that this is used for testing. Can you comment the virtual / internal methods that are intended to be used only for testing?

zacharycmontoya · 2026-04-28T01:02:30Z

+        => AppDomain.CurrentDomain.BaseDirectory;
+#pragma warning restore SA1202
+
+    private static XElement CreateDependentAssembly(AssemblyCatalog.AssemblyInfo assemblyInfo)


Can you isolate the helper methods like this, creating versions, etc. into another C# file? This file is very complex so keeping only the necessary loading logic in this file would be very helpful

zacharycmontoya · 2026-04-28T01:09:02Z

+        return null;
+    }
+
+    private List<string> GetProbeDirectories(XElement assemblyBinding, AppDomainSetup appDomainSetup)


I had a hard time following this method (maybe because of the inner static methods) so I fed it into Claude and it gave me the following summary, which I found helpful for giving some high-level context first then it was easier to look into each step. Do you think any of this is worth adding as a high-level comment?

GetProbeDirectories (line 388) Replicates Fusion's probe path order: 1. If DisallowApplicationBaseProbing → return empty. 2. Resolves ApplicationBase; if missing, falls back to GetCreatorApplicationBase() (virtual, returns AppDomain.CurrentDomain.BaseDirectory) and writes it back into the setup. (:430-445, 565-566) 3. Adds the application base — unless PrivateBinPathProbe != null, which disables it (matches CLR semantics). 4. For web.config, adds {base}/bin. (:534-537) 5. Splits PrivateBinPath on ; — for rooted entries, only keeps them if they're under ApplicationBase (security: prevents probing outside the app), warns otherwise. Relative entries are joined to the base. (:466-481) 6. Same treatment for <probing privatePath="..."> from the config. 7. Dedupes by full path, case-insensitive. Logs the resolved set with full context (config file, base, flags). (:393-428)

zacharycmontoya · 2026-04-28T01:22:35Z

This file was rather difficult to review because there's a lot of Fusion / probing logic that I wasn't already familiar with, so I have two requests to hopefully help other reviewers/maintainers:

If there are any public docs or links that explain some of the behaviors, can you link to it?

If reasonable, could you provide code comments to explain the high-level "what" is the goal/logic of a given method? No need to explain the "how" as that's in code.

When reviewing this file, I told Claude verbatim Walk me through the changes introduced in src/OpenTelemetry.AutoInstrumentation.Loader/AssemblyBindingUpdater.cs starting from ModifyAssemblyRedirectConfig and it was surprisingly insightful and provided a helpful method-by-method breakdown. This could also be a good source of method-level comments so I'd be happy to share if you'd like

zacharycmontoya

I've reviewed everything but the AssemblyBindingUpdaterTests.cs, which I can continue tomorrow

zacharycmontoya · 2026-04-28T18:12:33Z

+                <dependentAssembly>
+                  <assemblyIdentity name="{{TestAssemblyName}}" publicKeyToken="{TOKEN}" culture="neutral" />
+                  <bindingRedirect oldVersion="0.0.0.0-{{lowerRedirectVersion}}" newVersion="{{lowerRedirectVersion}}" />
+                  <codeBase version="{{customerVersion}}" href="{{customerHref}}" />


Is this typical? I'd expect the user config to only provide a codebase with the version matching the original redirect version

zacharycmontoya

Overall the changes and testing look good, thanks!

iskiselev commented Dec 24, 2025

View reviewed changes

iskiselev added 3 commits December 24, 2025 15:27

Newly added config variable name changed to `OTEL_DOTNET_AUTO_APP_DOM…

79f29d2

…AIN_STRATEGY` Added documentation and changlog information.

Formated changelog

d44cd9d

Updated environment variable for AspNetTests

ff52a59

Handled creation of config when not provided by AppDomain

iskiselev commented Jan 7, 2026

View reviewed changes

Comment thread src/OpenTelemetry.AutoInstrumentation.Loader/AppConfigUpdater.cs Outdated

iskiselev added 5 commits April 20, 2026 17:53

Removed unintended changes

bc14126

CI fixes

27d9d20

Merge remote-tracking branch 'origin/main' into redirect

bfca96e

Fixed assembly link probing

ba4cdf5

zacharycmontoya self-requested a review April 22, 2026 16:05

iskiselev added 3 commits April 23, 2026 09:11

Fixed app domains workaround strategy logging

d6d21f0

Probe application folder when creating bindingRedirect

868a2c7

Add documentation

5d2d760

iskiselev marked this pull request as ready for review April 24, 2026 06:20

iskiselev requested a review from a team as a code owner April 24, 2026 06:20

iskiselev added 2 commits April 23, 2026 23:23

Spelling

b97ed02

appdomain as spell exception

2d66a38

iskiselev requested a review from Copilot April 24, 2026 16:00

Copilot started reviewing on behalf of iskiselev April 24, 2026 16:02 View session

Copilot AI reviewed Apr 24, 2026

View reviewed changes

iskiselev added 3 commits April 24, 2026 10:13

Resolved CoPilot comments

d5b7f98

Double spacing fixed

d3bbb0b

Merge remote-tracking branch 'origin/main' into redirect

58c68e7

zacharycmontoya reviewed Apr 28, 2026

View reviewed changes

zacharycmontoya approved these changes Apr 28, 2026

View reviewed changes

iskiselev mentioned this pull request Apr 28, 2026

Assembly version redirect fails for entry point on .NET Framework #4905

Open

Rhynier approved these changes May 1, 2026

View reviewed changes

Conversation

iskiselev commented Dec 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

What

Tests

Checklist

Uh oh!

iskiselev commented Dec 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iskiselev Apr 28, 2026 • 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!

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 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zacharycmontoya left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

iskiselev commented Dec 24, 2025 •

edited

Loading

iskiselev commented Dec 24, 2025 •

edited

Loading

iskiselev Apr 28, 2026 •

edited

Loading