-
Notifications
You must be signed in to change notification settings - Fork 356
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add enable crash report flag to WriteDump client API #2715
Conversation
Uses new write dump command supported by 6.0 runtime to pass a set of flags instead of just the logging enabled bool. Issue: dotnet#2698
Runtime repro PR: dotnet/runtime#60995 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, modulo the few comments. Thanks Mike!
src/Microsoft.Diagnostics.NETCore.Client/DiagnosticsClient/DiagnosticsClient.cs
Show resolved
Hide resolved
@@ -70,6 +70,14 @@ public static Task<int> Main(string[] args) | |||
Argument = new Argument<bool>(name: "diag") | |||
}; | |||
|
|||
private static Option CrashReportOption() => | |||
new Option( | |||
alias: "--crashreport", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an issue filed or PR open to add the docs for this new feature to docs.microsoft.com?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doc issue: #2717
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That issue covers the APIs, but doesn't mention dotnet-dump the tool and the new argument being added here. Can you either expand the scope of the issue or add a new one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a comment about the new option to the doc issue.
@@ -86,6 +86,12 @@ public int Collect(IConsole console, int processId, string output, bool diag, Du | |||
|
|||
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) | |||
{ | |||
if (crashreport) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this not supported on Windows? I thought crash reports were possible on Windows. I seen many programs use such a thing before there too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The crash report referenced here is a special json file that the Linux and MacOS tool createdump tool generates. The Windows "createdump" just calls the OS's MiniDumpWriteDump to generate dump and doesn't have a way to generate this json file. On Linux and MacOS createdump explicitly gathers the runtime process's state and writes both the core dump and the crash report json from that data.
Uses new write dump command supported by 6.0 runtime to pass a set of flags instead
of just the logging enabled bool.
Issue: #2698