Detect whether an app has been trimmed#8281
Conversation
BenchmarksBenchmark execution time: 2026-03-11 16:44:56 Comparing candidate commit 4e4bce1 in PR branch Found 6 performance improvements and 7 performance regressions! Performance is the same for 168 metrics, 11 unstable metrics. scenario:Benchmarks.Trace.AgentWriterBenchmark.WriteAndFlushEnrichedTraces net6.0
scenario:Benchmarks.Trace.Asm.AppSecBodyBenchmark.AllCycleMoreComplexBody net6.0
scenario:Benchmarks.Trace.Asm.AppSecBodyBenchmark.ObjectExtractorSimpleBody net6.0
scenario:Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark.WriteAndFlushEnrichedTraces netcoreapp3.1
scenario:Benchmarks.Trace.CharSliceBenchmark.OptimizedCharSliceWithPool net6.0
scenario:Benchmarks.Trace.CharSliceBenchmark.OriginalCharSlice net6.0
scenario:Benchmarks.Trace.ILoggerBenchmark.EnrichedLog netcoreapp3.1
scenario:Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark net6.0
scenario:Benchmarks.Trace.RedisBenchmark.SendReceive net472
scenario:Benchmarks.Trace.SingleSpanAspNetCoreBenchmark.SingleSpanAspNetCore netcoreapp3.1
|
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8281) 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 (8281) - mean (73ms) : 71, 75
master - mean (74ms) : 72, 75
section Bailout
This PR (8281) - mean (77ms) : 76, 79
master - mean (79ms) : 77, 81
section CallTarget+Inlining+NGEN
This PR (8281) - mean (1,080ms) : 1017, 1143
master - mean (1,092ms) : 1045, 1138
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 (8281) - mean (115ms) : 111, 119
master - mean (118ms) : 112, 124
section Bailout
This PR (8281) - mean (116ms) : 113, 118
master - mean (119ms) : 116, 122
section CallTarget+Inlining+NGEN
This PR (8281) - mean (769ms) : 709, 829
master - mean (773ms) : 712, 835
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8281) - mean (102ms) : 98, 105
master - mean (103ms) : 100, 106
section Bailout
This PR (8281) - mean (102ms) : 99, 105
master - mean (103ms) : 101, 106
section CallTarget+Inlining+NGEN
This PR (8281) - mean (755ms) : 701, 810
master - mean (749ms) : 685, 813
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8281) - mean (100ms) : 98, 102
master - mean (101ms) : 99, 104
section Bailout
This PR (8281) - mean (101ms) : 99, 102
master - mean (103ms) : 99, 106
section CallTarget+Inlining+NGEN
This PR (8281) - mean (671ms) : 653, 690
master - mean (669ms) : 650, 688
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 (8281) - mean (193ms) : 189, 197
master - mean (194ms) : 189, 198
section Bailout
This PR (8281) - mean (196ms) : 193, 199
master - mean (196ms) : 194, 198
section CallTarget+Inlining+NGEN
This PR (8281) - mean (1,149ms) : 1103, 1195
master - mean (1,144ms) : 1102, 1185
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 (8281) - mean (276ms) : 270, 282
master - mean (281ms) : 276, 285
section Bailout
This PR (8281) - mean (277ms) : 274, 281
master - mean (279ms) : 274, 284
section CallTarget+Inlining+NGEN
This PR (8281) - mean (943ms) : 911, 975
master - mean (943ms) : 907, 979
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8281) - mean (271ms) : 266, 277
master - mean (270ms) : 265, 274
section Bailout
This PR (8281) - mean (270ms) : 267, 273
master - mean (271ms) : 266, 276
section CallTarget+Inlining+NGEN
This PR (8281) - mean (938ms) : 902, 975
master - mean (934ms) : 904, 964
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8281) - mean (269ms) : 264, 275
master - mean (269ms) : 262, 276
section Bailout
This PR (8281) - mean (270ms) : 264, 275
master - mean (269ms) : 263, 275
section CallTarget+Inlining+NGEN
This PR (8281) - mean (838ms) : 810, 866
master - mean (828ms) : 810, 846
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
a75f5b7 to
6ed5beb
Compare
There was a problem hiding this comment.
Pull request overview
Adds runtime detection for whether a customer app was published with trimming and whether the Datadog.Trace.Trimming descriptor was applied, surfacing the result via telemetry/log tags and a startup warning to aid supportability.
Changes:
- Introduces
TrimmingDetector(NET6+) that probes for selected BCL types to classify trimming state. - Adds
trim:err/yes/notagging to telemetry error-log tags and records new config telemetry flags. - Updates unit/integration tests and trimming/build artifacts to preserve “canary” types and validate behavior in trimmed sample apps.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tracer/test/test-applications/integrations/Samples.Trimming/Program.cs | Adds an env-var-controlled reflected call to emit an error log for telemetry assertions. |
| tracer/test/Datadog.Trace.Tests/Telemetry/TelemetryControllerLogTagBuilderTests.cs | Updates expected log tag strings to include the new trim:no suffix on NET6+. |
| tracer/test/Datadog.Trace.ClrProfiler.IntegrationTests/TelemetryTests.cs | Ensures redacted error logs include trim:no on NET6+. |
| tracer/test/Datadog.Trace.ClrProfiler.IntegrationTests/AppTrimmingTests.cs | Adds an integration test validating trim:yes in trimmed app telemetry error logs. |
| tracer/src/Datadog.Trace/Telemetry/TelemetryController.cs | Appends trim:err/yes/no to telemetry log tags (NET6+). |
| tracer/src/Datadog.Trace/Telemetry/DTOs/ConfigTelemetryData.cs | Adds config telemetry keys for trimming detection outcomes. |
| tracer/src/Datadog.Trace/PlatformHelpers/TrimmingDetector.cs | New detector that classifies trimming state by probing for specific BCL types (NET6+). |
| tracer/src/Datadog.Trace/Configuration/TracerSettings.cs | Records trimming detection results into configuration telemetry (NET6+). |
| tracer/src/Datadog.Trace/ClrProfiler/Instrumentation.cs | Emits a warning when trimming is detected without the trimming descriptor (NET6+). |
| tracer/src/Datadog.Trace.Trimming/build/Datadog.Trace.Trimming.xml | Preserves canary BCL types needed by the trimming detector. |
| tracer/build/_build/Build.Steps.cs | Ensures generated trimming descriptor includes canary types; ignores the new test log line in error-log scanning. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: a23b88fb9b
ℹ️ 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".
| if (Type.GetType("System.Resources.ResourceWriter, System.Resources.Writer", throwOnError: false) is null | ||
| || Type.GetType("System.IO.IsolatedStorage.IsolatedStorageScope, System.IO.IsolatedStorage", throwOnError: false) is null) |
There was a problem hiding this comment.
This branch assumes that if ResourceWriter and IsolatedStorageScope are present then the app must be using Datadog.Trace.Trimming.xml, but that is not guaranteed: a trimmed app can preserve both types through its own code or transitive dependencies even when our trim descriptor is not referenced. In that case this detector reports TrimmedAppUsingTrimmingFile instead of TrimmedAppMissingTrimmingFile, so the startup warning in Instrumentation.InitializeNoNativeParts() is skipped and support telemetry is mislabeled for the exact failure mode this change is trying to surface.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Yes, that's the point 🙄
|
Maybe a silly question but instead of Ping could we use something from Obviously we'd need to still choose something that is still obscure (and there aren't a ton of options) like an attribute? https://github.com/dotnet/dotnet/blob/main/src/runtime/src/libraries/Microsoft.VisualBasic.Core/src/Microsoft/VisualBasic/HideModuleNameAttribute.vb |
bouwkast
left a comment
bouwkast
left a comment
There was a problem hiding this comment.
Thanks!
Only thing I was wondering if we should use something other than that Ping object #8281 (comment)
| $",prof:{(_isProfilingEnabled ? '1' : '0')}" + | ||
| $",dyn:{(_isDynamicInstrumentationEnabled ? '1' : '0')}" + | ||
| #if NET6_0_OR_GREATER | ||
| $",trim:{(trimState == TrimmingDetector.TrimState.TrimmedAppMissingTrimmingFile |
Yeah, I considered it, and actually initially discounted it because obvs people using VB would have it loaded and give a false positive, but that does seem very unlikely 😅 However... it's 1.1MB... and it has a ton of references too, so I think it will end up being a lot "heavier" than Ping. Just to be clear, was your reasoning that it would provide a better signal to noise ratio than Ping? |
Co-authored-by: Copilot <[email protected]>
Yep I just assumed that the people using VB.NET these days may be less than the people using Ping, but if it is 1.1MB I'd say ping is good 👍 |
Summary of changes
Detects whether an app has been published with trimming and whether our Trimming.xml file has been used.
Reason for change
Using trimming and not using our trimming file can cause all sorts of strange errors. It would be useful for support (and for informing customers) if an app is using trimming and is not using our trimming file
Implementation details
Uses three
Types in the BCL as "probes" for whether an app is trimmed:System.Resources.ResourceWriter, System.Resources.WriterSystem.IO.IsolatedStorage.IsolatedStorageScope, System.IO.IsolatedStorageSystem.Net.NetworkInformation.PingCompletedEventArgs, System.Net.PingThese are intentionally somewhat obtuse types that we would expect to be trimmed. Settled on them by iterating with 🤖 but we could certainly change these.
The overall approach is
If we detect the bad situation, we add a warning to the logs. Either way, we tag the telemetry error logs with
trim:err/yes/nowhere:erris trimmed and didn't add our fileyesis trimmed but they added our filenonot trimmedTest coverage
Pushed an initial test without adding anything to the trimming file, and confirmed that we tagged as
errand write the error log (which correctly caused the integration tests and trimming smoke tests to fail).Other details
The main consideration is the performance impact of loading these extra types, from obtuse assemblies, on the hot path of app load. Each of the assemblies is ~80kb (I swapped from System.Net.Mail because it's ~5x as big), but then there's the dependency tree too... I considered using an ACL and unloading, but as I understand it, that wouldn't necessarily actually unload them, seeing as they're part of the shared framework, but I confess I'm trusting the AI on that one 😅