refactor(telemetry): fix convention violations from instrumentation audit

- rename command.invoked commandId -> command_id and CliCacheSource
  values to snake_case (file_path, not_found)
- lowercase ConnectionState values on connection.state_transitioned;
  rename the caller-set result key on connection.reconnect_resolved to
  outcome so result stays framework-managed
- thrown aborts mark spans aborted instead of error.type="aborted"
  (workspace.open, dev container open, credential store/clear)
- type the diagnostic export format as ExportFormat, wrap
  auth.session_lookup in AuthTelemetry, resolve _seconds measurement
  suffixes to an OTLP unit

Author: EhabY

src/core/cliManager.ts

@@ -43,8 +43,8 @@ import type { CliCredentialManager } from "./cliCredentialManager";
 import type { PathResolver } from "./pathResolver";
 
 type ResolvedBinary =
-	| { binPath: string; stat: Stats; source: "file-path" | "directory" }
-	| { binPath: string; source: "not-found" };
+	| { binPath: string; stat: Stats; source: "file_path" | "directory" }
+	| { binPath: string; source: "not_found" };
 
 type CliVerifyResult =
 	| { kind: "verified" }
@@ -77,7 +77,7 @@ export class CliManager {
 	public async locateBinary(url: string): Promise<string> {
 		const safeHostname = toSafeHost(url);
 		const resolved = await this.resolveBinaryPath(safeHostname);
-		if (resolved.source === "not-found") {
+		if (resolved.source === "not_found") {
 			throw new Error(`No CLI binary found at ${resolved.binPath}`);
 		}
 		return resolved.binPath;
@@ -86,9 +86,9 @@ export class CliManager {
 	/**
 	 * Resolve the CLI binary path from the configured cache path.
 	 *
-	 * Returns "file-path" when the cache path is an existing file (checked for
+	 * Returns "file_path" when the cache path is an existing file (checked for
 	 * version match and updated if needed), "directory" when a binary was found
-	 * inside the directory, or "not-found" with the platform-specific path for
+	 * inside the directory, or "not_found" with the platform-specific path for
 	 * the caller to download into.
 	 */
 	private async resolveBinaryPath(
@@ -98,14 +98,14 @@ export class CliManager {
 		const cacheStat = await cliUtils.stat(cachePath);
 
 		if (cacheStat?.isFile()) {
-			return { binPath: cachePath, stat: cacheStat, source: "file-path" };
+			return { binPath: cachePath, stat: cacheStat, source: "file_path" };
 		}
 
 		const fullNamePath = path.join(cachePath, cliUtils.fullName());
 
 		// Path does not exist yet; return the platform-specific path to download.
 		if (!cacheStat) {
-			return { binPath: fullNamePath, source: "not-found" };
+			return { binPath: fullNamePath, source: "not_found" };
 		}
 
 		// Directory exists; check platform-specific name, then simple name.
@@ -120,7 +120,7 @@ export class CliManager {
 			return { binPath: simpleNamePath, stat: simpleStat, source: "directory" };
 		}
 
-		return { binPath: fullNamePath, source: "not-found" };
+		return { binPath: fullNamePath, source: "not_found" };
 	}
 
 	/**
@@ -240,7 +240,7 @@ export class CliManager {
 			);
 		}
 
-		if (resolved.source === "not-found") {
+		if (resolved.source === "not_found") {
 			this.output.info("No existing binary found, starting download");
 			return {
 				buildInfo,
@@ -442,7 +442,7 @@ export class CliManager {
 		downloadBinPath: string,
 	): Promise<string> {
 		if (
-			resolved.source === "file-path" &&
+			resolved.source === "file_path" &&
 			downloadBinPath !== resolved.binPath
 		) {
 			this.output.info(

src/core/commandManager.ts

@@ -62,7 +62,7 @@ export class CommandManager implements vscode.Disposable {
 		}
 
 		const invoke = handler as (...args: unknown[]) => unknown;
-		const properties = { commandId: id };
+		const properties = { command_id: id };
 		const wrapped = (...args: unknown[]): Thenable<unknown> =>
 			this.telemetry.trace(
 				COMMAND_INVOKED_EVENT,

src/instrumentation/CONVENTIONS.md

@@ -116,7 +116,8 @@ Coder's own pipeline, so a bare `cache_source` can't collide with a future OTel
   export as record attributes, and a query can bucket the raw number at read
   time. `result` and `durationMs` are framework-managed and cannot be set.
 - **Units.** There is no unit field at emit time, so put the unit in the
-  measurement key as a `_ms` / `_mbits` suffix, the same way for every event.
+  measurement key as a `_ms` / `_seconds` / `_mbits` suffix, the same way for
+  every event.
   The OTLP exporter then resolves it per signal: for metric events
   (`http.requests`, `ssh.network.sampled`) it moves the suffix into the OTLP
   `unit` field and drops it from the metric name (`latency_ms` exports as metric

src/instrumentation/auth.ts

@@ -84,6 +84,11 @@ export class AuthTelemetry {
 		});
 	}
 
+	/** Wraps the secret-storage session read that seeds a remote connection. */
+	public traceSessionLookup<T>(fn: () => Promise<T>): Promise<T> {
+		return this.telemetry.trace("auth.session_lookup", fn);
+	}
+
 	public traceTokenRefresh<T>(
 		trigger: AuthTokenRefreshTrigger,
 		fn: () => Promise<T>,

src/instrumentation/cli.ts

@@ -5,7 +5,7 @@ import type { CallerPropertyValue } from "../telemetry/event";
 import type { TelemetryService } from "../telemetry/service";
 import type { Span } from "../telemetry/span";
 
-export type CliCacheSource = "file-path" | "directory" | "not-found";
+export type CliCacheSource = "file_path" | "directory" | "not_found";
 export type CliDownloadReason = "missing" | "version_mismatch" | "unreadable";
 export type CliDownloadAction = "download" | "fallback" | "blocked";
 export type CliCredentialSource = "session_token" | "empty_token";

src/instrumentation/credentials.ts

@@ -6,7 +6,7 @@ import type { WorkspaceConfiguration } from "vscode";
 import type { TelemetryReporter } from "../telemetry/reporter";
 import type { Span } from "../telemetry/span";
 
-export type CredentialErrorCategory = "aborted" | "binary" | "cli" | "file";
+export type CredentialErrorCategory = "binary" | "cli" | "file";
 
 type CredentialEvent = "auth.credential.store" | "auth.credential.clear";
 
@@ -47,12 +47,12 @@ export class CredentialTelemetry {
 				try {
 					await fn(span);
 				} catch (error) {
-					span.setProperty("error.type", categorizeCredentialError(error));
 					if (isAbortError(error)) {
 						span.markAborted();
 						aborted = error;
 						return;
 					}
+					span.setProperty("error.type", categorizeCredentialError(error));
 					throw error;
 				}
 			},
@@ -68,9 +68,6 @@ export class CredentialTelemetry {
 }
 
 function categorizeCredentialError(error: unknown): CredentialErrorCategory {
-	if (isAbortError(error)) {
-		return "aborted";
-	}
 	if (error instanceof CredentialFileError) {
 		return "file";
 	}

src/instrumentation/diagnostics.ts

@@ -2,6 +2,7 @@ import { recordAborted, recordError } from "./outcomes";
 
 import type { SpeedtestResult } from "@repo/shared";
 
+import type { ExportFormat } from "../telemetry/export/writers/types";
 import type { TelemetryReporter } from "../telemetry/reporter";
 import type { Span } from "../telemetry/span";
 
@@ -28,7 +29,7 @@ export interface DiagnosticTrace {
 	error(category?: DiagnosticErrorCategory): void;
 	setRequestedDuration(seconds: number): void;
 	succeedSpeedtest(result: SpeedtestResult): void;
-	succeedExport(format: string, eventCount: number): void;
+	succeedExport(format: ExportFormat, eventCount: number): void;
 }
 
 /** Emits `command.diagnostic.completed` around each diagnostic command. */
@@ -70,7 +71,7 @@ class SpanDiagnosticTrace implements DiagnosticTrace {
 		);
 	}
 
-	public succeedExport(format: string, eventCount: number): void {
+	public succeedExport(format: ExportFormat, eventCount: number): void {
 		this.span.setProperty("format", format);
 		this.span.setMeasurement("event.count", eventCount);
 	}

src/instrumentation/outcomes.ts

@@ -2,8 +2,6 @@ import { isAbortError } from "../error/errorUtils";
 
 import type { Span } from "../telemetry/span";
 
-export type AbortableErrorCategory = "aborted" | "error";
-
 /** Records a categorized error without attaching the raw error details. */
 export function recordError(span: Span, category: string): void {
 	span.setProperty("error.type", category);
@@ -16,8 +14,11 @@ export function recordAborted(span: Span, stage: string): void {
 	span.markAborted();
 }
 
-export function categorizeAbortableError(
-	error: unknown,
-): AbortableErrorCategory {
-	return isAbortError(error) ? "aborted" : "error";
+/** Marks a thrown abort as `aborted`; records anything else as a categorized `"error"`. */
+export function recordAbortableError(span: Span, error: unknown): void {
+	if (isAbortError(error)) {
+		span.markAborted();
+	} else {
+		recordError(span, "error");
+	}
 }

src/instrumentation/websocket.ts

@@ -29,12 +29,19 @@ export type ConnectionDropCause =
 	| "error";
 
 type ReconnectOutcome =
-	| { readonly result: "success" }
+	| { readonly outcome: "success" }
 	| {
-			readonly result: "error";
+			readonly outcome: "error";
 			readonly terminationReason: ConnectionStateReason;
 	  };
 
+/** `ConnectionState` lowercased into the OTel snake_case attribute value. */
+type ConnectionStateValue = Lowercase<`${ConnectionState}`>;
+
+function stateValue(state: ConnectionState): ConnectionStateValue {
+	return state.toLowerCase() as ConnectionStateValue;
+}
+
 interface ReconnectCycle {
 	readonly startMs: number;
 	readonly reason: ConnectionStateReason;
@@ -64,8 +71,8 @@ export class WebSocketTelemetry {
 		reason: ConnectionStateReason,
 	): void {
 		this.#telemetry.log("connection.state_transitioned", {
-			from,
-			to,
+			from: stateValue(from),
+			to: stateValue(to),
 			reason,
 		});
 	}
@@ -88,7 +95,7 @@ export class WebSocketTelemetry {
 			{ route: normalizeRoute(route) },
 			{ connect_duration_ms: now - start },
 		);
-		this.#finishReconnect({ result: "success" });
+		this.#finishReconnect({ outcome: "success" });
 	}
 
 	public dropped(
@@ -144,7 +151,7 @@ export class WebSocketTelemetry {
 	/** Drop and end the reconnect cycle as a failure. */
 	public terminated(reason: ConnectionStateReason, options: DropOptions): void {
 		this.dropped(options.cause, options.code, options.error);
-		this.#finishReconnect({ result: "error", terminationReason: reason });
+		this.#finishReconnect({ outcome: "error", terminationReason: reason });
 	}
 
 	/** Drop and (re)open a reconnect cycle. */
@@ -168,11 +175,11 @@ export class WebSocketTelemetry {
 		}
 		this.#reconnectCycle = undefined;
 
-		const properties: Record<string, string> = {
-			result: outcome.result,
+		const properties: CallerProperties = {
+			outcome: outcome.outcome,
 			reason: cycle.reason,
 		};
-		if (outcome.result === "error") {
+		if (outcome.outcome === "error") {
 			properties.termination_reason = outcome.terminationReason;
 		}
 		this.#telemetry.log("connection.reconnect_resolved", properties, {

src/instrumentation/workspaceOpen.ts

@@ -1,11 +1,6 @@
 import { extractAgents } from "../api/api-helper";
 
-import {
-	type AbortableErrorCategory,
-	categorizeAbortableError,
-	recordAborted,
-	recordError,
-} from "./outcomes";
+import { recordAbortableError, recordAborted, recordError } from "./outcomes";
 
 import type {
 	Workspace,
@@ -25,9 +20,7 @@ export type WorkspaceOpenSource =
 
 export type WorkspacePickerSource = "workspace_open" | "diagnostic";
 export type WorkspacePickerErrorCategory = "fetch_error";
-export type WorkspaceOpenErrorCategory =
-	| WorkspacePickerErrorCategory
-	| AbortableErrorCategory;
+export type WorkspaceOpenErrorCategory = WorkspacePickerErrorCategory | "error";
 export type WorkspacePickerResult =
 	| { readonly status: "selected"; readonly workspace: Workspace }
 	| { readonly status: "cancelled" }
@@ -114,8 +107,9 @@ export class WorkspaceOpenTelemetry {
 	}
 
 	/**
-	 * Runs `fn` inside the span, recording a thrown error as a categorized
-	 * error without its raw details, then rethrows outside the span.
+	 * Runs `fn` inside the span, recording a thrown abort as `aborted` and any
+	 * other error as a categorized error without its raw details, then rethrows
+	 * outside the span.
 	 */
 	private async traceRethrowing<T>(
 		eventName: string,
@@ -131,7 +125,7 @@ export class WorkspaceOpenTelemetry {
 					return await fn(span);
 				} catch (error) {
 					thrown = { error };
-					recordError(span, categorizeAbortableError(error));
+					recordAbortableError(span, error);
 					return fallback;
 				}
 			},