docs(telemetry): add instrumentation conventions, snake_case attribute keys

Add src/instrumentation/CONVENTIONS.md documenting how telemetry is structured
(explicit span threading, imperative setProperty, event/attribute naming,
namespace grouping, properties vs measurements) and link it from CONTRIBUTING.

Align existing instrumentation with the OTel naming convention:
- rename caller-supplied property/measurement keys from camelCase to snake_case
  (auth, ssh, workspace, activation, websocket, cli.download);
- strip unit suffixes from exported metric names into the OTLP unit field
  (latency_ms -> metric latency, unit ms) so Prometheus suffixes cleanly;
- group the token-refresh span under its namespace (auth.token_refreshed ->
  auth.token_refresh.completed) next to auth.token_refresh.deduped.

Framework-managed result/durationMs are left unchanged.

Author: EhabY

CONTRIBUTING.md

@@ -94,6 +94,15 @@ pnpm watch  # Rebuild extension and webviews on changes
 Press F5 to launch the Extension Development Host. Use "Developer: Reload
 Webviews" to see webview changes.
 
+## Telemetry
+
+Local telemetry instrumentation follows a shared style: how spans are threaded,
+how events and properties are named, and properties vs measurements. Read this
+before adding new telemetry so it stays consistent across the codebase. It lives
+next to the code:
+
+**[`src/instrumentation/CONVENTIONS.md`](src/instrumentation/CONVENTIONS.md)**
+
 ## Testing
 
 There are a few ways you can test the "Open in VS Code" flow:

src/instrumentation/CONVENTIONS.md

@@ -0,0 +1,124 @@
+# Telemetry conventions
+
+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.
+
+## Checklist
+
+- One instrumentation class per domain (`FooTelemetry`) wrapping
+  `TelemetryService`; business code imports that, never a raw span.
+- Event name is `domain.snake_case`; point-in-time logs use past tense.
+- Event names and attribute keys follow OTel: lowercase, `.` for hierarchy, `_`
+  to split words, never camelCase. Enumerated values are typed `snake_case`
+  unions, never bare `string`.
+- Numbers go in `measurements` (raw), never pre-bucketed into string properties.
+- Set attributes imperatively with `setProperty`/`setMeasurement`; never add a
+  return value that exists only to be logged.
+- 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`.
+
+## Layers
+
+- **Framework** (`src/telemetry`): `TelemetryService` (`trace`/`log`/`logError`)
+  hands out `Span` handles and owns IDs, timing, `result`, level-gating, and the
+  wire format. Telemetry-off is handled here (`NOOP_SPAN`), so instrumentation
+  never checks whether telemetry is enabled.
+- **Instrumentation** (`src/instrumentation/*`): one typed class per domain, the
+  only telemetry surface business code sees.
+
+## Threading
+
+Spans are passed **explicitly** as a callback argument; there is no
+ambient/active-span context. Two patterns keep telemetry out of business logic:
+
+1. **Imperative attributes** — `span.setProperty("outcome", "cache_hit")` at the
+   point the value is known. This is the standard OpenTelemetry model.
+2. **Typed phases** — wrap an async step in `span.phase(...)` and read one
+   property off its _natural_ return value, e.g.
+   `trace.versionCheck(() => this.checkBinary(...))`. Extraction stays out of
+   the business function.
+
+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.
+
+## Spans, phases, logs
+
+- `trace(name, fn, props?, meas?)` — a span with framework-set `result` and
+  `durationMs`. Use for an operation with a start and end.
+- `span.phase(name, fn, ...)` — the same, nested (composed as `parent.child`);
+  child names cannot contain `.`.
+- `span.log(name, ...)` / `span.logError(name, error, ...)` — point-in-time
+  events under a span, no `result`/`durationMs`. Use for discrete signals.
+
+## Naming
+
+| Thing                      | Convention                                  | Examples                                                  |
+| -------------------------- | ------------------------------------------- | --------------------------------------------------------- |
+| 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`         |
+| 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/):
+`.` is the namespace delimiter, `_` joins words within a segment, never
+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`).
+
+**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
+`auth.token_refresh.completed` / `.deduped` under `auth.token_refresh.*`. Keep
+the namespace node a pure prefix; don't also emit an event with that exact name,
+since dotted names otherwise read as span phases (`parent.child`). This is
+[OTel namespacing](https://opentelemetry.io/docs/specs/semconv/general/naming/),
+which exists precisely so related signals query together.
+
+**Namespacing.** OTel suggests prefixing custom attributes (`com.acme.*`) to
+avoid clashing with future semconv. We don't namespace event-level attributes:
+each is already scoped by its (namespaced) event name and only flows into
+Coder's own pipeline, so a bare `cache_source` can't collide with a future OTel
+`cache.source`. Resource and provenance attributes stay namespaced
+(`coder.deployment.url`, `coder.event.*`).
+
+## Properties vs measurements
+
+- **Properties** are low-cardinality string dimensions (the framework stringifies
+  `string | number | boolean`). Use them for what you group or filter by.
+- **Measurements** are raw numbers. Don't pre-bucket into string labels: both
+  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.
+  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
+  `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.
+
+## Outcomes, failures, 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.
+
+## 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.

src/instrumentation/activation.ts

@@ -20,25 +20,25 @@ export interface ActivationTracer {
 }
 
 /**
- * Emits `activation` with `authState`, plus a sibling `activation.deployment_init`
+ * Emits `activation` with `auth_state`, plus a sibling `activation.deployment_init`
  * trace (sibling, not child, because deployment init outlives the activation span).
  */
 export class ActivationTelemetry {
 	public constructor(private readonly telemetry: TelemetryService) {}
 
 	public trace<T>(fn: (tracer: ActivationTracer) => Promise<T>): Promise<T> {
 		return this.telemetry.trace("activation", async (span) => {
-			span.setProperty("authState", "none");
+			span.setProperty("auth_state", "none");
 			return fn({
-				setAuthState: (state) => span.setProperty("authState", state),
+				setAuthState: (state) => span.setProperty("auth_state", state),
 				traceDeploymentInit: (initFn) =>
 					this.telemetry.trace(
 						"activation.deployment_init",
 						async (childSpan) => {
-							childSpan.setProperty("authState", "unknown");
+							childSpan.setProperty("auth_state", "unknown");
 							const success = await initFn();
 							childSpan.setProperty(
-								"authState",
+								"auth_state",
 								success ? "valid_token" : "auth_failed",
 							);
 							return success;

src/instrumentation/auth.ts

@@ -88,7 +88,9 @@ export class AuthTelemetry {
 		trigger: AuthTokenRefreshTrigger,
 		fn: () => Promise<T>,
 	): Promise<T> {
-		return this.telemetry.trace("auth.token_refreshed", fn, { trigger });
+		return this.telemetry.trace("auth.token_refresh.completed", fn, {
+			trigger,
+		});
 	}
 
 	/** Logged when a refresh call joins an in-flight refresh and emits no span of its own. */
@@ -110,9 +112,9 @@ export class AuthTelemetry {
 					logReceived: () => span.log("received"),
 					setRecovery: (recovery) => span.setProperty("recovery", recovery),
 					setRefreshAttempted: (attempted) =>
-						span.setProperty("refreshAttempted", attempted),
+						span.setProperty("refresh_attempted", attempted),
 				}),
-			{ recovery: "none", refreshAttempted: false },
+			{ recovery: "none", refresh_attempted: false },
 		);
 	}
 

src/instrumentation/ssh.ts

@@ -57,7 +57,7 @@ export class SshTelemetry {
 		this.#telemetry.log(
 			"ssh.process.lost",
 			{ cause },
-			{ uptimeMs: now - this.#processStartedAtMs },
+			{ uptime_ms: now - this.#processStartedAtMs },
 		);
 	}
 
@@ -68,7 +68,7 @@ export class SshTelemetry {
 		this.#telemetry.log(
 			"ssh.process.recovered",
 			{},
-			{ recoveryDurationMs: performance.now() - this.#processLostAtMs },
+			{ recovery_duration_ms: performance.now() - this.#processLostAtMs },
 		);
 		this.#processLostAtMs = undefined;
 	}
@@ -80,14 +80,14 @@ export class SshTelemetry {
 		const now = performance.now();
 		if (this.#processStartedAtMs !== undefined) {
 			const measurements: Record<string, number> = {
-				previousUptimeMs: now - this.#processStartedAtMs,
+				previous_uptime_ms: now - this.#processStartedAtMs,
 			};
 			if (this.#processLostAtMs !== undefined) {
-				measurements.lostDurationMs = now - this.#processLostAtMs;
+				measurements.lost_duration_ms = now - this.#processLostAtMs;
 			}
 			this.#telemetry.log(
 				"ssh.process.replaced",
-				{ wasLost: this.#processLostAtMs !== undefined },
+				{ was_lost: this.#processLostAtMs !== undefined },
 				measurements,
 			);
 		}
@@ -105,8 +105,8 @@ export class SshTelemetry {
 		const now = performance.now();
 		this.#telemetry.log(
 			"ssh.process.disposed",
-			{ wasLost: this.#processLostAtMs !== undefined },
-			{ uptimeMs: now - this.#processStartedAtMs },
+			{ was_lost: this.#processLostAtMs !== undefined },
+			{ uptime_ms: now - this.#processStartedAtMs },
 		);
 		this.#processStartedAtMs = undefined;
 		this.#processLostAtMs = undefined;
@@ -130,12 +130,12 @@ export class SshTelemetry {
 			"ssh.network.sampled",
 			{
 				p2p: network.p2p,
-				preferredDerp: network.preferred_derp,
+				preferred_derp: network.preferred_derp,
 			},
 			{
-				latencyMs: network.latency,
-				downloadMbits: bytesPerSecondToMbits(network.download_bytes_sec),
-				uploadMbits: bytesPerSecondToMbits(network.upload_bytes_sec),
+				latency_ms: network.latency,
+				download_mbits: bytesPerSecondToMbits(network.download_bytes_sec),
+				upload_mbits: bytesPerSecondToMbits(network.upload_bytes_sec),
 			},
 		);
 	}

src/instrumentation/workspace.ts

@@ -43,7 +43,7 @@ interface ObservedAgentState {
 
 /**
  * Emits `workspace.state_transitioned` as a workspace progresses through
- * statuses, plus `observedBuildDurationMs` when a provisioner run resolves.
+ * statuses, plus `observed_build_duration_ms` when a provisioner run resolves.
  * Construct one per workspace; `WorkspaceMonitor` is the sole call site.
  */
 export class WorkspaceStateTelemetry {
@@ -73,7 +73,7 @@ export class WorkspaceStateTelemetry {
 
 		const now = performance.now();
 		const measurements: Record<string, number> = previous
-			? { observedDurationMs: now - previous.observedAtMs }
+			? { observed_duration_ms: now - previous.observedAtMs }
 			: {};
 
 		const wasProvisioning =
@@ -83,15 +83,15 @@ export class WorkspaceStateTelemetry {
 			this.buildStartedAtMs ??= now;
 		} else {
 			if (wasProvisioning && this.buildStartedAtMs !== undefined) {
-				measurements.observedBuildDurationMs = now - this.buildStartedAtMs;
+				measurements.observed_build_duration_ms = now - this.buildStartedAtMs;
 			}
 			this.buildStartedAtMs = undefined;
 		}
 
 		this.telemetry.log(
 			"workspace.state_transitioned",
 			{
-				workspaceName: this.workspaceName,
+				workspace_name: this.workspaceName,
 				from: previous?.status ?? INITIAL_STATE,
 				to: status,
 				"build.transition": buildTransition,
@@ -135,14 +135,14 @@ export class WorkspaceAgentTelemetry {
 		this.telemetry.log(
 			"workspace.agent.state_transitioned",
 			{
-				workspaceName: this.workspaceName,
-				agentName: agent.name,
+				workspace_name: this.workspaceName,
+				agent_name: agent.name,
 				"status.from": previous?.status ?? INITIAL_STATE,
 				"status.to": agent.status,
 				"lifecycle_state.from": previous?.lifecycleState ?? INITIAL_STATE,
 				"lifecycle_state.to": agent.lifecycle_state,
 			},
-			previous ? { observedDurationMs: now - previous.observedAtMs } : {},
+			previous ? { observed_duration_ms: now - previous.observedAtMs } : {},
 		);
 		this.observed = {
 			status: agent.status,
@@ -168,13 +168,13 @@ export class WorkspaceOperationTelemetry {
 
 	public traceUpdate<T>(fn: () => Promise<T>): Promise<T> {
 		return this.telemetry.trace("workspace.update.triggered", fn, {
-			workspaceName: this.workspaceName,
+			workspace_name: this.workspaceName,
 		});
 	}
 
 	public traceStart<T>(fn: () => Promise<T>): Promise<T> {
 		return this.telemetry.trace("workspace.start.triggered", fn, {
-			workspaceName: this.workspaceName,
+			workspace_name: this.workspaceName,
 		});
 	}
 
@@ -193,7 +193,7 @@ export class WorkspaceOperationTelemetry {
 				span.setProperty("action", action);
 				return action;
 			},
-			{ workspaceName: this.workspaceName, update_offered: outdated },
+			{ workspace_name: this.workspaceName, update_offered: outdated },
 		);
 	}
 
@@ -246,7 +246,7 @@ export class WorkspaceOperationTelemetry {
 	): Promise<T> {
 		return this.telemetry.trace("workspace.update.prompted", fn, {
 			prompt,
-			workspaceName: this.workspaceName,
+			workspace_name: this.workspaceName,
 		});
 	}
 }