-
Notifications
You must be signed in to change notification settings - Fork 383
Enable dotnet-trace and dotnet-counters collection from startup #1635
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
Conversation
src/Tools/Common/ReversedServerHelpers/ReversedServerHelpers.cs
Outdated
Show resolved
Hide resolved
sdmaclea
left a comment
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.
Quick review. Look at overall code structure.
We may want to consider an option to write the redirected stdout & stderr to files. That would reduce the subset of target apps which couldn't work with this approach.
| <Compile Include="..\Common\CommandExtensions.cs" Link="CommandExtensions.cs" /> | ||
| <Compile Include="..\Common\Commands\ProcessStatus.cs" Link="ProcessStatus.cs" /> | ||
| <Compile Include="..\Common\Commands\Utils.cs" Link="Utils.cs" /> | ||
| <Compile Include="..\Common\ReversedServerHelpers\ReversedServerHelpers.cs" Link="ReversedServerHelpers.cs" /> |
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.
Nit indentation
josalem
left a comment
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.
First pass of comments. I love seeing all these 5.0 investments come together to light up this functionality 😄
src/Microsoft.Diagnostics.NETCore.Client/DiagnosticsClient/EventPipeSession.cs
Outdated
Show resolved
Hide resolved
src/Tools/Common/ReversedServerHelpers/ReversedServerHelpers.cs
Outdated
Show resolved
Hide resolved
src/Tools/Common/ReversedServerHelpers/ReversedServerHelpers.cs
Outdated
Show resolved
Hide resolved
src/Tools/Common/ReversedServerHelpers/ReversedServerHelpers.cs
Outdated
Show resolved
Hide resolved
noahfalk
left a comment
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.
Looks good modulo a little suggested refactoring and handling for the "--" argument : )
src/Tools/Common/ReversedServerHelpers/ReversedServerHelpers.cs
Outdated
Show resolved
Hide resolved
src/Tools/Common/ReversedServerHelpers/ReversedServerHelpers.cs
Outdated
Show resolved
Hide resolved
| string parsedCommandName = parseResult.CommandResult.Command.Name; | ||
| if (parsedCommandName == "monitor" || parsedCommandName == "collect") | ||
| { | ||
| IReadOnlyCollection<string> unparsedTokens = parseResult.UnparsedTokens; |
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.
System.CommandLine parses the "--" token but not the content that follows it?
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.
It does, but we had an issue where it ate things like spaces in the arguments and parsed it as separate args, so now I'm using this as a way to check if the command has unparsed arguments, and parse the args manually in ProcessLauncher.
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.
Have you talked with @jonsequitur at all about the "--" handling? I am fine if we need workarounds but IMO "--" is a common enough convention that I would hope System.CommandLine gains some support for it in the future.
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 haven't yet. I'll post an issue over in the command-line-api repo.
| .AddCommand(MonitorCommand()) | ||
| .AddCommand(CollectCommand()) | ||
| .AddCommand(ListCommand()) | ||
| .AddCommand(ProcessStatusCommandHandler.ProcessStatusCommand("Lists the dotnet processes that can be monitored")) |
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.
This comment should go on the CounterList() above but github doesn't let me mark it there.
The "--" argument has a standardized meaning that it terminates option processing. Some examples:
LLDB, Posix convention, and Python argument parsing library.
I think we should be using it the same way other tools do, but at the moment we aren't.
I thought I had commented on this before but it turned out I never hit submit so you never saw those comments
#1522 (comment)
#1522 (comment)
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.
Thanks, I added the --counters argument and made it work only when we're attaching at startup so that we're backwards compatible.
i.e. dotnet-counters collect --counters System.Runtime,Microsoft.AspNetCore.Hosting -- myapp.exe will work now.
The old syntax dotnet-counters collect System.Runtime Microsoft.AspNetCore.Hosting --name myapp.exe also still works.
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 the --counters argument and made it work only when we're attaching at startup
I'd suggest making the --counters argument work all the time and document it as the intended way people should use the tool. Specifying counters as positional arguments becomes the thing that we allow when --process-id is specified to preserve back-compat for people running it via the previous command-line syntax.
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.
Done :) Example output below:
E:\diagnostics>dotnet-counters collect --process-id 83380 System.Runtime
[Warning] counter.csv already exists. This file will be overwritten.
Starting a counter session. Press Q to quit.
File saved to counter.csv
E:\diagnostics>dotnet-counters collect --process-id 83380 --counters System.Runtime
[Warning] counter.csv already exists. This file will be overwritten.
Starting a counter session. Press Q to quit.
File saved to counter.csv
E:\diagnostics>dotnet-counters collect --counters System.Runtime -- \temp\testapp\bin\Debug\net5.0\testapp.exe
[Warning] counter.csv already exists. This file will be overwritten.
Starting a counter session. Press Q to quit.
File saved to counter.csv
This PR enables dotnet-trace and dotnet-counters to attach to a target application at startup using the new reversed diagnostics transport feature in .NET 5.0 runtime. Some example commands that can be used to invoke this look like the following:
dotnet-trace collect:
dotnet-trace collect --providers Microsoft-Windows-DotNETRuntime:0x40:5 -- C:\Users\suwhang\targetapp\targetapp.exedotnet-counters collect:
dotnet-counters collect -- C:\Users\suwhang\targetapp\targetapp.exe arg1 arg2dotnet-counters monitor:
dotnet-counters monitor System.Runtime My-Custom-EventSource --refresh-interval 5 -- C:\Users\suwhang\targetapp\targetapp.exe arg1Using this, we are now able to collect some very early startup events emitted from the runtime, as shown below in the screenshot of a trace collected from running

dotnet-trace collecton a .NET 5 hello world console app:The target application is launched as a child process, and its stdin/stdout/stderr won't be hooked into the console. The console is entirely for the tool only, and once the user stops the tool, the target application exits as well.
For apps that need such scenarios to work (i.e. they need user interaction via stdin/stdout), @noahfalk suggested making a separate workflow (Make a command for dotnet-trace/dotnet-counters to create reversed pipe, user sets the necessary environment variable to start the target app, user resumes the app from dotnet-trace/dotnet-counters, letting the users have control of both the tool and the target app). I think this is a reasonable approach to go with, and make changes as we hear feedback from users.
Following this PR will be the
dotnet-trace monitorfeature built on top of this PR, which will be focused on lighting up specific diagnostic scenarios using the runtime provider.cc @tommcdon @noahfalk @josalem @shirhatti