Skip to content

Add --diagnostic-port option to dotnet-stack #5592

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

Open
Copilot wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Add --diagnostic-port option to dotnet-stack #5592

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
47ce0d2
Initial plan
Copilot Oct 8, 2025
682a87d
Add --diagnostic-port option to dotnet-stack report command
Copilot Oct 8, 2025
1c97f11
Add test for --diagnostic-port option registration
Copilot Oct 8, 2025
07c8f6c
Add braces around if statements on lines 41-43
Copilot Oct 8, 2025
a72e921
Add dotnet-stack to InternalsVisibleTo in NETCore.Client
Copilot Oct 8, 2025
877a49d
Use DiagnosticsClientBuilder to support both connect and listen modes
Copilot Oct 9, 2025
ea77b81
Include ReversedServerHelpers.cs in dotnet-stack.csproj
Copilot Oct 9, 2025
87df662
Add CommandLineErrorException.cs to dotnet-stack.csproj
Copilot Oct 9, 2025
3a3a765
Added a resume command and a little more clear error message
steveisok Oct 15, 2025
7e8ef6b
Silly me
steveisok Oct 15, 2025
79364ff
Follow dotnet-trace pattern for exception handling
Copilot Oct 21, 2025
0e21b47
Merge remote-tracking branch 'upstream/main' into copilot/add-diagnos…
steveisok Jan 24, 2026
b7b3cf7
Resolve bad merge
steveisok Jan 24, 2026
9fd1ccf
Fix dotnet-stack.csproj: Remove non-existent CommandLineErrorExceptio…
steveisok Jan 24, 2026
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
1 change: 1 addition & 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 @@ -41,6 +41,7 @@
<InternalsVisibleTo Include="dotnet-monitor" />
<InternalsVisibleTo Include="dotnet-trace" />
<InternalsVisibleTo Include="dotnet-dump" />
<InternalsVisibleTo Include="dotnet-stack" />
<InternalsVisibleTo Include="Microsoft.Diagnostics.Monitoring" />
<InternalsVisibleTo Include="Microsoft.Diagnostics.Monitoring.EventPipe" />
<InternalsVisibleTo Include="Microsoft.Diagnostics.WebSocketServer" />
Expand Down
75 changes: 61 additions & 14 deletions src/Tools/dotnet-stack/ReportCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,22 +27,44 @@ internal static class ReportCommandHandler
/// <param name="processId">The process to report the stack from.</param>
/// <param name="name">The name of process to report the stack from.</param>
/// <param name="duration">The duration of to trace the target for. </param>
/// <param name="diagnosticPort">The diagnostic port to connect to.</param>
/// <returns></returns>
private static async Task<int> Report(CancellationToken ct, TextWriter stdOutput, TextWriter stdError, int processId, string name, TimeSpan duration)
private static async Task<int> Report(CancellationToken ct, TextWriter stdOutput, TextWriter stdError, int processId, string name, TimeSpan duration, string diagnosticPort)
{
string tempNetTraceFilename = Path.Join(Path.GetTempPath(), Path.GetRandomFileName() + ".nettrace");
string tempEtlxFilename = "";

try
{
// Either processName or processId has to be specified.
// Validate that only one of processId, name, or diagnosticPort is specified
int optionCount = 0;
if (processId != 0)
{
optionCount++;
}
if (!string.IsNullOrEmpty(name))
{
optionCount++;
}
if (!string.IsNullOrEmpty(diagnosticPort))
{
optionCount++;
}

if (optionCount == 0)
{
stdError.WriteLine("--process-id, --name, or --diagnostic-port is required");
return -1;
}
else if (optionCount > 1)
{
stdError.WriteLine("Only one of --process-id, --name, or --diagnostic-port can be specified");
return -1;
}

// Resolve process name to ID if needed
if (!string.IsNullOrEmpty(name))
{
if (processId != 0)
{
Console.WriteLine("Can only specify either --name or --process-id option.");
return -1;
}
processId = CommandUtils.FindProcessIdWithName(name);
if (processId < 0)
{
Expand All @@ -55,14 +77,31 @@ private static async Task<int> Report(CancellationToken ct, TextWriter stdOutput
stdError.WriteLine("Process ID should not be negative.");
return -1;
}
else if (processId == 0)

DiagnosticsClient client;
Copy link
Copy Markdown
Member

@noahfalk noahfalk Oct 9, 2025 •
edited by steveisok
Loading

Choose a reason for hiding this comment

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

@copilot please use DiagnosticsClientBuilder, as other tools such as dotnet-counters and dotnet-trace already do.

Copy link
Copy Markdown
Contributor Author

Copilot AI Oct 9, 2025

Choose a reason for hiding this comment

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

Done! Refactored to use DiagnosticsClientBuilder in commit c8e0a36. This provides consistency with other tools and automatically supports both connect and listen modes.

if (!string.IsNullOrEmpty(diagnosticPort))
{
stdError.WriteLine("--process-id is required");
return -1;
try
Copy link
Copy Markdown
Member

@mdh1418 mdh1418 Oct 20, 2025

Choose a reason for hiding this comment

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

I'm not quite following, what was the rationale for adding try/catch for these three blocks (this plus the two above). Also couldn't this and the above be merged under the same try/catch, they seem like the same format

Copy link
Copy Markdown
Member

@noahfalk noahfalk Oct 21, 2025

Choose a reason for hiding this comment

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

There is broad catch handler here that serves as our fallback for unanticipated errors. Inside that scope we generally only catch narrower Exception types that we know how to handle or how to describe to the user in a meaningful way.

Copy link
Copy Markdown
Member

@noahfalk noahfalk Oct 21, 2025

Choose a reason for hiding this comment

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

@copilot, please remove this try catch handler and let Exceptions flow into the outer catch handler.

{
IpcEndpointConfig diagnosticPortConfig = IpcEndpointConfig.Parse(diagnosticPort);
if (!diagnosticPortConfig.IsConnectConfig)
{
stdError.WriteLine("--diagnostic-port only supports connect mode to a runtime.");
Copy link
Copy Markdown
Member

@noahfalk noahfalk Oct 9, 2025 •
edited by steveisok
Loading

Choose a reason for hiding this comment

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

@copilot please support both connect and listen modes, as other tools already do. Using DiagnosticsClientBuilder should give support for both modes.

Copy link
Copy Markdown
Contributor Author

Copilot AI Oct 9, 2025

Choose a reason for hiding this comment

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

Done! Using DiagnosticsClientBuilder now supports both connect and listen modes automatically. Commit c8e0a36.

return -1;
}
client = new DiagnosticsClient(diagnosticPortConfig);
}
catch (Exception ex)
{
stdError.WriteLine($"--diagnostic-port argument error: {ex.Message}");
return -1;
}
}
else
{
client = new DiagnosticsClient(processId);
}


DiagnosticsClient client = new(processId);
List<EventPipeProvider> providers = new()
{
new EventPipeProvider("Microsoft-DotNETCore-SampleProfiler", EventLevel.Informational)
Expand Down Expand Up @@ -189,15 +228,17 @@ public static Command ReportCommand()
{
ProcessIdOption,
NameOption,
DurationOption
DurationOption,
DiagnosticPortOption
};

reportCommand.SetAction((parseResult, ct) => Report(ct,
stdOutput: parseResult.Configuration.Output,
stdError: parseResult.Configuration.Error,
processId: parseResult.GetValue(ProcessIdOption),
name: parseResult.GetValue(NameOption),
duration: parseResult.GetValue(DurationOption)));
duration: parseResult.GetValue(DurationOption),
diagnosticPort: parseResult.GetValue(DiagnosticPortOption)));

return reportCommand;
}
Expand All @@ -221,5 +262,11 @@ public static Command ReportCommand()
{
Description = "The name of the process to report the stack."
};

private static readonly Option<string> DiagnosticPortOption =
new("--diagnostic-port", "--dport")
{
Description = "The path to a diagnostic port to be used."
};
}
}
21 changes: 21 additions & 0 deletions src/tests/dotnet-stack/StackTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,5 +100,26 @@ public async Task ReportsStacksCorrectly(TestConfiguration config)
Assert.True(correctStackParts[j] == stackParts[i], $"{correctStackParts[j]} != {stackParts[i]}");
}
}

[Fact]
public void DiagnosticPortOptionIsRegistered()
{
Command reportCommand = ReportCommandHandler.ReportCommand();

// Verify the diagnostic port option is registered
bool hasDiagnosticPortOption = false;
foreach (var option in reportCommand.Options)
{
if (option.Name == "diagnostic-port")
{
hasDiagnosticPortOption = true;
// Verify it has the short alias --dport
Assert.Contains("--dport", option.Aliases);
break;
}
}

Assert.True(hasDiagnosticPortOption, "The --diagnostic-port option should be registered in the report command");
}
}
}
Loading