-
Notifications
You must be signed in to change notification settings - Fork 41
feat: add telemetry for PET process failures and exits #1306
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -522,12 +522,39 @@ export async function activate(context: ExtensionContext): Promise<PythonEnviron | |
| * Below are all the contributed features using the APIs. | ||
| */ | ||
| setImmediate(async () => { | ||
| // Resolve the pet binary and spawn the finder process. Failures here are pet related. | ||
| let nativeFinder: NativePythonFinder; | ||
| try { | ||
| nativeFinder = await createNativePythonFinder(outputChannel, api, context); | ||
| } catch (error) { | ||
| traceError('Failed to start Python finder (pet):', error); | ||
|
|
||
| const errnoError = error as NodeJS.ErrnoException; | ||
| // Plain Error (no .code) = binary not found by getNativePythonToolsPath. | ||
| // Errno error (has .code) = spawn failed (ENOENT, EACCES, EPERM, etc.). | ||
| const reason = errnoError.code ? 'spawn_failed' : 'binary_not_found'; | ||
| sendTelemetryEvent(EventNames.PET_START_FAILED, undefined, { | ||
| errorCode: errnoError.code ?? 'UNKNOWN', | ||
| reason, | ||
| platform: process.platform, | ||
| arch: process.arch, | ||
| }); | ||
|
|
||
| window.showErrorMessage( | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @karthiknadig if we cannot find the binary, is that a game-over scenario ? how should we present / prompt the user? I guess they could enter a interpreter path themselves so maybe its a notification instead?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If |
||
| l10n.t( | ||
| 'Python Environments: Failed to start the Python finder. Some features may not work correctly. Check the Output panel for details.', | ||
| ), | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| context.subscriptions.push(nativeFinder); | ||
| const sysMgr = new SysPythonManager(nativeFinder, api, outputChannel); | ||
| sysPythonManager.resolve(sysMgr); | ||
|
|
||
| // Manager registration and post-registration setup. safeRegister() absorbs | ||
| // individual manager failures, so errors here are unexpected and non-pet-related. | ||
| try { | ||
| // This is the finder that is used by all the built in environment managers | ||
| const nativeFinder: NativePythonFinder = await createNativePythonFinder(outputChannel, api, context); | ||
| context.subscriptions.push(nativeFinder); | ||
| const sysMgr = new SysPythonManager(nativeFinder, api, outputChannel); | ||
| sysPythonManager.resolve(sysMgr); | ||
| // Each manager registers independently — one failure must not block the others. | ||
| await Promise.all([ | ||
| safeRegister( | ||
|
|
@@ -567,7 +594,6 @@ export async function activate(context: ExtensionContext): Promise<PythonEnviron | |
| await logDiscoverySummary(envManagers); | ||
| } catch (error) { | ||
| traceError('Failed to initialize environment managers:', error); | ||
| // Show a user-friendly error message | ||
| window.showErrorMessage( | ||
| l10n.t( | ||
| 'Python Environments: Failed to initialize environment managers. Some features may not work correctly. Check the Output panel for details.', | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,8 @@ import { spawnProcess } from '../../common/childProcess.apis'; | |
| import { ENVS_EXTENSION_ID, PYTHON_EXTENSION_ID } from '../../common/constants'; | ||
| import { getExtension } from '../../common/extension.apis'; | ||
| import { traceError, traceVerbose, traceWarn } from '../../common/logging'; | ||
| import { EventNames } from '../../common/telemetry/constants'; | ||
| import { sendTelemetryEvent } from '../../common/telemetry/sender'; | ||
| import { untildify, untildifyArray } from '../../common/utils/pathUtils'; | ||
| import { isWindows } from '../../common/utils/platformUtils'; | ||
| import { createRunningWorkerPool, WorkerPool } from '../../common/utils/workerPool'; | ||
|
|
@@ -78,17 +80,27 @@ export async function getNativePythonToolsPath(): Promise<string> { | |
| const envsExt = getExtension(ENVS_EXTENSION_ID); | ||
| if (envsExt) { | ||
| const petPath = path.join(envsExt.extensionPath, 'python-env-tools', 'bin', isWindows() ? 'pet.exe' : 'pet'); | ||
| if (await fs.pathExists(petPath)) { | ||
| const exists = await fs.pathExists(petPath); | ||
| traceVerbose(`[pet] Primary path (envs-ext): ${petPath} — exists: ${exists}`); | ||
| if (exists) { | ||
| return petPath; | ||
| } | ||
| } else { | ||
| traceVerbose(`[pet] Envs extension (${ENVS_EXTENSION_ID}) not found; trying Python extension fallback`); | ||
| } | ||
|
|
||
| const python = getExtension(PYTHON_EXTENSION_ID); | ||
| if (!python) { | ||
| throw new Error('Python extension not found'); | ||
| throw new Error('Python extension not found and envs extension pet binary is missing'); | ||
| } | ||
|
|
||
| return path.join(python.extensionPath, 'python-env-tools', 'bin', isWindows() ? 'pet.exe' : 'pet'); | ||
| const fallbackPath = path.join(python.extensionPath, 'python-env-tools', 'bin', isWindows() ? 'pet.exe' : 'pet'); | ||
| const fallbackExists = await fs.pathExists(fallbackPath); | ||
| traceVerbose(`[pet] Fallback path (python-ext): ${fallbackPath} — exists: ${fallbackExists}`); | ||
| if (!fallbackExists) { | ||
| throw new Error(`Python finder binary not found at: ${fallbackPath}`); | ||
| } | ||
| return fallbackPath; | ||
| } | ||
|
|
||
| export interface NativeEnvInfo { | ||
|
|
@@ -224,6 +236,7 @@ class NativePythonFinderImpl implements NativePythonFinder { | |
| private startFailed: boolean = false; | ||
| private restartAttempts: number = 0; | ||
| private isRestarting: boolean = false; | ||
| private isDisposed: boolean = false; | ||
| private readonly configureRetry = new ConfigureRetryState(); | ||
|
|
||
| constructor( | ||
|
|
@@ -426,6 +439,7 @@ class NativePythonFinderImpl implements NativePythonFinder { | |
| } | ||
|
|
||
| public dispose() { | ||
| this.isDisposed = true; | ||
| this.pool.stop(); | ||
| this.startDisposables.forEach((d) => d.dispose()); | ||
| this.connection.dispose(); | ||
|
|
@@ -475,11 +489,18 @@ class NativePythonFinderImpl implements NativePythonFinder { | |
| // Handle process exit - mark as exited so pending requests fail fast | ||
| this.proc.on('exit', (code, signal) => { | ||
| this.processExited = true; | ||
| const wasExpected = this.isRestarting || this.isDisposed; | ||
| if (code !== 0) { | ||
| this.outputChannel.error( | ||
| `[pet] Python Environment Tools exited unexpectedly with code ${code}, signal ${signal}`, | ||
| ); | ||
| } | ||
|
Comment on lines
+499
to
504
|
||
| sendTelemetryEvent(EventNames.PET_PROCESS_EXIT, undefined, { | ||
| exitCode: code, | ||
| signal: signal ?? null, | ||
| restartAttempt: this.restartAttempts, | ||
| wasExpected, | ||
| }); | ||
| }); | ||
|
|
||
| // Handle process errors (e.g., ENOENT if executable not found) | ||
|
|
@@ -898,5 +919,7 @@ export async function createNativePythonFinder( | |
| api: PythonProjectApi, | ||
| context: ExtensionContext, | ||
| ): Promise<NativePythonFinder> { | ||
| return new NativePythonFinderImpl(outputChannel, await getNativePythonToolsPath(), api, getCacheDirectory(context)); | ||
| const petPath = await getNativePythonToolsPath(); | ||
| traceVerbose(`[pet] Resolved pet binary path: ${petPath}`); | ||
| return new NativePythonFinderImpl(outputChannel, petPath, api, getCacheDirectory(context)); | ||
| } | ||
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.
The PET_START_FAILED
reasonclassification assumes that any error without anerrnoError.codemust be "binary_not_found", butcreateNativePythonFinder()can also throw plainErrors after the binary is found/spawned (e.g., missing stdio streams, JSON-RPC setup failures). This will misattribute failures in telemetry. Consider settingreasonto'unknown'by default and only using'binary_not_found'for the specific errors thrown bygetNativePythonToolsPath()(e.g., by throwing a dedicated error type or attaching a discriminator).