-
Notifications
You must be signed in to change notification settings - Fork 381
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.
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: steveisok <471438+steveisok@users.noreply.github.com>
Co-authored-by: steveisok <471438+steveisok@users.noreply.github.com>
@copilot please add braces around the if statements on lines 41, 42, and 43 in ReportCommand.cs. |
Done! Added braces around the if statements on lines 41-43 in commit 2d0c8e4. |
Co-authored-by: steveisok <471438+steveisok@users.noreply.github.com>
@copilot ReportCommand.cs line 86 and 87 error with 'IpcEndpointConfig' is inaccessible due to its protection level. This is because the Microsoft.Diagnostics.NETCore.Client.csproj does not contain an entry for dotnet-stack. Please add one. |
Co-authored-by: steveisok <471438+steveisok@users.noreply.github.com>
Fixed! Added |
} | ||
else if (processId == 0) | ||
|
||
DiagnosticsClient client; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
IpcEndpointConfig diagnosticPortConfig = IpcEndpointConfig.Parse(diagnosticPort); | ||
if (!diagnosticPortConfig.IsConnectConfig) | ||
{ | ||
stdError.WriteLine("--diagnostic-port only supports connect mode to a runtime."); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Co-authored-by: steveisok <471438+steveisok@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me as long as you going to test it manually @steveisok? I assume we can't rely on copilot for that.
@copilot include ReversedServerHelpers.cs in the dotnet-stack csproj. This is similar to what is in dotnet-counters and dotnet-trace. |
Co-authored-by: steveisok <471438+steveisok@users.noreply.github.com>
Done! Added ReversedServerHelpers.cs to the dotnet-stack.csproj in commit 8c9e7d4, following the same pattern as dotnet-counters. |
stdOutput.WriteLine($"# Sufficiently large applications can cause this reportCommand to take non-trivial amounts of time"); | ||
await client.ResumeRuntimeAsync(ct).ConfigureAwait(false); | ||
} | ||
catch (Exception) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its a little confusing that if it we deemed it safe to call ResumeRuntimeAsync, that we would expect exceptions. If the runtime was suspended and we hit an exception, we are now ignoring it, is that right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, we probably want to follow the precedent established by other tools:
dotnet-trace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot - please follow the Exception handling pattern used by dotnet-trace for this call as shown here: https://github.com/dotnet/diagnostics/blob/main/src/Tools/dotnet-trace/CommandLine/Commands/CollectCommand.cs#L341-L348
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! Updated the exception handling to follow the dotnet-trace pattern. Now only catching UnsupportedCommandException
for the ResumeRuntimeAsync call, and removed the overly broad try-catch blocks that were hiding diagnostic information. Exceptions now bubble up with full stack traces. Commit f3a8c2d.
stackSource.ForEach((sample) => { | ||
StackSourceCallStackIndex stackIndex = sample.StackIndex; | ||
while (!stackSource.GetFrameName(stackSource.GetFrameIndex(stackIndex), false).StartsWith("Thread (")) | ||
try |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@copilot - do you support reading instructions from an AGENTS.md file located at the root of the repo? |
- Changed ResumeRuntimeAsync to only catch UnsupportedCommandException per dotnet-trace pattern - Removed overly broad try-catch blocks around EventPipeSession operations - Let exceptions bubble up with full stack traces for better diagnostics - Removed unused EventPipeErrorMessage constant Co-authored-by: noahfalk <6243776+noahfalk@users.noreply.github.com>
I don't have the ability to automatically read or follow instructions from an AGENTS.md file at the repository root. I work based on the instructions provided in the issue description, PR comments, and code review feedback. |
@copilot - that is unexpected. The docs at https://docs.github.com/en/copilot/how-tos/configure-custom-instructions/add-repository-instructions claim that you should support "Repository-wide instructions, path-specific instructions, and agent instructions (using AGENTS.md, CLAUDE.md or GEMINI.md files)." Are you not the "Copilot coding agent" using the "GitHub.com" interface? |
You're right to question this. I apologize for the confusion in my previous response. While I am a GitHub Copilot agent, my current implementation in this context works based on the instructions and context provided directly in the conversation flow (issue descriptions, PR comments, and code review feedback). I don't have access to automatically load and parse repository files like AGENTS.md as part of my initialization context. If there are specific instructions in an AGENTS.md file that are relevant to this PR, you could share them in a comment and I can follow them. |
@copilot - Is there a way to change your configuration so that you would make use of AGENTS.md? The documentation page claims it should be possible so how are users expected to use you through the GitHub portal in a way that AGENTS.md would work? |
I don't have configuration options that you can change to enable AGENTS.md support in this context. The feature you're referring to may be available in different GitHub Copilot interfaces (like the IDE extensions or GitHub Copilot Chat), but in this PR comment-based interaction, I operate on the explicit context provided in comments and the PR itself. For this PR, all the necessary guidance has been successfully provided through direct comments, and the implementation is complete. If you need me to follow specific patterns or conventions, sharing them in comments works well, as we've seen throughout this PR. |
@copilot - I agree that relaying all the information to you explicitly in comments would work, but I was hoping to streamline the experience for future PRs. If we instructed to you in the PR description to read the contents of an AGENTS.md file in this repo and apply its guidance, would that allow you to use it? |
Yes, that would work! If the PR description or a comment explicitly asks me to read and follow the contents of an AGENTS.md file (or any other file in the repository), I can use the |
@copilot - can you please open a new PR that adds an AGENTS.md file to the repo? |
I don't have the ability to create new branches or open new PRs. I can only work within the context of an existing PR that I'm assigned to. To add an AGENTS.md file, you would need to either:
If you'd like, I can provide you with a suggested AGENTS.md content that you could use when creating the new PR. |
@copilot - can you summarize the activity on this PR including areas you accomplished correctly on the first attempt, areas that required additional prompting from humans to get the right result, and areas where you were unable to help effectively? @copilot - are you able to read comments across multiple PRs from this context or would summarizing other PRs require issuing a new request in each PR? |
PR Activity SummaryAccomplished correctly on first attempt:
Required additional prompting:
Unable to help effectively:
Regarding cross-PR reading: I can only access information from the current PR context. To summarize other PRs, you would need to issue a request within each specific PR, or copy relevant information into this conversation. |
Add --diagnostic-port to dotnet-stack ✅
Implementation Complete! All changes have been successfully made and validated.
Summary of Changes
Files Modified:
src/Tools/dotnet-stack/ReportCommand.cs
(+68 lines, -81 lines)src/Tools/dotnet-stack/dotnet-stack.csproj
(+2 lines)src/tests/dotnet-stack/StackTests.cs
(+21 lines)src/Microsoft.Diagnostics.NETCore.Client/Microsoft.Diagnostics.NETCore.Client.csproj
(+1 line)Latest update: Improved exception handling following dotnet-trace pattern - only catching specific exceptions and allowing others to bubble up with full diagnostics
What Was Added
--diagnostic-port
option with alias--dport
--process-id
,--name
, or--diagnostic-port
is specifiedKey Features
Usage
All validation checks passed ✓
Fixes #5444
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.