Enable command utils console redirection #5622

mdh1418 · 2025-10-29T00:15:34Z

With more tests validating output of diagnostic tools, useful console output from helpers should be redirectable as well.

This PR does the following:

Switch CounterMonitor back to using IConsole for output instead of TextWriters (see commit message for more details Update System.CommandLine version #5217 had switched from IConsole to TextWriters)
Enable redirecting CommandUtils helpers console output to be captured in tests

Previously, CounterMonitor had been using IConsole from System.CommandLine instead of the in-house IConsole. As a result, during work to bump to a System.CommandLine version where IConsole was deprecated, CounterMonitor switched to writing to specified TextWriters. Although that decision was fine, this change switches to the in-house IConsole to prepare for redirecting CommandUtils console output to facilitate output validation in tooling tests.

src/Tools/Common/Commands/Utils.cs

noahfalk · 2025-10-29T01:25:31Z

src/Tools/Common/DefaultConsole.cs

+    internal sealed class DefaultConsole : IConsole
    {
        private readonly bool _useAnsi;
        public DefaultConsole(bool useAnsi)


Suggested change

public DefaultConsole(bool useAnsi)

public DefaultConsole(bool useAnsi = false)

If we are going to be using the DefaultConsole repeatedly and always passing false we may as well make it the default.

I defaulted CollectLinux and Collect's to use false in prior PRs, but I'm not sure if they should try to emit ansi. Changed the default for DefaultConsole as false

This reverts commit 3422ff6.

…rException

mdh1418 · 2025-10-29T04:10:24Z

src/Tools/dotnet-gcdump/CommandLine/CollectCommandHandler.cs

+                Console.Error.WriteLine($"[ERROR] {e.Message}");
+                return -1;
+            }
+            catch (FormatException fe)


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);

.

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.

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.

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.

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.

lateralusX · 2025-10-29T09:01:13Z

src/Tools/Common/Commands/Utils.cs

                    {
-                        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}");


Maybe CommandLineErrorException is a little misplaced in a utility function, data might input parameter might come for command line or some other area. We should create a specific exception for these errors or use some more generic exception type.

I think its reasonable to use CommandLineErrorException when the code is sufficiently constrained that we know its going to called by the command handlers and we know we want any errors to get shown on the console afterwards. I think anything under Commands/Utils probably fits that condition. IMO creating more generic Exceptions will just lead to every caller changing a 1 line call into an 8 line try/catch to convert the exception type back:

try { UtilityFunction(); } catch(SomeOtherExceptionType e) { throw CommandLineErrorException(some_message); }

lateralusX · 2025-10-29T09:02:21Z

src/Tools/Common/Commands/Utils.cs

            {
-                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.");


Maybe use argument exception instead?

CommandLineErrorException means that we know the error message is suitable for display on the console, we've anticipated this error occuring, and there is no bug in the code. ArgumentException often implies the code has a bug and the error messages aren't intended for the end-user. I think CommandLineErrorException is appropriate here.

#5570 (comment)

maybe we should rename the exception then, clearly showing the intent, its not a command line error, but an error suitable to display on the console.

Feel free to do that. The current name was intended to be interpreted as an error that will be displayed on the command line, not necessarily an error caused by the command line. If another name coveys that better we can switch anytime.

DiagnosticToolException?

Sounds great 👍

Converted all CommandLineErrorException to DiagnosticToolException

lateralusX · 2025-10-29T09:04:59Z

src/Tools/Common/Commands/Utils.cs

        /// <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)


We probably shouldn't use the same exception for all errors. We should also do specific exceptions for the LaunchDSRouter errors.

I think multiple Exception types make sense if we expect callers to write catch blocks that discriminate based on those types. Is that the case here?

I agree that the LaunchDSRouter errors are not purely a problem with the args the user passes, so it overloads CommandLineErrorException to mean more than "user passed in invalid combination of args". (Though if they didnt pass in --dsrouter they wouldn't hit the error 😄). Then again, I think the catch behavior would be the same, we would emit the error message and stop the tool. So I'm conflicted in that it's a different class of error, yet I don't think this necessarily warrants a new copy/paste catch block.

Edit: After thinking of CommandLineErrorException as error message suitable to be displayed on console https://github.com/dotnet/diagnostics/pull/5622/files#r2472507775, it seems like we don't necessarily need a new catch block if in either case we just want to inform the user that something is wrong with their setup.

src/Tools/dotnet-counters/Program.cs

lateralusX · 2025-10-29T09:22:52Z

src/Tools/dotnet-trace/CommandLine/Commands/CollectCommand.cs

-                    }
-                    else
-                    {
-                        return (int)ReturnCode.ArgumentError;


Did we lose this error case, when function returns argument error code?

You're right, it changes the return error code to TracingError because of the catch at

diagnostics/src/Tools/dotnet-trace/CommandLine/Commands/CollectCommand.cs

Lines 477 to 482 in c0ea514

catch (CommandLineErrorException e)

{

Console.Error.WriteLine($"[ERROR] {e.Message}");

collectionStopped = true;

ret = (int)ReturnCode.TracingError;

}

. Maybe CommandLineErrorException, which I'm understanding represents an invalid combination of user's passed-in args, should all result in ReturnCode.ArgumentError instead? Otherwise, if we need to differentiate between some CommandLineErrorExceptions throwing ArgumentError and others throwing TracingError, that seems like the CommandLineErrorException is overloaded

It doesn't look like the ReturnCode is really being consumed anywhere except for the recently added functional tests. Are there known instances of users invoking dotnet-trace programmatically and checking the exit code?

Regarding #5622 (comment), it seems like the failure to launch DSRouter isn't an argument error, it feels more aligned with TracingError. If we want to treat the instances of CommandLineErrorException that are a result of invalid arguments as ReturnCode.ArgumentError, we can check the message for the dsrouter launch failure to differentiate, but 1) that doesn't feel like a clean resolution, and 2) changing the invalid args to ArgumentError would be breaking behavior if anyone consumed the exit codes.

Are there known instances of users invoking dotnet-trace programmatically and checking the exit code?

Not that I am aware of, though that doesn't mean it never happens. My thought would be make sure scripts can distinguish success from failure but don't worry about categorizing different kinds of error with different numeric codes.

src/tests/dotnet-counters/CounterMonitorPayloadTests.cs

mdh1418 added 3 commits October 28, 2025 22:37

[dotnet-trace][collect] Set console through constructor

079b98a

[CommandUtils] Enable output redirection

3422ff6

mdh1418 requested review from hoyosjs and noahfalk October 29, 2025 00:15

mdh1418 requested a review from a team as a code owner October 29, 2025 00:15

noahfalk reviewed Oct 29, 2025

View reviewed changes

mdh1418 added 7 commits October 29, 2025 02:34

Revert "[CommandUtils] Enable output redirection"

30d433d

This reverts commit 3422ff6.

[CommandUtils] Convert FindProcessIdWithName to throw CommandLineErro…

b116e70

…rException

[CommandUtils] Convert ValidateArgumentsForChildProcess to throw

44f7364

[CommandUtils] Convert ResolveProcessForAttach to throw

7a8450a

Add corresponding compile items

2e9cab0

Fix exception typo

f7f7704

Default DefaultConsole to not use ansi

90f7841

mdh1418 commented Oct 29, 2025

View reviewed changes

lateralusX reviewed Oct 29, 2025

View reviewed changes

src/Tools/dotnet-counters/Program.cs Show resolved Hide resolved

lateralusX reviewed Oct 29, 2025

View reviewed changes

src/Tools/dotnet-counters/Program.cs Show resolved Hide resolved

lateralusX reviewed Oct 29, 2025

View reviewed changes

src/tests/dotnet-counters/CounterMonitorPayloadTests.cs Show resolved Hide resolved

mdh1418 added 4 commits October 29, 2025 19:31

[dotnet-trace] Fix ReturnCode from CommandLineErrorException

7b0aa79

[dotnet-trace] Handle DSRouter Launch Failure ReturnCode

6be25ae

Cleanup ProcessLauncher Start

0e29459

Rename CommandLineErrorException

e2d336a

	public DefaultConsole(bool useAnsi)
	public DefaultConsole(bool useAnsi = false)

	catch (CommandLineErrorException e)
	{
	Console.Error.WriteLine($"[ERROR] {e.Message}");
	collectionStopped = true;
	ret = (int)ReturnCode.TracingError;
	}

Enable command utils console redirection #5622

Are you sure you want to change the base?

Enable command utils console redirection #5622

Uh oh!

Conversation

mdh1418 commented Oct 29, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lateralusX Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lateralusX Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noahfalk Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdh1418 Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lateralusX Oct 29, 2025 •

edited

Loading

lateralusX Oct 29, 2025 •

edited

Loading

noahfalk Oct 30, 2025 •

edited

Loading

mdh1418 Oct 29, 2025 •

edited

Loading