Skip to content

[ToolsCommon] Refactor Utils.cs #5622

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 22 commits into from
Nov 6, 2025
+265 −296
Merged

[ToolsCommon] Refactor Utils.cs #5622

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
155b9af
[dotnet-counters] Revert CounterMonitor to using IConsole
mdh1418 Oct 28, 2025
079b98a
[dotnet-trace][collect] Set console through constructor
mdh1418 Oct 28, 2025
3422ff6
[CommandUtils] Enable output redirection
mdh1418 Oct 29, 2025
30d433d
Revert "[CommandUtils] Enable output redirection"
mdh1418 Oct 29, 2025
b116e70
[CommandUtils] Convert FindProcessIdWithName to throw CommandLineErro…
mdh1418 Oct 29, 2025
44f7364
[CommandUtils] Convert ValidateArgumentsForChildProcess to throw
mdh1418 Oct 29, 2025
7a8450a
[CommandUtils] Convert ResolveProcessForAttach to throw
mdh1418 Oct 29, 2025
2e9cab0
Add corresponding compile items
mdh1418 Oct 29, 2025
f7f7704
Fix exception typo
mdh1418 Oct 29, 2025
90f7841
Default DefaultConsole to not use ansi
mdh1418 Oct 29, 2025
7b0aa79
[dotnet-trace] Fix ReturnCode from CommandLineErrorException
mdh1418 Oct 29, 2025
6be25ae
[dotnet-trace] Handle DSRouter Launch Failure ReturnCode
mdh1418 Oct 29, 2025
0e29459
Cleanup ProcessLauncher Start
mdh1418 Oct 29, 2025
e2d336a
Rename CommandLineErrorException
mdh1418 Oct 30, 2025
61d6413
Merge remote-tracking branch 'upstream/main' into enable_command_util…
mdh1418 Oct 31, 2025
2814b51
[CommandUtils] Convert ResolveProcess to throw
mdh1418 Oct 31, 2025
bdeb602
Add ReturnCode to DiagnosticToolException
mdh1418 Nov 4, 2025
cced459
[ToolsCommon] Break out ReturnCode
mdh1418 Nov 5, 2025
56d7451
[ToolsCommon] Break out LineRewriter
mdh1418 Nov 5, 2025
944c6ce
[CommonTools] Rename and move CommandUtils
mdh1418 Nov 5, 2025
f35c402
Cleanup remnant cast
mdh1418 Nov 6, 2025
0184d7c
Merge remote-tracking branch 'upstream/main' into enable_command_util…
mdh1418 Nov 6, 2025
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
4 changes: 2 additions & 2 deletions src/Microsoft.Diagnostics.NETCore.Client/DiagnosticsIpc/IpcEndpointConfig.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ public static IpcEndpointConfig Parse(string config)
string[] parts = config.Split(',');
if (parts.Length > 2)
{
throw new FormatException($"Unknow IPC endpoint config format, {config}.");
throw new FormatException($"Unknown IPC endpoint config format, {config}.");
}

if (string.IsNullOrEmpty(parts[0]))
Expand All @@ -156,7 +156,7 @@ public static IpcEndpointConfig Parse(string config)
}
else
{
throw new FormatException($"Unknow IPC endpoint config keyword, {parts[1]} in {config}.");
throw new FormatException($"Unknown IPC endpoint config keyword, {parts[1]} in {config}.");
}
}
}
Expand Down
42 changes: 15 additions & 27 deletions src/Tools/Common/Commands/Utils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Diagnostics;
using System.Collections.Generic;
using Microsoft.Diagnostics.NETCore.Client;
using Microsoft.Diagnostics.Tools;
using Microsoft.Diagnostics.Tools.Common;

namespace Microsoft.Internal.Common.Utils
Expand All @@ -27,15 +28,14 @@ public static int FindProcessIdWithName(string name)
{
if (commonId != -1)
{
Console.WriteLine("There are more than one active processes with the given name: {0}", name);
return -1;
throw new CommandLineErrorException($"There are more than one active processes with the given name: {name}");
}
commonId = processesWithMatchingName[i].Id;
}
}
if (commonId == -1)
{
Console.WriteLine("There is no active process with the given name: {0}", name);
throw new CommandLineErrorException($"There is no active process with the given name: {name}");
}
return commonId;
}
Expand All @@ -61,15 +61,12 @@ public static int LaunchDSRouterProcess(string dsrouterCommand)
/// <param name="name">name</param>
/// <param name="port">port</param>
/// <returns></returns>
public static bool ValidateArgumentsForChildProcess(int processId, string name, string port)
public static void ValidateArgumentsForChildProcess(int processId, string name, string port)
{
if (processId != 0 || name != null || !string.IsNullOrEmpty(port))
{
Console.WriteLine("None of the --name, --process-id, or --diagnostic-port options may be specified when launching a child process.");
return false;
throw new CommandLineErrorException("None of the --name, --process-id, or --diagnostic-port options may be specified when launching a child process.");
}

return true;
}

/// <summary>
Expand All @@ -83,64 +80,55 @@ public static bool ValidateArgumentsForChildProcess(int processId, string name,
/// <param name="dsrouter">dsrouter</param>
/// <param name="resolvedProcessId">resolvedProcessId</param>
/// <returns></returns>
public static bool ResolveProcessForAttach(int processId, string name, string port, string dsrouter, out int resolvedProcessId)
public static void ResolveProcessForAttach(int processId, string name, string port, string dsrouter, out int resolvedProcessId)
{
resolvedProcessId = -1;
if (processId == 0 && string.IsNullOrEmpty(name) && string.IsNullOrEmpty(port) && string.IsNullOrEmpty(dsrouter))
{
Console.WriteLine("Must specify either --process-id, --name, --diagnostic-port, or --dsrouter.");
return false;
throw new CommandLineErrorException("Must specify either --process-id, --name, --diagnostic-port, or --dsrouter.");
}
else if (processId < 0)
{
Console.WriteLine($"{processId} is not a valid process ID");
return false;
throw new CommandLineErrorException($"{processId} is not a valid process ID");
}
else if ((processId != 0 ? 1 : 0) +
(!string.IsNullOrEmpty(name) ? 1 : 0) +
(!string.IsNullOrEmpty(port) ? 1 : 0) +
(!string.IsNullOrEmpty(dsrouter) ? 1 : 0)
!= 1)
{
Console.WriteLine("Only one of the --name, --process-id, --diagnostic-port, or --dsrouter options may be specified.");
return false;
throw new CommandLineErrorException("Only one of the --name, --process-id, --diagnostic-port, or --dsrouter options may be specified.");
}
// If we got this far it means only one of --name/--diagnostic-port/--process-id/--dsrouter was specified
else if (!string.IsNullOrEmpty(port))
{
return true;
return;
}
// Resolve name option
else if (!string.IsNullOrEmpty(name))
{
if ((processId = FindProcessIdWithName(name)) < 0)
{
return false;
}
processId = FindProcessIdWithName(name);
}
else if (!string.IsNullOrEmpty(dsrouter))
{
if (dsrouter != "ios" && dsrouter != "android" && dsrouter != "ios-sim" && dsrouter != "android-emu")
{
Console.WriteLine("Invalid value for --dsrouter. Valid values are 'ios', 'ios-sim', 'android' and 'android-emu'.");
return false;
throw new CommandLineErrorException("Invalid value for --dsrouter. Valid values are 'ios', 'ios-sim', 'android' and 'android-emu'.");
}
if ((processId = LaunchDSRouterProcess(dsrouter)) < 0)
{
if (processId == -2)
{
Console.WriteLine($"Failed to launch dsrouter: {dsrouter}. Make sure that dotnet-dsrouter is not already running. You can connect to an already running dsrouter with -p.");
throw new CommandLineErrorException($"Failed to launch dsrouter: {dsrouter}. Make sure that dotnet-dsrouter is not already running. You can connect to an already running dsrouter with -p.");
}
else
{
Console.WriteLine($"Failed to launch dsrouter: {dsrouter}. Please make sure that dotnet-dsrouter is installed and available in the same directory as dotnet-trace.");
Console.WriteLine("You can install dotnet-dsrouter by running 'dotnet tool install --global dotnet-dsrouter'. More info at https://learn.microsoft.com/en-us/dotnet/core/diagnostics/dotnet-dsrouter");
throw new CommandLineErrorException($"Failed to launch dsrouter: {dsrouter}. Please make sure that dotnet-dsrouter is installed and available in the same directory as dotnet-trace.\n" +
"You can install dotnet-dsrouter by running 'dotnet tool install --global dotnet-dsrouter'. More info at https://learn.microsoft.com/en-us/dotnet/core/diagnostics/dotnet-dsrouter");
}
return false;
}
}
resolvedProcessId = processId;
return true;
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/Tools/Common/DefaultConsole.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ namespace Microsoft.Diagnostics.Tools.Common
internal class DefaultConsole : IConsole
{
private readonly bool _useAnsi;
public DefaultConsole(bool useAnsi)
public DefaultConsole(bool useAnsi = false)
{
_useAnsi = useAnsi;
}
Expand Down
30 changes: 14 additions & 16 deletions src/Tools/dotnet-counters/CounterMonitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,7 @@ internal class CounterMonitor : ICountersLogger
private const int BufferDelaySecs = 1;
private const string EventCountersProviderPrefix = "EventCounters\\";
private int _processId;
private TextWriter _stdOutput;
private TextWriter _stdError;
private IConsole _console;
private List<EventPipeCounterGroup> _counterList;
private ICounterRenderer _renderer;
private string _output;
Expand All @@ -43,12 +42,11 @@ private class ProviderEventState
private readonly Dictionary<string, ProviderEventState> _providerEventStates = new();
private readonly Queue<CounterPayload> _bufferedEvents = new();

public CounterMonitor(TextWriter stdOutput, TextWriter stdError)
public CounterMonitor(IConsole console = null)
{
_pauseCmdSet = false;
_shouldExit = new TaskCompletionSource<ReturnCode>();
_stdOutput = stdOutput;
_stdError = stdError;
_console = console ?? new DefaultConsole();
}

private void MeterInstrumentEventObserved(string meterName, DateTime timestamp)
Expand Down Expand Up @@ -187,9 +185,9 @@ public async Task<ReturnCode> Monitor(
// to it.
ValidateNonNegative(maxHistograms, nameof(maxHistograms));
ValidateNonNegative(maxTimeSeries, nameof(maxTimeSeries));
if (!ProcessLauncher.Launcher.HasChildProc && !CommandUtils.ResolveProcessForAttach(processId, name, diagnosticPort, dsrouter, out _processId))
if (!ProcessLauncher.Launcher.HasChildProc)
{
return ReturnCode.ArgumentError;
CommandUtils.ResolveProcessForAttach(processId, name, diagnosticPort, dsrouter, out _processId);
}
ct.Register(() => _shouldExit.TrySetResult((int)ReturnCode.Ok));

Expand Down Expand Up @@ -238,14 +236,14 @@ public async Task<ReturnCode> Monitor(
{
//Cancellation token should automatically stop the session

_stdOutput.WriteLine($"Complete");
_console.Out.WriteLine($"Complete");
return ReturnCode.Ok;
}
}
}
catch (CommandLineErrorException e)
{
_stdError.WriteLine(e.Message);
_console.Error.WriteLine(e.Message);
return ReturnCode.ArgumentError;
}
finally
Expand Down Expand Up @@ -276,9 +274,9 @@ public async Task<ReturnCode> Collect(
// to it.
ValidateNonNegative(maxHistograms, nameof(maxHistograms));
ValidateNonNegative(maxTimeSeries, nameof(maxTimeSeries));
if (!ProcessLauncher.Launcher.HasChildProc && !CommandUtils.ResolveProcessForAttach(processId, name, diagnosticPort, dsrouter, out _processId))
if (!ProcessLauncher.Launcher.HasChildProc)
{
return ReturnCode.ArgumentError;
CommandUtils.ResolveProcessForAttach(processId, name, diagnosticPort, dsrouter, out _processId);
}
ct.Register(() => _shouldExit.TrySetResult((int)ReturnCode.Ok));

Expand Down Expand Up @@ -306,7 +304,7 @@ public async Task<ReturnCode> Collect(
_diagnosticsClient = holder.Client;
if (_output.Length == 0)
{
_stdError.WriteLine("Output cannot be an empty string");
_console.Error.WriteLine("Output cannot be an empty string");
return ReturnCode.ArgumentError;
}
if (format == CountersExportFormat.csv)
Expand All @@ -330,7 +328,7 @@ public async Task<ReturnCode> Collect(
}
else
{
_stdError.WriteLine($"The output format {format} is not a valid output format.");
_console.Error.WriteLine($"The output format {format} is not a valid output format.");
return ReturnCode.ArgumentError;
}

Expand All @@ -352,7 +350,7 @@ public async Task<ReturnCode> Collect(
}
catch (CommandLineErrorException e)
{
_stdError.WriteLine(e.Message);
_console.Error.WriteLine(e.Message);
return ReturnCode.ArgumentError;
}
finally
Expand Down Expand Up @@ -388,7 +386,7 @@ internal List<EventPipeCounterGroup> ConfigureCounters(string commaSeparatedProv

if (counters.Count == 0)
{
_stdOutput.WriteLine($"--counters is unspecified. Monitoring System.Runtime counters by default.");
_console.Out.WriteLine($"--counters is unspecified. Monitoring System.Runtime counters by default.");
ParseCounterProvider("System.Runtime", counters);
}
return counters;
Expand Down Expand Up @@ -537,7 +535,7 @@ private async Task<ReturnCode> Start(MetricsPipeline pipeline, CancellationToken
}
catch (DiagnosticsClientException ex)
{
Console.WriteLine($"Failed to start the counter session: {ex}");
_console.WriteLine($"Failed to start the counter session: {ex}");
}
catch (Exception ex)
{
Expand Down
4 changes: 2 additions & 2 deletions src/Tools/dotnet-counters/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ private static Command MonitorCommand()

monitorCommand.TreatUnmatchedTokensAsErrors = false; // see the logic in Main

monitorCommand.SetAction(static (parseResult, ct) => new CounterMonitor(parseResult.Configuration.Output, parseResult.Configuration.Error).Monitor(
monitorCommand.SetAction(static (parseResult, ct) => new CounterMonitor().Monitor(
ct,
counters: parseResult.GetValue(CounterOption),
processId: parseResult.GetValue(ProcessIdOption),
Expand Down Expand Up @@ -79,7 +79,7 @@ private static Command CollectCommand()

collectCommand.TreatUnmatchedTokensAsErrors = false; // see the logic in Main

collectCommand.SetAction((parseResult, ct) => new CounterMonitor(parseResult.Configuration.Output, parseResult.Configuration.Error).Collect(
collectCommand.SetAction((parseResult, ct) => new CounterMonitor().Collect(
ct,
counters: parseResult.GetValue(CounterOption),
processId: parseResult.GetValue(ProcessIdOption),
Expand Down
15 changes: 7 additions & 8 deletions src/Tools/dotnet-dump/Dumper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,14 +35,8 @@ public int Collect(TextWriter stdOutput, TextWriter stdError, int processId, str
{
try
{
if (CommandUtils.ResolveProcessForAttach(processId, name, diagnosticPort, string.Empty, out int resolvedProcessId))
{
processId = resolvedProcessId;
}
else
{
return -1;
}
CommandUtils.ResolveProcessForAttach(processId, name, diagnosticPort, string.Empty, out int resolvedProcessId);
processId = resolvedProcessId;

if (output == null)
{
Expand Down Expand Up @@ -132,6 +126,11 @@ public int Collect(TextWriter stdOutput, TextWriter stdError, int processId, str
client.WriteDump(dumpType, output, flags);
}
}
catch (CommandLineErrorException e)
{
stdError.WriteLine($"[ERROR] {e.Message}");
return -1;
}
catch (Exception ex)
{
if (diag)
Expand Down
1 change: 1 addition & 0 deletions src/Tools/dotnet-dump/dotnet-dump.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
<Compile Include="$(MSBuildThisFileDirectory)..\Common\WindowsProcessExtension\WindowsProcessExtension.cs" Link="WindowsProcessExtension.cs" />
<Compile Include="$(MSBuildThisFileDirectory)..\Common\DsRouterProcessLauncher.cs" Link="DsRouterProcessLauncher.cs" />
<Compile Include="$(MSBuildThisFileDirectory)..\Common\IConsole.cs" Link="IConsole.cs" />
<Compile Include="$(MSBuildThisFileDirectory)..\Common\CommandLineErrorException.cs" Link="CommandLineErrorException.cs" />
</ItemGroup>

<ItemGroup>
Expand Down
34 changes: 16 additions & 18 deletions src/Tools/dotnet-gcdump/CommandLine/CollectCommandHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,35 +30,23 @@ internal static class CollectCommandHandler
/// <returns></returns>
private static async Task<int> Collect(CancellationToken ct, int processId, string output, int timeout, bool verbose, string name, string diagnosticPort, string dsrouter)
{
if (!CommandUtils.ResolveProcessForAttach(processId, name, diagnosticPort, dsrouter, out int resolvedProcessId))
try
{
return -1;
}
CommandUtils.ResolveProcessForAttach(processId, name, diagnosticPort, dsrouter, out int resolvedProcessId);
processId = resolvedProcessId;

processId = resolvedProcessId;

if (!string.IsNullOrEmpty(diagnosticPort))
{
try
if (!string.IsNullOrEmpty(diagnosticPort))
{
IpcEndpointConfig config = IpcEndpointConfig.Parse(diagnosticPort);
if (!config.IsConnectConfig)
{
Console.Error.WriteLine("--diagnostic-port is only supporting connect mode.");
return -1;
}
}
catch (Exception ex)
{
Console.Error.WriteLine($"--diagnostic-port argument error: {ex.Message}");
return -1;
}

processId = 0;
}
processId = 0;
}

try
{
output = string.IsNullOrEmpty(output)
? $"{DateTime.Now:yyyyMMdd\\_HHmmss}_{processId}.gcdump"
: output;
Expand Down Expand Up @@ -106,6 +94,16 @@ private static async Task<int> Collect(CancellationToken ct, int processId, stri
return -1;
}
}
catch (CommandLineErrorException e)
{
Console.Error.WriteLine($"[ERROR] {e.Message}");
return -1;
}
catch (FormatException fe)
Copy link
Member Author

@mdh1418 mdh1418 Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was considering changing IpcEndpointConfig.Parse to throw CommandLineErrorException as well, but it seems like a breaking change if any users had used a public API that eventually invoked IpcEndpoingConfig.Pargse and specifically conditioned on FormatException, for example

diagnostics/src/Microsoft.Diagnostics.NETCore.Client/DiagnosticsClient/DiagnosticsClientConnector.cs

Line 61 in c0ea514

IpcEndpointConfig portConfig = IpcEndpointConfig.Parse(diagnosticPort);
.

Copy link
Member

@lateralusX lateralusX Oct 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the point why that would be needed, it doesn't log anything directly to console, but instead throws different exceptions with message based on identified error conditions, and its up to the caller to handle the exceptions it might throw and take actions based on exception. I believe this falls into comment above that API's should throw exceptions that makes sense for the errors identified and callers should handle them accordingly.

Copy link
Member Author

@mdh1418 mdh1418 Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I was just thinking it would be good to have consistency across the Tools, where any invalid combination of user passed-in args results in a CommandLineErrorException.

Copy link
Member

@noahfalk noahfalk Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree such consistency would be good, we just need to do the CommandLineErrorException on the tool side of the code rather than in M.D.NetCore.Client library. From what I can see we've got at least three different helpers that play a role in converting the command-line args into a DiagnosticClient: ProcessLauncher.Launcher, DiagnosticsClientBuilder, and CommandUtils.ResolveProcessForAttach. Our tools don't use them consistently nor do they all handle errors in a consistent way, but those things could be fixed. With refactoring all the tools might converge to a unified helper something like:

// this does any work needed to get an active connection to the target process including
// launching a new process, binding by name, binding by id, binding by port,
// routing through dsrouter, resuming the process, and hooking up child process
// IO to the current console.
// If anything goes wrong it throws CommandLineErrorException
using (var target = new ProcessTarget(commandLineArgs, console, cancellationToken))
{
    // do whatever tool specific work we want
    // target can have any APIs needed to describe or interact with the target process
    target.DiagnosticClient.SendWhateverCommand(...);
    ...
}

I'm not encouraging you to do that in the scope of this PR, but I hope we are making progress in that direction.

Copy link
Member

@lateralusX lateralusX Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, we should at least stay to conventional error handling on the library side. If we would like to do something different on the tool side where we previously would have passed around IConsole to log to stdout and/or stderr and we would like to not do that if a utility function only logs errors and return an error code, then fine. In that case its more like a generic DiagnosticToolException that could be thrown that would only be known to tools and something that can be catched and logged to stderr.

{
Console.Error.WriteLine($"--diagnostic-port argument error: {fe.Message}");
return -1;
}
catch (Exception ex)
{
Console.Error.WriteLine($"[ERROR] {ex}");
Expand Down
Loading