feat(telemetry): honest wizard connect metric + v7 churn instrumentation (Spec 080)#813
Merged
Conversation
…pec 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
…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
…_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
…nt, 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)
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.
…0, review round 3)
…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).
…ers (Spec 080, review round 5)
…pec 080, review round 6)
#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.
Deploying mcpproxy-docs with
|
| Latest commit: |
1cb5fe4
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://78f5f3e0.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://080-telemetry-v7-churn.mcpproxy-docs.pages.dev |
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 28840539581 --repo smart-mcp-proxy/mcpproxy-go
|
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements specs/080-telemetry-v7-churn: makes the activation funnel honest, observable, self-contained, and churn-diagnostic. Heartbeat schema bumped 6 → 7 (additive; every pre-v7 field unchanged).
Changes
completed_externalstatus, decided server-side inhandleMarkOnboardingStateat dismissal time (connected_client_count > 0ORfirst_mcp_client_ever), so connections made via CLI / ConnectModal / manual config stop being counted as skips. Evidence check is recover-guarded and never blocks dismissal (FR-002a);completed/completed_externalnever regress (FR-004). Client-suppliedcompleted_externalis rejected — the server-side check is the only producer. Zero frontend changes.wizard_shown(fromFirstShownAt),web_ui_opened(lifetime BBolt counter on embedded-UI index serves only;surface_requests.webuiuntouched),days_since_install(persisted first-install day stamp, clamps at 0),active_days_30d(compact per-day set on disk, only the count on the wire).previous_shutdown(clean|crash|unknown) via an armed/resolved BBolt marker — armed as the first DB op at startup, resolved as the last DB write of graceful shutdown;last_error_code= most recentMCPX_*code validated against the diagnostics catalog on write, read, and in the anonymity scanner.scanV7Fields) structurally enforcing bool/non-negative-int/fixed-enum shape,docs/features/telemetry.md(all v7 fields, widened enum, unknown-enum consumer guidance), show-payload rendering pinned by test.Cross-model review (Codex, 6 fix rounds — all findings verified genuine before fixing)
r1: diagnostics-catalog validation; reject client
completed_external;Close()restructure (also fixes pre-existing early-return branches leaking cache/index/storage closes); synchronous error-code notifier.r2: marker armed first DB op after
NewManager; resolve moved to end of graceful path.r3:
storage.Manager.StopAsync()split — async op queue drains before the marker resolves.r4: diagnostics counter write synchronous;
ActivityService.Stop()idempotent + awaited inClose()(pre-existing race made observable by the marker invariant).r5: supervisor initial-reconcile goroutine joined via
s.wg(timer-vs-ctx select); ActivityService terminal stopped state (Stop-before-Start ⇒ late Start no-op).r6:
telemetry.Service.Stop()barrier — heartbeat loop joined before marker resolve (v7'sRecordActivitywrites BBolt from the heartbeat path).Testing
completed_external; day-window aging, clock-clamp, index-serve counting; clean/crash/first-run shutdown detection across restart sequences; catalog-onlylast_error_code; SC-007 fully-populated-payload anonymity pass; SC-008 v6 shape-compat.go buildboth editions;go test -racegreen on telemetry/storage/httpapi/runtime/web; fullinternal/servere2e suite (-race, ~485s) green on the final state; golangci-lint v2 strict config 0 issues; swagger-verify in sync.🤖 Generated with Claude Code
https://claude.ai/code/session_01AgygWSPVq7bxN7a2PVv8if