Skip to content

Commit 9f18ea1

Browse files
committed
Restructure telemetry plan: move to plans/, reorder sections
- Moved PLAN.md to plans/cli_mcp_telemetry_instrumentation_plan.md - Reordered so all phases (1-6) are contiguous under Implementation - Promoted CI detection to Phase 6 (was nested under Phase 5) - Moved File Change Summary, Risks, Key Source Files, Testing after phases - Added CI-specific test cases to Testing Strategy https://claude.ai/code/session_01Y2eJZgKG78nDyN6Uw2tWQx
1 parent c5ab581 commit 9f18ea1

1 file changed

Lines changed: 36 additions & 34 deletions

File tree

plans/cli_mcp_telemetry_instrumentation_plan.md

Lines changed: 36 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -302,39 +302,13 @@ Not in scope for this CLI PR, but documents the expected server-side changes:
302302
4. Use `command_path` as the event name / action property
303303
5. Use `mcp_client` to break down MCP usage by AI agent
304304

305-
## File Change Summary
306-
307-
| File | Change | Phase | Complexity |
308-
|------|--------|-------|-----------|
309-
| `pkg/hookdeck/telemetry.go` | Add `Source`, `Environment`, `InvocationID` fields; `NewInvocationID()` generator; `DetectEnvironment()` for CI; update `telemetryOptedOut()` to accept config flag | 1, 5 | Small |
310-
| `pkg/hookdeck/telemetry_test.go` | Update tests for new fields, opt-out with config flag, environment detection | 1, 5 | Small |
311-
| `pkg/hookdeck/client.go` | Add `Telemetry` field + `TelemetryDisabled` field; `WithTelemetry()` clone method; update `PerformRequest` to use per-request telemetry override and config-based opt-out | 2, 5 | Small |
312-
| `pkg/cmd/root.go` | Add `PersistentPreRun` with `initTelemetry()` helper | 1 | Small |
313-
| `pkg/cmd/connection.go` | Call `initTelemetry()` in existing `PersistentPreRun` | 1 | Trivial |
314-
| `pkg/gateway/mcp/server.go` | Capture MCP client info, store on Server struct | 2 | Small |
315-
| `pkg/gateway/mcp/tools.go` | Add telemetry wrapping in tool dispatch (central `WithTelemetry` per tool call) | 2 | Medium |
316-
| `pkg/hookdeck/sdkclient.go` | Add `TelemetryDisabled` field, thread through opt-out check | 5 | Small |
317-
| `pkg/config/config.go` | Add `TelemetryDisabled` field, read from viper in `constructConfig()` | 5 | Small |
318-
319-
## Risks and Edge Cases
320-
321-
1. **Cobra PersistentPreRun chaining**: Cobra doesn't chain `PersistentPreRun` from parent to child. Any command with its own `PersistentPreRun` must explicitly call `initTelemetry()`. Currently only `connection` has one. Must audit for future additions.
322-
323-
2. **MCP session info timing**: `ServerSession.InitializeParams()` is available after handshake. Tool calls only happen after handshake, so this is safe. But if the server ever supports multiple sessions, we'd need per-session client info.
324-
325-
3. **Invocation ID uniqueness**: 8 random bytes = 16 hex chars. Collision probability is negligible for our use case (not a security-critical ID).
326-
327-
4. **SDK client static headers**: The `listen` command's SDK client gets one invocation ID baked in. If `listen` ran for days and we wanted to track "sessions," we'd need a different mechanism. Fine for now — we're tracking command invocations, not long-lived sessions.
328-
329-
5. **Backward compatibility**: The server must handle both old (empty) and new telemetry payloads. Since it's JSON with new fields, old servers will simply ignore unknown keys. New servers should treat missing `source` as `"cli"` for backward compat.
330-
331-
## Phase 5: Telemetry Opt-Out via Config
305+
### Phase 5: Telemetry Opt-Out via Config
332306

333-
### Problem
307+
#### Problem
334308

335309
Telemetry opt-out is currently only possible via the `HOOKDECK_CLI_TELEMETRY_OPTOUT` environment variable. This requires users to set it in their shell profile or per-invocation, which is fragile and inconvenient. Users who want telemetry permanently disabled (corporate policy, personal preference) need a persistent config-based option.
336310

337-
### Design
311+
#### Design
338312

339313
Add a **top-level** `telemetry` setting to `config.toml`. No per-profile override initially — telemetry is a user-level concern, not a project-level one. We can always add profile granularity later if needed.
340314

@@ -353,7 +327,7 @@ profile = "default"
353327
api_key = "..."
354328
```
355329

356-
### Implementation
330+
#### Implementation
357331

358332
**File: `pkg/config/config.go`**
359333

@@ -398,9 +372,9 @@ hookdeck config set telemetry_disabled false
398372

399373
This depends on whether a generic `config set` command exists or is planned. If not, a simple `hookdeck telemetry off/on` subcommand is the alternative.
400374

401-
### CI/CD Environment Detection
375+
### Phase 6: CI/CD Environment Detection
402376

403-
**Context:** Some CLI tools (e.g., `npx @anthropic-ai/claude-code`) detect CI environments and disable telemetry by default. The question is whether hookdeck-cli should do the same.
377+
**Context:** Some CLI tools (e.g., `npx @anthropic-ai/claude-code`) detect CI environments and disable telemetry by default. The question is whether hookdeck-cli should do the same. Decision: keep telemetry enabled in CI, but tag it with `environment: "ci"` so it can be filtered server-side.
404378

405379
**How CI detection works:** Check for well-known environment variables:
406380
- `CI=true` (GitHub Actions, GitLab CI, CircleCI, Travis, most CI systems)
@@ -453,6 +427,32 @@ func DetectEnvironment() string {
453427

454428
This integrates cleanly with Phase 1's telemetry struct as a new `environment` field. Server-side, PostHog dashboards get two clean dimensions to slice by.
455429

430+
## File Change Summary
431+
432+
| File | Change | Phase | Complexity |
433+
|------|--------|-------|-----------|
434+
| `pkg/hookdeck/telemetry.go` | Add `Source`, `Environment`, `InvocationID` fields; `NewInvocationID()` generator; `DetectEnvironment()` for CI; update `telemetryOptedOut()` to accept config flag | 1, 5, 6 | Small |
435+
| `pkg/hookdeck/telemetry_test.go` | Update tests for new fields, opt-out with config flag, environment detection | 1, 5, 6 | Small |
436+
| `pkg/hookdeck/client.go` | Add `Telemetry` field + `TelemetryDisabled` field; `WithTelemetry()` clone method; update `PerformRequest` to use per-request telemetry override and config-based opt-out | 2, 5 | Small |
437+
| `pkg/cmd/root.go` | Add `PersistentPreRun` with `initTelemetry()` helper | 1 | Small |
438+
| `pkg/cmd/connection.go` | Call `initTelemetry()` in existing `PersistentPreRun` | 1 | Trivial |
439+
| `pkg/gateway/mcp/server.go` | Capture MCP client info, store on Server struct | 2 | Small |
440+
| `pkg/gateway/mcp/tools.go` | Add telemetry wrapping in tool dispatch (central `WithTelemetry` per tool call) | 2 | Medium |
441+
| `pkg/hookdeck/sdkclient.go` | Add `TelemetryDisabled` field, thread through opt-out check | 5 | Small |
442+
| `pkg/config/config.go` | Add `TelemetryDisabled` field, read from viper in `constructConfig()` | 5 | Small |
443+
444+
## Risks and Edge Cases
445+
446+
1. **Cobra PersistentPreRun chaining**: Cobra doesn't chain `PersistentPreRun` from parent to child. Any command with its own `PersistentPreRun` must explicitly call `initTelemetry()`. Currently only `connection` has one. Must audit for future additions.
447+
448+
2. **MCP session info timing**: `ServerSession.InitializeParams()` is available after handshake. Tool calls only happen after handshake, so this is safe. But if the server ever supports multiple sessions, we'd need per-session client info.
449+
450+
3. **Invocation ID uniqueness**: 8 random bytes = 16 hex chars. Collision probability is negligible for our use case (not a security-critical ID).
451+
452+
4. **SDK client static headers**: The `listen` command's SDK client gets one invocation ID baked in. If `listen` ran for days and we wanted to track "sessions," we'd need a different mechanism. Fine for now — we're tracking command invocations, not long-lived sessions.
453+
454+
5. **Backward compatibility**: The server must handle both old (empty) and new telemetry payloads. Since it's JSON with new fields, old servers will simply ignore unknown keys. New servers should treat missing `source` as `"cli"` for backward compat.
455+
456456
## Key Source Files
457457

458458
These are the files an implementer needs to read before starting:
@@ -473,5 +473,7 @@ These are the files an implementer needs to read before starting:
473473

474474
1. **Unit tests for telemetry struct**: Verify JSON serialization includes new fields
475475
2. **Unit tests for `WithTelemetry`**: Verify cloned client uses override telemetry
476-
3. **Integration test**: Wire up an MCP test with a mock API server, verify the `Hookdeck-CLI-Telemetry` header on requests contains correct `source`, `command_path`, and `invocation_id`
477-
4. **Manual test**: Run `hookdeck listen` against a local proxy, inspect the telemetry header on outgoing requests
476+
3. **Unit tests for `DetectEnvironment`**: Verify CI env var detection
477+
4. **Unit tests for config-based opt-out**: Verify `telemetryOptedOut()` with config flag
478+
5. **Integration test**: Wire up an MCP test with a mock API server, verify the `Hookdeck-CLI-Telemetry` header on requests contains correct `source`, `environment`, `command_path`, and `invocation_id`
479+
6. **Manual test**: Run `hookdeck listen` against a local proxy, inspect the telemetry header on outgoing requests

0 commit comments

Comments
 (0)