refactor(telemetry): align outcome/count naming, fold review findings

- rename the trace outcome hook fail() -> error() and the category
  fetch_failed -> fetch_error to match the recordError vocabulary
- name count measurements <entity>.count (agent.count, workspace.count,
  event.count, interval.count) per OTel, replacing flat _count keys
- set error.type on auth error spans (login/logout exceptions, auth_failed)
  and split auth outcomes so errors carry error.type, aborts carry reason
- fold the general telemetry review findings into CONVENTIONS.md and trim
  for conciseness

Author: EhabY

src/commands.ts

@@ -285,7 +285,7 @@ export class Commands {
 			return;
 		}
 		if (resolved.status === "failed") {
-			telemetry.fail(resolved.category);
+			telemetry.error(resolved.category);
 			return;
 		}
 
@@ -346,7 +346,7 @@ export class Commands {
 				telemetry.abort("progress");
 				return;
 			}
-			telemetry.fail();
+			telemetry.error();
 			this.logger.error("Speed test failed", result.error);
 			vscode.window.showErrorMessage(
 				`Speed test failed: ${toError(result.error).message}`,
@@ -364,14 +364,14 @@ export class Commands {
 			});
 		} catch (err) {
 			if (err instanceof ZodError || err instanceof SyntaxError) {
-				telemetry.fail("parse_error");
+				telemetry.error("parse_error");
 				this.logger.error("Failed to parse speedtest output", err);
 				vscode.window.showErrorMessage(
 					"Speed test output did not match the expected format. Check `Output > Coder` for details.",
 				);
 				return;
 			}
-			telemetry.fail();
+			telemetry.error();
 			this.logger.error("Failed to display speedtest results", err);
 			vscode.window.showErrorMessage(
 				`Speed test returned unexpected output: ${toError(err).message}`,
@@ -395,7 +395,7 @@ export class Commands {
 			return;
 		}
 		if (resolved.status === "failed") {
-			telemetry.fail(resolved.category);
+			telemetry.error(resolved.category);
 			return;
 		}
 
@@ -444,7 +444,7 @@ export class Commands {
 				telemetry.abort("progress");
 				return;
 			}
-			telemetry.fail(
+			telemetry.error(
 				result.error instanceof SupportBundleUnsupportedCliError
 					? "unsupported_cli"
 					: undefined,
@@ -915,7 +915,7 @@ export class Commands {
 				return false;
 			}
 			if (pick.status === "failed") {
-				telemetry.fail(pick.category);
+				telemetry.error(pick.category);
 				return false;
 			}
 			workspace = pick.workspace;
@@ -1224,7 +1224,7 @@ export class Commands {
 						}
 					})
 					.catch((ex) => {
-						fetchErrorCategory = "fetch_failed";
+						fetchErrorCategory = "fetch_error";
 						this.logger.error("Failed to fetch workspaces", ex);
 						if (ex instanceof CertificateError) {
 							void ex.showNotification();

src/instrumentation/CONVENTIONS.md

@@ -2,8 +2,7 @@
 
 How to add telemetry so every instrumentation reads the same way. The framework
 lives in `src/telemetry`; the per-domain instrumentation business code talks to
-lives here in `src/instrumentation`. See `cli.ts` (spans + phases) and `ssh.ts`
-(point-in-time logs) as references.
+lives here in `src/instrumentation`.
 
 ## Checklist
 
@@ -19,8 +18,8 @@ lives here in `src/instrumentation`. See `cli.ts` (spans + phases) and `ssh.ts`
 - No secrets, tokens, query strings, or unbounded values in properties; routes
   go through `normalizeRoute`.
 - Let the framework set `result`; add a domain `outcome` only when an operation
-  has several success modes. Failures go to a typed union; non-error early exits
-  call `markAborted`.
+  has several success modes. Errors go to a typed `error.type` union; non-error
+  early exits call `markAborted`.
 
 ## Layers
 
@@ -31,6 +30,15 @@ lives here in `src/instrumentation`. See `cli.ts` (spans + phases) and `ssh.ts`
 - **Instrumentation** (`src/instrumentation/*`): one typed class per domain, the
   only telemetry surface business code sees.
 
+## Structure
+
+- Split instrumentation files along the same boundaries as the business code,
+  not one catch-all module.
+- Shared span helpers (`recordError`, `recordAborted`) live in one shared module,
+  not duplicated per file.
+- Record-error-then-rethrow-outside-the-span logic lives once per class, in a
+  single private helper, not in every `traceX` method.
+
 ## Threading
 
 Spans are passed **explicitly** as a callback argument; there is no
@@ -46,6 +54,15 @@ ambient/active-span context. Two patterns keep telemetry out of business logic:
 Never return a value purely so a caller can log it; that couples the return type
 to observability. Returning is fine when the business uses the value too.
 
+## Callers
+
+- Declare telemetry dimensions explicitly at the call site; pass `source: "uri"`
+  rather than inferring it from which arguments happen to be set.
+- Keep business bodies in named private `runX(args, trace)` methods; the public
+  method just opens the span and wraps them. Small diffs, named telemetry seam.
+- When sibling events share a correlating property, emit it on every event in the
+  family; don't drop it from new ones.
+
 ## Spans, phases, logs
 
 - `trace(name, fn, props?, meas?)` — a span with framework-set `result` and
@@ -62,7 +79,7 @@ to observability. Returning is fine when the business uses the value too.
 | Event name                 | `domain.snake_case`                         | `cli.resolve`, `remote.setup`, `connection.dropped`       |
 | Point-in-time log          | past tense                                  | `connection.dropped`, `ssh.process.lost`                  |
 | Child phase                | bare `snake_case`                           | `cache_lookup`, `version_check`, `connection_handoff`     |
-| Property / measurement key | lowercase; `_` splits words, `.` for groups | `cache_source`, `failure_category`, `status.from`         |
+| Property / measurement key | lowercase; `_` splits words, `.` for groups | `cache_source`, `error.type`, `status.from`               |
 | Enumerated value           | typed `snake_case` union                    | `"cache_hit"`, `"session_token"`, `"unrecoverable_close"` |
 
 This is the [OTel attribute convention](https://opentelemetry.io/docs/specs/semconv/general/naming/):
@@ -71,6 +88,10 @@ camelCase. Default to a flat `snake_case` key; use `.` only to group genuinely
 related attributes (a `status.from` / `status.to` pair). Keep phase names
 subject-first within a domain (`agent_resolve`, not `resolve_agent`).
 
+**Methods.** Span-wrapper methods are `trace<Noun>` (`traceOpen`,
+`traceConfirmationPrompt`); don't echo the event's past-tense suffix
+(`traceUpdateConfirmationPrompted`), and drop qualifiers the class implies.
+
 **Grouping.** Group related events under a shared dotted namespace so a prefix
 query returns the whole family: `workspace.update.triggered` and
 `workspace.update.prompted` both sit under `workspace.update.*`, as do
@@ -102,23 +123,39 @@ Coder's own pipeline, so a bare `cache_source` can't collide with a future OTel
   `latency`, unit `ms`, which Prometheus then suffixes itself); log and span
   attributes keep the key as written. You always author `latency_ms`; only the
   exported metric name changes.
+- **Counts.** Name a count `<entity>.count`, singular entity (`agent.count`,
+  `workspace.count`), per OTel (`system.process.count`) — not flat `agent_count`.
+  Related counts share the namespace (`agent.count`, `agent.connected_count`); a
+  count with no entity (`retry_count`) stays flat.
 
-## Outcomes, failures, aborts
+## Outcomes, errors, aborts
 
 - The framework sets `result` (`success` / `error` / `aborted`) on every span;
   don't duplicate it.
 - Add a domain `outcome` property only when an operation has several success
   modes worth distinguishing (e.g. `cli.resolve`: `cache_hit`, `downloaded`).
-- Classify failures into a typed union (`failure_category`, or a domain
-  `reason`) via a `categorize*Failure` helper rather than emitting raw error
-  strings; the
-  framework already captures the error block.
-- For a non-error early exit (cancelled, not-found), call `span.markAborted()`
-  rather than throwing.
+- Classify errors into a typed `error.type` union via a `categorize*Error` helper
+  rather than emitting raw error strings; the framework captures the error block.
+- For a non-error early exit (backed out, not-found), call `span.markAborted()`
+  rather than throwing, recording its reason in a separate key, not `error.type`.
+- A trace exposes one outcome trio — `abort(stage)`, `error(category?)`,
+  `succeed*(payload)` — over the shared `recordAborted` / `recordError` helpers;
+  each outcome sets one, never two.
+- Prefer **error** over "failure" and **abort** over "cancel" here (`recordError`
+  / `error.type` / `error()`; `recordAborted` / `markAborted` / `abort()`).
+  Point-in-time logs keep past tense (`recovery_failed`) — they state what
+  happened, not a span's `error` result.
 
 ## Safety
 
 Never put tokens, credentials, full URLs with query strings, or unbounded user
 input into properties. Routes go through `normalizeRoute`
 (`src/logging/routeNormalization.ts`). Prefer a closed union over a free-form
 string for any property a dashboard groups by.
+
+## Tests
+
+- Telemetry-only tests of business code are `<subject>.telemetry.test.ts`;
+  instrumentation modules keep `<module>.test.ts` and split when the module does.
+- Assert what privacy intends to omit, not only what is present (e.g.
+  `workspace_name` undefined on command events).

src/instrumentation/auth.ts

@@ -58,7 +58,7 @@ export class AuthTelemetry {
 					}
 					return result;
 				} catch (error) {
-					span.setProperty("reason", "exception");
+					span.setProperty("error.type", "exception");
 					throw error;
 				}
 			},
@@ -78,7 +78,7 @@ export class AuthTelemetry {
 				}
 				return result;
 			} catch (error) {
-				span.setProperty("reason", "exception");
+				span.setProperty("error.type", "exception");
 				throw error;
 			}
 		});
@@ -143,11 +143,11 @@ export class AuthTelemetry {
 
 /** `auth_failed` is a real error; user/URL dismissals are intentional aborts. */
 function recordReason(span: Span, reason: LoginPromptReason): void {
-	span.setProperty("reason", reason);
 	if (reason === "auth_failed") {
 		span.setProperty("error.type", reason);
 		span.markError();
 	} else {
+		span.setProperty("reason", reason);
 		span.markAborted();
 	}
 }

src/instrumentation/diagnostics.ts

@@ -25,7 +25,7 @@ export type DiagnosticAbortStage =
 
 export interface DiagnosticTrace {
 	abort(stage: DiagnosticAbortStage): void;
-	fail(category?: DiagnosticErrorCategory): void;
+	error(category?: DiagnosticErrorCategory): void;
 	setRequestedDuration(seconds: number): void;
 	succeedSpeedtest(result: SpeedtestResult): void;
 	succeedExport(format: string, eventCount: number): void;
@@ -54,7 +54,7 @@ class SpanDiagnosticTrace implements DiagnosticTrace {
 		recordAborted(this.span, stage);
 	}
 
-	public fail(category: DiagnosticErrorCategory = "error"): void {
+	public error(category: DiagnosticErrorCategory = "error"): void {
 		recordError(this.span, category);
 	}
 
@@ -63,7 +63,7 @@ class SpanDiagnosticTrace implements DiagnosticTrace {
 	}
 
 	public succeedSpeedtest(result: SpeedtestResult): void {
-		this.span.setMeasurement("interval_count", result.intervals.length);
+		this.span.setMeasurement("interval.count", result.intervals.length);
 		this.span.setMeasurement(
 			"throughput_mbits",
 			result.overall.throughput_mbits,
@@ -72,6 +72,6 @@ class SpanDiagnosticTrace implements DiagnosticTrace {
 
 	public succeedExport(format: string, eventCount: number): void {
 		this.span.setProperty("format", format);
-		this.span.setMeasurement("event_count", eventCount);
+		this.span.setMeasurement("event.count", eventCount);
 	}
 }

src/instrumentation/workspaceOpen.ts

@@ -24,7 +24,7 @@ export type WorkspaceOpenSource =
 	| "uri";
 
 export type WorkspacePickerSource = "workspace_open" | "diagnostic";
-export type WorkspacePickerErrorCategory = "fetch_failed";
+export type WorkspacePickerErrorCategory = "fetch_error";
 export type WorkspaceOpenErrorCategory =
 	| WorkspacePickerErrorCategory
 	| AbortableErrorCategory;
@@ -56,7 +56,7 @@ export interface WorkspaceOpenTrace {
 		stage: WorkspaceOpenAbortStage,
 		selection?: WorkspaceOpenSelection,
 	): void;
-	fail(category: WorkspaceOpenErrorCategory): void;
+	error(category: WorkspaceOpenErrorCategory): void;
 	handoff(kind: "folder" | "empty_window"): void;
 }
 
@@ -148,7 +148,7 @@ class SpanWorkspacePickerTrace implements WorkspacePickerTrace {
 	public constructor(private readonly span: Span) {}
 
 	public finish(result: WorkspacePickerResult, resultCount: number): void {
-		this.span.setMeasurement("workspace_count", resultCount);
+		this.span.setMeasurement("workspace.count", resultCount);
 		if (result.status === "selected") {
 			recordWorkspaceContext(this.span, result.workspace);
 			return;
@@ -178,7 +178,7 @@ class SpanWorkspaceOpenTrace implements WorkspaceOpenTrace {
 		recordAborted(this.span, stage);
 	}
 
-	public fail(category: WorkspaceOpenErrorCategory): void {
+	public error(category: WorkspaceOpenErrorCategory): void {
 		recordError(this.span, category);
 	}
 
@@ -195,9 +195,9 @@ function recordWorkspaceContext(
 	const agents = extractAgents(workspace.latest_build.resources);
 	span.setProperty("workspace_status", workspace.latest_build.status);
 	span.setProperty("workspace_outdated", workspace.outdated);
-	span.setMeasurement("agent_count", agents.length);
+	span.setMeasurement("agent.count", agents.length);
 	span.setMeasurement(
-		"connected_agent_count",
+		"agent.connected_count",
 		agents.filter((candidate) => candidate.status === "connected").length,
 	);
 	if (!agent) {

src/telemetry/export/command.ts

@@ -35,7 +35,7 @@ const PROGRESS_OPTIONS = {
  */
 export interface ExportTelemetryObserver {
 	abort(stage: "prompt" | "progress"): void;
-	fail(): void;
+	error(): void;
 	succeedExport(format: ExportFormat, eventCount: number): void;
 }
 
@@ -102,7 +102,7 @@ async function reportOutcome(
 			observer.abort("progress");
 			return;
 		}
-		observer.fail();
+		observer.error();
 		logger.error("Telemetry export failed", result.error);
 		void vscode.window.showErrorMessage(
 			`Telemetry export failed: ${toError(result.error).message}`,

test/unit/commands.telemetry.test.ts

@@ -207,7 +207,7 @@ describe("Commands", () => {
 				properties: {
 					method: "cli_token",
 					result: "error",
-					reason: "auth_failed",
+					"error.type": "auth_failed",
 				},
 			});
 			expect(mocks.deploymentManager.setDeployment).not.toHaveBeenCalled();
@@ -267,7 +267,7 @@ describe("Commands", () => {
 			await expect(commands.logout()).rejects.toThrow("secret clear failed");
 
 			expect(sink.expectOne("auth.logout")).toMatchObject({
-				properties: { result: "error", reason: "exception" },
+				properties: { result: "error", "error.type": "exception" },
 				error: { message: "secret clear failed" },
 			});
 		});

test/unit/instrumentation/auth.test.ts

@@ -46,7 +46,7 @@ describe("AuthTelemetry", () => {
 					source: "uri",
 					method: "oauth",
 					result: "error",
-					reason: "auth_failed",
+					"error.type": "auth_failed",
 				},
 			},
 		])("$name", async ({ outcome, traceMethod, properties }) => {
@@ -101,7 +101,7 @@ describe("AuthTelemetry", () => {
 			expect(sink.expectOne("auth.logout")).toMatchObject({
 				properties: {
 					result: "error",
-					reason: "exception",
+					"error.type": "exception",
 				},
 				error: { message: "clear failed" },
 			});

test/unit/instrumentation/diagnostics.test.ts

@@ -18,7 +18,7 @@ describe("DiagnosticTelemetry", () => {
 			return Promise.resolve();
 		});
 		await telemetry.trace("support_bundle", (trace) => {
-			trace.fail("unsupported_cli");
+			trace.error("unsupported_cli");
 			return Promise.resolve();
 		});
 
@@ -59,7 +59,7 @@ describe("DiagnosticTelemetry", () => {
 
 		expect(sink.expectOne("command.diagnostic.completed")).toMatchObject({
 			measurements: {
-				interval_count: 1,
+				"interval.count": 1,
 				throughput_mbits: 42,
 			},
 			properties: { result: "success" },