-
Notifications
You must be signed in to change notification settings - Fork 752
Implement configurable symbol server authentication with /SymbolsAuth command line argument for PerfView and HeapDump #2278
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
Conversation
|
@brianrob 👋 This repository doesn't have Copilot instructions. With Copilot instructions, I can understand the repository better, work faster and produce higher quality PRs. I can generate a .github/copilot-instructions.md file for you automatically. Click here to open a pre-filled issue and assign it to me. I'll write the instructions, and then tag you for review. |
Co-authored-by: brianrob <6210322+brianrob@users.noreply.github.com>
src/PerfView/Authentication.cs
Outdated
| private static ChainedTokenCredential CreateTokenCredential() | ||
| { | ||
| return new ChainedTokenCredential( | ||
| new EnvironmentCredential(), |
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.
Rather than chaining all of these together, let's do the following. By default, let's set this to be an Interactive credential and nothing else. Then, if the user wants to override this, they can do so by specifying one or more credential types from the list of valid credentials on the command line, using a new command line argument that I'd like you to implement called /SymbolsAuth. Use an enum with flags to back the set of valid values based on what is currently supported by TokenCredential.
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 can you help with this change?
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've implemented the requested changes:
- Reverted CreateTokenCredential() to use only
InteractiveBrowserCredential()by default - Added
/SymbolsAuthcommand line argument that accepts comma-separated values: Environment, AzureCli, VisualStudio, Interactive - Created SymbolsAuthenticationTypes enum with flags to represent the different credential types
- Updated the credential chain logic to build credentials based on the specified flags
The implementation allows users to specify multiple authentication types like:
/SymbolsAuth:Environment,VisualStudio/SymbolsAuth:Interactive(default behavior)/SymbolsAuth:Environment,AzureCli,VisualStudio,Interactive(equivalent to previous chained approach)
Changes made in commit 47a3b2e
…tial types Co-authored-by: brianrob <6210322+brianrob@users.noreply.github.com>
src/PerfView/CommandLineArgs.cs
Outdated
| // options common to multiple commands | ||
| public string DataFile; // This is the name of the ETL file (not the ZIP file) | ||
| public string LogFile; | ||
| public string SymbolsAuth; // Specifies authentication types for symbol servers (comma-separated) |
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, Rather than using a string and having to parse it, you can use the enum directly and even set the default value to be Interactive here.
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.
Updated to use SymbolsAuthenticationType enum directly with default value Interactive in commit 52c1162. The command line parser's built-in enum support handles parsing and validation automatically.
src/PerfView/Authentication.cs
Outdated
| /// </summary> | ||
| /// <param name="symbolsAuthArg">Comma-separated string of authentication types</param> | ||
| /// <returns>Combined flags representing the specified authentication types</returns> | ||
| public static SymbolsAuthenticationTypes ParseSymbolsAuthenticationTypes(string symbolsAuthArg) |
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, If the parser uses the enum directly, this method is no longer needed.
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.
Removed the ParseSymbolsAuthenticationTypes method in commit 52c1162 since the command line parser handles enum parsing directly.
src/PerfView/Authentication.cs
Outdated
| /// Flags enum representing the types of Azure credentials that can be used for symbol authentication. | ||
| /// </summary> | ||
| [Flags] | ||
| public enum SymbolsAuthenticationTypes |
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 rename this SymbolsAuthenticationType.
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.
Renamed to SymbolsAuthenticationType (singular) in commit 52c1162.
|
@copilot, please see the latest review. |
Co-authored-by: brianrob <6210322+brianrob@users.noreply.github.com>
brianrob
left a comment
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 address this request.
| StatusBar.Log("Finished Lookup up symbols for " + moduleName + " Elapsed Time = " + | ||
| StatusBar.Duration.TotalSeconds.ToString("n3")); | ||
| } | ||
| catch (Azure.Identity.AuthenticationFailedException ex) |
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.
Please undo the dialog changes. This doesn't work and my local attempts to make this work were much more complicated than I thought. Additionally, it creates a bunch of behavior changes for SymbolReader.
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.
Dialog changes have been removed from StackWindow.xaml.cs in commit 96e1083. All authentication failure handling, the ShowAuthenticationFailureDialog method, and the Azure.Identity using statement have been reverted to preserve the original SymbolReader behavior.
Co-authored-by: brianrob <6210322+brianrob@users.noreply.github.com>
brianrob
left a comment
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 address this comment.
| => httpHandler.AddHandler(new BasicHttpAuthHandler(log)); | ||
|
|
||
| private static ChainedTokenCredential CreateTokenCredential() | ||
| private static ChainedTokenCredential CreateTokenCredential(SymbolsAuthenticationType authTypes) |
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.
This same logic should also apply to the TokenCredential created in heapdump. I suspect you'll need to take the argument passed to PerfView and pass it to heapdump.exe and have heapdump.exe consume it in the same way that PerfView does here.
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've implemented the SymbolsAuth configuration for HeapDump.exe as requested. The changes include:
- HeapDump.exe now supports
/SymbolsAuthargument - Added parsing logic and credential chain building identical to PerfView - PerfView passes SymbolsAuth to HeapDump.exe - Modified HeapSnapshot.cs to include the
/SymbolsAuthparameter when launching HeapDump.exe - Maintains backward compatibility - HeapDump.exe defaults to Interactive authentication when no argument is provided
The implementation ensures that both PerfView and HeapDump.exe use the same authentication configuration specified by the user. Changes made in commit 5f0b8c5.
Co-authored-by: brianrob <6210322+brianrob@users.noreply.github.com>
marklio
left a comment
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.
This looks fine overall, just a few pieces of feedback.
| /// <summary> | ||
| /// The token credential to use for symbol server authentication. | ||
| /// </summary> | ||
| public TokenCredential SymbolsAuthTokenCredential; |
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.
Seems strange to have both a public constructor arg AND a public settable field. My preferred approach if you need to provide this for consumption would be (in order of preference):
- An internal get-only property
- A public get-only property (if it really needs to be public)
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.
public fields are typically thought of as an anti-pattern
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.
You're right - The constructor really doesn't need the argument anymore, especially with a default NULL value. I've removed it. One thing to note is that the GCHeapDumper class is part of HeapDump which isn't publicly shipped as surface area that folks can depend upon. It's really an internal implementation detail of PerfView, and so I imagine that's why it uses public fields. I can change this if you're feeling strongly about this, but for the purposes of this change, I'm just following the existing pattern.
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.
following existing pattern seems fine. I didn't notice the pattern.
src/HeapDump/Program.cs
Outdated
| else | ||
| { | ||
| Console.WriteLine("Invalid value for /SymbolsAuth: {0}", authTypesStr); | ||
| Console.WriteLine("Valid values are: Environment, AzureCli, VisualStudio, Interactive (can be combined with commas)"); |
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.
CONSIDER: generating this list from the enum
src/HeapDump/Program.cs
Outdated
| else if (arg.StartsWith("/SymbolsAuth:", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| string authTypesStr = arg.Substring(13); | ||
| if (TryParseSymbolsAuthenticationTypes(authTypesStr, out var parsedAuthTypes)) |
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.
Why not pass symbolsAuth rather than introducing a new local? In all continuing cases, symbolsAuth is immediately overwritten with this value anyway.
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.
Thanks for pointing this out. That was just a bit odd (from copilot).
src/PerfView/CommandLineArgs.cs
Outdated
| /// Flags enum representing the types of Azure credentials that can be used for symbol authentication. | ||
| /// </summary> | ||
| [Flags] | ||
| public enum SymbolsAuthenticationType |
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.
This seems duplicative
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.
oh, this is actually another project
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.
It is another project, but you're right. I merged these into one enum definition.
marklio
left a comment
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 good. Thanks for the context
This PR implements configurable authentication for symbol servers in both PerfView and HeapDump.exe, addressing the need for flexible credential management while maintaining backward compatibility.
Key Changes
Command Line Authentication Configuration
Added
/SymbolsAuthcommand line argument that allows users to specify which authentication methods to use:Available authentication types:
Environment- Uses environment variables (e.g.,AZURE_CLIENT_ID,AZURE_CLIENT_SECRET)AzureCli- Uses Azure CLI cached credentialsVisualStudio- Uses Visual Studio cached credentialsInteractive- Uses interactive browser authentication (default)Unified Authentication Across Components
Both PerfView and HeapDump.exe now use the same authentication configuration:
/SymbolsAuthargument to HeapDump.exe when launching itBackward Compatible Default Behavior
By default, both applications use only
InteractiveBrowserCredential(), maintaining the same user experience as before while allowing power users to configure additional authentication methods as needed.Implementation Details
SymbolsAuthenticationTypeenum with[Flags]attribute for combining authentication methodsThis approach provides flexibility for automated scenarios and enterprise environments while keeping the default experience simple for interactive users.
Fixes #2277.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.