[Crashtracking] Fix crashtracking in single-file apps #6677
Conversation
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
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:
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.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6677) - mean (69ms) : 67, 72
. : milestone, 69,
master - mean (70ms) : 65, 74
. : milestone, 70,
section CallTarget+Inlining+NGEN
This PR (6677) - mean (1,003ms) : 984, 1022
. : milestone, 1003,
master - mean (1,003ms) : 977, 1029
. : milestone, 1003,
gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6677) - mean (103ms) : 101, 105
. : milestone, 103,
master - mean (102ms) : 100, 105
. : milestone, 102,
section CallTarget+Inlining+NGEN
This PR (6677) - mean (688ms) : 670, 705
. : milestone, 688,
master - mean (688ms) : 664, 711
. : milestone, 688,
gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6677) - mean (90ms) : 87, 92
. : milestone, 90,
master - mean (89ms) : 87, 91
. : milestone, 89,
section CallTarget+Inlining+NGEN
This PR (6677) - mean (638ms) : 622, 653
. : milestone, 638,
master - mean (640ms) : 626, 654
. : milestone, 640,
gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6677) - mean (190ms) : 187, 193
. : milestone, 190,
master - mean (191ms) : 187, 196
. : milestone, 191,
section CallTarget+Inlining+NGEN
This PR (6677) - mean (1,109ms) : 1083, 1135
. : milestone, 1109,
master - mean (1,107ms) : 1081, 1133
. : milestone, 1107,
gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6677) - mean (271ms) : 267, 274
. : milestone, 271,
master - mean (271ms) : 266, 275
. : milestone, 271,
section CallTarget+Inlining+NGEN
This PR (6677) - mean (879ms) : 851, 907
. : milestone, 879,
master - mean (877ms) : 842, 913
. : milestone, 877,
gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6677) - mean (262ms) : 257, 268
. : milestone, 262,
master - mean (262ms) : 259, 266
. : milestone, 262,
section CallTarget+Inlining+NGEN
This PR (6677) - mean (854ms) : 818, 890
. : milestone, 854,
master - mean (855ms) : 817, 892
. : milestone, 855,
|
andrewlock
approved these changes
Feb 18, 2025
Benchmarks Report for tracer 🐌Benchmarks for #6677 compared to master:
The following thresholds were used for comparing the benchmark speeds:
Allocation changes below 0.5% are ignored. Benchmark detailsBenchmarks.Trace.ActivityBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.ElasticsearchBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.SpanBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.TraceAnnotationsBenchmark - Same speed ✔️ Same allocations ✔️Raw results
|
andrewlock
added a commit
that referenced
this pull request
Feb 18, 2025
…ts for the NuGet packages (#6678) ## Summary of changes - Pack the trimming file into the _Datadog.Trace_ NuGet package - Add smoke tests for the _Datadog.Trace_ and _Datadog.Trace.Trimming_ NuGet packages - Remove the unnecessary sdk.props file from _Datadog.Trace.Trimming_ ## Reason for change When users are building trimmed (non-aot) apps, they need to reference our trimming file, which is currently exposed via the _Datadog.Trace.Trimming_ NuGet package. We created this as a separate package, because we didn't want people to have to reference the _Datadog.Trace_ NuGet if they didn't need it. However, there's not really a good reason to force people to _have_ to reference another NuGet package if they are _already_ referencing _Datadog.Trace_. And there's no harm in including the trimming file for non-trimmed apps, it has no impact. ## Implementation details - Pack the same trimming.XML file into the _Datadog.Trace_ NuGet package - Include a _Datadog.Trace.props_ file to automatically reference the trimmer file when the NuGet is referenced - Added smoke tests that confirm the _NuGet_ packaging is correct for both the new package, and the existing package - We already had "integration" tests that confirm the trimmer file works, but we didn't have any tests that were using the NuGet package, to confirm the `props` are correctly added etc. - Removed the `sdk/sdk.props` file from _Datadog.Trace.Trimming_ as it's unused. This file is only useful if you're creating a "[custom .NET SDK](https://learn.microsoft.com/en-us/visualstudio/msbuild/how-to-use-project-sdk?view=vs-2022#use-the-sdk-attribute-on-the-project-element)" to reference in the `<Project SDK="Some.SDK">` element (which we're not). ## Test coverage Added smoke tests to confirm both the _Datadog.Trace_ and _Datadog.Trace.Trimming_ packages work as expected. For now, these are: - `linux-x64`/`linux-musl-x64` only - `net8.0`/`net9.0` only We could expand that further if we want, but I'm not massively inclined to do so - we primarily want to ensure the _packages_ are valid, and we don't really need to run that across all the different platforms IMO. Plus it will just increase the chance of flake. ## Other details As part of this, noticed that crash tracking didn't work. - Disabled crash tracking in these tests for now - #6677 was raised in response - Ideally we should merge this, and then re-enable the crash tracking tests in that PR I've kept these in the "standard, every PR" set of smoke tests for now, but maybe we should move them to the master-only set? 🤷♂️ --------- Co-authored-by: Kevin Gosse <[email protected]>
andrewlock
force-pushed
the
kevin/wer_single_file
branch
2 times, most recently
from
6b51050 to
f488df3
Compare
kevingosse
force-pushed
the
kevin/wer_single_file
branch
from
f488df3 to
a48a732
Compare
Datadog ReportAll test runs ✅ 2 Total Test Services: 0 Failed, 2 Passed Test Services
|
ddyurchenko
pushed a commit
that referenced
this pull request
Mar 31, 2025
…ts for the NuGet packages (#6678) ## Summary of changes - Pack the trimming file into the _Datadog.Trace_ NuGet package - Add smoke tests for the _Datadog.Trace_ and _Datadog.Trace.Trimming_ NuGet packages - Remove the unnecessary sdk.props file from _Datadog.Trace.Trimming_ ## Reason for change When users are building trimmed (non-aot) apps, they need to reference our trimming file, which is currently exposed via the _Datadog.Trace.Trimming_ NuGet package. We created this as a separate package, because we didn't want people to have to reference the _Datadog.Trace_ NuGet if they didn't need it. However, there's not really a good reason to force people to _have_ to reference another NuGet package if they are _already_ referencing _Datadog.Trace_. And there's no harm in including the trimming file for non-trimmed apps, it has no impact. ## Implementation details - Pack the same trimming.XML file into the _Datadog.Trace_ NuGet package - Include a _Datadog.Trace.props_ file to automatically reference the trimmer file when the NuGet is referenced - Added smoke tests that confirm the _NuGet_ packaging is correct for both the new package, and the existing package - We already had "integration" tests that confirm the trimmer file works, but we didn't have any tests that were using the NuGet package, to confirm the `props` are correctly added etc. - Removed the `sdk/sdk.props` file from _Datadog.Trace.Trimming_ as it's unused. This file is only useful if you're creating a "[custom .NET SDK](https://learn.microsoft.com/en-us/visualstudio/msbuild/how-to-use-project-sdk?view=vs-2022#use-the-sdk-attribute-on-the-project-element)" to reference in the `<Project SDK="Some.SDK">` element (which we're not). ## Test coverage Added smoke tests to confirm both the _Datadog.Trace_ and _Datadog.Trace.Trimming_ packages work as expected. For now, these are: - `linux-x64`/`linux-musl-x64` only - `net8.0`/`net9.0` only We could expand that further if we want, but I'm not massively inclined to do so - we primarily want to ensure the _packages_ are valid, and we don't really need to run that across all the different platforms IMO. Plus it will just increase the chance of flake. ## Other details As part of this, noticed that crash tracking didn't work. - Disabled crash tracking in these tests for now - #6677 was raised in response - Ideally we should merge this, and then re-enable the crash tracking tests in that PR I've kept these in the "standard, every PR" set of smoke tests for now, but maybe we should move them to the master-only set? 🤷♂️ --------- Co-authored-by: Kevin Gosse <[email protected]>
ddyurchenko
pushed a commit
that referenced
this pull request
Mar 31, 2025
## Summary of changes Register the crashtracking WER handler even if we fail to unregister the .NET one. ## Reason for change For crashtracking to work, we need to unregister the WER handler registered by .NET, because it claims the crash and we won't be called. To unregister it, we need two information: the path to `coreclr.dll`, and its address in memory. It fails in single-file apps because there is no `coreclr.dll`. However, it also means that the .NET registration is invalid, so we will still be called if we register our own handler. Regardless, even if the .NET WER handler registration is correct, it doesn't hurt to still register our own. ## Test coverage Tested on my machine. I don't think it's worth the hassle of publishing Samples.Console as single file. --------- Co-authored-by: Andrew Lock <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Register the crashtracking WER handler even if we fail to unregister the .NET one.
Reason for change
For crashtracking to work, we need to unregister the WER handler registered by .NET, because it claims the crash and we won't be called. To unregister it, we need two information: the path to
coreclr.dll, and its address in memory. It fails in single-file apps because there is nocoreclr.dll. However, it also means that the .NET registration is invalid, so we will still be called if we register our own handler.Regardless, even if the .NET WER handler registration is correct, it doesn't hurt to still register our own.
Test coverage
Tested on my machine. I don't think it's worth the hassle of publishing Samples.Console as single file.