Conversation
…, toolbar ui; notifications and profile session commands.
…ub.com/microsoft/vscode-mssql into dev/allancascante/profiler_integration
…te a session to start event session
…ng a message if user is trying to profile fabric as not supported
…localtestprofilerfilter
allancascante
requested review from
Benjin,
aasimkhan30,
caohai,
kburtram,
laurenastrid1,
lewis-sanchez,
manujoseph85 and
ssreerama
as code owners
There was a problem hiding this comment.
Pull request overview
This pull request enhances the profiler functionality by adding template and session selection during launch, implementing server type detection to handle Azure SQL and Fabric connections appropriately, and auto-starting profiler sessions after creation. The changes include fixing Azure SQL Extended Events templates by removing unsupported actions, adding database selection for Azure SQL connections, and improving the overall user experience when starting profiler sessions.
Changes:
- Added server type detection to filter available templates by engine type (Azure vs on-premises) and block Fabric connections
- Implemented database selection for Azure SQL connections when no user database is specified
- Changed profiler session flow to prompt for template and session name on launch, then auto-create and start the session
- Fixed Azure SQL Extended Events templates by removing
sqlserver.server_principal_nameaction which is not supported in Azure SQL Database
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| localization/xliff/vscode-mssql.xlf | Added localization strings for Fabric warning, database selection prompt, and session started message |
| extensions/mssql/l10n/bundle.l10n.json | Added English translations for new localization strings |
| extensions/mssql/src/constants/locConstants.ts | Defined new localization constants for profiler database selection and Fabric warning |
| extensions/mssql/src/profiler/profilerDefaultConfig.ts | Renamed template names to include engine type suffix and removed unsupported server_principal_name action from Azure templates |
| extensions/mssql/src/profiler/profilerController.ts | Implemented server type detection, Azure database selection, template filtering by engine type, and auto-start session logic |
| extensions/mssql/test/unit/profiler/profilerController.test.ts | Added comprehensive tests for server type handling, database selection, template selection, and session auto-start |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
PR Changes
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #21074 +/- ##
==========================================
+ Coverage 68.29% 68.60% +0.31%
==========================================
Files 242 242
Lines 23543 23661 +118
Branches 3114 3131 +17
==========================================
+ Hits 16078 16233 +155
+ Misses 7336 7299 -37
Partials 129 129
🚀 New features to boost your workflow:
|
Updates to localized strings requiredPlease update the localized strings in the PR with following steps:
The localization system extracts strings from both extensions to a centralized location. |
…_start_session_on_load
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| this._logger.verbose(`Session '${sessionName}' created successfully`); | ||
|
|
||
| // Auto-start the session | ||
| await this.startSession(sessionName, webviewController); |
There was a problem hiding this comment.
The UI is now driven by user-selected templates, but startSession still creates ProfilerSession instances with templateName: "Standard" (hard-coded). This makes the session metadata inconsistent with the selected template and can lead to incorrect UI state/telemetry if anything relies on templateName. Pass the selected template name/id through to startSession and into createSession instead of hard-coding.
| this._logger.verbose(`Server types detected: ${serverTypes.join(", ")}`); | ||
|
|
||
| // Determine engine type based on server type | ||
| this._currentEngineType = serverTypes.includes(ServerType.Azure) |
There was a problem hiding this comment.
Please move this to some sort of shared util.
aasimkhan30
left a comment
aasimkhan30
left a comment
There was a problem hiding this comment.
Looks good. Please do the cleanup as a follow up PR.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
extensions/mssql/src/profiler/profilerController.ts:536
setupProfilerUIperforms several awaited operations (template selection, session name input,getXEventSessions) before the webview dispose override is installed. If any of these throw, the profiler connection and_profilerEngineTypesentry created inlaunchProfilerWithConnectioncan be leaked. Wrap the body in a try/catch/finally (or catch aroundgetXEventSessions) that disconnectsprofilerUriand deletes the engine-type mapping on failure.
private async setupProfilerUI(profilerUri: string, engineType: EngineType): Promise<void> {
this._profilerUri = profilerUri;
// Step 1: Show template selection quick pick (filtered by engine type)
const configService = getProfilerConfigService();
const templates = configService.getTemplatesForEngine(engineType);
this._logger.verbose(
`Filtered templates for engine ${engineType}: ${templates.length} available`,
);
if (templates.length === 0) {
vscode.window.showWarningMessage(LocProfiler.noTemplatesAvailable);
// Disconnect since no templates are available and we can't proceed
await this._connectionManager.disconnect(profilerUri);
return;
}
const templateItems = templates.map((t) => ({
label: t.name,
description: t.description,
detail: LocProfiler.engineLabel(t.engineType),
template: t,
}));
const selectedTemplate = await vscode.window.showQuickPick(templateItems, {
placeHolder: LocProfiler.selectTemplate,
ignoreFocusOut: true,
title: LocProfiler.newSessionSelectTemplate,
});
if (!selectedTemplate) {
this._logger.verbose("User cancelled template selection");
// Disconnect since user cancelled
await this._connectionManager.disconnect(profilerUri);
return;
}
this._logger.verbose(`Selected template: ${selectedTemplate.template.name}`);
// Step 2: Show session name input (default to template name)
const sessionName = await vscode.window.showInputBox({
prompt: LocProfiler.enterSessionName,
placeHolder: LocProfiler.sessionNamePlaceholder,
value: selectedTemplate.template.name.replace(/\s+/g, "_"), // Default to template name with underscores
title: LocProfiler.newSessionEnterName,
ignoreFocusOut: true,
validateInput: (value) => {
if (!value || value.trim().length === 0) {
return LocProfiler.sessionNameEmpty;
}
if (value.length > SESSION_NAME_MAX_LENGTH) {
return LocProfiler.sessionNameTooLong(SESSION_NAME_MAX_LENGTH);
}
// Check for invalid characters (basic validation)
if (!/^[a-zA-Z0-9_-]+$/.test(value)) {
return LocProfiler.sessionNameInvalidChars;
}
return undefined;
},
});
if (!sessionName) {
this._logger.verbose("User cancelled session name input");
// Disconnect since user cancelled
await this._connectionManager.disconnect(profilerUri);
return;
}
this._logger.verbose(`Session name: ${sessionName}`);
// Fetch available XEvent sessions from the server
this._logger.verbose("Fetching available XEvent sessions...");
const xeventSessions = await this._sessionManager.getXEventSessions(profilerUri);
this._logger.verbose(`Found ${xeventSessions.length} available XEvent sessions`);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Get the engine type for the current profiler URI | ||
| const engineType = | ||
| this._profilerEngineTypes.get(this._profilerUri) ?? EngineType.Standalone; | ||
| const templates = configService.getTemplatesForEngine(engineType); |
There was a problem hiding this comment.
handleCreateSession looks up the engine type using this._profilerUri (a single mutable field) instead of the specific URI for the webview that invoked the action. If multiple profiler webviews are opened, actions from an older webview can end up using the most recently assigned _profilerUri, leading to wrong template filtering and RPC calls against the wrong connection. Consider threading the owning profilerUri through the webview event handlers (capture it in the closure in setupProfilerUI) and remove reliance on the global _profilerUri for per-webview operations.
| // Block Fabric connections - profiler is not supported | ||
| if (serverTypes.includes(ServerType.Fabric)) { | ||
| this._logger.verbose("Profiler not supported on Fabric"); | ||
| vscode.window.showWarningMessage(LocProfiler.profilerNotSupportedOnFabric); | ||
| return; |
There was a problem hiding this comment.
The PR description says Fabric connections should show an error, but the implementation uses showWarningMessage. Either update the description or switch to showErrorMessage if this is intended to be an error-level UX.
| { | ||
| id: TEMPLATE_ID_TSQL_ONPREM, | ||
| name: "TSQL", | ||
| name: "TSQL_OnPrem", |
There was a problem hiding this comment.
Template name values appear user-facing (used as quick-pick labels and exposed to the webview). Renaming them to Standard_OnPrem / Standard_Azure / TSQL_OnPrem / TSQL_Azure makes the UI less readable and inconsistent with the other templates (e.g. TSQL_Locks). Consider keeping a friendly display name (e.g. “Standard”, “TSQL”) and using id (or a separate internal key) for disambiguation/default session naming instead.
| name: "TSQL_OnPrem", | |
| name: "TSQL", |
| test("should prompt for database when Azure SQL has no database selected", async () => { | ||
| const mockTreeNodeInfo = { | ||
| connectionProfile: { | ||
| server: "testserver.database.windows.net", | ||
| authenticationType: "AzureMFA", | ||
| database: "", // No database selected | ||
| }, | ||
| }; | ||
|
|
||
| showQuickPickStub.resolves("UserDB1"); | ||
|
|
||
| createController(); | ||
| const launchCommand = registeredCommands.get("mssql.profiler.launchFromObjectExplorer"); | ||
|
|
||
| await launchCommand!(mockTreeNodeInfo); | ||
|
|
||
| expect(showQuickPickStub).to.have.been.called; | ||
| }); | ||
|
|
||
| test("should prompt for database when Azure SQL has system database selected", async () => { | ||
| const mockTreeNodeInfo = { | ||
| connectionProfile: { | ||
| server: "testserver.database.windows.net", | ||
| authenticationType: "AzureMFA", | ||
| database: "master", // System database | ||
| }, | ||
| }; | ||
|
|
||
| showQuickPickStub.resolves("UserDB1"); | ||
|
|
||
| createController(); | ||
| const launchCommand = registeredCommands.get("mssql.profiler.launchFromObjectExplorer"); | ||
|
|
||
| await launchCommand!(mockTreeNodeInfo); | ||
|
|
||
| expect(showQuickPickStub).to.have.been.called; | ||
| }); |
There was a problem hiding this comment.
These Azure database-selection tests stub showQuickPick to return a string (e.g. "UserDB1"), but the launch flow subsequently uses showQuickPick again for template selection and expects an object with a .template property. This causes the code under test to go down the error path (caught by launchProfilerWithConnection) and the assertions no longer validate the intended behavior. Update the stubs to return different values per call (e.g. onFirstCall() for DB selection, onSecondCall() for template selection, and stub showInputBox accordingly), and add assertions for call count/args so the tests fail if the flow regresses.
| // Set up session created handler to resolve immediately | ||
| (mockProfilerService.onSessionCreated as sinon.SinonStub).callsFake( | ||
| (_ownerUri: string, handler: (params: unknown) => void) => { | ||
| // Simulate session created notification after a short delay | ||
| setTimeout(() => { | ||
| handler({ sessionName: "TestSession", templateName: "Standard" }); | ||
| }, 10); | ||
| return { dispose: sandbox.stub() }; | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Several tests simulate onSessionCreated via setTimeout(..., 10), which can introduce timing flakiness in CI. Prefer using Sinon fake timers or invoking the handler synchronously / via Promise.resolve().then(...) so the test outcome doesn’t depend on real timeouts.
| "Select a database for profiling (Azure SQL requires a specific database)", | ||
| ); | ||
| public static noDatabasesFound = l10n.t( | ||
| "No databases found on the server. Please check your connection.", |
There was a problem hiding this comment.
The warning text noDatabasesFound is shown when no user databases remain after filtering out system DBs, but the message says “No databases found on the server…”. This can be misleading when the server only returns system databases. Consider adjusting the string to explicitly mention “no user databases found” (or similar) to match the actual condition.
| "No databases found on the server. Please check your connection.", | |
| "No user databases found on the server. If this is unexpected, please check your connection or permissions.", |
| // Register handler for session created notification | ||
| const sessionCreatedPromise = new Promise<void>((resolve, reject) => { | ||
| const timeout = setTimeout(() => { | ||
| disposable.dispose(); | ||
| reject(new Error(LocProfiler.sessionCreationTimedOut)); | ||
| }, 30000); // 30 second timeout | ||
|
|
||
| const disposable = this._sessionManager.onSessionCreated( | ||
| profilerUri, | ||
| (params) => { | ||
| clearTimeout(timeout); | ||
| disposable.dispose(); | ||
| this._logger.verbose( | ||
| `Session created notification received: ${params.sessionName}`, | ||
| ); | ||
| resolve(); | ||
| }, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Same issue as above: disposable is referenced in the timeout callback before it’s declared inside the Promise executor, which can fail TypeScript compilation. Refactor to declare let disposable first (and use optional chaining when disposing) so this compiles cleanly.
3 participants
Description
When starting a profiler session, user must select a template and session to start; added also a way to detect if the session is for Azure to filter the templates that can be selected to only those available for Azure. Also added an error if the connection is for Fabric to let the user know it is not supported. Also, for Azure display the database names if the connection doesn't include one so the user can pick the database to be used when starting the session, fixed an error with the Azure templates and added logic to start the session selected if it exists, and to create it if it doesn't.
Code Changes Checklist
npm run test)Reviewers: Please read our reviewer guidelines