feat(telemetry): roll up http requests

[EhabY]

Summary

• Add http.requests telemetry rollups for active (method, route) buckets from the existing axios logging interceptor path.
• Normalize routes to strip queries and replace IDs/user/workspace names before telemetry emission.
• Add coder.telemetry.local.httpRequests.windowSeconds as an advanced JSON setting with a 60s default.
• Wire telemetry into the primary and remote-scoped CoderApi clients while keeping throwaway clients opt-in.

Closes #907.

Validation

• pnpm test:extension ./test/unit/settings/telemetry.test.ts ./test/unit/logging/httpRequestsTelemetry.test.ts ./test/unit/api/coderApi.test.ts
• pnpm test:extension
• pnpm format:check
• pnpm lint
• pnpm typecheck

Implementation plan and decisions

Decisions

1. coder.telemetry.local is treated as advanced local telemetry runtime configuration, not just local sink configuration.
2. The HTTP rollup setting lives at coder.telemetry.local.httpRequests.windowSeconds with a default of 60.
3. Throwaway CoderApi instances remain telemetry-free by default; long-lived primary and remote-scoped API clients receive telemetry.
4. HTTP aggregation hooks into the existing axios logging interceptor path rather than adding a parallel interceptor.
5. Route normalization rules are kept alongside the aggregator for easy route maintenance.

Implementation

• Added HttpRequestsTelemetry for tumbling-window aggregation and http.requests emission.
• Added per-route normalization for IDs, numeric segments, user names, and workspace names.
• Added a small TelemetryReporter interface/no-op reporter to decouple CoderApi from concrete telemetry service construction.
• Extended coder.telemetry.local schema and runtime readers with httpRequests.windowSeconds.
• Added tests for same-bucket rollup, status buckets, empty windows, p95, route normalization, settings parsing, and axios interceptor wiring.

Generated by Coder Agents.

[EhabY]

/coder-agents-review

[coder-agents-review]

Good addition. The tumbling-window aggregation design is proportional to the problem, the TelemetryReporter interface extraction keeps coupling narrow, and the NOOP sentinel pattern for throwaway clients is clean. The test harness is well-structured with fake timers, meaningful assertions, and non-vacuous coverage.

Severity summary: 1 P2, 13 P3, 1 P4, 5 Nit, 2 Note.

The P2 is the UUID regex version nibble constraint ([1-5]), which rejects UUIDv6/v7/v8 in the fallback normalizer. Seven reviewers flagged it independently. The fix is one character.

Of the P3s, the most impactful cluster: dispose() drops accumulated data without flushing, the timer callback has no try/catch (a throw permanently kills the chain), and average()/percentile95() produce NaN/undefined on empty input. All are straightforward defensive fixes.

On the design side: the route normalization fallback passes name-bearing tail segments (like usernames after /members/) through unchanged, which partially contradicts the stated goal of stripping user/workspace names. And the interceptor registration order means telemetry records phantom errors for requests that are transparently retried by the cert-refresh or auth interceptors. Neither is urgent, but both are worth acknowledging.

One reviewer questioned whether the windowSeconds config knob earns its ~115 lines of schema, reader, watcher, update, and tests given nobody will change the default. Worth a human decision.

"A public surface with zero in-tree callers is a foot-gun, not dead code." (Kite, on the UUID fallback path)

🤖 This review was automatically generated with Coder Agents.

[EhabY]

/coder-agents-review

[coder-agents-review]

Strong response to R1. All 19 addressed findings verified: UUID regex eliminated entirely (class-of-bug fix), config surface removed, dispose flushes, try/finally protects the timer chain, empty-array guards return 0, naming and docs cleaned up. The normalization redesign (position-based rules, no UUID regex) is cleaner than what it replaced.

DEREM-20 (contested, interceptor ordering): panel closed 3/3 accept. The author's defense holds: each interceptor-level attempt is a real network event, cert-refresh frequency is bounded, and the added comment at coderApi.ts:520-522 documents the counting semantics.

Severity summary for new findings: 1 P2, 4 P3.

The P2 is a regression from the R1 refactor: removing the NOOP sentinel check means every CoderApi.create() now constructs a real HttpRequestsTelemetry with a running 60-second timer, including 5-6 throwaway call sites that never dispose. The fix is straightforward: lazy-start the timer on first record() instead of in the constructor.

"What happens when the extension runs for a week in a remote window?" (Hisoka, on the timer leak)

🤖 This review was automatically generated with Coder Agents.

[coder-agents-review]

P2 [DEREM-28] Constructor eagerly starts a 60-second repeating timer via this.#scheduleNextWindow(). Every CoderApi.create() call instantiates a real HttpRequestsTelemetry, including 5-6 throwaway call sites that never call dispose() (loginCoordinator.ts:194, loginCoordinator.ts:225, sessionManager.ts:291, authorizer.ts:67, container.ts:87, deploymentManager.ts:98). Each leaks a setTimeout chain that fires every 60s, iterates an empty bucket map, calls a NOOP log(), and reschedules forever. The timer closure prevents GC.

This is a regression from R1, where createHttpRequestsTelemetry checked telemetry === NOOP_TELEMETRY_REPORTER and returned a NOOP object with no timer. That sentinel was removed in the refactor.

prepareOAuthOperation runs on every OAuth token refresh (typically hourly). A day-long session accumulates ~24 leaked timers.

Fix (cheapest): defer #scheduleNextWindow() until the first #record() call. Clients that never record a request never start a timer:

#record(...): void {
  if (this.#disposed) { return; }
  if (!this.#timer) { this.#scheduleNextWindow(); }
  // ... rest of record
}

Remove the constructor call. (Hisoka P3, Kite P3)

🤖

[coder-agents-review]

P3 [DEREM-29] dispose() is not exception-safe. If #flush() throws at line 85:

1. #disposed stays false (line 86 not reached)
2. Timer is never cleared (lines 87-89 not reached)
3. The throw propagates into CoderApi.dispose(), skipping WebSocket and socket-set cleanup

The timer callback already has try/finally (DEREM-19 fix). Apply the same pattern here:

public dispose(): void {
  if (this.#disposed) { return; }
  try { this.#flush(); } finally {
    this.#disposed = true;
    if (this.#timer) {
      clearTimeout(this.#timer);
      this.#timer = null;
    }
  }
}

Current TelemetryService.log() uses #safeEmit (catch-all), so this requires a future reporter implementation that doesn't swallow. Defensive fix, same justification as DEREM-19. (Mafuuu P3, Meruem P3, Razor P3)

🤖

[coder-agents-review]

P3 [DEREM-30] Dispose-time flush emits window_seconds: 60 regardless of actual elapsed time since the last flush. When the extension deactivates mid-window, a consumer computing request rates from count / window_seconds will understate the rate for the final event.

This is the residual of DEREM-9 (which was closed because updateConfig was removed). The disposal case fires once at shutdown, so the practical impact is minor. Noting it because four reviewers flagged it independently and the fix (track window-start time, emit actual elapsed) would also benefit any future partial-window flush. (Chopper P3, Mafuuu Note, Kite Note, Meruem Note)

🤖

[coder-agents-review]

P3 [DEREM-31] TelemetryReporter interface has no doc comment explaining why it exists. This is the new abstraction boundary between CoderApi and TelemetryService. A developer opening this file sees three method signatures and a NOOP constant with no explanation of the decoupling contract.

Similarly, HttpRequestsTelemetry (httpRequestsTelemetry.ts:51) has no doc comment. It starts a repeating timer in its constructor (a side effect not obvious from the class name), recordError silently drops non-Axios errors, and dispose flushes pending data. These are preconditions and postconditions that belong on the class.

Suggested:

// reporter.ts
/** Decouples callers from the concrete TelemetryService so throwaway
 *  or telemetry-disabled clients can use NOOP_TELEMETRY_REPORTER
 *  without constructing a full service. */

// httpRequestsTelemetry.ts
/** Aggregates HTTP request counts and latencies into 60-second tumbling
 *  windows and emits "http.requests" events via the provided reporter.
 *  Starts a repeating flush timer on construction. Call dispose() to
 *  flush pending data and stop the timer. */

(Leorio)

🤖

[coder-agents-review]

P3 [DEREM-32] CoderApi.create doc comment says "Automatically sets up logging interceptors and certificate handling." This PR added a telemetry parameter and wires HttpRequestsTelemetry into the instance. The comment now lies by omission. Someone writing a second factory path or test helper might skip the telemetry parameter thinking it is cosmetic.

Add "and HTTP request telemetry" to the list. (Leorio P3, Gon Nit)

🤖