From 178511f8ff08c10a70d5df66e457b1df9b8f8aa0 Mon Sep 17 00:00:00 2001 From: Christian McDermott Date: Wed, 16 Sep 2026 14:51:04 -0400 Subject: [PATCH 1/2] feat(console): give connector events a failure reason and a duration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `connector_installed` carried an outcome and no reason, so a failed connector install was a bare count with nothing to act on — the one event of its kind in the catalogue without a `failure_reason`, and the underlying `SwitchSetupResult` carries only a message, which cannot be sent. Add `TelemetryConnectorFailure`, named at the point of failure rather than mapped from an error type. The codes separate walls that need different fixes: a marketplace that will not register, a host CLI that refuses the plugin, and the app's own file writing. The reinstall split is the one that matters most — Codex has no update verb, so an update is remove-then-add, and `was_reinstall` is true for both halves. It cannot tell "nothing changed" from "the agent now has no connector at all"; `uninstall_command_failed` vs `install_command_failed` can. Add `duration_ms` to the three connector events and to `agent_cli_action`, where a CLI install taking four minutes and one taking ten seconds were otherwise the same row. Measured on a monotonic clock so a clock step cannot yield a negative duration, in whole milliseconds, and deliberately uncapped: a capped duration is a number the operation did not take. Timed around the operation alone, excluding manager and SSH resolution, so the first measurement of a session is not systematically different from the rest. Along the way: - `connector_installed` uses `agentTypeOf()` like its siblings rather than an inline `isValidProviderId` ternary. - `runFiles` folds resolving into its own try on both drivers, which deletes the duplicated guard in `runInstall` and fixes `update` and `uninstall` on a files connector with no behavior — those threw out to the UI as a stack with nothing reported. - Remote's `resolveFiles` carries the connector version like local's, removing three `connectorVersion()` call sites. - The consent copy gains a line. Widening what is sent is a consent decision, not a code change. - `sentAttributes()` in the telemetry service test read only `stringValue`, so every number and boolean in a payload read back as `undefined` — an assertion that a numeric property was carried would have passed against a payload that dropped it. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/main/core/providers/controller.ts | 17 +- .../src/main/core/remote-hosts/controller.ts | 17 +- .../switch-setup/remote-switch-setup.test.ts | 13 ++ .../core/switch-setup/remote-switch-setup.ts | 166 ++++++++-------- .../switch-setup/switch-setup-service.test.ts | 146 ++++++++++++++ .../core/switch-setup/switch-setup-service.ts | 184 ++++++++++-------- .../src/main/core/telemetry/catalogue.test.ts | 7 +- .../src/main/core/telemetry/duration.test.ts | 31 +++ .../src/main/core/telemetry/duration.ts | 25 +++ .../src/main/core/telemetry/events.ts | 85 +++++++- .../main/core/telemetry/relay-client.test.ts | 8 +- .../core/telemetry/telemetry-service.test.ts | 29 ++- .../features/telemetry/telemetry-copy.ts | 1 + 13 files changed, 555 insertions(+), 174 deletions(-) create mode 100644 console/apps/switch-console-desktop/src/main/core/telemetry/duration.test.ts create mode 100644 console/apps/switch-console-desktop/src/main/core/telemetry/duration.ts diff --git a/console/apps/switch-console-desktop/src/main/core/providers/controller.ts b/console/apps/switch-console-desktop/src/main/core/providers/controller.ts index ffc9a18b8..aa7ad8d27 100644 --- a/console/apps/switch-console-desktop/src/main/core/providers/controller.ts +++ b/console/apps/switch-console-desktop/src/main/core/providers/controller.ts @@ -7,6 +7,7 @@ import type { } from '@switch-console/core/deps/runtime'; import { agentTypeOf } from '@main/core/telemetry/agent-type'; import { cliFailureReason } from '@main/core/telemetry/cli-failure'; +import { startTimer } from '@main/core/telemetry/duration'; import type { TelemetryCliAction } from '@main/core/telemetry/events'; import { installMethodOf } from '@main/core/telemetry/narrow'; import { trackEvent } from '@main/core/telemetry/telemetry-service'; @@ -43,7 +44,8 @@ function reportCliAction( action: TelemetryCliAction, id: string, method: InstallMethod | undefined, - result: { success: boolean; error?: { type?: string } } + result: { success: boolean; error?: { type?: string } }, + durationMs: number ): void { trackEvent('agent_cli_action', { agent_type: agentTypeOf(id), @@ -52,6 +54,7 @@ function reportCliAction( action, outcome: result.success ? 'success' : 'failure', failure_reason: cliFailureReason(result), + duration_ms: durationMs, }); } @@ -98,8 +101,12 @@ export const providersController = createRPCController({ install: async (id: AgentProviderId, connectionId?: string, method?: InstallMethod) => { const mgr = await getDependencyManager(connectionId); + // Timed around the operation alone. Resolving the manager is a lookup that + // says nothing about how long an install takes, and including it would make + // the first measurement of a session differ from the rest for no reason. + const elapsed = startTimer(); const result = await mgr.install(id, method); - reportCliAction('install', id, method, result); + reportCliAction('install', id, method, result, elapsed()); if (result.success) { // Persist the chosen method as an override, or clear to auto when no method was chosen. // Do NOT auto-promote the inferred method — that would freeze a heuristic guess. @@ -112,15 +119,17 @@ export const providersController = createRPCController({ update: async (id: AgentProviderId, connectionId?: string, method?: InstallMethod) => { const mgr = await getDependencyManager(connectionId); + const elapsed = startTimer(); const result = await mgr.update(id, method); - reportCliAction('update', id, method, result); + reportCliAction('update', id, method, result, elapsed()); return result; }, uninstall: async (id: AgentProviderId, connectionId?: string, method?: InstallMethod) => { const mgr = await getDependencyManager(connectionId); + const elapsed = startTimer(); const result = await mgr.uninstall(id, method); - reportCliAction('uninstall', id, method, result); + reportCliAction('uninstall', id, method, result, elapsed()); return result; }, diff --git a/console/apps/switch-console-desktop/src/main/core/remote-hosts/controller.ts b/console/apps/switch-console-desktop/src/main/core/remote-hosts/controller.ts index b7c8769ac..40cce3003 100644 --- a/console/apps/switch-console-desktop/src/main/core/remote-hosts/controller.ts +++ b/console/apps/switch-console-desktop/src/main/core/remote-hosts/controller.ts @@ -15,6 +15,7 @@ import { import { getRemoteSwitchSetupService } from '@main/core/switch-setup/remote-switch-setup'; import { agentTypeOf } from '@main/core/telemetry/agent-type'; import { cliFailureReason } from '@main/core/telemetry/cli-failure'; +import { startTimer } from '@main/core/telemetry/duration'; import type { TelemetryCliAction } from '@main/core/telemetry/events'; import { installMethodOf } from '@main/core/telemetry/narrow'; import { trackEvent } from '@main/core/telemetry/telemetry-service'; @@ -102,7 +103,8 @@ function reportRemoteCliAction( action: TelemetryCliAction, id: string, method: InstallMethod | undefined, - result: { success: boolean; error?: { type?: string } } + result: { success: boolean; error?: { type?: string } }, + durationMs: number ): void { trackEvent('agent_cli_action', { agent_type: agentTypeOf(id), @@ -111,6 +113,7 @@ function reportRemoteCliAction( action, outcome: result.success ? 'success' : 'failure', failure_reason: cliFailureReason(result), + duration_ms: durationMs, }); } @@ -237,15 +240,20 @@ export const remoteHostsController = createRPCController({ method?: InstallMethod; }): Promise => { const manager = await getRemoteDependencyManager(params.sshHost); + // Timed around the operation alone: resolving the manager may open the SSH + // connection, which is not part of how long an install takes and would show + // up only on the first one of a session. + const elapsed = startTimer(); const result = await manager.install(params.id, params.method); - reportRemoteCliAction('install', params.id, params.method, result); + reportRemoteCliAction('install', params.id, params.method, result, elapsed()); return result; }, updateDep: async (params: { sshHost: string; id: string }): Promise => { const manager = await getRemoteDependencyManager(params.sshHost); + const elapsed = startTimer(); const result = await manager.update(params.id); - reportRemoteCliAction('update', params.id, undefined, result); + reportRemoteCliAction('update', params.id, undefined, result, elapsed()); return result; }, @@ -254,8 +262,9 @@ export const remoteHostsController = createRPCController({ id: string; }): Promise => { const manager = await getRemoteDependencyManager(params.sshHost); + const elapsed = startTimer(); const result = await manager.uninstall(params.id); - reportRemoteCliAction('uninstall', params.id, undefined, result); + reportRemoteCliAction('uninstall', params.id, undefined, result, elapsed()); return result; }, diff --git a/console/apps/switch-console-desktop/src/main/core/switch-setup/remote-switch-setup.test.ts b/console/apps/switch-console-desktop/src/main/core/switch-setup/remote-switch-setup.test.ts index a3c26a433..dad9b1008 100644 --- a/console/apps/switch-console-desktop/src/main/core/switch-setup/remote-switch-setup.test.ts +++ b/console/apps/switch-console-desktop/src/main/core/switch-setup/remote-switch-setup.test.ts @@ -408,6 +408,9 @@ describe('RemoteSwitchSetupService.install', () => { agent_type: 'codex', target: 'remote', outcome: 'success', + failure_reason: 'none', + // Elapsed wall time: a real number, but not one a test can pin. + duration_ms: expect.any(Number), }); }); @@ -430,6 +433,8 @@ describe('RemoteSwitchSetupService.install', () => { agent_type: 'codex', target: 'remote', outcome: 'failure', + failure_reason: 'install_command_failed', + duration_ms: expect.any(Number), }); }); @@ -489,6 +494,8 @@ describe('RemoteSwitchSetupService.install', () => { agent_type: 'opencode', target: 'remote', outcome: 'success', + failure_reason: 'none', + duration_ms: expect.any(Number), }); }); @@ -503,6 +510,8 @@ describe('RemoteSwitchSetupService.install', () => { agent_type: 'opencode', target: 'remote', outcome: 'failure', + failure_reason: 'files_write_failed', + duration_ms: expect.any(Number), }); }); @@ -519,10 +528,14 @@ describe('RemoteSwitchSetupService.install', () => { const result = await service.install('opencode'); expect(result.success).toBe(false); + // Its own code, not `files_write_failed`: a fault in the plugin rather + // than on the host, and the two would otherwise be one number. expect(mocks.trackEvent).toHaveBeenCalledWith('connector_installed', { agent_type: 'opencode', target: 'remote', outcome: 'failure', + failure_reason: 'files_unimplemented', + duration_ms: expect.any(Number), }); }); }); diff --git a/console/apps/switch-console-desktop/src/main/core/switch-setup/remote-switch-setup.ts b/console/apps/switch-console-desktop/src/main/core/switch-setup/remote-switch-setup.ts index 8c1d2da1f..2c854c684 100644 --- a/console/apps/switch-console-desktop/src/main/core/switch-setup/remote-switch-setup.ts +++ b/console/apps/switch-console-desktop/src/main/core/switch-setup/remote-switch-setup.ts @@ -6,14 +6,19 @@ import { SshExecutionContext } from '@main/core/execution-context/ssh-execution- import { sshConnectionIdForHost } from '@main/core/locations/location-transport'; import { ensureSshConnected } from '@main/core/ssh/connect/connect-agent-ssh'; import { agentTypeOf } from '@main/core/telemetry/agent-type'; +import { startTimer } from '@main/core/telemetry/duration'; import { trackEvent } from '@main/core/telemetry/telemetry-service'; import { log } from '@main/lib/logger'; import { isNewerVersion } from '@main/lib/semver'; -import { isValidProviderId } from '@shared/core/providers/agent-provider-registry'; import { getPlugin, listPlugins } from '../providers/plugin-registry'; import { cliRulesFor, type SwitchSetupCliRules } from './switch-setup-cli-dialect'; -import type { SwitchSetupResult, SwitchSetupStatus } from './switch-setup-service'; -import { marketplaceMatchesSource } from './switch-setup-service'; +import type { ConnectorRun, SwitchSetupResult, SwitchSetupStatus } from './switch-setup-service'; +import { + connectorFailed, + connectorSucceeded, + connectorUnsupported, + marketplaceMatchesSource, +} from './switch-setup-service'; const EXEC_TIMEOUT_MS = 120_000; @@ -121,7 +126,11 @@ export class RemoteSwitchSetupService { `Agent '${agentId}' declares a file-based Switch connector but implements no behavior for it.` ); } - return { files, homeFs: createRemoteHomePluginFs(this.ctx) }; + return { + files, + homeFs: createRemoteHomePluginFs(this.ctx), + version: connectorVersion(agentId), + }; } private async resolve(agentId: string) { @@ -269,9 +278,10 @@ export class RemoteSwitchSetupService { * there is, and an install stamped with an older one is what "update * available" means. */ - private async filesStatus(agentId: string, version: string): Promise { + private async filesStatus(agentId: string): Promise { const resolved = this.resolveFiles(agentId); if (!resolved) return unsupported(agentId); + const { version } = resolved; const installedVersion = await resolved.files.installedVersion(resolved.homeFs); return { agentId, @@ -284,26 +294,45 @@ export class RemoteSwitchSetupService { }; } - /** Install, update and uninstall for a file-based connector on this host. */ + /** + * Install, update and uninstall for a file-based connector on this host. + * + * Resolving is inside the try for the same reason as locally: it throws for a + * connector that declares files and implements none, and an operation the user + * asked for must come back as a failed result rather than as a rejection that + * skips the report and reaches the UI as a stack. + */ private async runFiles( agentId: string, - action: (files: ISwitchSetupFilesBehavior, homeFs: PluginFs) => Promise - ): Promise { - const resolved = this.resolveFiles(agentId); - if (!resolved) - return { success: false, message: 'Switch setup is not supported for this agent.' }; + action: ( + files: ISwitchSetupFilesBehavior, + homeFs: PluginFs, + version: string + ) => Promise + ): Promise { + let resolved: ReturnType; try { - await action(resolved.files, resolved.homeFs); - return { success: true }; + resolved = this.resolveFiles(agentId); + } catch (err) { + log.error('remote-switch-setup: file-based connector declares no behavior', { agentId, err }); + return connectorFailed(String(err), 'files_unimplemented'); + } + if (!resolved) return connectorUnsupported(); + try { + await action(resolved.files, resolved.homeFs, resolved.version); + return connectorSucceeded(); } catch (err) { log.error('remote-switch-setup: file-based connector operation failed', { agentId, err }); - return { success: false, message: err instanceof Error ? err.message : String(err) }; + return connectorFailed( + err instanceof Error ? err.message : String(err), + 'files_write_failed' + ); } } async getStatus(agentId: string): Promise { if (getPlugin(agentId).capabilities.switchSetup.kind === 'files') { - return this.filesStatus(agentId, connectorVersion(agentId)); + return this.filesStatus(agentId); } const resolved = await this.resolve(agentId); if (!resolved) return unsupported(agentId); @@ -342,7 +371,7 @@ export class RemoteSwitchSetupService { async checkForUpdates(agentId: string): Promise { // A file-based connector ships inside the app: nothing to refresh. if (getPlugin(agentId).capabilities.switchSetup.kind === 'files') { - return this.filesStatus(agentId, connectorVersion(agentId)); + return this.filesStatus(agentId); } const resolved = await this.resolve(agentId); if (!resolved) return unsupported(agentId); @@ -364,58 +393,45 @@ export class RemoteSwitchSetupService { } async install(agentId: string): Promise { - const { result, attempted } = await this.runInstall(agentId); + const elapsed = startTimer(); + const { run, attempted } = await this.runInstall(agentId); // An agent type with no connector to install did not fail to install one. if (attempted) { trackEvent('connector_installed', { - agent_type: isValidProviderId(agentId) ? agentId : 'unknown', + agent_type: agentTypeOf(agentId), target: 'remote', - outcome: result.success ? 'success' : 'failure', + outcome: run.result.success ? 'success' : 'failure', + failure_reason: run.failure, + duration_ms: elapsed(), }); } - return result; + return run.result; } - private async runInstall( - agentId: string - ): Promise<{ result: SwitchSetupResult; attempted: boolean }> { + private async runInstall(agentId: string): Promise<{ run: ConnectorRun; attempted: boolean }> { if (getPlugin(agentId).capabilities.switchSetup.kind === 'files') { - // `runFiles` resolves the behavior outside its own try, and that throws - // for a connector that declares files and implements none. An install the - // user asked for must come back as a failed result either way — a - // rejection here would skip the report and reach the UI as a stack. - try { - const version = connectorVersion(agentId); - const result = await this.runFiles(agentId, (files, fs) => files.install(fs, { version })); - return { result, attempted: true }; - } catch (err) { - return { - result: { success: false, message: String(err) }, - attempted: true, - }; - } + const run = await this.runFiles(agentId, (files, fs, version) => + files.install(fs, { version }) + ); + return { run, attempted: true }; } const resolved = await this.resolve(agentId); - if (!resolved) - return { - result: { success: false, message: 'Switch setup is not supported for this agent.' }, - attempted: false, - }; + if (!resolved) return { run: connectorUnsupported(), attempted: false }; const { descriptor, bin, ref, marketplaceSource, rules } = resolved; try { await this.ensureMarketplace(bin, descriptor.marketplaceName, marketplaceSource, rules); } catch (err) { return { - result: { success: false, message: `Could not add marketplace: ${String(err)}` }, + run: connectorFailed(`Could not add marketplace: ${String(err)}`, 'marketplace_failed'), attempted: true, }; } const res = await this.run(bin, rules.installArgs(ref, descriptor.scope)); return { - result: + run: res.code === 0 - ? { success: true } - : { success: false, message: res.stderr.trim() || 'Install failed.' }, + ? connectorSucceeded() + : connectorFailed(res.stderr.trim() || 'Install failed.', 'install_command_failed'), attempted: true, }; } @@ -427,42 +443,39 @@ export class RemoteSwitchSetupService { */ async update(agentId: string): Promise { if (getPlugin(agentId).capabilities.switchSetup.kind === 'none') { - return { success: false, message: 'Switch setup is not supported for this agent.' }; + return connectorUnsupported().result; } - const { result, wasReinstall } = await this.runUpdate(agentId); + const elapsed = startTimer(); + const { run, wasReinstall } = await this.runUpdate(agentId); trackEvent('connector_updated', { agent_type: agentTypeOf(agentId), target: 'remote', - outcome: result.success ? 'success' : 'failure', + outcome: run.result.success ? 'success' : 'failure', was_reinstall: wasReinstall, + failure_reason: run.failure, + duration_ms: elapsed(), }); - return result; + return run.result; } - private async runUpdate( - agentId: string - ): Promise<{ result: SwitchSetupResult; wasReinstall: boolean }> { + private async runUpdate(agentId: string): Promise<{ run: ConnectorRun; wasReinstall: boolean }> { // Installing overwrites in place, so update is the same operation — there // is no removed-but-not-reinstalled window to report on. if (getPlugin(agentId).capabilities.switchSetup.kind === 'files') { - const version = connectorVersion(agentId); - const result = await this.runFiles(agentId, (files, fs) => files.install(fs, { version })); - return { result, wasReinstall: false }; + const run = await this.runFiles(agentId, (files, fs, version) => + files.install(fs, { version }) + ); + return { run, wasReinstall: false }; } const resolved = await this.resolve(agentId); - if (!resolved) { - return { - result: { success: false, message: 'Switch setup is not supported for this agent.' }, - wasReinstall: false, - }; - } + if (!resolved) return { run: connectorUnsupported(), wasReinstall: false }; const { descriptor, bin, ref, marketplaceSource, rules } = resolved; try { await this.ensureMarketplace(bin, descriptor.marketplaceName, marketplaceSource, rules); } catch (err) { return { - result: { success: false, message: `Could not add marketplace: ${String(err)}` }, + run: connectorFailed(`Could not add marketplace: ${String(err)}`, 'marketplace_failed'), wasReinstall: false, }; } @@ -471,10 +484,10 @@ export class RemoteSwitchSetupService { if (updateArgs) { const res = await this.run(bin, updateArgs); return { - result: + run: res.code === 0 - ? { success: true } - : { success: false, message: res.stderr.trim() || 'Update failed.' }, + ? connectorSucceeded() + : connectorFailed(res.stderr.trim() || 'Update failed.', 'update_command_failed'), wasReinstall: false, }; } @@ -484,24 +497,23 @@ export class RemoteSwitchSetupService { const removed = await this.run(bin, rules.uninstallArgs(ref, descriptor.scope)); if (removed.code !== 0) { return { - result: { - success: false, - message: removed.stderr.trim() || 'Update failed: could not remove the installed plugin.', - }, + run: connectorFailed( + removed.stderr.trim() || 'Update failed: could not remove the installed plugin.', + 'uninstall_command_failed' + ), wasReinstall: true, }; } const added = await this.run(bin, rules.installArgs(ref, descriptor.scope)); return { - result: + run: added.code === 0 - ? { success: true } - : { - success: false, - message: - added.stderr.trim() || + ? connectorSucceeded() + : connectorFailed( + added.stderr.trim() || 'Update failed: the plugin was removed but could not be reinstalled. Install it again for this host.', - }, + 'install_command_failed' + ), wasReinstall: true, }; } diff --git a/console/apps/switch-console-desktop/src/main/core/switch-setup/switch-setup-service.test.ts b/console/apps/switch-console-desktop/src/main/core/switch-setup/switch-setup-service.test.ts index 57857ffdc..815895cb5 100644 --- a/console/apps/switch-console-desktop/src/main/core/switch-setup/switch-setup-service.test.ts +++ b/console/apps/switch-console-desktop/src/main/core/switch-setup/switch-setup-service.test.ts @@ -407,6 +407,9 @@ describe('switchSetupService mutations', () => { agent_type: 'claude', target: 'local', outcome: 'success', + failure_reason: 'none', + // Elapsed wall time: a real number, but not one a test can pin. + duration_ms: expect.any(Number), }); }); @@ -448,10 +451,14 @@ describe('switchSetupService mutations', () => { expect(result.success).toBe(false); expect(result.message).toBe('no write access'); + // The host CLI refused the plugin — distinct from the marketplace failing + // to register, which is the other way this same button fails. expect(mocks.trackEvent).toHaveBeenCalledWith('connector_installed', { agent_type: 'claude', target: 'local', outcome: 'failure', + failure_reason: 'install_command_failed', + duration_ms: expect.any(Number), }); }); @@ -726,6 +733,8 @@ describe('file-based connector version', () => { agent_type: 'opencode', target: 'local', outcome: 'success', + failure_reason: 'none', + duration_ms: expect.any(Number), }); }); @@ -735,10 +744,14 @@ describe('file-based connector version', () => { const result = await switchSetupService.install('opencode'); expect(result.success).toBe(false); + // The app's own write failed. Nothing about a marketplace or a host CLI is + // involved in this connector, and the code has to say so. expect(mocks.trackEvent).toHaveBeenCalledWith('connector_installed', { agent_type: 'opencode', target: 'local', outcome: 'failure', + failure_reason: 'files_write_failed', + duration_ms: expect.any(Number), }); }); @@ -751,10 +764,143 @@ describe('file-based connector version', () => { const result = await switchSetupService.install('opencode'); expect(result.success).toBe(false); + // Its own code, not `files_write_failed`: nothing was written because there + // was nothing to write it with, which is a fault in the plugin rather than + // on this machine — and the two would otherwise be one number. expect(mocks.trackEvent).toHaveBeenCalledWith('connector_installed', { agent_type: 'opencode', target: 'local', outcome: 'failure', + failure_reason: 'files_unimplemented', + duration_ms: expect.any(Number), + }); + }); +}); + +/** + * The codes exist to tell apart failures a user experiences identically. + * + * Every case below reaches the UI as some variant of "it did not work", and + * each one needs a different fix — a marketplace source that will not resolve, + * a host CLI that refuses the plugin, an update that got halfway. Counting them + * as one number answers none of those questions, which is what this pins. + */ +describe('why a connector operation failed', () => { + function reported(name: string): Record { + const call = mocks.trackEvent.mock.calls.find((c) => c[0] === name); + if (!call) throw new Error(`nothing reported ${name}`); + return call[1] as Record; + } + + /** Every exec fails, except the listings the operation reads first. */ + function execFailingAfterListings(stderr: string) { + return (_bin: string, args: string[] = []) => { + const a = args.join(' '); + if (a === 'plugin list --json' || a === 'plugin marketplace list --json') { + return Promise.resolve({ stdout: JSON.stringify([]), stderr: '' }); + } + return Promise.reject(Object.assign(new Error('boom'), { code: 1, stderr })); + }; + } + + it('blames the marketplace when it is the marketplace that would not register', async () => { + // An empty listing sends `install` through `marketplace add`, which fails + // here — before the plugin command is ever reached. + mocks.exec.mockImplementation(execFailingAfterListings('could not resolve source')); + + await switchSetupService.install('claude'); + + expect(reported('connector_installed')).toMatchObject({ + outcome: 'failure', + failure_reason: 'marketplace_failed', + }); + }); + + it('blames the update verb when the host has one and it failed', async () => { + const base = execImpl('0.1.0'); + mocks.exec.mockImplementation((bin: string, args: string[] = []) => { + if (args.join(' ').startsWith('plugin update')) { + return Promise.reject(Object.assign(new Error('boom'), { code: 1, stderr: 'locked' })); + } + return base(bin, args); + }); + + await switchSetupService.update('claude'); + + expect(reported('connector_updated')).toMatchObject({ + outcome: 'failure', + failure_reason: 'update_command_failed', + was_reinstall: false, + }); + }); + + it('blames the uninstall when a reinstall-style update cannot remove the old plugin', async () => { + // Codex has no update verb, so an update is remove-then-add. Failing at the + // remove leaves the previous connector in place: nothing was lost. + mocks.getPlugin.mockReturnValue(CODEX_AGENT); + mocks.resolveCommandPath.mockResolvedValue('/usr/bin/codex'); + const base = codexExecImpl('0.1.0'); + mocks.exec.mockImplementation((bin: string, args: string[] = []) => { + if (args.join(' ') === `plugin remove ${CODEX_REF}`) { + return Promise.reject(Object.assign(new Error('boom'), { code: 1, stderr: 'in use' })); + } + return base(bin, args); + }); + + await switchSetupService.update('codex'); + + expect(reported('connector_updated')).toMatchObject({ + outcome: 'failure', + failure_reason: 'uninstall_command_failed', + was_reinstall: true, + }); + }); + + it('blames the install when a reinstall-style update removed the plugin and could not put it back', async () => { + // The same button, one step later, and a materially worse outcome: the + // agent now has no connector at all. `was_reinstall` alone cannot separate + // this from the case above — both are true — so the code has to. + mocks.getPlugin.mockReturnValue(CODEX_AGENT); + mocks.resolveCommandPath.mockResolvedValue('/usr/bin/codex'); + const base = codexExecImpl('0.1.0'); + mocks.exec.mockImplementation((bin: string, args: string[] = []) => { + if (args.join(' ') === `plugin add ${CODEX_REF}`) { + return Promise.reject(Object.assign(new Error('boom'), { code: 1, stderr: 'no network' })); + } + return base(bin, args); + }); + + await switchSetupService.update('codex'); + + expect(reported('connector_updated')).toMatchObject({ + outcome: 'failure', + failure_reason: 'install_command_failed', + was_reinstall: true, + }); + }); + + it('blames the uninstall command when removing the connector failed', async () => { + mocks.exec.mockImplementation(execFailingAfterListings('permission denied')); + + await switchSetupService.uninstall('claude'); + + expect(reported('connector_uninstalled')).toMatchObject({ + outcome: 'failure', + failure_reason: 'uninstall_command_failed', + }); + }); + + it('reports no reason at all when the operation worked', async () => { + // `none` rather than an absent property: every connector_installed then + // carries the same keys, so a gap in the data is a send that went wrong + // rather than an outcome nobody thought about. + mocks.exec.mockImplementation(execImpl(null)); + + await switchSetupService.install('claude'); + + expect(reported('connector_installed')).toMatchObject({ + outcome: 'success', + failure_reason: 'none', }); }); }); diff --git a/console/apps/switch-console-desktop/src/main/core/switch-setup/switch-setup-service.ts b/console/apps/switch-console-desktop/src/main/core/switch-setup/switch-setup-service.ts index 0424650ee..3c616a434 100644 --- a/console/apps/switch-console-desktop/src/main/core/switch-setup/switch-setup-service.ts +++ b/console/apps/switch-console-desktop/src/main/core/switch-setup/switch-setup-service.ts @@ -6,10 +6,11 @@ import { resolveCommandPath } from '@switch-console/core/deps/runtime'; import { type ArtifactName, artifactVersion } from '@switch-console/shared'; import { LocalExecutionContext } from '@main/core/execution-context/local-execution-context'; import { agentTypeOf } from '@main/core/telemetry/agent-type'; +import { startTimer } from '@main/core/telemetry/duration'; +import type { TelemetryConnectorFailure } from '@main/core/telemetry/events'; import { trackEvent } from '@main/core/telemetry/telemetry-service'; import { log } from '@main/lib/logger'; import { isNewerVersion } from '@main/lib/semver'; -import { isValidProviderId } from '@shared/core/providers/agent-provider-registry'; import type { AgentTypeAvailability } from '@shared/core/switch-setup/agent-type-availability'; import { createPluginFs } from '../providers/plugin-fs'; import { getPlugin, listPlugins } from '../providers/plugin-registry'; @@ -45,6 +46,34 @@ export function marketplaceMatchesSource(entry: RegisteredMarketplace, source: s /** Outcome of a mutating operation, mirroring the providers controller shape. */ export type SwitchSetupResult = { success: boolean; message?: string }; +/** + * A completed connector operation: what the caller gets back, and the + * enumerated reason it failed. + * + * The two travel together because a `SwitchSetupResult` carries only a message, + * and a message cannot be reported — so the code has to be named where the + * failure is known rather than recovered from the text afterwards. Shared with + * the remote driver, which reports the same event from the same points. + */ +export type ConnectorRun = { + result: SwitchSetupResult; + failure: TelemetryConnectorFailure; +}; + +/** A connector operation that did what was asked. */ +export function connectorSucceeded(): ConnectorRun { + return { result: { success: true }, failure: 'none' }; +} + +export function connectorFailed(message: string, failure: TelemetryConnectorFailure): ConnectorRun { + return { result: { success: false, message }, failure }; +} + +/** The answer for an agent whose connector nothing here can manage. */ +export function connectorUnsupported(): ConnectorRun { + return connectorFailed('Switch setup is not supported for this agent.', 'unsupported'); +} + const EXEC_TIMEOUT_MS = 120_000; /** @@ -363,7 +392,15 @@ class SwitchSetupService { return { ...(await this.getStatus(agentId)), refreshError }; } - /** Install, update and uninstall for a file-based connector. */ + /** + * Install, update and uninstall for a file-based connector. + * + * Resolving is inside the try because it throws for a connector that declares + * files and implements none. An operation the user asked for must come back as + * a failed result either way — a rejection would skip the report and reach the + * UI as a stack — and that case is its own failure code rather than a write + * that went wrong, because it is a fault in the plugin and not on the machine. + */ private async runFiles( agentId: string, action: ( @@ -371,58 +408,52 @@ class SwitchSetupService { homeFs: PluginFs, version: string ) => Promise - ): Promise { - const resolved = this.resolveFiles(agentId); - if (!resolved) - return { success: false, message: 'Switch setup is not supported for this agent.' }; + ): Promise { + let resolved: ReturnType; + try { + resolved = this.resolveFiles(agentId); + } catch (err) { + log.error('switch-setup: file-based connector declares no behavior', { agentId, err }); + return connectorFailed(installFailureMessage(String(err)), 'files_unimplemented'); + } + if (!resolved) return connectorUnsupported(); try { await action(resolved.files, resolved.homeFs, resolved.version); - return { success: true }; + return connectorSucceeded(); } catch (err) { log.error('switch-setup: file-based connector operation failed', { agentId, err }); - return { success: false, message: err instanceof Error ? err.message : String(err) }; + return connectorFailed( + err instanceof Error ? err.message : String(err), + 'files_write_failed' + ); } } async install(agentId: string): Promise { - const { result, attempted } = await this.runInstall(agentId); + const elapsed = startTimer(); + const { run, attempted } = await this.runInstall(agentId); // An agent type with no connector to install did not fail to install one. if (attempted) { trackEvent('connector_installed', { - agent_type: isValidProviderId(agentId) ? agentId : 'unknown', + agent_type: agentTypeOf(agentId), target: 'local', - outcome: result.success ? 'success' : 'failure', + outcome: run.result.success ? 'success' : 'failure', + failure_reason: run.failure, + duration_ms: elapsed(), }); } - return result; + return run.result; } - private async runInstall( - agentId: string - ): Promise<{ result: SwitchSetupResult; attempted: boolean }> { + private async runInstall(agentId: string): Promise<{ run: ConnectorRun; attempted: boolean }> { if (getPlugin(agentId).capabilities.switchSetup.kind === 'files') { - // `runFiles` resolves the behavior outside its own try, and that throws - // for a connector that declares files and implements none. An install the - // user asked for must come back as a failed result either way — a - // rejection here would skip the report and reach the UI as a stack. - try { - const result = await this.runFiles(agentId, (files, fs, version) => - files.install(fs, { version }) - ); - return { result, attempted: true }; - } catch (err) { - return { - result: { success: false, message: installFailureMessage(String(err)) }, - attempted: true, - }; - } + const run = await this.runFiles(agentId, (files, fs, version) => + files.install(fs, { version }) + ); + return { run, attempted: true }; } const resolved = await this.resolve(agentId); - if (!resolved) - return { - result: { success: false, message: 'Switch setup is not supported for this agent.' }, - attempted: false, - }; + if (!resolved) return { run: connectorUnsupported(), attempted: false }; const { descriptor, bin, ref, rules } = resolved; try { await this.ensureMarketplace( @@ -433,16 +464,16 @@ class SwitchSetupService { ); } catch (err) { return { - result: { success: false, message: installFailureMessage(String(err)) }, + run: connectorFailed(installFailureMessage(String(err)), 'marketplace_failed'), attempted: true, }; } const res = await this.run(bin, rules.installArgs(ref, descriptor.scope)); return { - result: + run: res.code === 0 - ? { success: true } - : { success: false, message: installFailureMessage(res.stderr.trim()) }, + ? connectorSucceeded() + : connectorFailed(installFailureMessage(res.stderr.trim()), 'install_command_failed'), attempted: true, }; } @@ -466,16 +497,19 @@ class SwitchSetupService { */ async update(agentId: string): Promise { if (getPlugin(agentId).capabilities.switchSetup.kind === 'none') { - return { success: false, message: 'Switch setup is not supported for this agent.' }; + return connectorUnsupported().result; } - const { result, wasReinstall } = await this.runUpdate(agentId); + const elapsed = startTimer(); + const { run, wasReinstall } = await this.runUpdate(agentId); trackEvent('connector_updated', { agent_type: agentTypeOf(agentId), target: 'local', - outcome: result.success ? 'success' : 'failure', + outcome: run.result.success ? 'success' : 'failure', was_reinstall: wasReinstall, + failure_reason: run.failure, + duration_ms: elapsed(), }); - return result; + return run.result; } /** @@ -484,25 +518,18 @@ class SwitchSetupService { * verb, so for it every update is the second kind, with a window in between * where nothing is installed. */ - private async runUpdate( - agentId: string - ): Promise<{ result: SwitchSetupResult; wasReinstall: boolean }> { + private async runUpdate(agentId: string): Promise<{ run: ConnectorRun; wasReinstall: boolean }> { // Installing a file-based connector overwrites in place, so update is the // same operation — there is no removed-but-not-reinstalled window. if (getPlugin(agentId).capabilities.switchSetup.kind === 'files') { - const result = await this.runFiles(agentId, (files, fs, version) => + const run = await this.runFiles(agentId, (files, fs, version) => files.install(fs, { version }) ); // Overwritten in place: neither a verb update nor a remove-and-replace. - return { result, wasReinstall: false }; + return { run, wasReinstall: false }; } const resolved = await this.resolve(agentId); - if (!resolved) { - return { - result: { success: false, message: 'Switch setup is not supported for this agent.' }, - wasReinstall: false, - }; - } + if (!resolved) return { run: connectorUnsupported(), wasReinstall: false }; const { descriptor, bin, ref, rules } = resolved; try { @@ -514,7 +541,7 @@ class SwitchSetupService { ); } catch (err) { return { - result: { success: false, message: `Could not add marketplace: ${String(err)}` }, + run: connectorFailed(`Could not add marketplace: ${String(err)}`, 'marketplace_failed'), wasReinstall: false, }; } @@ -523,10 +550,10 @@ class SwitchSetupService { if (updateArgs) { const res = await this.run(bin, updateArgs); return { - result: + run: res.code === 0 - ? { success: true } - : { success: false, message: res.stderr.trim() || 'Update failed.' }, + ? connectorSucceeded() + : connectorFailed(res.stderr.trim() || 'Update failed.', 'update_command_failed'), wasReinstall: false, }; } @@ -534,53 +561,54 @@ class SwitchSetupService { const removed = await this.run(bin, rules.uninstallArgs(ref, descriptor.scope)); if (removed.code !== 0) { return { - result: { - success: false, - message: removed.stderr.trim() || 'Update failed: could not remove the installed plugin.', - }, + run: connectorFailed( + removed.stderr.trim() || 'Update failed: could not remove the installed plugin.', + 'uninstall_command_failed' + ), wasReinstall: true, }; } const added = await this.run(bin, rules.installArgs(ref, descriptor.scope)); return { - result: + run: added.code === 0 - ? { success: true } - : { - success: false, - message: - added.stderr.trim() || + ? connectorSucceeded() + : connectorFailed( + added.stderr.trim() || 'Update failed: the plugin was removed but could not be reinstalled. Install it again from Settings → Agents.', - }, + 'install_command_failed' + ), wasReinstall: true, }; } async uninstall(agentId: string): Promise { if (getPlugin(agentId).capabilities.switchSetup.kind === 'none') { - return { success: false, message: 'Switch setup is not supported for this agent.' }; + return connectorUnsupported().result; } - const result = await this.runUninstall(agentId); + const elapsed = startTimer(); + const run = await this.runUninstall(agentId); trackEvent('connector_uninstalled', { agent_type: agentTypeOf(agentId), target: 'local', - outcome: result.success ? 'success' : 'failure', + outcome: run.result.success ? 'success' : 'failure', + failure_reason: run.failure, + duration_ms: elapsed(), }); - return result; + return run.result; } - private async runUninstall(agentId: string): Promise { + private async runUninstall(agentId: string): Promise { if (getPlugin(agentId).capabilities.switchSetup.kind === 'files') { return this.runFiles(agentId, (files, fs) => files.uninstall(fs)); } const resolved = await this.resolve(agentId); - if (!resolved) - return { success: false, message: 'Switch setup is not supported for this agent.' }; + if (!resolved) return connectorUnsupported(); const { descriptor, bin, ref, rules } = resolved; const res = await this.run(bin, rules.uninstallArgs(ref, descriptor.scope)); return res.code === 0 - ? { success: true } - : { success: false, message: res.stderr.trim() || 'Uninstall failed.' }; + ? connectorSucceeded() + : connectorFailed(res.stderr.trim() || 'Uninstall failed.', 'uninstall_command_failed'); } } diff --git a/console/apps/switch-console-desktop/src/main/core/telemetry/catalogue.test.ts b/console/apps/switch-console-desktop/src/main/core/telemetry/catalogue.test.ts index 43656fd71..9fd7600dd 100644 --- a/console/apps/switch-console-desktop/src/main/core/telemetry/catalogue.test.ts +++ b/console/apps/switch-console-desktop/src/main/core/telemetry/catalogue.test.ts @@ -32,12 +32,13 @@ const CONTEXT: TelemetryContext = { /** * A value of the right kind for each property, chosen by name. * - * Counts are numbers, the yes/no properties are booleans, and everything else - * is a placeholder string — the builder does not check a string against the - * catalogue's unions, so any string exercises the same path. + * Counts and durations are numbers, the yes/no properties are booleans, and + * everything else is a placeholder string — the builder does not check a string + * against the catalogue's unions, so any string exercises the same path. */ function sampleFor(property: string): string | number | boolean { if (property.endsWith('_count')) return 3; + if (property.endsWith('_ms')) return 1234; if ( property.startsWith('has_') || property.startsWith('was_') || diff --git a/console/apps/switch-console-desktop/src/main/core/telemetry/duration.test.ts b/console/apps/switch-console-desktop/src/main/core/telemetry/duration.test.ts new file mode 100644 index 000000000..7c316d69a --- /dev/null +++ b/console/apps/switch-console-desktop/src/main/core/telemetry/duration.test.ts @@ -0,0 +1,31 @@ +import { describe, expect, it } from 'vitest'; +import { startTimer } from './duration'; + +describe('timing an operation', () => { + it('reports whole milliseconds, never a fraction', async () => { + // The emitter refuses a non-finite number and the far end averages what it + // gets, so the contract worth pinning is that this is a plain integer. + const elapsed = startTimer(); + await new Promise((resolve) => setTimeout(resolve, 5)); + + const ms = elapsed(); + + expect(Number.isInteger(ms)).toBe(true); + expect(ms).toBeGreaterThan(0); + }); + + it('reports zero rather than a negative for an operation too fast to measure', async () => { + // A negative duration in a payload is indistinguishable from real data at + // the far end, which is worse than no data. + expect(startTimer()()).toBeGreaterThanOrEqual(0); + }); + + it('can be read more than once, and does not restart', async () => { + const elapsed = startTimer(); + await new Promise((resolve) => setTimeout(resolve, 5)); + const first = elapsed(); + await new Promise((resolve) => setTimeout(resolve, 5)); + + expect(elapsed()).toBeGreaterThanOrEqual(first); + }); +}); diff --git a/console/apps/switch-console-desktop/src/main/core/telemetry/duration.ts b/console/apps/switch-console-desktop/src/main/core/telemetry/duration.ts new file mode 100644 index 000000000..286175875 --- /dev/null +++ b/console/apps/switch-console-desktop/src/main/core/telemetry/duration.ts @@ -0,0 +1,25 @@ +import type { TelemetryDurationMs } from './events'; + +/** + * Start timing an operation that reports how long it took. + * + * `performance.now()` rather than `Date.now()`: it is monotonic, so an NTP step + * or a machine waking mid-install cannot yield a negative duration or an hour + * that never passed. `Date.now()` can do both, and a negative number reaching a + * payload is worse than no number — it is data nobody can tell from real data. + * + * Whole milliseconds. Sub-millisecond precision says nothing about an operation + * that shells out to a package manager, and it keeps the value a plain integer + * at the far end. Nothing is capped or bucketed here; see `TelemetryDurationMs` + * for why, and for how to read the result. + * + * Call it before the work and call the returned function at the point the + * outcome is known — not after the event is built, and not around the send, + * which is fire-and-forget and has nothing to do with what the user waited for. + */ +export function startTimer(): () => TelemetryDurationMs { + const start = performance.now(); + // Clamped defensively rather than because the clock can go backwards: it + // cannot. A future caller passing its own start value is the case this covers. + return () => Math.max(0, Math.round(performance.now() - start)); +} diff --git a/console/apps/switch-console-desktop/src/main/core/telemetry/events.ts b/console/apps/switch-console-desktop/src/main/core/telemetry/events.ts index 582bb22ca..cbc9960f4 100644 --- a/console/apps/switch-console-desktop/src/main/core/telemetry/events.ts +++ b/console/apps/switch-console-desktop/src/main/core/telemetry/events.ts @@ -135,6 +135,57 @@ export type TelemetryCliFailure = | 'still_present' | 'error'; +/** + * Why installing, updating or removing a **Switch connector** failed — not the + * agent's own CLI, which `TelemetryCliFailure` covers. + * + * Named at the point of failure rather than mapped from an error type: these + * paths report a message, and a message cannot be sent. The codes separate the + * walls that need different fixes — a marketplace that will not register, a host + * CLI that refuses the plugin, and the app's own file writing — because + * "connector install failed" on its own tells nobody which of the three to look + * at. + * + * `uninstall_command_failed` and `install_command_failed` both occur on an + * update: a host with no update verb removes the connector and puts it back, and + * which half failed is the difference between "nothing changed" and "the agent + * now has no connector at all". + */ +export type TelemetryConnectorFailure = + | 'none' + /** The agent declares a connector but nothing could be resolved to manage it. */ + | 'unsupported' + /** The plugin marketplace could not be registered or re-pointed at its source. */ + | 'marketplace_failed' + | 'install_command_failed' + | 'update_command_failed' + | 'uninstall_command_failed' + /** The app writes this connector itself, and the write failed. */ + | 'files_write_failed' + /** The agent declares a file-based connector and implements no behavior for it. */ + | 'files_unimplemented' + | 'error'; + +/** + * How long an operation took, in whole milliseconds. + * + * The one dimension here that is not a fixed set of values, which is why it is + * worth being explicit about what it is and is not. It is measured on a + * monotonic clock around the operation itself, so a clock step or a machine + * waking mid-install cannot produce a negative number or an hour that never + * passed. + * + * **Nothing is capped.** A capped duration is a number the operation did not + * take, and inventing one is exactly what the rest of this catalogue refuses to + * do. Some of these legitimately include a password prompt somebody left on + * screen, so read them as percentiles rather than as a mean — a p50 that moves + * is a real regression, an arithmetic mean over this is meaningless. + * + * It carries nothing about the machine: an elapsed time is not a fingerprint at + * this resolution, and it names no path, host or command. + */ +export type TelemetryDurationMs = number; + /** * Which messaging platform a room or bridge is on. * @@ -318,10 +369,20 @@ export type TelemetryEventMap = { server_kind: 'local' | 'remote_managed' | 'external'; outcome: TelemetryOutcome; }; + /** + * A Switch connector was installed. + * + * `agent_type` is also what says *which kind* of connector ran: a host with a + * plugin marketplace is driven through its CLI, and a host without one has its + * connector written by the app. The two fail in entirely different places, + * which is what `failure_reason` separates. + */ connector_installed: { agent_type: TelemetryAgentType; target: TelemetryLocationKind; outcome: 'success' | 'failure'; + failure_reason: TelemetryConnectorFailure; + duration_ms: TelemetryDurationMs; }; /** * The app checked for an update. `trigger` separates a check someone asked for @@ -378,6 +439,8 @@ export type TelemetryEventMap = { target: 'local' | 'remote'; outcome: TelemetryOutcome; was_reinstall: boolean; + failure_reason: TelemetryConnectorFailure; + duration_ms: TelemetryDurationMs; }; /** * The connector was removed. The churn signal. @@ -389,6 +452,8 @@ export type TelemetryEventMap = { agent_type: TelemetryAgentType; target: 'local'; outcome: TelemetryOutcome; + failure_reason: TelemetryConnectorFailure; + duration_ms: TelemetryDurationMs; }; /** * An agent was removed. `delete_in_switch` says whether its identity on the @@ -452,6 +517,11 @@ export type TelemetryEventMap = { * connector, which `connector_installed` and friends report. * * The single biggest wall a new user hits, and until now entirely uncounted. + * + * `duration_ms` matters most here: a CLI install that takes four minutes and + * one that takes ten seconds are otherwise the same row, and a package manager + * getting slower is the kind of regression nobody reports because it never + * fails. */ agent_cli_action: { agent_type: TelemetryAgentType; @@ -460,6 +530,7 @@ export type TelemetryEventMap = { action: TelemetryCliAction; outcome: TelemetryOutcome; failure_reason: TelemetryCliFailure; + duration_ms: TelemetryDurationMs; }; /** * A room was created on a server. `bridge_unavailable` is the failure worth @@ -604,15 +675,22 @@ export const TELEMETRY_EVENT_PROPERTIES = { ], session_ended: ['agent_type', 'location', 'outcome'], server_added: ['server_kind', 'outcome'], - connector_installed: ['agent_type', 'target', 'outcome'], + connector_installed: ['agent_type', 'target', 'outcome', 'failure_reason', 'duration_ms'], update_checked: ['trigger', 'result'], update_downloaded: ['outcome'], update_install_started: ['outcome'], bridge_connected: ['bridge_platform', 'outcome', 'failure_reason'], bridge_disconnected: ['bridge_platform', 'outcome'], bridge_identity_claimed: ['bridge_platform', 'outcome'], - connector_updated: ['agent_type', 'target', 'outcome', 'was_reinstall'], - connector_uninstalled: ['agent_type', 'target', 'outcome'], + connector_updated: [ + 'agent_type', + 'target', + 'outcome', + 'was_reinstall', + 'failure_reason', + 'duration_ms', + ], + connector_uninstalled: ['agent_type', 'target', 'outcome', 'failure_reason', 'duration_ms'], agent_removed: [ 'agent_type', 'location', @@ -633,6 +711,7 @@ export const TELEMETRY_EVENT_PROPERTIES = { 'action', 'outcome', 'failure_reason', + 'duration_ms', ], room_created: [ 'server_kind', diff --git a/console/apps/switch-console-desktop/src/main/core/telemetry/relay-client.test.ts b/console/apps/switch-console-desktop/src/main/core/telemetry/relay-client.test.ts index 73286b1a7..d4fde46af 100644 --- a/console/apps/switch-console-desktop/src/main/core/telemetry/relay-client.test.ts +++ b/console/apps/switch-console-desktop/src/main/core/telemetry/relay-client.test.ts @@ -194,7 +194,13 @@ describe('the record that gets built', () => { // The guard drops any record with more than 128 attributes. const payload = buildOtlpPayload( 'connector_installed', - { agent_type: 'claude', target: 'remote', outcome: 'success' }, + { + agent_type: 'claude', + target: 'remote', + outcome: 'success', + failure_reason: 'none', + duration_ms: 4200, + }, CONTEXT ); diff --git a/console/apps/switch-console-desktop/src/main/core/telemetry/telemetry-service.test.ts b/console/apps/switch-console-desktop/src/main/core/telemetry/telemetry-service.test.ts index b8deb3485..26cd6c54a 100644 --- a/console/apps/switch-console-desktop/src/main/core/telemetry/telemetry-service.test.ts +++ b/console/apps/switch-console-desktop/src/main/core/telemetry/telemetry-service.test.ts @@ -19,7 +19,21 @@ function sentBody(): Record { return JSON.parse(init.body) as Record; } -type Attribute = { key: string; value: { stringValue: string } }; +type Attribute = { + key: string; + value: { stringValue?: string; doubleValue?: number; boolValue?: boolean }; +}; + +/** + * The value out of an OTLP attribute, whichever of the three shapes it took. + * + * Reading `stringValue` alone would make every number and boolean in a payload + * read back as `undefined` — so an assertion that a numeric property is carried + * would pass against a payload that dropped it. + */ +function attributeValue(attribute: Attribute): string | number | boolean | undefined { + return attribute.value.stringValue ?? attribute.value.doubleValue ?? attribute.value.boolValue; +} function sentRecord(): Record { const resourceLogs = sentBody().resourceLogs as [ @@ -28,16 +42,17 @@ function sentRecord(): Record { return resourceLogs[0].scopeLogs[0].logRecords[0]; } -function sentResource(): Record { +/** Resource attributes are strings by construction, unlike the record's own. */ +function sentResource(): Record { const resourceLogs = sentBody().resourceLogs as [{ resource: { attributes: Attribute[] } }]; return Object.fromEntries( resourceLogs[0].resource.attributes.map((a) => [a.key, a.value.stringValue]) ); } -function sentAttributes(): Record { +function sentAttributes(): Record { return Object.fromEntries( - (sentRecord().attributes as Attribute[]).map((a) => [a.key, a.value.stringValue]) + (sentRecord().attributes as Attribute[]).map((a) => [a.key, attributeValue(a)]) ); } @@ -212,6 +227,8 @@ describe('the payload', () => { agent_type: 'claude', target: 'remote', outcome: 'failure', + failure_reason: 'install_command_failed', + duration_ms: 4200, }); expect(sentResource()['service.version']).toBe('1.2.3'); @@ -220,6 +237,10 @@ describe('the payload', () => { agent_type: 'claude', target: 'remote', outcome: 'failure', + failure_reason: 'install_command_failed', + // A duration goes as a number, not as text: `4200` and not `"4200"`, so + // the far end can average it without parsing it back. + duration_ms: 4200, build: 'dev', }); }); diff --git a/console/apps/switch-console-desktop/src/renderer/features/telemetry/telemetry-copy.ts b/console/apps/switch-console-desktop/src/renderer/features/telemetry/telemetry-copy.ts index b0367de1c..fa1f5b3f1 100644 --- a/console/apps/switch-console-desktop/src/renderer/features/telemetry/telemetry-copy.ts +++ b/console/apps/switch-console-desktop/src/renderer/features/telemetry/telemetry-copy.ts @@ -14,6 +14,7 @@ export const TELEMETRY_SHARED = [ 'Which features are used, and how often', 'Which coding agents you use, and whether they run here or on a remote host', 'Whether sessions end normally or fail', + 'How long installs and setup steps take', 'App version and operating system', 'A random id for this install, so one copy of the app can be told from another', ]; From fcdda9f8d0968e26729063b5bcfa92f561da56ba Mon Sep 17 00:00:00 2001 From: Christian McDermott Date: Thu, 17 Sep 2026 14:03:05 -0400 Subject: [PATCH 2/2] fix(console): correct what the connector telemetry claims about itself MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of #488. Three of the findings were the same mistake: a comment asserting a property that had not been verified. - `TelemetryDurationMs` said "**Nothing is capped**". Every command the connector drivers run carries `EXEC_TIMEOUT_MS` (120 s), so a hung `plugin install` reports ~120000 and a reinstall-style update stacks two of those. The docblock now names the timeout wall, and names the other thing that shapes the distribution: the connector events time the whole operation, which on a machine that has never registered the plugin marketplace includes cloning a repository. First install is legitimately much slower than the rest, and a p50 that moves can be a change in that mix rather than a regression. - The providers controller said the timer excluded manager resolution because including it "would make the first measurement of a session differ from the rest". `getDependencyManager` is `return localDependencyManager` — it cannot produce that artefact, so the comment defended against nothing. Removed. The remote one is kept and narrowed: there, resolution may open the SSH connection. - `startTimer` clamped with `Math.max(0, …)` and justified it with "a future caller passing its own start value", an API the function does not have. The clock is monotonic, so the clamp was unreachable and its test passed with it deleted. Both are gone. Then the real defects: - **`TelemetryConnectorFailure.error` was unreachable.** Nothing mapped onto it and there is no catch-all, unlike `TelemetryCliFailure` whose `cliFailureReason` defaults to it. A bucket nothing can fill implies a coverage the code has not got, so the variant is removed and the gap it was pretending to cover — an operation that throws reports nothing at all — is written down on `connector_installed` instead. - **`unsupported` was reported asymmetrically.** A `cli` connector whose host binary cannot be resolved was silenced on install by the `attempted` flag, while `update` and `uninstall` reported it. The condition looked like it only ever happened on update. `install` now uses the same `kind === 'none'` guard as the other two — that case is genuinely not an attempt — and reports everything past it. `attempted` is gone. - **`console/AGENTS.md` was not updated.** It enumerates what a payload may carry, a duration was not on the list, and it says widening is a consent decision. `telemetry-copy.ts` names that rule as its authority, so the two halves of the promise disagreed. Also: `ConnectorRun` and its constructors move to their own leaf module, because the remote driver was importing three pure functions from a module that ends in `new SwitchSetupService()` over a `LocalExecutionContext` — the lesson already recorded on `telemetry/agent-type.ts`. The `files_unimplemented` message no longer differs between the two drivers. Both new duration tests were weak: the clamp test pinned nothing, and "does not restart" passed against a timer that restarts, since one sleep's worth satisfies `second >= first`. Declined: unifying the two drivers (right, but far past this PR), and catching throws at the six CLI call sites (pre-existing, and none of them had a catch before this change either). Co-Authored-By: Claude Opus 5 (1M context) --- console/AGENTS.md | 5 +- .../src/main/core/providers/controller.ts | 3 - .../src/main/core/remote-hosts/controller.ts | 5 +- .../main/core/switch-setup/connector-run.ts | 37 +++++++ .../core/switch-setup/remote-switch-setup.ts | 61 ++++++------ .../switch-setup/switch-setup-service.test.ts | 25 +++++ .../core/switch-setup/switch-setup-service.ts | 98 +++++++------------ .../src/main/core/telemetry/duration.test.ts | 26 ++--- .../src/main/core/telemetry/duration.ts | 9 +- .../src/main/core/telemetry/events.ts | 34 +++++-- 10 files changed, 180 insertions(+), 123 deletions(-) create mode 100644 console/apps/switch-console-desktop/src/main/core/switch-setup/connector-run.ts diff --git a/console/AGENTS.md b/console/AGENTS.md index 1e7a13ee8..7ed295fe0 100644 --- a/console/AGENTS.md +++ b/console/AGENTS.md @@ -425,7 +425,10 @@ pnpm run lint enough — excess-property checking does not apply through a spread — so the runtime filter is what makes "nothing free-text can reach a payload" true rather than intended. Permitted: which of the catalogued things happened, agent type, local-vs-remote, - success-vs-failure, app version, operating system, and the random install id. Never: + success-vs-failure, how long an operation took, app version, operating system, and the + random install id. A duration is the one permitted value that is not from a fixed set, + so it is held to `TelemetryDurationMs`: measured on a monotonic clock, whole + milliseconds, and never a span that could encode something else. Never: prompts, code, file paths, working directories, error messages or stack traces (use an enumerated code), machine or user names, IP or MAC addresses, email or sign-in, and no agent, room, project, location or server names or ids. Widening this is a consent diff --git a/console/apps/switch-console-desktop/src/main/core/providers/controller.ts b/console/apps/switch-console-desktop/src/main/core/providers/controller.ts index aa7ad8d27..19646c14b 100644 --- a/console/apps/switch-console-desktop/src/main/core/providers/controller.ts +++ b/console/apps/switch-console-desktop/src/main/core/providers/controller.ts @@ -101,9 +101,6 @@ export const providersController = createRPCController({ install: async (id: AgentProviderId, connectionId?: string, method?: InstallMethod) => { const mgr = await getDependencyManager(connectionId); - // Timed around the operation alone. Resolving the manager is a lookup that - // says nothing about how long an install takes, and including it would make - // the first measurement of a session differ from the rest for no reason. const elapsed = startTimer(); const result = await mgr.install(id, method); reportCliAction('install', id, method, result, elapsed()); diff --git a/console/apps/switch-console-desktop/src/main/core/remote-hosts/controller.ts b/console/apps/switch-console-desktop/src/main/core/remote-hosts/controller.ts index 40cce3003..27b4c8bfd 100644 --- a/console/apps/switch-console-desktop/src/main/core/remote-hosts/controller.ts +++ b/console/apps/switch-console-desktop/src/main/core/remote-hosts/controller.ts @@ -240,9 +240,8 @@ export const remoteHostsController = createRPCController({ method?: InstallMethod; }): Promise => { const manager = await getRemoteDependencyManager(params.sshHost); - // Timed around the operation alone: resolving the manager may open the SSH - // connection, which is not part of how long an install takes and would show - // up only on the first one of a session. + // Resolving the manager is outside the timer because it may open the SSH + // connection, which is not part of how long an install takes. const elapsed = startTimer(); const result = await manager.install(params.id, params.method); reportRemoteCliAction('install', params.id, params.method, result, elapsed()); diff --git a/console/apps/switch-console-desktop/src/main/core/switch-setup/connector-run.ts b/console/apps/switch-console-desktop/src/main/core/switch-setup/connector-run.ts new file mode 100644 index 000000000..fd53a5838 --- /dev/null +++ b/console/apps/switch-console-desktop/src/main/core/switch-setup/connector-run.ts @@ -0,0 +1,37 @@ +import type { TelemetryConnectorFailure } from '@main/core/telemetry/events'; + +/** Outcome of a mutating operation, mirroring the providers controller shape. */ +export type SwitchSetupResult = { success: boolean; message?: string }; + +/** + * A completed connector operation: what the caller gets back, and the + * enumerated reason it failed. + * + * The two travel together because a `SwitchSetupResult` carries only a message, + * and a message cannot be reported — so the code has to be named where the + * failure is known rather than recovered from the text afterwards. + * + * This is a leaf on purpose. Both the local and the remote driver build these, + * and the local one's module ends in `new SwitchSetupService()` over a + * `LocalExecutionContext` — so putting the constructors there made the remote + * driver import a local execution context to get three pure functions. See the + * same lesson recorded on `telemetry/agent-type.ts`. + */ +export type ConnectorRun = { + result: SwitchSetupResult; + failure: TelemetryConnectorFailure; +}; + +/** A connector operation that did what was asked. */ +export function connectorSucceeded(): ConnectorRun { + return { result: { success: true }, failure: 'none' }; +} + +export function connectorFailed(message: string, failure: TelemetryConnectorFailure): ConnectorRun { + return { result: { success: false, message }, failure }; +} + +/** The answer for an agent whose connector nothing here can manage. */ +export function connectorUnsupported(): ConnectorRun { + return connectorFailed('Switch setup is not supported for this agent.', 'unsupported'); +} diff --git a/console/apps/switch-console-desktop/src/main/core/switch-setup/remote-switch-setup.ts b/console/apps/switch-console-desktop/src/main/core/switch-setup/remote-switch-setup.ts index 2c854c684..e8caed0b7 100644 --- a/console/apps/switch-console-desktop/src/main/core/switch-setup/remote-switch-setup.ts +++ b/console/apps/switch-console-desktop/src/main/core/switch-setup/remote-switch-setup.ts @@ -11,14 +11,16 @@ import { trackEvent } from '@main/core/telemetry/telemetry-service'; import { log } from '@main/lib/logger'; import { isNewerVersion } from '@main/lib/semver'; import { getPlugin, listPlugins } from '../providers/plugin-registry'; -import { cliRulesFor, type SwitchSetupCliRules } from './switch-setup-cli-dialect'; -import type { ConnectorRun, SwitchSetupResult, SwitchSetupStatus } from './switch-setup-service'; import { + type ConnectorRun, connectorFailed, connectorSucceeded, connectorUnsupported, - marketplaceMatchesSource, -} from './switch-setup-service'; + type SwitchSetupResult, +} from './connector-run'; +import { cliRulesFor, type SwitchSetupCliRules } from './switch-setup-cli-dialect'; +import type { SwitchSetupStatus } from './switch-setup-service'; +import { marketplaceMatchesSource } from './switch-setup-service'; const EXEC_TIMEOUT_MS = 120_000; @@ -392,48 +394,43 @@ export class RemoteSwitchSetupService { return { ...(await this.getStatus(agentId)), refreshError }; } + /** + * Install the connector on this host, reporting the outcome. The `none` guard + * and the reporting of `unsupported` mirror the local driver exactly — see the + * note there for why the two must not differ. + */ async install(agentId: string): Promise { - const elapsed = startTimer(); - const { run, attempted } = await this.runInstall(agentId); - // An agent type with no connector to install did not fail to install one. - if (attempted) { - trackEvent('connector_installed', { - agent_type: agentTypeOf(agentId), - target: 'remote', - outcome: run.result.success ? 'success' : 'failure', - failure_reason: run.failure, - duration_ms: elapsed(), - }); + if (getPlugin(agentId).capabilities.switchSetup.kind === 'none') { + return connectorUnsupported().result; } + const elapsed = startTimer(); + const run = await this.runInstall(agentId); + trackEvent('connector_installed', { + agent_type: agentTypeOf(agentId), + target: 'remote', + outcome: run.result.success ? 'success' : 'failure', + failure_reason: run.failure, + duration_ms: elapsed(), + }); return run.result; } - private async runInstall(agentId: string): Promise<{ run: ConnectorRun; attempted: boolean }> { + private async runInstall(agentId: string): Promise { if (getPlugin(agentId).capabilities.switchSetup.kind === 'files') { - const run = await this.runFiles(agentId, (files, fs, version) => - files.install(fs, { version }) - ); - return { run, attempted: true }; + return this.runFiles(agentId, (files, fs, version) => files.install(fs, { version })); } const resolved = await this.resolve(agentId); - if (!resolved) return { run: connectorUnsupported(), attempted: false }; + if (!resolved) return connectorUnsupported(); const { descriptor, bin, ref, marketplaceSource, rules } = resolved; try { await this.ensureMarketplace(bin, descriptor.marketplaceName, marketplaceSource, rules); } catch (err) { - return { - run: connectorFailed(`Could not add marketplace: ${String(err)}`, 'marketplace_failed'), - attempted: true, - }; + return connectorFailed(`Could not add marketplace: ${String(err)}`, 'marketplace_failed'); } const res = await this.run(bin, rules.installArgs(ref, descriptor.scope)); - return { - run: - res.code === 0 - ? connectorSucceeded() - : connectorFailed(res.stderr.trim() || 'Install failed.', 'install_command_failed'), - attempted: true, - }; + return res.code === 0 + ? connectorSucceeded() + : connectorFailed(res.stderr.trim() || 'Install failed.', 'install_command_failed'); } /** diff --git a/console/apps/switch-console-desktop/src/main/core/switch-setup/switch-setup-service.test.ts b/console/apps/switch-console-desktop/src/main/core/switch-setup/switch-setup-service.test.ts index 815895cb5..62626ce5a 100644 --- a/console/apps/switch-console-desktop/src/main/core/switch-setup/switch-setup-service.test.ts +++ b/console/apps/switch-console-desktop/src/main/core/switch-setup/switch-setup-service.test.ts @@ -463,6 +463,8 @@ describe('switchSetupService mutations', () => { }); it('reports nothing when the agent type has no Switch setup to attempt', async () => { + // Not a failed install — there was never one to attempt. Reporting it would + // put every agent type in the app into the connector failure rate. mocks.getPlugin.mockReturnValue(NONE_AGENT); const result = await switchSetupService.install('no-switch-agent'); @@ -473,6 +475,29 @@ describe('switchSetupService mutations', () => { }); expect(mocks.trackEvent).not.toHaveBeenCalled(); }); + + it('reports `unsupported` when a declared connector has no binary to drive it', async () => { + // The other way an install ends with nothing installed, and this one IS a + // failure of what the user asked for: the agent type declares a CLI + // connector and the host binary cannot be resolved. It used to be silenced + // by the same flag as the case above while `update` and `uninstall` + // reported it, so the condition looked like it only happened on update. + mocks.getPlugin.mockReturnValue({ + ...CLI_AGENT, + capabilities: { ...CLI_AGENT.capabilities, hostDependency: { binaryNames: [] } }, + }); + + const result = await switchSetupService.install('claude'); + + expect(result.success).toBe(false); + expect(mocks.trackEvent).toHaveBeenCalledWith('connector_installed', { + agent_type: 'claude', + target: 'local', + outcome: 'failure', + failure_reason: 'unsupported', + duration_ms: expect.any(Number), + }); + }); }); describe('switchSetupService with the codex dialect', () => { diff --git a/console/apps/switch-console-desktop/src/main/core/switch-setup/switch-setup-service.ts b/console/apps/switch-console-desktop/src/main/core/switch-setup/switch-setup-service.ts index 3c616a434..dd4381997 100644 --- a/console/apps/switch-console-desktop/src/main/core/switch-setup/switch-setup-service.ts +++ b/console/apps/switch-console-desktop/src/main/core/switch-setup/switch-setup-service.ts @@ -7,13 +7,19 @@ import { type ArtifactName, artifactVersion } from '@switch-console/shared'; import { LocalExecutionContext } from '@main/core/execution-context/local-execution-context'; import { agentTypeOf } from '@main/core/telemetry/agent-type'; import { startTimer } from '@main/core/telemetry/duration'; -import type { TelemetryConnectorFailure } from '@main/core/telemetry/events'; import { trackEvent } from '@main/core/telemetry/telemetry-service'; import { log } from '@main/lib/logger'; import { isNewerVersion } from '@main/lib/semver'; import type { AgentTypeAvailability } from '@shared/core/switch-setup/agent-type-availability'; import { createPluginFs } from '../providers/plugin-fs'; import { getPlugin, listPlugins } from '../providers/plugin-registry'; +import { + type ConnectorRun, + connectorFailed, + connectorSucceeded, + connectorUnsupported, + type SwitchSetupResult, +} from './connector-run'; import { cliRulesFor, type InstalledPlugin, @@ -43,37 +49,6 @@ export function marketplaceMatchesSource(entry: RegisteredMarketplace, source: s return entry.source === source; } -/** Outcome of a mutating operation, mirroring the providers controller shape. */ -export type SwitchSetupResult = { success: boolean; message?: string }; - -/** - * A completed connector operation: what the caller gets back, and the - * enumerated reason it failed. - * - * The two travel together because a `SwitchSetupResult` carries only a message, - * and a message cannot be reported — so the code has to be named where the - * failure is known rather than recovered from the text afterwards. Shared with - * the remote driver, which reports the same event from the same points. - */ -export type ConnectorRun = { - result: SwitchSetupResult; - failure: TelemetryConnectorFailure; -}; - -/** A connector operation that did what was asked. */ -export function connectorSucceeded(): ConnectorRun { - return { result: { success: true }, failure: 'none' }; -} - -export function connectorFailed(message: string, failure: TelemetryConnectorFailure): ConnectorRun { - return { result: { success: false, message }, failure }; -} - -/** The answer for an agent whose connector nothing here can manage. */ -export function connectorUnsupported(): ConnectorRun { - return connectorFailed('Switch setup is not supported for this agent.', 'unsupported'); -} - const EXEC_TIMEOUT_MS = 120_000; /** @@ -414,7 +389,7 @@ class SwitchSetupService { resolved = this.resolveFiles(agentId); } catch (err) { log.error('switch-setup: file-based connector declares no behavior', { agentId, err }); - return connectorFailed(installFailureMessage(String(err)), 'files_unimplemented'); + return connectorFailed(String(err), 'files_unimplemented'); } if (!resolved) return connectorUnsupported(); try { @@ -429,31 +404,39 @@ class SwitchSetupService { } } + /** + * Install the connector, reporting the outcome. + * + * An agent type that declares no connector did not fail to install one, so it + * returns before the timer and reports nothing — the same guard `update` and + * `uninstall` use. Everything past it is an attempt a person made and is + * reported, `unsupported` included: a `cli` descriptor whose binary cannot be + * resolved is a real failure of the thing the user asked for, and suppressing + * it here while `update` reports it made the same condition look like it only + * ever happened on update. + */ async install(agentId: string): Promise { - const elapsed = startTimer(); - const { run, attempted } = await this.runInstall(agentId); - // An agent type with no connector to install did not fail to install one. - if (attempted) { - trackEvent('connector_installed', { - agent_type: agentTypeOf(agentId), - target: 'local', - outcome: run.result.success ? 'success' : 'failure', - failure_reason: run.failure, - duration_ms: elapsed(), - }); + if (getPlugin(agentId).capabilities.switchSetup.kind === 'none') { + return connectorUnsupported().result; } + const elapsed = startTimer(); + const run = await this.runInstall(agentId); + trackEvent('connector_installed', { + agent_type: agentTypeOf(agentId), + target: 'local', + outcome: run.result.success ? 'success' : 'failure', + failure_reason: run.failure, + duration_ms: elapsed(), + }); return run.result; } - private async runInstall(agentId: string): Promise<{ run: ConnectorRun; attempted: boolean }> { + private async runInstall(agentId: string): Promise { if (getPlugin(agentId).capabilities.switchSetup.kind === 'files') { - const run = await this.runFiles(agentId, (files, fs, version) => - files.install(fs, { version }) - ); - return { run, attempted: true }; + return this.runFiles(agentId, (files, fs, version) => files.install(fs, { version })); } const resolved = await this.resolve(agentId); - if (!resolved) return { run: connectorUnsupported(), attempted: false }; + if (!resolved) return connectorUnsupported(); const { descriptor, bin, ref, rules } = resolved; try { await this.ensureMarketplace( @@ -463,19 +446,12 @@ class SwitchSetupService { rules ); } catch (err) { - return { - run: connectorFailed(installFailureMessage(String(err)), 'marketplace_failed'), - attempted: true, - }; + return connectorFailed(installFailureMessage(String(err)), 'marketplace_failed'); } const res = await this.run(bin, rules.installArgs(ref, descriptor.scope)); - return { - run: - res.code === 0 - ? connectorSucceeded() - : connectorFailed(installFailureMessage(res.stderr.trim()), 'install_command_failed'), - attempted: true, - }; + return res.code === 0 + ? connectorSucceeded() + : connectorFailed(installFailureMessage(res.stderr.trim()), 'install_command_failed'); } /** diff --git a/console/apps/switch-console-desktop/src/main/core/telemetry/duration.test.ts b/console/apps/switch-console-desktop/src/main/core/telemetry/duration.test.ts index 7c316d69a..1ea8d0ae8 100644 --- a/console/apps/switch-console-desktop/src/main/core/telemetry/duration.test.ts +++ b/console/apps/switch-console-desktop/src/main/core/telemetry/duration.test.ts @@ -1,12 +1,18 @@ import { describe, expect, it } from 'vitest'; import { startTimer } from './duration'; +const SLEEP_MS = 20; + +function sleep(ms: number): Promise { + return new Promise((resolve) => setTimeout(resolve, ms)); +} + describe('timing an operation', () => { it('reports whole milliseconds, never a fraction', async () => { // The emitter refuses a non-finite number and the far end averages what it // gets, so the contract worth pinning is that this is a plain integer. const elapsed = startTimer(); - await new Promise((resolve) => setTimeout(resolve, 5)); + await sleep(SLEEP_MS); const ms = elapsed(); @@ -14,18 +20,16 @@ describe('timing an operation', () => { expect(ms).toBeGreaterThan(0); }); - it('reports zero rather than a negative for an operation too fast to measure', async () => { - // A negative duration in a payload is indistinguishable from real data at - // the far end, which is worse than no data. - expect(startTimer()()).toBeGreaterThanOrEqual(0); - }); - - it('can be read more than once, and does not restart', async () => { + it('accumulates across reads rather than restarting at each one', async () => { + // Asserting only that the second read is >= the first would pass against a + // timer that resets on every read: both reads would return one sleep's + // worth. The second read has to cover BOTH sleeps for that bug to show. const elapsed = startTimer(); - await new Promise((resolve) => setTimeout(resolve, 5)); + await sleep(SLEEP_MS); const first = elapsed(); - await new Promise((resolve) => setTimeout(resolve, 5)); + await sleep(SLEEP_MS); - expect(elapsed()).toBeGreaterThanOrEqual(first); + expect(first).toBeGreaterThanOrEqual(SLEEP_MS - 5); + expect(elapsed()).toBeGreaterThanOrEqual(first + SLEEP_MS - 5); }); }); diff --git a/console/apps/switch-console-desktop/src/main/core/telemetry/duration.ts b/console/apps/switch-console-desktop/src/main/core/telemetry/duration.ts index 286175875..9e83cdddd 100644 --- a/console/apps/switch-console-desktop/src/main/core/telemetry/duration.ts +++ b/console/apps/switch-console-desktop/src/main/core/telemetry/duration.ts @@ -7,11 +7,12 @@ import type { TelemetryDurationMs } from './events'; * or a machine waking mid-install cannot yield a negative duration or an hour * that never passed. `Date.now()` can do both, and a negative number reaching a * payload is worse than no number — it is data nobody can tell from real data. + * Because the clock only moves forwards, the result needs no clamping. * * Whole milliseconds. Sub-millisecond precision says nothing about an operation * that shells out to a package manager, and it keeps the value a plain integer - * at the far end. Nothing is capped or bucketed here; see `TelemetryDurationMs` - * for why, and for how to read the result. + * at the far end. See `TelemetryDurationMs` for what bounds the value and how to + * read the result. * * Call it before the work and call the returned function at the point the * outcome is known — not after the event is built, and not around the send, @@ -19,7 +20,5 @@ import type { TelemetryDurationMs } from './events'; */ export function startTimer(): () => TelemetryDurationMs { const start = performance.now(); - // Clamped defensively rather than because the clock can go backwards: it - // cannot. A future caller passing its own start value is the case this covers. - return () => Math.max(0, Math.round(performance.now() - start)); + return () => Math.round(performance.now() - start); } diff --git a/console/apps/switch-console-desktop/src/main/core/telemetry/events.ts b/console/apps/switch-console-desktop/src/main/core/telemetry/events.ts index cbc9960f4..7bf84303d 100644 --- a/console/apps/switch-console-desktop/src/main/core/telemetry/events.ts +++ b/console/apps/switch-console-desktop/src/main/core/telemetry/events.ts @@ -150,6 +150,12 @@ export type TelemetryCliFailure = * update: a host with no update verb removes the connector and puts it back, and * which half failed is the difference between "nothing changed" and "the agent * now has no connector at all". + * + * There is deliberately no `error` catch-all. Every variant here is produced by + * a named branch, so the set describes what the code can actually report; a + * bucket nothing fills would imply a coverage this has not got. An operation + * that throws reports nothing at all today — see the note on + * `connector_installed`. */ export type TelemetryConnectorFailure = | 'none' @@ -163,8 +169,7 @@ export type TelemetryConnectorFailure = /** The app writes this connector itself, and the write failed. */ | 'files_write_failed' /** The agent declares a file-based connector and implements no behavior for it. */ - | 'files_unimplemented' - | 'error'; + | 'files_unimplemented'; /** * How long an operation took, in whole milliseconds. @@ -175,11 +180,22 @@ export type TelemetryConnectorFailure = * waking mid-install cannot produce a negative number or an hour that never * passed. * - * **Nothing is capped.** A capped duration is a number the operation did not - * take, and inventing one is exactly what the rest of this catalogue refuses to - * do. Some of these legitimately include a password prompt somebody left on - * screen, so read them as percentiles rather than as a mean — a p50 that moves - * is a real regression, an arithmetic mean over this is meaningless. + * **Nothing is rounded into buckets and nothing is clamped**, so read these as + * percentiles rather than as a mean: some legitimately include a password prompt + * somebody left on screen, and an arithmetic mean over that is meaningless. + * + * Two things do bound it, and both shape the distribution: + * + * - Every command the connector drivers run carries `EXEC_TIMEOUT_MS` (120 s), + * so a `plugin install` that hangs on an auth prompt reports ~120000 rather + * than however long it would have hung. Expect a pile-up there, and at + * multiples of it for a reinstall-style update, which runs two commands. It is + * the timeout wall, not a latency distribution. + * - The connector events time the **whole operation the user waited on**, which + * includes registering the plugin marketplace. On a machine that has never had + * it, that step clones a repository; on every later install it is a no-op. So + * the first install on a machine is legitimately much slower than the rest, + * and a p50 that moves can be a change in that mix rather than a regression. * * It carries nothing about the machine: an elapsed time is not a fingerprint at * this resolution, and it names no path, host or command. @@ -376,6 +392,10 @@ export type TelemetryEventMap = { * plugin marketplace is driven through its CLI, and a host without one has its * connector written by the app. The two fail in entirely different places, * which is what `failure_reason` separates. + * + * Known gap: an operation that *throws* rather than returning a failed result + * is not reported at all, so the denominator is attempts that got far enough + * to produce a result. The same is true of `agent_cli_action`. */ connector_installed: { agent_type: TelemetryAgentType;