Fix PET timeout telemetry attribution

[Karthik Nadig (karthiknadig)]

Summary

Correct PET/setup telemetry so timeout investigations retain numeric context and reliable binary attribution.

• send refresh, configure, and restart counts through telemetry measurements
• classify PET JSON-RPC timeouts by method and preserve refresh failure context
• retain partial locator timing on failed refreshes
• retry transient PET info timeouts while retaining metadata only for an unchanged binary
• derive Conda counts from the caller-owned enum identity
• regression-test setup stage duration, timeout categories, refresh measures, retry bounds, and binary attribution

Validation

• npm run lint
• npm run compile-tests
• npm run unittest (1504 passing, 5 pending)
• npm run compile

Fixes microsoft/python-environment-tools#478

[Copilot]

Pull request overview

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

This PR adjusts PET/setup telemetry so timeout investigations keep numeric measurement context and more accurate timeout attribution (including JSON-RPC method classification), while improving resilience of PET info attribution across restarts.

Changes:

• Move refresh/configure/restart numeric context into telemetry measurements (and add refresh measurement builder helper).
• Classify RpcTimeoutError by JSON-RPC method and update telemetry “timeout vs error” result attribution.
• Retry transient PET info timeouts with bounded attempts and preserve last-known build metadata across restarts; add unit tests for these behaviors.

Show a summary per file

File	Description
src/managers/common/nativePythonFinder.ts	Adds refresh measurement builder + RPC-timeout retry helper; updates PET telemetry payloads, timeout attribution, and info fetch retry/persistence.
src/common/telemetry/errorClassifier.ts	Updates timeout classification to be method-specific for RpcTimeoutError.
src/common/telemetry/constants.ts	Adjusts telemetry property mappings to reflect movement of numeric fields into measurements.
src/test/managers/common/nativePythonFinder.telemetry.unit.test.ts	Adds unit tests for refresh measurement building and RPC-timeout retry behavior.
src/test/common/telemetry/sender.unit.test.ts	Adds regression test asserting sender passes durations as measurements.
src/test/common/telemetry/errorClassifier.unit.test.ts	Updates tests to validate method-specific RPC timeout categorization.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 4
• Review effort level: Lite

[Copilot]

Review details

Suppressed comments (4)

src/managers/common/petTelemetry.ts:46

• The conda detection relies on the magic string 'Conda', which can drift from the actual NativePythonEnvironmentKind values (e.g., casing changes or enum string values). Consider normalizing info.kind (e.g., lowercasing) and comparing against a single canonical value, or accepting both expected representations, so condaEnvCount remains stable if upstream naming changes.

    for (const info of input.nativeInfo) {
        if (info.tool) {
            managerCount++;
        } else {
            envCount++;
            if (info.kind === 'Conda') {
                condaEnvCount++;
            }
        }
    }

src/managers/common/nativePythonFinder.ts:777

• With this.petInfo no longer reset on start(), a newly started PET process can inherit stale build attribution if the PET binary is replaced in-place between restarts (same path, different build) and the info call fails (e.g., transient failures). To avoid incorrect attribution, consider invalidating petInfo when you can detect a binary change (e.g., by storing and comparing executable mtime/size, or clearing the cache when the failure indicates an older binary such as a method-not-found response).

        connection.listen();

        // Stamp PET telemetry with version/buildId/commitSha. Fire-and-forget — must not block refresh.
        this.kickoffInfoFetch(connection);

src/managers/common/nativePythonFinder.ts:973

• On refresh failures, locatorsJson (and any available locator timing context) is not included even when refreshPerf may have partial data. Since the success path includes locatorsJson, consider also attaching locatorsJson in the error path when refreshPerf is present to improve failure investigations without changing the numeric measures payload.

                {
                    result: ex instanceof RpcTimeoutError ? 'timeout' : 'error',
                    errorType,
                    ...this.getPetInfoProperties(),
                },

src/managers/common/nativePythonFinder.ts:294

• The new input-validation behavior for retryRpcTimeout (throwing RangeError when maxAttempts < 1 or non-integer) isn’t covered by the new unit tests. Add a unit test to assert the RangeError is thrown for invalid attempt limits (e.g., 0 and 1.5) so the contract stays stable.

    if (!Number.isInteger(maxAttempts) || maxAttempts < 1) {
        throw new RangeError('maxAttempts must be a positive integer');
    }

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

Review details

Suppressed comments (1)

src/common/telemetry/errorClassifier.ts:37

• classifyError() now maps RpcTimeoutError to rpc_*_timeout/rpc_timeout instead of spawn_timeout. Any callers that infer result: 'timeout' by checking only 'spawn_timeout' will now misclassify PET JSON-RPC timeouts as 'error' (e.g. src/internal.api.ts uses errorType === 'spawn_timeout' when setting ENVIRONMENT_DISCOVERY.result). Update those call sites to treat rpc_timeout and rpc_*_timeout as timeouts as well (or centralize that logic in a shared helper).

    if (ex instanceof RpcTimeoutError) {
        switch (ex.method) {
            case 'configure':
                return 'rpc_configure_timeout';
            case 'refresh':

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

Review details

Suppressed comments (3)

src/managers/common/nativePythonFinder.ts:997

• In this error path, result is based on ex instanceof RpcTimeoutError, but errorType may be a timeout category even when the exception is not RpcTimeoutError (e.g. spawn_timeout). This risks emitting result: "error" with a timeout errorType, which makes timeout slices unreliable. Use the classified errorType to decide the result.

            const errorType = classifyError(ex);
            sendTelemetryEvent(
                EventNames.PET_REFRESH,
                getRefreshTelemetryMeasures({
                    duration: sw.elapsedTime,
                    nativeInfo,
                    condaKind: NativePythonEnvironmentKind.conda,
                    unresolvedCount,
                    workspaceDirCount,
                    searchPathCount,
                    attempt,
                    refreshPerformance: refreshPerf,
                }),
                {
                    result: ex instanceof RpcTimeoutError ? 'timeout' : 'error',
                    errorType,
                    locatorsJson: refreshPerf ? JSON.stringify(refreshPerf.locators) : undefined,
                    ...this.getPetInfoProperties(),
                },

src/managers/common/nativePythonFinder.ts:1066

• errorType is already classified for telemetry, but result is still derived from ex instanceof RpcTimeoutError. If classifyError(ex) returns a timeout category for a non-RpcTimeoutError (e.g. spawn_timeout from message matching), telemetry could report result: "error" with a timeout errorType. Consider computing errorType once and deriving result from it.

        } catch (ex) {
            sendTelemetryEvent(
                EventNames.PET_CONFIGURE,
                { duration: sw.elapsedTime, workspaceDirCount, envDirCount, retryCount },
                {
                    result: ex instanceof RpcTimeoutError ? 'timeout' : 'error',
                    errorType: classifyError(ex),
                },

src/managers/common/nativePythonFinder.ts:427

• Telemetry 'result' is derived from ex instanceof RpcTimeoutError, but the errorType used for the same event can represent timeouts that are not RpcTimeoutError instances (e.g. spawn_timeout via message matching, or future timeout categories). This can yield inconsistent telemetry like result: "error" with errorType: "spawn_timeout". Consider basing result on the classified errorType instead.

This issue also appears in the following locations of the same file:

• line 979
• line 1059

            const errorType = classifyError(ex);
            sendTelemetryEvent(
                EventNames.PET_RESOLVE,
                sw.elapsedTime,
                {
                    result: ex instanceof RpcTimeoutError ? 'timeout' : 'error',
                    errorType,
                    ...this.getPetInfoProperties(),
                },

• Files reviewed: 8/8 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

Review details

• Files reviewed: 8/8 changed files
• Comments generated: 0 new
• Review effort level: Lite