From 4f0c6739ec2fa01a38d6768b47711c318f5e22d3 Mon Sep 17 00:00:00 2001 From: eleanorjboyd <26030610+eleanorjboyd@users.noreply.github.com> Date: Fri, 27 Feb 2026 08:20:54 -0800 Subject: [PATCH 1/4] feat: enhance telemetry with error classification and duration tracking for environment manager registration --- src/common/telemetry/constants.ts | 3 +- src/common/telemetry/errorClassifier.ts | 48 +++++++++++++++++++++++++ src/extension.ts | 6 ++++ src/features/envManagers.ts | 4 ++- src/internal.api.ts | 7 ++-- 5 files changed, 63 insertions(+), 5 deletions(-) create mode 100644 src/common/telemetry/errorClassifier.ts diff --git a/src/common/telemetry/constants.ts b/src/common/telemetry/constants.ts index bde80901..46e0a0ac 100644 --- a/src/common/telemetry/constants.ts +++ b/src/common/telemetry/constants.ts @@ -60,7 +60,8 @@ export interface IEventNamePropertyMapping { /* __GDPR__ "environment_manager.registered": { - "managerId" : { "classification": "SystemMetaData", "purpose": "FeatureInsight", "owner": "eleanorjboyd" } + "managerId" : { "classification": "SystemMetaData", "purpose": "FeatureInsight", "owner": "eleanorjboyd" }, + "": { "classification": "SystemMetaData", "purpose": "FeatureInsight", "isMeasurement": true, "owner": "eleanorjboyd" } } */ [EventNames.ENVIRONMENT_MANAGER_REGISTERED]: { diff --git a/src/common/telemetry/errorClassifier.ts b/src/common/telemetry/errorClassifier.ts new file mode 100644 index 00000000..0ca8809a --- /dev/null +++ b/src/common/telemetry/errorClassifier.ts @@ -0,0 +1,48 @@ +import { CancellationError } from 'vscode'; + +export type DiscoveryErrorType = + | 'spawn_timeout' + | 'spawn_enoent' + | 'permission_denied' + | 'canceled' + | 'parse_error' + | 'unknown'; + +/** + * Classifies an error into a telemetry-safe category for the `errorType` property. + * Does NOT include raw error messages (PII risk from file paths) — only the category. + */ +export function classifyError(ex: unknown): DiscoveryErrorType { + if (ex instanceof CancellationError) { + return 'canceled'; + } + + if (!(ex instanceof Error)) { + return 'unknown'; + } + + // Check error code for spawn failures (Node.js sets `code` on spawn errors) + const code = (ex as NodeJS.ErrnoException).code; + if (code === 'ENOENT') { + return 'spawn_enoent'; + } + if (code === 'EACCES' || code === 'EPERM') { + return 'permission_denied'; + } + + // Check message patterns + const msg = ex.message.toLowerCase(); + if (msg.includes('timed out') || msg.includes('timeout')) { + return 'spawn_timeout'; + } + if (msg.includes('parse') || msg.includes('unexpected token') || msg.includes('json')) { + return 'parse_error'; + } + + // Check error name for cancellation variants + if (ex.name === 'CancellationError' || ex.name === 'AbortError') { + return 'canceled'; + } + + return 'unknown'; +} diff --git a/src/extension.ts b/src/extension.ts index b03c5132..704352d1 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -547,6 +547,12 @@ export async function activate(context: ExtensionContext): Promise Date: Fri, 27 Feb 2026 09:15:00 -0800 Subject: [PATCH 2/4] address comments --- src/common/telemetry/errorClassifier.ts | 5 ++ src/extension.ts | 11 ++- .../telemetry/errorClassifier.unit.test.ts | 68 +++++++++++++++++++ 3 files changed, 81 insertions(+), 3 deletions(-) create mode 100644 src/test/common/telemetry/errorClassifier.unit.test.ts diff --git a/src/common/telemetry/errorClassifier.ts b/src/common/telemetry/errorClassifier.ts index 0ca8809a..f9708bcc 100644 --- a/src/common/telemetry/errorClassifier.ts +++ b/src/common/telemetry/errorClassifier.ts @@ -1,4 +1,5 @@ import { CancellationError } from 'vscode'; +import { RpcTimeoutError } from '../../managers/common/nativePythonFinder'; export type DiscoveryErrorType = | 'spawn_timeout' @@ -17,6 +18,10 @@ export function classifyError(ex: unknown): DiscoveryErrorType { return 'canceled'; } + if (ex instanceof RpcTimeoutError) { + return 'spawn_timeout'; + } + if (!(ex instanceof Error)) { return 'unknown'; } diff --git a/src/extension.ts b/src/extension.ts index 704352d1..b0ad9745 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -542,9 +542,14 @@ export async function activate(context: ExtensionContext): Promise { + suite('classifyError', () => { + test('should classify CancellationError as canceled', () => { + assert.strictEqual(classifyError(new CancellationError()), 'canceled'); + }); + + test('should classify RpcTimeoutError as spawn_timeout', () => { + assert.strictEqual(classifyError(new RpcTimeoutError('resolve', 30000)), 'spawn_timeout'); + }); + + test('should classify non-Error values as unknown', () => { + assert.strictEqual(classifyError('string error'), 'unknown'); + assert.strictEqual(classifyError(42), 'unknown'); + assert.strictEqual(classifyError(null), 'unknown'); + assert.strictEqual(classifyError(undefined), 'unknown'); + }); + + test('should classify ENOENT errors as spawn_enoent', () => { + const err = new Error('spawn python ENOENT') as NodeJS.ErrnoException; + err.code = 'ENOENT'; + assert.strictEqual(classifyError(err), 'spawn_enoent'); + }); + + test('should classify EACCES errors as permission_denied', () => { + const err = new Error('permission denied') as NodeJS.ErrnoException; + err.code = 'EACCES'; + assert.strictEqual(classifyError(err), 'permission_denied'); + }); + + test('should classify EPERM errors as permission_denied', () => { + const err = new Error('operation not permitted') as NodeJS.ErrnoException; + err.code = 'EPERM'; + assert.strictEqual(classifyError(err), 'permission_denied'); + }); + + test('should classify timeout messages as spawn_timeout', () => { + assert.strictEqual(classifyError(new Error('Request timed out')), 'spawn_timeout'); + assert.strictEqual(classifyError(new Error('Connection timeout')), 'spawn_timeout'); + }); + + test('should classify parse errors as parse_error', () => { + assert.strictEqual(classifyError(new Error('Failed to parse output')), 'parse_error'); + assert.strictEqual(classifyError(new Error('Unexpected token < in JSON')), 'parse_error'); + assert.strictEqual(classifyError(new Error('Invalid JSON response')), 'parse_error'); + }); + + test('should classify AbortError name as canceled', () => { + const err = new Error('aborted'); + err.name = 'AbortError'; + assert.strictEqual(classifyError(err), 'canceled'); + }); + + test('should classify error with CancellationError name as canceled', () => { + const err = new Error('cancelled'); + err.name = 'CancellationError'; + assert.strictEqual(classifyError(err), 'canceled'); + }); + + test('should classify unrecognized errors as unknown', () => { + assert.strictEqual(classifyError(new Error('something went wrong')), 'unknown'); + }); + }); +}); From 0a0706f91e739d6b46591e4ef7120e3cc1d3092b Mon Sep 17 00:00:00 2001 From: eleanorjboyd <26030610+eleanorjboyd@users.noreply.github.com> Date: Fri, 27 Feb 2026 10:42:29 -0800 Subject: [PATCH 3/4] cleanup --- src/common/telemetry/errorClassifier.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/common/telemetry/errorClassifier.ts b/src/common/telemetry/errorClassifier.ts index f9708bcc..55daf5a3 100644 --- a/src/common/telemetry/errorClassifier.ts +++ b/src/common/telemetry/errorClassifier.ts @@ -11,7 +11,7 @@ export type DiscoveryErrorType = /** * Classifies an error into a telemetry-safe category for the `errorType` property. - * Does NOT include raw error messages (PII risk from file paths) — only the category. + * Does NOT include raw error messages — only the category. */ export function classifyError(ex: unknown): DiscoveryErrorType { if (ex instanceof CancellationError) { From 1df677a7ae6f121664005de1d916d8b1969dfa6d Mon Sep 17 00:00:00 2001 From: eleanorjboyd <26030610+eleanorjboyd@users.noreply.github.com> Date: Fri, 27 Feb 2026 15:43:31 -0800 Subject: [PATCH 4/4] logging to determine CI error --- src/features/envManagers.ts | 11 +++++++ src/managers/builtin/sysPythonManager.ts | 29 +++++++++++++++---- .../pythonProjects.integration.test.ts | 19 +++++++++--- src/test/testUtils.ts | 5 ++-- 4 files changed, 53 insertions(+), 11 deletions(-) diff --git a/src/features/envManagers.ts b/src/features/envManagers.ts index 2cfe62ae..d2b6b70d 100644 --- a/src/features/envManagers.ts +++ b/src/features/envManagers.ts @@ -329,6 +329,13 @@ export class PythonEnvironmentManagers implements EnvironmentManagers { }, ]); } + traceVerbose( + `[setEnvironment] scope=${scope instanceof Uri ? scope.fsPath : scope}, ` + + `env=${environment?.envId?.id ?? 'undefined'}, manager=${manager.id}, ` + + `project=${project?.uri?.toString() ?? 'none'}, ` + + `packageManager=${this.getPackageManager(environment)?.id ?? 'UNDEFINED'}, ` + + `settingsPersisted=${!!(project && this.getPackageManager(environment))}`, + ); } const key = project ? project.uri.toString() : 'global'; @@ -505,6 +512,10 @@ export class PythonEnvironmentManagers implements EnvironmentManagers { async getEnvironment(scope: GetEnvironmentScope): Promise { const manager = this.getEnvironmentManager(scope); if (!manager) { + traceVerbose( + `[getEnvironment] No manager found for scope=${scope instanceof Uri ? scope.fsPath : scope}, ` + + `settingsManagerId=${getDefaultEnvManagerSetting(this.pm, scope instanceof Uri ? scope : undefined)}`, + ); return undefined; } diff --git a/src/managers/builtin/sysPythonManager.ts b/src/managers/builtin/sysPythonManager.ts index f0d5661f..6604471c 100644 --- a/src/managers/builtin/sysPythonManager.ts +++ b/src/managers/builtin/sysPythonManager.ts @@ -166,13 +166,20 @@ export class SysPythonManager implements EnvironmentManager { const pw = this.api.getPythonProject(scope); if (!pw) { this.log.warn( - `Unable to set environment for ${scope.fsPath}: Not a python project, folder or PEP723 script.`, - this.api.getPythonProjects().map((p) => p.uri.fsPath), + `[SYS_SET] Unable to set environment for ${scope.fsPath}: Not a python project. ` + + `Known projects: [${this.api + .getPythonProjects() + .map((p) => p.uri.fsPath) + .join(', ')}]`, ); return; } const normalizedPwPath = normalizePath(pw.uri.fsPath); + this.log.info( + `[SYS_SET] scope=${scope.fsPath}, project=${pw.uri.fsPath}, ` + + `normalizedKey=${normalizedPwPath}, env=${environment?.envId?.id ?? 'undefined'}`, + ); if (environment) { this.fsPathToEnv.set(normalizedPwPath, environment); } else { @@ -297,16 +304,28 @@ export class SysPythonManager implements EnvironmentManager { } private fromEnvMap(uri: Uri): PythonEnvironment | undefined { + const normalizedUri = normalizePath(uri.fsPath); // Find environment directly using the URI mapping - const env = this.fsPathToEnv.get(normalizePath(uri.fsPath)); + const env = this.fsPathToEnv.get(normalizedUri); if (env) { return env; } // Find environment using the Python project for the Uri const project = this.api.getPythonProject(uri); - if (project) { - return this.fsPathToEnv.get(normalizePath(project.uri.fsPath)); + const projectKey = project ? normalizePath(project.uri.fsPath) : undefined; + const projectEnv = projectKey ? this.fsPathToEnv.get(projectKey) : undefined; + + this.log.info( + `[SYS_GET] uri=${uri.fsPath}, normalizedKey=${normalizedUri}, ` + + `project=${project?.uri?.fsPath ?? 'none'}, projectKey=${projectKey ?? 'none'}, ` + + `mapKeys=[${Array.from(this.fsPathToEnv.keys()).join(', ')}], ` + + `directHit=${!!env}, projectHit=${!!projectEnv}, ` + + `fallbackToGlobal=${!projectEnv}, globalEnv=${this.globalEnv?.envId?.id ?? 'none'}`, + ); + + if (projectEnv) { + return projectEnv; } return this.globalEnv; diff --git a/src/test/integration/pythonProjects.integration.test.ts b/src/test/integration/pythonProjects.integration.test.ts index fd0feb68..cb1b730d 100644 --- a/src/test/integration/pythonProjects.integration.test.ts +++ b/src/test/integration/pythonProjects.integration.test.ts @@ -185,6 +185,7 @@ suite('Integration: Python Projects', function () { // Track what getEnvironment returns during polling for diagnostics let pollCount = 0; let lastRetrievedId: string | undefined; + let lastRetrievedManagerId: string | undefined; // Wait for the environment to be retrievable with the correct ID // This handles async persistence across platforms @@ -194,16 +195,18 @@ suite('Integration: Python Projects', function () { const retrieved = await api.getEnvironment(project.uri); pollCount++; const retrievedId = retrieved?.envId?.id; + lastRetrievedManagerId = retrieved?.envId?.managerId; if (retrievedId !== lastRetrievedId) { console.log( - `[TEST DEBUG] Poll #${pollCount}: getEnvironment returned envId=${retrievedId ?? 'undefined'}`, + `[TEST DEBUG] Poll #${pollCount}: getEnvironment returned envId=${retrievedId ?? 'undefined'}, managerId=${lastRetrievedManagerId ?? 'undefined'}`, ); lastRetrievedId = retrievedId; } return retrieved !== undefined && retrieved.envId.id === env.envId.id; }, 15_000, - `Environment was not set correctly. Expected envId: ${env.envId.id}, last retrieved: ${lastRetrievedId}`, + () => + `Environment was not set correctly. Expected envId: ${env.envId.id} (manager: ${env.envId.managerId}), last retrieved: ${lastRetrievedId ?? 'undefined'} (manager: ${lastRetrievedManagerId ?? 'undefined'}) after ${pollCount} polls`, ); // Final verification @@ -285,13 +288,16 @@ suite('Integration: Python Projects', function () { // Wait for it to be set // Use 15s timeout - CI runners can be slow with settings persistence + let clearTestLastId: string | undefined; await waitForCondition( async () => { const retrieved = await api.getEnvironment(project.uri); + clearTestLastId = retrieved?.envId?.id; return retrieved !== undefined && retrieved.envId.id === env.envId.id; }, 15_000, - 'Environment was not set before clearing', + () => + `Environment was not set before clearing. Expected: ${env.envId.id} (manager: ${env.envId.managerId}), got: ${clearTestLastId ?? 'undefined'}`, ); // Verify it was set @@ -344,13 +350,18 @@ suite('Integration: Python Projects', function () { // Wait for it to be set // Use 15s timeout - CI runners can be slow with settings persistence + let fileTestLastId: string | undefined; + let fileTestLastManagerId: string | undefined; await waitForCondition( async () => { const retrieved = await api.getEnvironment(project.uri); + fileTestLastId = retrieved?.envId?.id; + fileTestLastManagerId = retrieved?.envId?.managerId; return retrieved !== undefined && retrieved.envId.id === env.envId.id; }, 15_000, - 'Environment was not set for project', + () => + `Environment was not set for project. Expected: ${env.envId.id} (manager: ${env.envId.managerId}), got: ${fileTestLastId ?? 'undefined'} (manager: ${fileTestLastManagerId ?? 'undefined'})`, ); // Create a hypothetical file path inside the project diff --git a/src/test/testUtils.ts b/src/test/testUtils.ts index 5d0e4195..87df07a5 100644 --- a/src/test/testUtils.ts +++ b/src/test/testUtils.ts @@ -48,7 +48,7 @@ export function sleep(ms: number): Promise { export async function waitForCondition( condition: () => boolean | Promise, timeoutMs: number = 10_000, - errorMessage: string = 'Condition not met within timeout', + errorMessage: string | (() => string) = 'Condition not met within timeout', pollIntervalMs: number = 100, ): Promise { return new Promise((resolve, reject) => { @@ -66,7 +66,8 @@ export async function waitForCondition( } if (Date.now() - startTime >= timeoutMs) { - reject(new Error(`${errorMessage} (waited ${timeoutMs}ms)`)); + const msg = typeof errorMessage === 'function' ? errorMessage() : errorMessage; + reject(new Error(`${msg} (waited ${timeoutMs}ms)`)); return; }