Skip to content

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.

Sign up for GitHub

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

Merged
merged 31 commits into from
Oct 13, 2020
Merged

Enable dotnet-trace and dotnet-counters collection from startup #1635

Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
cc40349
add run to counters
sywhang Sep 1, 2020
f9d96f7
Add trace run command
sywhang Sep 1, 2020
51dca9b
use acceptasync instead of sleeping... :p
sywhang Sep 2, 2020
907ada3
Counter run works now
sywhang Sep 2, 2020
4eaba69
Fix incorrect cpu-usage descriptor
sywhang Sep 23, 2020
7a873d0
Merge remote-tracking branch 'upstream/master'
sywhang Oct 2, 2020
e4e463e
Merge remote-tracking branch 'upstream/master'
sywhang Oct 5, 2020
c5e542d
Merge remote-tracking branch 'upstream/master' into dev/suwhang/tools…
sywhang Oct 5, 2020
643cc1c
use processlauncher to handle --
sywhang Oct 6, 2020
8df4435
Merge branch 'master' into dev/suwhang/tools-run
sywhang Oct 6, 2020
2229e45
remove unused code
sywhang Oct 6, 2020
583fcef
make dotnet-counters collect attachable at startup
sywhang Oct 7, 2020
8529324
cleanup
sywhang Oct 7, 2020
cd96473
more refactoring
sywhang Oct 7, 2020
0878632
Fix misbehaviors when stopping counters/child proc
sywhang Oct 7, 2020
c1b6003
dotnet-trace collect can attach at startup now
sywhang Oct 7, 2020
8c01c55
Fix IOException being thrown for Stop command
sywhang Oct 7, 2020
c451374
Defer ResumeRuntime after we create EventPipeSession
sywhang Oct 7, 2020
02451f6
Change the default behavior to not redirect stdin/stdout/stderr
sywhang Oct 7, 2020
f381430
terminate child proc upon exit
sywhang Oct 7, 2020
154bb17
remove useless code
sywhang Oct 7, 2020
70ebf8d
gracefully handle older versions that can't connect to reversed server.
sywhang Oct 8, 2020
8e80aa4
terminate child proc
sywhang Oct 8, 2020
7a16985
Fix build err'
sywhang Oct 8, 2020
326cdd3
code review feedback
sywhang Oct 8, 2020
bcdae64
Code review feedback
sywhang Oct 9, 2020
b32f366
Handle escape characters in argument string to child process
sywhang Oct 12, 2020
27a2565
fix build failure
sywhang Oct 12, 2020
2500d03
fix build failure
sywhang Oct 13, 2020
e25dd08
Remove unused code
sywhang Oct 13, 2020
ae4eec3
Enable --counters option for default scenario as well
sywhang Oct 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion src/Microsoft.Diagnostics.NETCore.Client/DiagnosticsClient/EventPipeSession.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,16 @@ public void Stop()
Debug.Assert(_sessionId > 0);

byte[] payload = BitConverter.GetBytes(_sessionId);
var response = IpcClient.SendMessage(_endpoint, new IpcMessage(DiagnosticsServerCommandSet.EventPipe, (byte)EventPipeCommandId.StopTracing, payload));
IpcMessage response;
try
{
response = IpcClient.SendMessage(_endpoint, new IpcMessage(DiagnosticsServerCommandSet.EventPipe, (byte)EventPipeCommandId.StopTracing, payload));
}
// On non-abrupt exits (i.e. the target process has already exited and pipe is gone, it sending Stop command will fail).
catch (IOException)
{
throw new ServerNotAvailableException("Could not send Stop command. The target process may have exited.");
}

switch ((DiagnosticsServerResponseId)response.Header.CommandId)
{
Expand Down
2 changes: 2 additions & 0 deletions src/Microsoft.Diagnostics.NETCore.Client/Microsoft.Diagnostics.NETCore.Client.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,7 @@
<InternalsVisibleTo Include="DotnetMonitor.UnitTests" />
<InternalsVisibleTo Include="Microsoft.Diagnostics.Monitoring" />
<InternalsVisibleTo Include="Microsoft.Diagnostics.NETCore.Client.UnitTests" />
<InternalsVisibleTo Include="dotnet-counters" />
<InternalsVisibleTo Include="dotnet-trace" />
</ItemGroup>
</Project>
112 changes: 112 additions & 0 deletions src/Tools/Common/ReversedServerHelpers/ReversedServerHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using Microsoft.Diagnostics.NETCore.Client;
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
using System.Threading;
using System.Threading.Tasks;

namespace Microsoft.Internal.Common.Utils
{
// <summary>
// ProcessLauncher is a child-process launcher for "diagnostics tools at startup" scenarios
// It launches the target process at startup and passes its processId to the corresponding Command handler.
// </summary>
internal class ProcessLauncher
{
private Process _childProc;
public ManualResetEvent HasExited;

internal static ProcessLauncher Launcher = new ProcessLauncher();

public void PrepareChildProcess(List<string> args)
{
_childProc = new Process();
_childProc.StartInfo.FileName = args[0];
_childProc.StartInfo.Arguments = String.Join(" ", args.GetRange(1, args.Count - 1));
}

public bool HasChildProc
{
get
{
return _childProc != null;
}
}

public Process ChildProc
{
get
{
return _childProc;
}
}
public bool Start(string diagnosticTransportName)
{
HasExited = new ManualResetEvent(false);
_childProc.StartInfo.UseShellExecute = false;
_childProc.StartInfo.RedirectStandardOutput = true;
_childProc.StartInfo.RedirectStandardError = true;
_childProc.StartInfo.RedirectStandardInput = true;
_childProc.StartInfo.Environment.Add("DOTNET_DiagnosticPorts", $"{diagnosticTransportName},suspend");
_childProc.Exited += new EventHandler(OnExited);
try
{
_childProc.Start();
}
catch (Exception e)
{
Console.WriteLine($"Cannot start target process: {_childProc.StartInfo.FileName}");
Console.WriteLine(e.ToString());
return false;
}
return true;
}

private static void OnExited(object sender, EventArgs args)
{
ProcessLauncher.Launcher.HasExited.Set();
}
}

// <summary>
// This class acts a helper class for building a DiagnosticsClient instance
// </summary>
internal class ReversedDiagnosticsClientBuilder
{
private static string GetRandomTransportName() => "DOTNET_TOOL_REVERSE_TRANSPORT_" + Path.GetRandomFileName();
private string diagnosticTransportName;
private ReversedDiagnosticsServer server;
private ProcessLauncher _childProcLauncher;

public ReversedDiagnosticsClientBuilder(ProcessLauncher childProcLauncher)
{
diagnosticTransportName = GetRandomTransportName();
_childProcLauncher = childProcLauncher;
server = new ReversedDiagnosticsServer(diagnosticTransportName);
server.Start();
}

// <summary>
// Starts the child process and returns the diagnostics client once the child proc connects to the reversed diagnostics pipe.
// The callee needs to resume the diagnostics client at appropriate time.
// </summary>
public DiagnosticsClient Build(int timeoutInSec)
{
if (!_childProcLauncher.HasChildProc)
{
throw new InvalidOperationException("Must have a valid child process to launch.");
}
if (!_childProcLauncher.Start(diagnosticTransportName))
{
throw new InvalidOperationException("Failed to start dotnet-counters.");
}
IpcEndpointInfo endpointInfo = server.Accept(TimeSpan.FromSeconds(timeoutInSec));
return new DiagnosticsClient(endpointInfo.Endpoint);
}
}
}
111 changes: 84 additions & 27 deletions src/Tools/dotnet-counters/CounterMonitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,16 +90,24 @@ private void StopMonitor()

public async Task<int> Monitor(CancellationToken ct, List<string> counter_list, IConsole console, int processId, int refreshInterval, string name)
{
if (name != null)
if (!ProcessLauncher.Launcher.HasChildProc)
{
if (processId != 0)
if (name != null)
{
Console.WriteLine("Can only specify either --name or --process-id option.");
return 0;
if (processId != 0)
{
Console.WriteLine("Can only specify either --name or --process-id option.");
return 0;
}
processId = CommandUtils.FindProcessIdWithName(name);
if (processId < 0)
{
return 0;
}
}
processId = CommandUtils.FindProcessIdWithName(name);
if (processId < 0)
else if (processId == 0)
{
Console.WriteLine("Must specify either --name or --process-id option to start monitoring.");
return 0;
}
}
Expand All @@ -112,11 +120,30 @@ public async Task<int> Monitor(CancellationToken ct, List<string> counter_list,
_processId = processId;
_interval = refreshInterval;
_renderer = new ConsoleWriter();
_diagnosticsClient = new DiagnosticsClient(processId);

// Check if we are attaching at startup.
if (ProcessLauncher.Launcher.HasChildProc)
{
ReversedDiagnosticsClientBuilder builder = new ReversedDiagnosticsClientBuilder(ProcessLauncher.Launcher);
try
{
_diagnosticsClient = builder.Build(10);
}
catch (TimeoutException)
{
Console.Error.WriteLine("Unable to start counter session - the target app failed to connect to the diagnostics transport. This may happen if the target application is running .NET Core 3.1 or older versions. Attaching at startup is only available from .NET 5.0 or later.");
if (!ProcessLauncher.Launcher.ChildProc.HasExited)
{
ProcessLauncher.Launcher.ChildProc.Kill();
}
return 0;
}
}
else
{
_diagnosticsClient = new DiagnosticsClient(processId);
}
return await Start();
}

catch (OperationCanceledException)
{
try
Expand All @@ -130,22 +157,30 @@ public async Task<int> Monitor(CancellationToken ct, List<string> counter_list,
}
}


public async Task<int> Collect(CancellationToken ct, List<string> counter_list, IConsole console, int processId, int refreshInterval, CountersExportFormat format, string output, string name)
{
if (name != null)
if (!ProcessLauncher.Launcher.HasChildProc)
{
if (processId != 0)
if (name != null)
{
Console.WriteLine("Can only specify either --name or --process-id option.");
return 0;
if (processId != 0)
{
Console.WriteLine("Can only specify either --name or --process-id option.");
return 0;
}
processId = CommandUtils.FindProcessIdWithName(name);
if (processId < 0)
{
return 0;
}
}
processId = CommandUtils.FindProcessIdWithName(name);
if (processId < 0)
else if (processId == 0)
{
Console.WriteLine("Must specify either --name or --process-id option to start collecting.");
return 0;
}
}

try
{
_ct = ct;
Expand All @@ -154,8 +189,6 @@ public async Task<int> Collect(CancellationToken ct, List<string> counter_list,
_processId = processId;
_interval = refreshInterval;
_output = output;
_diagnosticsClient = new DiagnosticsClient(processId);

if (_output.Length == 0)
{
_console.Error.WriteLine("Output cannot be an empty string");
Expand All @@ -175,19 +208,40 @@ public async Task<int> Collect(CancellationToken ct, List<string> counter_list,
processName = Process.GetProcessById(_processId).ProcessName;
}
catch (Exception) { }
_renderer = new JSONExporter(output, processName); ;
_renderer = new JSONExporter(output, processName);
}
else
{
_console.Error.WriteLine($"The output format {format} is not a valid output format.");
return 0;
}

if (ProcessLauncher.Launcher.HasChildProc)
{
ReversedDiagnosticsClientBuilder builder = new ReversedDiagnosticsClientBuilder(ProcessLauncher.Launcher);
try
{
_diagnosticsClient = builder.Build(10);
}
catch (TimeoutException)
{
Console.Error.WriteLine("Unable to start tracing session - the target app failed to connect to the diagnostics transport. This may happen if the target application is running .NET Core 3.1 or older versions. Attaching at startup is only available from .NET 5.0 or later.");
if (!ProcessLauncher.Launcher.ChildProc.HasExited)
{
ProcessLauncher.Launcher.ChildProc.Kill();
}
return 0;
}
}
else
{
_diagnosticsClient = new DiagnosticsClient(processId);
}
return await Start();
}
catch (OperationCanceledException)
{
}

return 1;
}

Expand Down Expand Up @@ -251,12 +305,6 @@ private string BuildProviderString()

private async Task<int> Start()
{
if (_processId == 0)
{
_console.Error.WriteLine("--process-id is required.");
return 1;
}

string providerString = BuildProviderString();
if (providerString.Length == 0)
{
Expand All @@ -271,6 +319,10 @@ private async Task<int> Start()
try
{
_session = _diagnosticsClient.StartEventPipeSession(Trace.Extensions.ToProviders(providerString), false, 10);
if (ProcessLauncher.Launcher.HasChildProc)
{
_diagnosticsClient.ResumeRuntime();
}
var source = new EventPipeEventSource(_session.EventStream);
source.Dynamic.All += DynamicAllMonitor;
_renderer.EventPipeSourceConnected();
Expand All @@ -282,7 +334,7 @@ private async Task<int> Start()
}
catch (Exception ex)
{
Debug.WriteLine($"[ERROR] {ex.ToString()}");
Console.WriteLine($"[ERROR] {ex.ToString()}");
}
finally
{
Expand Down Expand Up @@ -322,6 +374,11 @@ private async Task<int> Start()
}
}

// If we launched a child proc that hasn't exited yet, terminate it before we exit
if (ProcessLauncher.Launcher.HasChildProc)
{
ProcessLauncher.Launcher.ChildProc.Kill();
}
return await Task.FromResult(0);
}
}
Expand Down
25 changes: 24 additions & 1 deletion src/Tools/dotnet-counters/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// See the LICENSE file in the project root for more information.

using Microsoft.Internal.Common.Commands;
using Microsoft.Internal.Common.Utils;
using Microsoft.Tools.Common;
using System;
using System.Collections.Generic;
Expand All @@ -23,6 +24,15 @@ internal class Program
{
delegate Task<int> ExportDelegate(CancellationToken ct, List<string> counter_list, IConsole console, int processId, int refreshInterval, CountersExportFormat format, string output, string processName);

private IEnumerable<Command> StartupCommands
{
get
{
yield return MonitorCommand();
yield return CollectCommand();
}
}

private static Command MonitorCommand() =>
new Command(
name: "monitor",
Expand All @@ -33,7 +43,7 @@ private static Command MonitorCommand() =>
// Arguments and Options
CounterList(), ProcessIdOption(), RefreshIntervalOption(), NameOption()
};

private static Command CollectCommand() =>
new Command(
name: "collect",
Expand Down Expand Up @@ -145,6 +155,19 @@ private static Task<int> Main(string[] args)
.AddCommand(ProcessStatusCommandHandler.ProcessStatusCommand("Lists the dotnet processes that can be monitored"))
Copy link
Member

@noahfalk noahfalk Oct 9, 2020

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)

Copy link
Contributor Author

@sywhang sywhang Oct 13, 2020 •
edited
Loading

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.

Copy link
Member

@noahfalk noahfalk Oct 13, 2020

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.

Copy link
Contributor Author

@sywhang sywhang Oct 13, 2020

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

.UseDefaults()
.Build();

ParseResult parseResult = parser.Parse(args);
string parsedCommandName = parseResult.CommandResult.Command.Name;
if (parsedCommandName == "monitor" || parsedCommandName == "collect")
{
IReadOnlyCollection<string> unparsedTokens = parseResult.UnparsedTokens;
Copy link
Member

@noahfalk noahfalk Oct 8, 2020

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?

Copy link
Contributor Author

@sywhang sywhang Oct 13, 2020

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.

Copy link
Member

@noahfalk noahfalk Oct 13, 2020

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.

Copy link
Contributor Author

@sywhang sywhang Oct 13, 2020

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.

// If we notice there are unparsed tokens, user might want to attach on startup.
if (unparsedTokens.Count > 0)
{
ProcessLauncher.Launcher.PrepareChildProcess(unparsedTokens.ToList());
}
}

return parser.InvokeAsync(args);
}
}
Expand Down
Loading