[ToolsCommon] Refactor Utils.cs

[mdh1418]

With more tests validating output of diagnostic tools, errors from CommandUtils helpers should be validated as well. In facilitating error checking, src/Tools/Common/Commands/Utils.cs has been refactored to break unnecessary dependencies (e.g. using CommandUtils.FindProcessIdWithName requires adding IConsole.cs to project Compile items because LineRewriter depends on IConsole, regardless of whether LineRewriter is used)

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)
• Rename CommandLineErrorException -> DiagnosticToolException to suggest a more generic invalid usage of diagnostic tools, rather specifically a command line exception.
• Update CommandUtils helpers to throw DiagnosticToolException to allow callers to decide how to handle the message.
• Extend DiagnosticToolException (formerly CommandLineErrorException) to specify ReturnCode
• Break ReturnCode + LineRewriter out of Utils.cs to have discrete dependency chains for Project compile items.
• Rename Utils.cs -> CommandUtils.cs
• Keep src/Tools/Common/Commands command-only

[mdh1418]

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

.

[lateralusX]

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.

[mdh1418]

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.

[noahfalk]

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.

[lateralusX]

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]

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

[mdh1418]

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

[mdh1418]

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.

[noahfalk]

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.

[mdh1418]

@lateralusX @noahfalk Could this get another review?

[noahfalk]

Looked good to me other than one comment inline

[noahfalk]

@lateralusX - do you think it is important to preserve this distinction in error codes? I'm not expecting it matters much.

If we do want to assign specific return codes I'd suggest add an int ReturnCode property to the DiagnosticToolException, return that value here, and set it when the Exception is constructed. Using error message content for control flow looks very fragile.

[lateralusX]

We should definitely not depend on message output to convert to error codes, either we have sub classes of exception types so we can distinguish or have an error code on DiagnosticToolException.

[mdh1418]

Added the ReturnCode, I opted to just comment that the default maps to ReturnCode.ArgumentError, because otherwise, adding a dependency on Microsoft.Internal.Common.Utils to DiagnosticToolException will propagate and require additional manual compile items for all of Microsoft.Internal.Common.Utils dependencies. (atleast for dotnet-dsrouter which just uses DiagnosticToolException and doesn't use Utils)

[lateralusX]

I think we should move out the ReturnCode into its own source file under Common, maybe name it DiagnosticToolReturnCode.cs, that way it could be used by tools without additional dependencies. We should probably do the same with LineReWriter and maybe rename utils.cs to then name of the class it actually holds, CommandUtils.

[lateralusX]

LGTM!

[noahfalk]

👍