managed loader: refactor for testability, add unit tests #7594

lucaspimentel · 2025-09-30T21:29:53Z

Summary of changes

Refactored the managed loader (Datadog.Trace.ClrProfiler.Managed.Loader) to improve testability and added comprehensive unit tests.

Reason for change

The managed loader code was difficult to test due to tight coupling with environment variables and filesystem access. This refactoring enables better test coverage and makes the code more maintainable.

Implementation details

New abstractions:
- Added IEnvironmentVariableProvider and EnvironmentVariableProvider to abstract environment variable access, enabling dependency injection for tests
- Implemented as readonly struct with generic type parameters to avoid heap allocations and boxing
Refactored startup logic:
- Moved TFM directory computation into dedicated methods that can be tested independently
- Separated directory existence checks from path computation
- Improved error messages and logging consistency
Code improvements:
- Added nullable reference type annotations throughout
- Standardized Boolean environment variable parsing to match tracer conventions
- Cleaned up comments and improved code clarity

Test coverage

Added 5 new test files with coverage of:

Environment variable reading and Boolean parsing (EnvironmentVariableProviderTests)
Log directory resolution across platforms (StartupLoggerTests)
TFM directory computation for .NET Core (StartupNetCoreTests)
TFM directory computation for .NET Framework (StartupNetFrameworkTests)
Edge cases and error conditions
Added MockEnvironmentVariableProvider for test isolation

Other details

This refactoring is preparatory work for #7568 which makes DD_DOTNET_TRACER_HOME optional, but the changes were hard to test.

dd-trace-dotnet-ci-bot · 2025-09-30T22:34:16Z

Execution-Time Benchmarks Report ⏱️

Execution-time results for samples comparing the following branches/commits:

Execution-time benchmarks measure the whole time it takes to execute a program. And are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are shown in red. The following thresholds were used for comparing the execution times:

Welch test with statistical test for significance of 5%
Only results indicating a difference greater than 5% and 5 ms are considered.

Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard.

Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph).

gantt
    title Execution time (ms) FakeDbCommand (.NET Framework 4.8) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7594) - mean (72ms)  : 71, 73
     .   : milestone, 72,
    master - mean (72ms)  : 71, 73
     .   : milestone, 72,

    section Baseline
    This PR (7594) - mean (68ms)  : 65, 71
     .   : milestone, 68,
    master - mean (68ms)  : 66, 71
     .   : milestone, 68,

    section CallTarget+Inlining+NGEN
    This PR (7594) - mean (1,047ms)  : 991, 1103
     .   : milestone, 1047,
    master - mean (1,043ms)  : 1007, 1080
     .   : milestone, 1043,

gantt
    title Execution time (ms) FakeDbCommand (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7594) - mean (107ms)  : 105, 108
     .   : milestone, 107,
    master - mean (106ms)  : 105, 107
     .   : milestone, 106,

    section Baseline
    This PR (7594) - mean (105ms)  : 103, 108
     .   : milestone, 105,
    master - mean (105ms)  : 103, 108
     .   : milestone, 105,

    section CallTarget+Inlining+NGEN
    This PR (7594) - mean (739ms)  : 720, 759
     .   : milestone, 739,
    master - mean (741ms)  : 722, 760
     .   : milestone, 741,

gantt
    title Execution time (ms) FakeDbCommand (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7594) - mean (94ms)  : 93, 95
     .   : milestone, 94,
    master - mean (94ms)  : 93, 95
     .   : milestone, 94,

    section Baseline
    This PR (7594) - mean (94ms)  : 91, 96
     .   : milestone, 94,
    master - mean (93ms)  : 91, 95
     .   : milestone, 93,

    section CallTarget+Inlining+NGEN
    This PR (7594) - mean (700ms)  : 678, 723
     .   : milestone, 700,
    master - mean (703ms)  : 683, 723
     .   : milestone, 703,

gantt
    title Execution time (ms) FakeDbCommand (.NET 8) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7594) - mean (93ms)  : 92, 94
     .   : milestone, 93,
    master - mean (93ms)  : 92, 94
     .   : milestone, 93,

    section Baseline
    This PR (7594) - mean (92ms)  : 90, 94
     .   : milestone, 92,
    master - mean (92ms)  : 90, 94
     .   : milestone, 92,

    section CallTarget+Inlining+NGEN
    This PR (7594) - mean (659ms)  : 646, 672
     .   : milestone, 659,
    master - mean (664ms)  : 648, 680
     .   : milestone, 664,

gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.8) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7594) - mean (201ms)  : 197, 205
     .   : milestone, 201,
    master - mean (213ms)  : 209, 217
     .   : milestone, 213,

    section Baseline
    This PR (7594) - mean (197ms)  : 192, 202
     .   : milestone, 197,
    master - mean (207ms)  : 203, 212
     .   : milestone, 207,

    section CallTarget+Inlining+NGEN
    This PR (7594) - mean (1,180ms)  : 1111, 1249
     .   : milestone, 1180,
    master - mean (1,229ms)  : 1145, 1313
     .   : milestone, 1229,

gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7594) - mean (282ms)  : 277, 288
     .   : milestone, 282,
    master - mean (300ms)  : 294, 305
     .   : milestone, 300,

    section Baseline
    This PR (7594) - mean (283ms)  : 277, 289
     .   : milestone, 283,
    master - mean (299ms)  : 292, 306
     .   : milestone, 299,

    section CallTarget+Inlining+NGEN
    This PR (7594) - mean (955ms)  : 914, 996
     .   : milestone, 955,
    master - mean (988ms)  : 958, 1019
     .   : milestone, 988,

gantt
    title Execution time (ms) HttpMessageHandler (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7594) - mean (276ms)  : 272, 281
     .   : milestone, 276,
    master - mean (291ms)  : 284, 298
     .   : milestone, 291,

    section Baseline
    This PR (7594) - mean (277ms)  : 269, 284
     .   : milestone, 277,
    master - mean (291ms)  : 283, 299
     .   : milestone, 291,

    section CallTarget+Inlining+NGEN
    This PR (7594) - mean (939ms)  : 885, 993
     .   : milestone, 939,
    master - mean (977ms)  : 940, 1015
     .   : milestone, 977,

gantt
    title Execution time (ms) HttpMessageHandler (.NET 8) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Bailout
    This PR (7594) - mean (276ms)  : 271, 281
     .   : milestone, 276,
    master - mean (291ms)  : 286, 296
     .   : milestone, 291,

    section Baseline
    This PR (7594) - mean (275ms)  : 270, 280
     .   : milestone, 275,
    master - mean (291ms)  : 281, 301
     .   : milestone, 291,

    section CallTarget+Inlining+NGEN
    This PR (7594) - mean (860ms)  : 835, 886
     .   : milestone, 860,
    master - mean (923ms)  : 838, 1007
     .   : milestone, 923,

tonyredondo

I have to do something else, I'll finish later.

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

tracer/src/Datadog.Trace.ClrProfiler.Managed.Loader/StartupLogger.cs

…use generic constraints - Convert EnvironmentVariableProvider from class to readonly struct for zero allocation - Add generic type parameter TEnvVars to extension methods to enable struct usage without boxing - Update GetLogDirectoryFromEnvVars and GetDefaultLogDirectory to use generic constraints 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

…s disabled

## Summary of changes - Add performance optimization patterns, testing guidelines, and logging guidelines to `AGENTS.md`. - Bonus: Fix noop pipeline path exclusions to properly skip CI for documentation-only PRs. ## Reason for change **Documentation:** Document reusable patterns that apply broadly across the tracer codebase: - Performance patterns for startup code and hot paths (from PR #7594) - Testing patterns for testability and isolation (from PR #7594) - Logging terminology to avoid customer confusion (from PR #7467) **CI Optimization:** The noop pipeline was not triggering for documentation-only PRs because it uses `StartsWith()` matching, not glob patterns. The `**/CLAUDE.md` pattern never matched `.claude/CLAUDE.md`. ## Implementation details **New documentation sections:** - Performance Guidelines: Identifies performance-critical code paths (bootstrap/startup and hot paths) - Performance Optimization Patterns: Zero-allocation structs, logging best practices, avoiding params arrays - Testing Patterns: Abstraction-for-testability with examples - Logging Guidelines: Consistent customer-facing terminology (Datadog SDK, Instrumentation, Continuous Profiler, Datadog.Trace.dll) **Pipeline fixes:** - Changed `**/CLAUDE.md` glob to `.claude/` prefix (compatible with `StartsWith()`) - Added `.devcontainer/`, `.vscode/`, `LICENSE`, `NOTICE` to exclusions **Other changes:** - Fixed `.claude/CLAUDE.md` relative path reference - Improved formatting in Coding Style section ## Test coverage N/A - documentation and CI configuration only --------- Co-authored-by: Claude <noreply@anthropic.com>

lucaspimentel force-pushed the lpimentel/managed-loader-cleanup branch from a5aefab to 71d026c Compare September 30, 2025 21:30

lucaspimentel force-pushed the lpimentel/managed-loader-cleanup branch 2 times, most recently from f7a0f93 to f708780 Compare October 1, 2025 21:55

lucaspimentel changed the title ~~managed loader cleanup and testability~~ managed loader: refactoring for testability, add unit tests Oct 1, 2025

lucaspimentel force-pushed the lpimentel/managed-loader-cleanup branch from f708780 to bf77e28 Compare October 1, 2025 23:30

lucaspimentel changed the title ~~managed loader: refactoring for testability, add unit tests~~ managed loader: refactor for testability, add unit tests Oct 2, 2025

lucaspimentel mentioned this pull request Oct 2, 2025

make DD_DOTNET_TRACER_HOME optional #7568

Open

lucaspimentel marked this pull request as ready for review October 2, 2025 02:53

lucaspimentel requested review from a team as code owners October 2, 2025 02:53

This comment has been minimized.

Sign in to view

tonyredondo reviewed Oct 2, 2025

View reviewed changes

tracer/src/Datadog.Trace.ClrProfiler.Managed.Loader/Startup.NetCore.cs Outdated Show resolved Hide resolved

tracer/src/Datadog.Trace.ClrProfiler.Managed.Loader/Startup.NetCore.cs Outdated Show resolved Hide resolved

tonyredondo reviewed Oct 2, 2025

View reviewed changes

tracer/src/Datadog.Trace.ClrProfiler.Managed.Loader/StartupLogger.cs Show resolved Hide resolved

lucaspimentel force-pushed the lpimentel/managed-loader-cleanup branch from caaa220 to 7d426a8 Compare October 2, 2025 14:01

lucaspimentel requested a review from a team October 2, 2025 14:04

lucaspimentel added 14 commits October 2, 2025 13:52

add a project reference and [InternalsVisibleTo]

0961236

add IEnvironmentVariableProvider and EnvironmentVariableProvider

690af1c

clean up logs and code comments

57ee898

refactor: use EnvironmentVariableProvider, add nullable annotations, etc

3338f40

more refactoring in StartupLogger

e2975b6

rename method, move the check for Directory.Exists() to shared code

9628467

move the check for DD_DOTNET_TRACER_HOME being set to shared code

5ce4b2d

refactor GetBooleanEnvironmentVariable() to match our other ToBoolean()

452ebe3

make DependencyLoadContext private

8184d52

remove redundant null check

2145319

add unit tests

23a6ae3

cleanup

1bb9f38

fix xml doc comment

8959266

add argument name

93c1c96

lucaspimentel and others added 13 commits October 2, 2025 13:52

convert DependencyLoadContext to readonly field

33f1b79

delete empty file

6f2d251

suppress nullable warnings

41b1cb1

CompileManagedUnitTests now depends on CompileManagedLoader

325234a

remove log message that accesses assembly.FullName

0d391eb

log before Path.Combine() in case it throws

dcd6661

build: change After() to DependsOn()

f214a13

avoid allocating strings and params arrays if we can't log or debug i…

c07fea6

…s disabled

do the same for Log(Exception, ...)

497c779

rename LogCore() to Log()

89890ac

disable further logging on exception

f3b6b02

cleanup

2df2a24

lucaspimentel force-pushed the lpimentel/managed-loader-cleanup branch from 3a9626d to 2df2a24 Compare October 2, 2025 17:52

lucaspimentel mentioned this pull request Oct 2, 2025

Add performance, testing, logging, Azure Functions, and glossary to AGENTS.md #7609

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

managed loader: refactor for testability, add unit tests #7594

managed loader: refactor for testability, add unit tests #7594

Uh oh!

lucaspimentel commented Sep 30, 2025 •

edited

Loading

Uh oh!

dd-trace-dotnet-ci-bot bot commented Sep 30, 2025 •

edited

Loading

Uh oh!

This comment has been minimized.

tonyredondo left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

managed loader: refactor for testability, add unit tests #7594

Are you sure you want to change the base?

managed loader: refactor for testability, add unit tests #7594

Uh oh!

Conversation

lucaspimentel commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Reason for change

Implementation details

Test coverage

Other details

Uh oh!

dd-trace-dotnet-ci-bot bot commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Execution-Time Benchmarks Report ⏱️

Uh oh!

This comment has been minimized.

tonyredondo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lucaspimentel commented Sep 30, 2025 •

edited

Loading

dd-trace-dotnet-ci-bot bot commented Sep 30, 2025 •

edited

Loading