[Crashtracking] Implement support for Windows#6088
Merged
Conversation
kevingosse
force-pushed
the
kevin/crashtracking_windows
branch
from
September 27, 2024 11:51
5afcd6f to
92d35d8
Compare
Datadog ReportBranch report: ✅ 0 Failed, 368206 Passed, 2357 Skipped, 18h 1m 26.83s Total Time New Flaky Tests (2)
|
Member
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 (6088) - mean (71ms) : 67, 74
. : milestone, 71,
master - mean (70ms) : 68, 73
. : milestone, 70,
section CallTarget+Inlining+NGEN
This PR (6088) - mean (1,108ms) : 1089, 1126
. : milestone, 1108,
master - mean (1,110ms) : 1090, 1131
. : milestone, 1110,
gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6088) - mean (110ms) : 107, 113
. : milestone, 110,
master - mean (109ms) : 106, 112
. : milestone, 109,
section CallTarget+Inlining+NGEN
This PR (6088) - mean (775ms) : 760, 790
. : milestone, 775,
master - mean (775ms) : 755, 795
. : milestone, 775,
gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6088) - mean (93ms) : 91, 96
. : milestone, 93,
master - mean (93ms) : 89, 97
. : milestone, 93,
section CallTarget+Inlining+NGEN
This PR (6088) - mean (730ms) : 714, 746
. : milestone, 730,
master - mean (732ms) : 717, 747
. : milestone, 732,
gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6088) - mean (191ms) : 186, 195
. : milestone, 191,
master - mean (190ms) : 187, 192
. : milestone, 190,
section CallTarget+Inlining+NGEN
This PR (6088) - mean (1,200ms) : 1182, 1218
. : milestone, 1200,
master - mean (1,201ms) : 1178, 1224
. : milestone, 1201,
gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6088) - mean (276ms) : 271, 282
. : milestone, 276,
master - mean (275ms) : 272, 279
. : milestone, 275,
section CallTarget+Inlining+NGEN
This PR (6088) - mean (940ms) : 917, 963
. : milestone, 940,
master - mean (939ms) : 924, 955
. : milestone, 939,
gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (6088) - mean (264ms) : 261, 268
. : milestone, 264,
master - mean (265ms) : 261, 269
. : milestone, 265,
section CallTarget+Inlining+NGEN
This PR (6088) - mean (922ms) : 903, 942
. : milestone, 922,
master - mean (926ms) : 907, 946
. : milestone, 926,
|
Member
Benchmarks Report for tracer 🐌Benchmarks for #6088 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
|
Member
Throughput/Crank Report ⚡Throughput results for AspNetCoreSimpleController comparing the following branches/commits: Cases where throughput results for the PR are worse than latest master (5% drop or greater), results are shown in red. Note that these results are based on a single point-in-time result for each branch. For full results, see one of the many, many dashboards! gantt
title Throughput Linux x64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (6088) (11.064M) : 0, 11063518
master (11.009M) : 0, 11009316
benchmarks/2.9.0 (11.081M) : 0, 11080577
section Automatic
This PR (6088) (7.358M) : 0, 7358411
master (7.392M) : 0, 7391896
benchmarks/2.9.0 (7.732M) : 0, 7732233
section Trace stats
master (7.658M) : 0, 7657594
section Manual
master (10.944M) : 0, 10944483
section Manual + Automatic
This PR (6088) (6.649M) : 0, 6649316
master (6.786M) : 0, 6785950
section DD_TRACE_ENABLED=0
master (10.229M) : 0, 10229067
gantt
title Throughput Linux arm64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (6088) (9.626M) : 0, 9625677
master (9.438M) : 0, 9438257
benchmarks/2.9.0 (9.798M) : 0, 9798067
section Automatic
This PR (6088) (6.409M) : 0, 6409158
master (6.590M) : 0, 6590052
section Trace stats
master (7.018M) : 0, 7018387
section Manual
master (9.760M) : 0, 9760011
section Manual + Automatic
This PR (6088) (5.989M) : 0, 5988723
master (6.122M) : 0, 6122138
section DD_TRACE_ENABLED=0
master (8.792M) : 0, 8791745
gantt
title Throughput Windows x64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (6088) (10.144M) : 0, 10143954
master (9.927M) : 0, 9927076
benchmarks/2.9.0 (10.067M) : 0, 10067315
section Automatic
This PR (6088) (6.652M) : 0, 6651784
master (6.595M) : 0, 6594814
benchmarks/2.9.0 (7.552M) : 0, 7552193
section Trace stats
master (7.253M) : 0, 7253184
section Manual
master (10.031M) : 0, 10030951
section Manual + Automatic
This PR (6088) (6.292M) : 0, 6291909
master (6.128M) : 0, 6128464
section DD_TRACE_ENABLED=0
master (9.284M) : 0, 9283843
|
kevingosse
force-pushed
the
kevin/crashtracking_windows
branch
from
September 30, 2024 09:07
1623a8a to
8d064ca
Compare
kevingosse
changed the title
added 2 commits
September 30, 2024 15:47
kevingosse
marked this pull request as ready for review
September 30, 2024 14:07
chrisnas
reviewed
Oct 1, 2024
added 2 commits
October 1, 2024 15:46
gleocadie
approved these changes
Oct 2, 2024
gleocadie
left a comment
gleocadie
left a comment
Collaborator
There was a problem hiding this comment.
little comment there and there but overall LGTM
andrewlock
approved these changes
Oct 2, 2024
andrewlock
left a comment
andrewlock
left a comment
Member
There was a problem hiding this comment.
LGTM, but I'm 100% trusting the C++ is ok 🙈
Member
There was a problem hiding this comment.
Mostly unrelated, but we should probably update the codeowners for all the crashreporting files to be shared between profiler and tracing do you think?
bouwkast
approved these changes
Oct 2, 2024
bouwkast
left a comment
bouwkast
left a comment
Collaborator
There was a problem hiding this comment.
Looks good to me (didn't review the native stuff, but looks like others did)
kevingosse
pushed a commit
that referenced
this pull request
Oct 3, 2024
## Summary of changes Register `Datadog.Trace.ClrProfiler.Native.dll` in the `HKLM\Software\Microsoft\Windows\Windows Error Reporting\RuntimeExceptionHelperModules` key. ## Reason for change This is needed by crashtracking. If the value is absent, the tracer will create it in HKCU (the instrumented application is unlikely to have permission to write to HKLM). However, registering the crash handler in HKCU is only supported in somewhat versions of Windows (since Windows 10 2004). So we should rely on HKLM whenever possible. ## Other details Depends on #6088 --------- Co-authored-by: Andrew Lock <[email protected]>
agocs
pushed a commit
that referenced
this pull request
Oct 8, 2024
## Summary of changes Add crashtracking support for Windows. At the moment, only x64 is supported. Supporting x86 would require to include a 32-bit version of `dd-dotnet` in the package. ## Reason for change Until now, crashtracking only supported Linux. There are many crashes (due to either netfx or IIS) that only occur on Windows. ## Implementation details Crashtracking on Windows relies on [WER](https://learn.microsoft.com/en-us/windows/win32/wer/windows-error-reporting). When the native loader is loaded, we use `WerRegisterRuntimeExceptionModule` to register it as a crash handler. Note that .NET also registers a crash handler, and they're invoked by order of registration. To be invoked first, we unregister the .NET crash handler, register our own, then re-register the .NET crash handler. The .NET crash handler is in the DAC (mscordawks.dll for .NET Framework, mscordaccore.dll for .NET Core). The context argument is set to the address of `clr.dll`/`coreclr.dll` in memory (this is important because `WerUnregisterRuntimeExceptionModule` fails if the context value is different). When a crash occurs, Windows freezes the process and launches `WerFault.exe`. In WerFault, the crash handler is loaded and given a chance to inspect the crash (using the exported `OutOfProcessExceptionEventCallback` function). We invoke `dd-dotnet` to inspect the process and generate the crash report. The environment variables of the crashing process are not propagated to `WerFault.exe`. Because we need them (for instance, to know the location of the agent), we make a copy of all the `DD_*` environment variables and give the address of that copy as the "context" argument of `WerRegisterRuntimeExceptionModule`. When `OutOfProcessExceptionEventCallback` is invoked in `WerFault.exe`, we use `ReadProcessMemory` to read the saved environment variables from the crashing process, and set them for `dd-dotnet`. ## Test coverage Reusing the existing crashtracking tests. ## Other details More information on WER: https://minidump.net/windows-error-reporting/
agocs
pushed a commit
that referenced
this pull request
Oct 8, 2024
## Summary of changes Register `Datadog.Trace.ClrProfiler.Native.dll` in the `HKLM\Software\Microsoft\Windows\Windows Error Reporting\RuntimeExceptionHelperModules` key. ## Reason for change This is needed by crashtracking. If the value is absent, the tracer will create it in HKCU (the instrumented application is unlikely to have permission to write to HKLM). However, registering the crash handler in HKCU is only supported in somewhat versions of Windows (since Windows 10 2004). So we should rely on HKLM whenever possible. ## Other details Depends on #6088 --------- Co-authored-by: Andrew Lock <[email protected]>
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
5 participants
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
Add crashtracking support for Windows.
At the moment, only x64 is supported. Supporting x86 would require to include a 32-bit version of
dd-dotnetin the package.Reason for change
Until now, crashtracking only supported Linux. There are many crashes (due to either netfx or IIS) that only occur on Windows.
Implementation details
Crashtracking on Windows relies on WER.
When the native loader is loaded, we use
WerRegisterRuntimeExceptionModuleto register it as a crash handler. Note that .NET also registers a crash handler, and they're invoked by order of registration. To be invoked first, we unregister the .NET crash handler, register our own, then re-register the .NET crash handler.The .NET crash handler is in the DAC (mscordawks.dll for .NET Framework, mscordaccore.dll for .NET Core). The context argument is set to the address of
clr.dll/coreclr.dllin memory (this is important becauseWerUnregisterRuntimeExceptionModulefails if the context value is different).When a crash occurs, Windows freezes the process and launches
WerFault.exe. In WerFault, the crash handler is loaded and given a chance to inspect the crash (using the exportedOutOfProcessExceptionEventCallbackfunction). We invokedd-dotnetto inspect the process and generate the crash report.The environment variables of the crashing process are not propagated to
WerFault.exe. Because we need them (for instance, to know the location of the agent), we make a copy of all theDD_*environment variables and give the address of that copy as the "context" argument ofWerRegisterRuntimeExceptionModule. WhenOutOfProcessExceptionEventCallbackis invoked inWerFault.exe, we useReadProcessMemoryto read the saved environment variables from the crashing process, and set them fordd-dotnet.Test coverage
Reusing the existing crashtracking tests.
Other details
More information on WER: https://minidump.net/windows-error-reporting/
https://datadoghq.atlassian.net/browse/APMSP-1388