[tracing] fix bundle detection for check-process for AAS - #8910
Open
chojomok wants to merge 21 commits into
Open
[tracing] fix bundle detection for check-process for AAS#8910chojomok wants to merge 21 commits into
chojomok wants to merge 21 commits into
Conversation
chojomok
force-pushed
the
mohammad/fix-bundle-detection
branch
from
July 20, 2026 20:47
50d5262 to
eb7beb6
Compare
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8910) and master. ✅ No regressions detected - check the details below Full Metrics ComparisonFakeDbCommand
HttpMessageHandler
Comparison explanationExecution-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 highlighted in **red**. The following thresholds were used for comparing the execution times:
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). Duration chartsFakeDbCommand (.NET Framework 4.8)gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8910) - mean (74ms) : 69, 78
master - mean (71ms) : 69, 73
section Bailout
This PR (8910) - mean (77ms) : 73, 80
master - mean (79ms) : 73, 84
section CallTarget+Inlining+NGEN
This PR (8910) - mean (1,093ms) : 1039, 1147
master - mean (1,096ms) : 1041, 1152
FakeDbCommand (.NET Core 3.1)gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8910) - mean (113ms) : 107, 120
master - mean (113ms) : 107, 120
section Bailout
This PR (8910) - mean (111ms) : 108, 114
master - mean (115ms) : 110, 120
section CallTarget+Inlining+NGEN
This PR (8910) - mean (785ms) : 762, 809
master - mean (785ms) : 766, 803
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8910) - mean (97ms) : 94, 101
master - mean (100ms) : 96, 103
section Bailout
This PR (8910) - mean (98ms) : 96, 101
master - mean (102ms) : 98, 107
section CallTarget+Inlining+NGEN
This PR (8910) - mean (953ms) : 913, 994
master - mean (948ms) : 900, 995
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8910) - mean (100ms) : 95, 106
master - mean (97ms) : 94, 99
section Bailout
This PR (8910) - mean (98ms) : 95, 101
master - mean (99ms) : 95, 104
section CallTarget+Inlining+NGEN
This PR (8910) - mean (822ms) : 787, 857
master - mean (824ms) : 775, 873
HttpMessageHandler (.NET Framework 4.8)gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8910) - mean (210ms) : 203, 216
master - mean (209ms) : 203, 215
section Bailout
This PR (8910) - mean (213ms) : 211, 216
master - mean (214ms) : 210, 217
section CallTarget+Inlining+NGEN
This PR (8910) - mean (1,252ms) : 1213, 1292
master - mean (1,251ms) : 1208, 1294
HttpMessageHandler (.NET Core 3.1)gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8910) - mean (299ms) : 293, 306
master - mean (299ms) : 293, 306
section Bailout
This PR (8910) - mean (301ms) : 296, 305
master - mean (301ms) : 296, 306
section CallTarget+Inlining+NGEN
This PR (8910) - mean (1,000ms) : 981, 1018
master - mean (995ms) : 971, 1019
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8910) - mean (294ms) : 289, 300
master - mean (294ms) : 289, 299
section Bailout
This PR (8910) - mean (294ms) : 290, 298
master - mean (293ms) : 289, 298
section CallTarget+Inlining+NGEN
This PR (8910) - mean (1,197ms) : 1159, 1235
master - mean (1,191ms) : 1147, 1236
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8910) - mean (293ms) : 287, 298
master - mean (293ms) : 288, 299
section Bailout
This PR (8910) - mean (293ms) : 288, 298
master - mean (294ms) : 290, 298
section CallTarget+Inlining+NGEN
This PR (8910) - mean (1,069ms) : 1013, 1124
master - mean (1,076ms) : 1006, 1146
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
BenchmarksBenchmark execution time: 2026-07-31 20:42:05 Comparing candidate commit 7fbb973 in PR branch Found 0 performance improvements and 2 performance regressions! Performance is the same for 70 metrics, 0 unstable metrics, 68 known flaky benchmarks, 58 flaky benchmarks without significant changes.
|
chojomok
added
area:tools
AI Generated
TracingWithBundle is exposed as internal (not private) so ProcessBasicCheckTests can exercise it directly. No behavior change yet - DetectsBundleWhenLaunchedAsDotnetDll documents the still-open bug where `dotnet app.dll` launches (e.g. Azure App Service Linux) report the dotnet host as MainModule instead of the app directory, so the bundle check never matches. Other tests confirm existing working scenarios (matching MainModule, non-bundle rejection, Windows suffixes) are unaffected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The test project only targeted net7.0, which CI's default (non-thorough) test matrix never runs on Linux/macOS - it runs netcoreapp3.1/net9.0/net10.0 instead. That meant ProcessBasicCheckTests silently never executed in CI. Match the frameworks already supported by Datadog.Trace.Tools.dd_dotnet itself (net7.0;net8.0;net9.0;net10.0) so the suite actually runs. Verified locally across all four frameworks: 43 passed, 4 skipped (Windows-only cases), 1 failed (the regression case) - consistent, no new breakage introduced. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
TracingWithBundle reconstructed the expected profiler path from process.MainModule's directory. For a process launched as `dotnet app.dll` (e.g. Azure App Service Linux, or any framework-dependent deploy), MainModule is the dotnet host executable, not the app's own directory, so the reconstructed path never matched the real CORECLR_PROFILER_PATH - even on a correctly configured bundle (Datadog.Trace.Bundle NuGet) install. The check fell through to the installer-package path instead, producing a confusing, unrelated error. Match by suffix instead of reconstructing the full expected path, so detection no longer depends on where the process's main module happens to live. All ProcessBasicCheckTests now pass, including the regression case DetectsBundleRegardlessOfAppDirectory (previously DetectsBundleWhenLaunchedAsDotnetDll, the test added and verified failing against the unfixed code in CI). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
TracingWithBundle now tries the original exact match against process.MainModule's directory first (the more precise signal when MainModule correctly reflects the app directory). Only when that fails does it fall back to matching the profiler path by suffix alone - which is what's needed for `dotnet <app>.dll` launches (e.g. Azure App Service Linux), where MainModule is the dotnet host, not the app directory. The fallback path calls out via Utils.WriteInfo so it's visible when the less precise heuristic was needed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
TracingWithBundle previously printed its fallback explanation as a side effect regardless of the overall check result, so it showed up even on fully healthy runs. It's now a pure detection function that reports whether the fallback path was used via an out parameter, and the caller only prints the explanation inside the branch where it's actually relevant (the check is failing and the fallback is what identifies it as a bundle install instead of falling through to the installer message). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
chojomok
force-pushed
the
mohammad/fix-bundle-detection
branch
from
July 22, 2026 21:07
63a8b65 to
396628a
Compare
ProcessBasicCheckRunTests exercises ProcessBasicCheck.Run() directly (via a redirected AnsiConsole, same pattern already used by the IntegrationTests ConsoleHelper) against the real failure scenario: a correctly configured Datadog.Trace.Bundle Nuget install launched as `dotnet <app>.dll`, with an unrelated check also failing so the run overall fails. Asserts the printed output identifies it as a bundle install rather than falling through to the confusing installer error that mentions /opt/datadog. Verified this test fails (along with the existing unit test) when the suffix-only fallback is disabled, and passes once restored. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This reverts commit 65c80e6.
Path.GetDirectoryName normalizes a leading forward-slash root differently on real Windows than on Linux/macOS, so the exact-match scenario these tests exercise (mainModule built with a hardcoded "/app" Unix-style path) doesn't hold there - usedFallback comes back true instead of false, even though the underlying detection logic is correct. This only became visible once the tests started asserting on usedFallback directly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
chojomok
changed the title
chojomok
marked this pull request as ready for review
July 27, 2026 17:36
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21b9c0be34
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
chojomok
marked this pull request as draft
July 27, 2026 18:04
chojomok
marked this pull request as ready for review
July 30, 2026 17:42
chojomok
force-pushed
the
mohammad/fix-bundle-detection
branch
2 times, most recently
from
July 31, 2026 19:52
7295536 to
3476221
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes
Fixes
dd-dotnet check processbundle detection for Azure App Service Linux apps launched withdotnet <app>.dll.Reason for change
For these apps,
MainModuleis thedotnethost rather than the application executable. The check therefore reconstructed the bundle profiler path from the wrong directory and showed installer guidance instead of bundle guidance.Implementation details
When
WEBSITE_SITE_NAMEis present,check processrecognizes Azure App Service and uses/home/site/wwwrootas the bundle root.Other processes retain the existing
MainModule-directory path check. Bundle matching remains an exact profiler-path comparison.Test coverage
Added Linux-only coverage through
ProcessBasicCheck.Run()for Azure App Service. The test setsWEBSITE_SITE_NAMEand verifies bundle detection for each supported Linux profiler path: x64, musl x64, arm64, and musl arm64.