-
Notifications
You must be signed in to change notification settings - Fork 383
[dotnet-trace] Add collect-linux verb #5570
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
base: main
Are you sure you want to change the base?
Conversation
src/Tools/dotnet-trace/CommandLine/Commands/CollectLinuxCommand.cs
Outdated
Show resolved
Hide resolved
src/Tools/dotnet-trace/CommandLine/Commands/CollectLinuxCommand.cs
Outdated
Show resolved
Hide resolved
src/Tools/dotnet-trace/CommandLine/Commands/CollectLinuxCommand.cs
Outdated
Show resolved
Hide resolved
src/Tools/dotnet-trace/CommandLine/Commands/CollectLinuxCommand.cs
Outdated
Show resolved
Hide resolved
c09b095
to
0989d21
Compare
Extensions houses provider composition helpers, instead of literal extension methods for strings. Centralize all provider parsing + composition logic and keep Profile as a data container.
76e65e1
to
61b2569
Compare
61b2569
to
c8e53ea
Compare
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 is looking pretty good. Aside from some small comments inline a few broader things:
- Historically we relied on manual testing to keep these tools operating well but now that we have less time available from the testers I think we need to improve our automated testing. Partly this is to ensure we aren't inadvertently changing the original 'collect' verb and partly to ensure going forward the 'collect-linux' behavior doesn't regress either. I think the best way to do this would be:
- Open a new PR that we'll check in first containing some basic tests of the existing collect verb.
- Commit this PR 2nd and all the tests in the 1st PR should continue to pass. This ensures we didn't change anything unintended.
To do the testing we probably need to create some small interface shims. We already have an IConsole interface defined that could be moved to the shared Common folder. We could also create a small interface around the DiagnosticsClient.EventPipeCollect() API so that a test can return some dummy data in a stream instead. dotnet-counters has some example tests that show how we can run some code and then confirm the console output is what we expect. In this case I imagine we'd be running the Collect() function and giving some chosen input arguments.
- I think there is a bit more adjustment to be done on some of the output text, but it should be fine to get this one in first, then tweak afterwards in some 3rd PR.
src/Tools/dotnet-trace/CommandLine/Commands/CollectLinuxCommand.cs
Outdated
Show resolved
Hide resolved
src/Tools/dotnet-trace/CommandLine/Commands/CollectLinuxCommand.cs
Outdated
Show resolved
Hide resolved
src/Tools/dotnet-trace/CommandLine/Commands/CollectLinuxCommand.cs
Outdated
Show resolved
Hide resolved
src/Tools/dotnet-trace/CommandLine/Commands/CollectLinuxCommand.cs
Outdated
Show resolved
Hide resolved
789fb2b
to
1f75125
Compare
Record-Trace has a bug for multithreaded .NET apps when specifying a process to trace. microsoft/one-collect#192 Until that is resolved, the scenario is broken.
The failure is unrelated I think it's dotnet/runtime#120317 even after the refactor #5585 |
private static Stopwatch s_stopwatch = new(); | ||
private static LineRewriter s_rewriter = new() { LineToClear = Console.CursorTop - 1 }; | ||
private static LineRewriter s_rewriter; | ||
private static bool s_printingStatus; |
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.
If testing caused multiple instances of CollectLinuxCommandHandler to run in parallel, would we want those instances to be sharing any of these static fields? I'm guessing we'd want all these fields to be instanced 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.
Changed to non-static. From some learning, I'm seeing that parallelization occurs at the test class level, so it doesn't seem like the theory members would run in parallel.
LinuxHeader, | ||
LinuxProfile("cpu-sampling"), | ||
"" | ||
} |
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.
Shouldn't there be some output about where the test output is being written to and telling the user to press Enter to stop the trace?
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 output file to Collect-linux, thanks for the catch. As per the press enter/ctrl+c message, normally that is part of the status printing.
For collect-linux, I added the line rewriting/status printing to the callback, so to mock that for the tests, extended the RecordTraceInvoker test seam to invoke the callback once with the progress output type, causing the status+ Press Enter/Ctrl+C to emit once.
eff3e5f
to
40c5905
Compare
Implements dotnet/docs#47894
Following the addition of emitting native runtime and custom EventSource events as user_events through dotnet/runtime#115265 and the public release of https://github.com/microsoft/one-collect which supports collecting both .NET user_events and Linux perf events into a single .nettrace file,
dotnet-trace
will support a new verb,collect-linux
, that wraps aroundrecord-trace
.This PR does the following:
collect-linux
verb and serializes a subset ofdotnet-trace collect
options in addition to acollect-linux
specific--perf-events
option intorecord-trace
args. (see [Diagnostics][dotnet-trace] Add collect-linux verb docs#47894 for overarching details)dotnet-trace
cpu-sampling
->dotnet-common
+dotnet-sampled-thread-time
) and addscollect-linux
specific profileslist-profiles
verb with revamped profiles + multiline description formattingEventPipeProvider
composition logic (MergeProfileAndProviders
+ToProviders
->ComputeProviderConfig
) and renameExtensions.cs
->ProviderUtils.cs
ProviderParsing.cs
->ProviderCompositionTests.cs
)collect
logic + expanddotnet-trace
common optionsTesting
dotnet-trace collect-linux
On Linux
collect-linux
collect-linux --help
`collect-linux` without elevated privileges
`collect-linux` with elevated privileges
On Windows (and I presume other non-Linux OS):
`collect-linux`
dotnet-trace list-profiles
`list-profiles`