Skip to content

Commit 321ff89

Browse files
authored
feat(telemetry): honest wizard connect metric + v7 churn instrumentation (Spec 080) (#813)
* feat(telemetry): honest wizard connect step via completed_external (Spec 080 US1) Widens the onboarding connect-step enum with completed_external so connections made outside the wizard (CLI, ConnectModal, manual config) stop being recorded as skips. The decision is made server-side in the /api/v1/onboarding/mark handler, where both evidence sources live: a dismissal-time "skipped" on a previously untouched connect step is upgraded to completed_external when at least one supported client is connected (connect.Service) OR an MCP client has ever handshaked (activation.first_mcp_client_ever). Backend-only, so every dismissal path benefits without frontend changes. ## Changes - storage: StepStatus* constants + widened ConnectStepStatus enum doc (FR-001) - httpapi: nextConnectStepStatus decision fn — evidence upgrade (FR-002), skipped fallback that never blocks dismissal, with a recover guard on evidence gathering (FR-002a), completed / completed_external never regress (FR-004), historical skipped never rewritten (FR-003); request validation accepts the widened enum for the connect step only - telemetry: wizard_connect_step enum comment widened (value passes through the existing snapshot pipeline unchanged; schema bump is separate) - oas: regenerated for the widened request-field description ## Testing - Pure decision-matrix table test (11 cases) + endpoint-level tests: handshake-evidence upgrade, connected-client upgrade via a real connect.Service on a fake home, no-evidence skip, evidence-panic fallback to skipped with 200, non-regression for both terminal statuses, historical-skip immutability, enum validation - Heartbeat surfacing test: completed_external serializes as a distinct wizard_connect_step value and passes the PII scanner * feat(telemetry): funnel observability — wizard_shown, web_ui_opened, install age, active days (Spec 080 US2) Adds the four heartbeat fields the day-2-cliff analysis showed are missing (FR-005..FR-009), all additive, omitempty, and BBolt-backed with the same nil-safety as Activation: - wizard_shown: derived from OnboardingState.FirstShownAt, making "shown but ignored" (wizard_shown=true, wizard_engaged absent) observable for the first time. - web_ui_opened: persistent lifetime counter in a new telemetry_funnel bucket, incremented only when the embedded Web UI entrypoint (index document) is served — extensionless SPA routes count, asset/API requests never do (missing-asset index fallbacks included). Fully independent of surface_requests.webui, which is unchanged. - days_since_install: write-once first-install UTC day stamp (independent of anonymous_id); surfaced as a non-negative whole-day count via *int so day 0 is transmitted while "store not wired" is omitted. Clamped at 0 on backwards clock; no timestamp on the wire. - active_days_30d: compact persisted set of distinct active UTC day ordinals, pruned to the trailing 30-day window on write and aged at read time; tolerates out-of-order days and excludes future-dated members. Only the cardinality (1..30) is transmitted. Activity days are recorded at process start (runtime wiring, so short sessions and first runs persist a stamp before any heartbeat) and at each heartbeat build. SchemaVersion intentionally stays at 6 — the v7 bump and version-history entry belong to the US4 slice. ## Testing - funnel_test.go: counter persistence, install-day boundary math + clock-skew clamp, same-day dedup, 30d window aging (write- and read-time), out-of-order days, 30 cap, empty-bucket zero state - payload_funnel_test.go: shown-vs-engaged independence, store-not-wired omission (v6 shape compat), populated-field surfacing incl. days_since_install:0 on the wire, no per-day structure leakage, ScanForPII pass, RecordWebUIOpen nil-safety - web_test.go: index/SPA-route serves fire the counter callback, asset requests (present or missing) never do, nil-callback safety * feat(telemetry): pre-churn snapshot — previous_shutdown marker + last_error_code (Spec 080 US3) Makes the final heartbeat before an install goes silent carry a cause-of-death record (FR-010..FR-013): - New BBolt bucket telemetry_prechurn (internal/telemetry/prechurn.go) with an armed/resolved shutdown marker and a single most-recent MCPX_* code. ArmShutdownMarker derives the previous instance's outcome (clean|crash|unknown) and re-arms in one transaction. - Runtime arms the marker in runtime.New immediately after storage opens (crash loops visible) and resolves it to clean as the first act of Runtime.Close — before the long container-cleanup phase and its early returns, so a handled SIGTERM never reads as a crash. A second instance never touches the marker: the BBolt file lock fails storage.NewManager first (pinned by test). - previous_shutdown is derived once at startup and stable across all heartbeats of the instance (FR-011); first run is unknown/absent, never crash (FR-013). last_error_code rides the existing supervisor error-code notifier (same stream as error_code_counts_24h), is strictly validated to ^MCPX_[A-Z0-9_]+$ on write AND read, and survives restart so the post-crash heartbeat carries the pre-crash code (FR-012). - Heartbeat fields previous_shutdown + last_error_code, both omitempty; SchemaVersion intentionally stays 6 (US4's slice). ## Testing - Clean/crash/first-run detection across simulated restart sequences, full unknown→clean→crash→clean lifecycle, locked-out second instance cannot clobber the marker - last_error_code enum-only (free text/paths/malformed dropped, corrupt on-disk value never surfaced), most-recent-wins, restart persistence - Payload: within-instance stability, omitempty shape on first run and when store not wired, anonymity scan on populated payload - Runtime end-to-end lifecycle test through real New/SetTelemetry/Close * feat(telemetry): schema v7 contract — version bump, scanner enforcement, docs (Spec 080 US4) Bumps the heartbeat SchemaVersion 6 -> 7 with a version-history entry covering every v7 addition (completed_external enum widening, wizard_shown, web_ui_opened, days_since_install, active_days_30d, previous_shutdown, last_error_code), following the v3-v6 additive pattern. Extends ScanForPII with a structural rule for the v7 fields: booleans, non-negative bare-integer counters (quoted numbers rejected), and documented fixed enums only — last_error_code is gated to stable MCPX_* codes so free text or paths can never ride it even if a producer check regresses. Documents every v7 field (type, enums, when set, privacy rationale), the widened wizard_connect_step enum, and unknown-enum guidance for consumers in docs/features/telemetry.md. ## Testing - Fully-populated v7 payload passes the anonymity scanner with zero violations; all-new-fields-zero payload serializes shape-compatible with v6 except schema_version (omitempty discipline) - Table-driven scanner rejection cases per field + valid-enum pass cases - /api/v1/telemetry/payload (the `mcpproxy telemetry show-payload` source) renders every v7 field when populated (FR-019) - Existing suite retargeted to v7 (payload_v2, privacy, schema tripwire) * fix(telemetry): address cross-model review round 1 (Spec 080) Finding 1 (MAJOR, fixed): last_error_code accepted any MCPX_-shaped string instead of the fixed diagnostic catalog required by FR-012. isValidMCPXCode now also requires diagnostics.Has() (internal/diagnostics has zero internal deps, so telemetry can import it cycle-free); the gate applies on write (RecordLastErrorCode), on read (LastErrorCode), and in the anonymity scanner's v7 rule. Tests now use real catalog codes (MCPX_HTTP_CONN_REFUSED, MCPX_DOCKER_IMAGE_PULL_FAILED, MCPX_OAUTH_REFRESH_EXPIRED, MCPX_CONFIG_PARSE_ERROR) and assert shape-valid-but-uncataloged codes are rejected at write, read, and scan. Finding 2 (MAJOR, fixed): clients could POST connect_step_status="completed_external" and persist it with zero evidence, violating the "never guess completed_external without positive evidence" edge case. The request enum is back to ""/completed/skipped (validConnectStepStatus removed); completed_external remains a valid STORED/read value that only the server-side evidence check (nextConnectStepStatus) can produce. Validation test flipped to expect 400; swagger regenerated for the field-comment change. Finding 3 (MAJOR, fixed): ResolveCleanShutdown fired as the FIRST act of runtime.Close(), so a hang/SIGKILL/panic DURING shutdown still read as previous_shutdown="clean" — FR-010 wants an unfinished shutdown to read as crash. The Docker container-cleanup verification loop (whose early `return nil` branches forced the early resolve and also leaked cacheManager/indexManager/storageManager/configSvc) is extracted into verifyContainerCleanup; its exits now return to Close, which resolves the marker at the END of the graceful path, just before storage closes. Ordering and timeouts preserved; new lifecycle test drives the former early-return branch and asserts the marker resolves clean AND storage actually closes. Finding 4 (MINOR, fixed): last_error_code rode the async `go notifier` hand-off, so a crash right after classification could lose the final pre-crash code — the one case the field exists for. The supervisor now captures the classified code inside the stateView update and delivers it synchronously after the stateview lock is released (notifyErrorCode); the runtime callback writes the pre-churn code synchronously (sub-ms BBolt Update, no supervisor re-entry, no lock cycle — callers may hold stateMu but the callback never re-enters the supervisor) and keeps the loss-tolerant 24h counter on its pre-existing async posture. New race-safe test proves the notifier fires before updateStateView returns. * fix(telemetry): tighten shutdown-marker arm/resolve window (Spec 080, review round 2) * fix(telemetry): resolve shutdown marker after async-op drain (Spec 080, review round 3) * fix(telemetry): drain remaining BBolt writers before shutdown-marker resolve (Spec 080, review round 4) Two writers could still hit BBolt after Close() resolved the clean-shutdown marker (FR-010: the marker resolve must be the final DB write of a graceful shutdown) or even after the DB handle closed: 1. Error-code notifier (runtime.go SetTelemetry wiring): review round 1 made the notifier synchronous for FR-012 but kept the 24h diagnostics counter write in an untracked goroutine — no better tracked than the pre-branch `go notifier(code)` in the supervisor, and now able to race the marker resolve. The counter write is now synchronous too: it completes inside the supervisor's call stack, so Supervisor.Stop() (which joins its goroutines before Close touches storage) is a hard barrier. Safe: sub-ms BBolt Update, the callback never re-enters the supervisor (documented at notifyErrorCode), and RecordLastErrorCode already writes synchronously under identical locking. SetErrorCodeNotifier's contract now forbids callback goroutines that outlive the call. The untracked-goroutine exposure itself pre-dates the branch; the marker invariant it violates is new here, and the specific goroutine was introduced in round 1 (1ec8109). 2. ActivityService (activity records, retention pruning, usage-snapshot flushes, async sensitive-data detection — all BBolt writes): Close() only context-cancelled it and never awaited it, so the flush-on-shutdown (persistUsage) and any in-flight worker could land after the marker resolve or be lost at DB close. Pre-existing on main (Stop() existed but was never called from Close); the Spec 080 marker invariant makes it observable, so it is fixed on this branch. ActivityService.Stop() is now idempotent, returns immediately if Start never ran, and waits for the event loop's final flush plus a WaitGroup over the retention loop, the usage-flush loop, and per-event detection goroutines. Close() calls it after appCancel (so the final flush is captured) and BEFORE StopAsync/ResolveCleanShutdown/storage Close. Tests: TestRuntimeCloseWaitsForActivityWritersBeforeMarkerResolve proves the shutdown flush lands before the DB closes AND the marker still resolves to clean; TestActivityServiceStopSafeWhenNeverStartedAndIdempotent covers the never-started and repeated-Stop paths (every existing prechurn test now exercises the never-started branch through Close). * fix(telemetry): close startup-vs-shutdown races in marker write barriers (Spec 080, review round 5) * fix(telemetry): join heartbeat loop before shutdown-marker resolve (Spec 080, review round 6) * chore(ci): align codeql-action init/analyze with autobuild at v4.36.3 #811 bumped only autobuild to 4.36.3, leaving init/analyze at 4.36.2; CodeQL rejects the cross-version configuration ('Loaded a configuration file for version 4.36.2, but running version 4.36.3'), failing Analyze (go) on every PR.
1 parent e5aa21b commit 321ff89

34 files changed

Lines changed: 3855 additions & 138 deletions

docs/features/telemetry.md

Lines changed: 46 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ MCPProxy collects anonymous usage statistics to help improve the product. This p
44

55
## What is collected
66

7-
MCPProxy sends a **daily heartbeat** containing only aggregate, non-identifying information. The current schema is **version 6** (`schema_version: 6` in the JSON payload); the schema is forward-compatible so older consumers simply ignore fields they don't recognize.
7+
MCPProxy sends a **daily heartbeat** containing only aggregate, non-identifying information. The current schema is **version 7** (`schema_version: 7` in the JSON payload); the schema is forward-compatible so older consumers simply ignore fields they don't recognize.
88

99
| Field | Example | Purpose |
1010
|-------|---------|---------|
@@ -25,6 +25,13 @@ MCPProxy sends a **daily heartbeat** containing only aggregate, non-identifying
2525
| `server_docker_isolated_count` | `2` | How many configured servers the runtime actually wraps in Docker isolation (schema v3) |
2626
| `feature_flags.docker_isolation_enabled` | `true` | Whether global Docker isolation is turned on (schema v5). Lets us tell "isolation on, 0 matching servers" apart from "isolation off" |
2727
| `feature_flags.docker_cli_source` | `bundled` | How the `docker` CLI was located — fixed enum `path` / `bundled` / `login_shell` / `absent` (schema v5). The direct signal for "Docker installed but not on the spawn PATH" (issue #696). **Never** the path string itself |
28+
| `wizard_shown` | `true` | Whether the onboarding wizard ever rendered for this install (schema v7). Makes "shown but ignored" measurable |
29+
| `wizard_connect_step` | `completed_external` | Onboarding connect-step outcome — fixed enum, widened in v7 (see below) |
30+
| `web_ui_opened` | `12` | Lifetime count of embedded Web UI entrypoint serves (schema v7) |
31+
| `days_since_install` | `14` | Whole-day age of the install (schema v7). A day count, never a timestamp |
32+
| `active_days_30d` | `5` | Distinct UTC days with process activity in the trailing 30 days (schema v7). Only the count — never the per-day breakdown |
33+
| `previous_shutdown` | `clean` | How the previous process instance ended — fixed enum `clean` / `crash`, absent on first run (schema v7) |
34+
| `last_error_code` | `MCPX_DOCKER_CLI_NOT_FOUND` | Most recent stable `MCPX_*` diagnostic code (schema v7). Enum code only, never error text |
2835

2936
The `server_protocol_counts` map uses a **fixed enum of keys** (`stdio`, `http`, `sse`, `streamable_http`, `auto`) — server names and URLs are never included. Unknown or misconfigured protocol values are bucketed into `auto`.
3037

@@ -65,6 +72,44 @@ The `anonymous_id` above is a UUID persisted in the config file. In **ephemeral
6572

6673
`machine_id` respects the **same opt-out** as every other field: when telemetry is disabled (see below), the entire heartbeat — including `machine_id` — is never sent.
6774

75+
## Schema v7 — activation funnel & churn fields (Spec 080)
76+
77+
Schema v7 adds seven purely **additive** signals so we can measure whether installs come back after day one — and, when they don't, whether the last session ended cleanly or crashed. Every field keeps the established privacy posture: **booleans, non-negative integers, or documented fixed enums only** — no timestamps, no per-server identity, no free text. The anonymity scanner (`internal/telemetry/anonymity.go`) enforces these shapes on the serialized payload before every send, and all fields use `omitempty`, so a payload with none of them set is shape-identical to a v6 payload except for `schema_version`.
78+
79+
### Widened enum: `wizard_connect_step`
80+
81+
The onboarding connect-step status (a v4 field) gains a fourth value in v7:
82+
83+
| Value | Meaning |
84+
|-------|---------|
85+
| *(absent)* | Step never shown to this install |
86+
| `completed` | User completed the connect step inside the wizard |
87+
| `completed_external` | **New in v7.** User dismissed the wizard with the connect step untouched, but the install was already connected (via `mcpproxy connect`, the ConnectModal, or manual config). Previously miscounted as `skipped` |
88+
| `skipped` | User dismissed the wizard with the connect step untouched and **no** connection evidence existed |
89+
90+
**Guidance for consumers**: this is a string enum that may widen again. Code that switches on `completed` / `skipped` must treat **unknown values as "other/engaged"**, never as a skip or an error. Statuses recorded before v7 are never rewritten — segment analyses by `schema_version`.
91+
92+
### New fields
93+
94+
| Field | Type | When it is set | Privacy rationale |
95+
|-------|------|----------------|-------------------|
96+
| `wizard_shown` | boolean | `true` once the onboarding wizard has rendered at least once for this install; omitted otherwise. Together with `wizard_engaged` it distinguishes "shown but ignored" from "never shown" | A single boolean about our own UI; carries no user data |
97+
| `web_ui_opened` | non-negative integer | Lifetime count of serves of the embedded Web UI **entrypoint** (index document). Asset and API requests never increment it; it is independent of `surface_requests.webui`. Coarse by design — health checkers fetching `/` count too | A counter of our own page serves; no URLs, sessions, or timing |
98+
| `days_since_install` | non-negative integer | Whole-day UTC age of the install, from a persisted first-install day stamp (independent of `anonymous_id`). `0` on install day; clamped at 0 on clock skew. Omitted when the local store isn't available (short-lived CLI commands) | Only a day **count** is transmitted — the install timestamp itself never leaves the machine |
99+
| `active_days_30d` | non-negative integer (1–30) | Number of distinct UTC days with process activity in the trailing 30-day window. Old days age out | The per-day set is stored locally and **never transmitted** — counters, not timelines |
100+
| `previous_shutdown` | fixed enum `clean` \| `crash` | How the **previous** process instance ended: `clean` = the graceful-shutdown path ran; `crash` = it didn't (SIGKILL, panic, power loss). Absent on a first-ever run — a fresh install is never reported as a crash. Stable across all heartbeats of the current instance | One enum value about our own process lifecycle; no stack traces, no session timing |
101+
| `last_error_code` | fixed enum (`MCPX_*`) | The most recently observed stable diagnostic code (same fixed set as `diagnostics.error_code_counts_24h`), persisted across restarts so a post-crash heartbeat carries the pre-crash code. Absent when no error was ever recorded | Only the enum code is stored and sent — **never** error messages, server names, paths, or stack traces. The scanner rejects any value outside the fixed diagnostics catalog |
102+
103+
Why these exist: telemetry showed most installs connect successfully but never return after day one. `days_since_install` + `active_days_30d` make retention computable from a single heartbeat (no cross-heartbeat identity joins), and `previous_shutdown` + `last_error_code` let the final heartbeat before an install goes silent distinguish "crashed and never came back" from "exited cleanly and never returned".
104+
105+
All v7 fields ride the **same opt-out** as the rest of the heartbeat: when telemetry is disabled, nothing is transmitted. Local counters may still persist on disk (so re-enabling doesn't fabricate a fresh-install picture), but they never leave the machine.
106+
107+
You can inspect exactly what would be sent — including every v7 field — with:
108+
109+
```bash
110+
mcpproxy telemetry show-payload
111+
```
112+
68113
## One-time opt-out signal
69114

70115
When telemetry transitions from **enabled to disabled** (via the CLI, the config

internal/httpapi/onboarding.go

Lines changed: 76 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,13 @@ type OnboardingMarkRequest struct {
7676
Engaged bool `json:"engaged"`
7777

7878
// ConnectStepStatus is one of: "", "completed", "skipped". Empty
79-
// preserves the existing value.
79+
// preserves the existing value. The stored enum is wider (Spec 080
80+
// FR-001): a "skipped" request for a previously untouched connect step
81+
// is upgraded server-side to "completed_external" when the install
82+
// shows positive evidence of an external connection (Spec 080 FR-002).
83+
// "completed_external" is NOT accepted from clients — it must never be
84+
// persisted without that server-verified evidence (edge case: "never
85+
// guess completed_external without positive evidence").
8086
ConnectStepStatus string `json:"connect_step_status,omitempty"`
8187

8288
// ServerStepStatus is one of: "", "completed", "skipped". Empty
@@ -151,7 +157,8 @@ func (s *Server) handleMarkOnboardingState(w http.ResponseWriter, r *http.Reques
151157
state.FirstShownAt = &t
152158
}
153159
if req.ConnectStepStatus != "" {
154-
state.ConnectStepStatus = req.ConnectStepStatus
160+
state.ConnectStepStatus = nextConnectStepStatus(
161+
state.ConnectStepStatus, req.ConnectStepStatus, s.externalConnectionEvidence)
155162
}
156163
if req.ServerStepStatus != "" {
157164
state.ServerStepStatus = req.ServerStepStatus
@@ -225,7 +232,72 @@ func (s *Server) computeOnboardingState() (*OnboardingStateResponse, error) {
225232
return resp, nil
226233
}
227234

228-
// validStepStatus returns true if v is an allowed step-status value.
235+
// validStepStatus returns true if v is an allowed step-status REQUEST value.
236+
// Note the request enum is deliberately narrower than the stored connect-step
237+
// enum (Spec 080 FR-001): "completed_external" is server-derived only (see
238+
// nextConnectStepStatus) and is rejected when a client sends it directly —
239+
// no shipped client produces it, and accepting it would let a caller persist
240+
// external-connection evidence that was never verified.
229241
func validStepStatus(v string) bool {
230-
return v == "" || v == "completed" || v == "skipped"
242+
return v == "" || v == storage.StepStatusCompleted || v == storage.StepStatusSkipped
243+
}
244+
245+
// nextConnectStepStatus decides what to persist for the wizard's connect
246+
// step given its current value and the requested update (Spec 080 US1).
247+
// requested has passed validStepStatus, so it is "", "completed", or
248+
// "skipped" — "completed_external" can only ever be produced HERE, from the
249+
// server-side evidence check, never accepted from a client.
250+
//
251+
// - "completed" and "completed_external" never regress to "skipped" once
252+
// recorded, even if clients are later disconnected (FR-004). In-wizard
253+
// completion may still upgrade "completed_external" to "completed".
254+
// - A "skipped" request against a previously untouched step ("" current)
255+
// is the wizard-dismissal case: if the install shows positive evidence
256+
// of an external connection, record "completed_external" instead
257+
// (FR-002). Without evidence, fall back to "skipped" (FR-002a).
258+
// - An already-persisted "skipped" is never rewritten retroactively
259+
// (FR-003): the evidence check only applies to the untouched step.
260+
//
261+
// hasExternalEvidence is evaluated lazily, only for the dismissal case.
262+
func nextConnectStepStatus(current, requested string, hasExternalEvidence func() bool) string {
263+
if requested == "" {
264+
return current
265+
}
266+
switch current {
267+
case storage.StepStatusCompleted:
268+
// Terminal: strongest status, never changes (FR-004).
269+
return storage.StepStatusCompleted
270+
case storage.StepStatusCompletedExternal:
271+
// Never regresses; in-wizard completion is an allowed upgrade.
272+
if requested == storage.StepStatusCompleted {
273+
return storage.StepStatusCompleted
274+
}
275+
return storage.StepStatusCompletedExternal
276+
}
277+
if requested == storage.StepStatusSkipped && current == "" && hasExternalEvidence() {
278+
return storage.StepStatusCompletedExternal
279+
}
280+
return requested
281+
}
282+
283+
// externalConnectionEvidence reports whether this install shows positive
284+
// evidence of an MCP client connection made outside the wizard: at least one
285+
// supported client currently connected, or an MCP client has ever completed
286+
// an initialize handshake (Spec 080 FR-002).
287+
//
288+
// FR-002a: wizard dismissal must never be blocked or delayed by this check —
289+
// any failure while gathering evidence downgrades to "no evidence" so the
290+
// caller records "skipped" exactly as before Spec 080.
291+
func (s *Server) externalConnectionEvidence() (evidence bool) {
292+
defer func() {
293+
if r := recover(); r != nil {
294+
s.logger.Warnf("onboarding: external-connection evidence check failed, falling back to skipped: %v", r)
295+
evidence = false
296+
}
297+
}()
298+
if svc := s.getConnectService(); svc != nil && svc.GetConnectedCount() > 0 {
299+
return true
300+
}
301+
firstEver, _ := s.controller.GetActivationFirstMCPClient()
302+
return firstEver
231303
}

0 commit comments

Comments
 (0)