Deep audit + correctness overhaul: fix broken/FP alerts, add correctness harness, harden pipeline & security - #2
Open
RO-29 wants to merge 12 commits into
Open
Deep audit + correctness overhaul: fix broken/FP alerts, add correctness harness, harden pipeline & security#2RO-29 wants to merge 12 commits into
RO-29 wants to merge 12 commits into
Conversation
- PHASE0-AUDIT.md: architecture map, theory summary, live-vs-repo drift, correctness findings, 68-alert verdict tally, bloat/duplication list, proposed direction - Companion reports: collectors, alerting/API, frontend, docs/deploy, live-site drift (full file:line evidence)
…arness Correctness harness (internal/testsupport/fakech + collector tests): - fakech: in-process fake ClickHouse HTTP server exercising the real client/SQL/decode path; drives collectors through known states. - scenario_test.go: asserts exact alerts+values for background_pool, async_inserts, system CPU, inserts RejectedInserts delta, parts_age, schema_drift per-instance isolation. - sql_contract_test.go: fails if any collector/web SQL references a ClickHouse identifier that does not exist (the class of bug below). Fixes (each check was silently dead, fired forever, or used wrong data): - async_inserts: status enum 'ExceptionWhileFlushing'/'Flushed' -> 'FlushError'/'Ok' (real asynchronous_insert_log values). - background_pool: BackgroundMergesMutationsPoolTask -> ...AndMutations...; drop removed ProcessingPool, add CommonPool. - keeper: system.zookeeper_connection has no outstanding_requests/ avg_latency/max_latency; replace with is_expired session check. - restart: system.crash_log.trace_str -> trace_full. - system + web/history CPU: OSUserTimeCPU/... -> *Normalized (+gate fix); OSS/history CPU path was always dead. - cache_health + web/compare: MarkCacheBytes/UncompressedCacheBytes read from asynchronous_metrics, not system.metrics. - mvs bloat: join implicit inner table by name .inner_id.<uuid>, not uuid. - analyzer: drop 3 cross-alerts reading nonexistent metric keys (also redundant with dedicated collector alerts); keep OOM-risk. - inserts RejectedInserts: alert on per-poll delta, not the cumulative counter that fired critical forever after one rejection. - parts_age: metric-only; old active parts are normal, not merge pressure. - schema_drift: key baseline by instance (was clobbered across a fleet). - k8s: route alerts to the polled instance, not literal "k8s" (never persisted; PD re-fired with no resolve). - tables merges-stalled: fix contradictory defaults (min 30 > max 20 made every merge count simultaneously stalled+too-many). - advisor no_ttl_large: system.tables has no has_ttl_expression column.
Replaces the out-of-band pages.dev landing page (whose source was never in the repo and had drifted: wrong Slack commands, wrong version floor, fabricated console, stale collector/view counts). - Self-contained single HTML file (inline CSS, no external requests, no build step) — deploys to Cloudflare Pages as-is. - Mirrors the real app design tokens from web/frontend/src/index.css (dark #0b1120 ground, violet #7c3aed accent, Inter stack, both themes). - Hero is a faithful CSS rebuild of the real Overview dashboard. - Accurate copy only: 24 collectors, OSS 23.3+/Cloud 25.3+, real /ch Slack subcommands, real nav/view groups, the correctness-harness story. - Separate primary 'Live demo' CTA (scrolls to the in-page interactive Overview; README documents wiring it to a hosted instance for Phase 3). - web/landing/README.md: deploy steps + why this exists.
…ed fleet actions
- analyzer health score: remove the hard floor of 50 (deductions were
capped at 50, so score never dropped below it). The UI 'critical' band
(<50) and the SLO metric counting score<50 were both unreachable.
Recalibrate to crit=30/warn=8/info=2 (per distinct category) so bands
map sensibly: 1 critical->70 (warning), 2->40 (critical). +test.
This also de-degenerates SLO UptimePct (was 100% by construction).
- store health trend: max(criticals)/max(warns) per bucket, not sum() —
one alert firing across a 4h bucket at 1m polls showed as 240.
- store LogAction: fall back to the first instance when called with an
empty instance (fleet-wide actions) so those actions are audited
instead of silently dropped (clientFor("") returned nil).
- store GetActiveAlerts/GetAlertHistory: SELECT first_seen_at + fire_count
(already parsed but never queried) so Slack 'xN' repeat counts and
'firing since' reflect reality instead of always 1/CreatedAt.
Query noise (queries.go + config): - failed-query and timeout alerts now require a count floor (warn 10 / crit 50) — a single exception in 5m no longer pages; timeouts (usually intended client max_execution_time) demoted behind the same floor. - long-running defaults 30s/60s -> 2m/5m (OLAP queries run for minutes). - 'full table scan' (>1B rows) demoted warn->info (often the intended shape of an aggregation). - query-storm floor raised 5 -> 20 concurrent-per-user. Cumulative-counter fix (errors.go): system.errors.value is cumulative since restart; alert on the per-poll delta (new occurrences) instead of a lifetime total reported as 'last hour'. Per-instance state, baseline poll is silent. Lazy-load fix (dictionaries.go): only a dictionary with a real last_exception is actionable; a bare NOT_LOADED (lazy_load / transient reloading) and element_count==0 on a legitimately-empty source no longer alert. Others: - tables merges 'too many' demoted critical->single warn (active merges are the system healing; contradicted merges-stalled). - ttl: scope stuck-mutation query to real TTL commands (was matching every ALTER MODIFY); 'TTL not running' demoted to metric-only (can't judge without the table's TTL interval). - tables JBOD imbalance demoted to metric-only (compared across storage tiers that are imbalanced by design). - analyzer anomaly/sustained: only unlabeled scalar metrics feed detection; labeled per-table/query series collapsed by name made every z-score noise. Tests: fp_rework_test.go covers the failure/timeout floors, errors delta, and dictionary lazy-load via the fake-CH harness.
slow_query_fingerprint fired *critical* at 200 execs/5min (0.67 QPS) — ordinary app traffic. Severity now requires frequency AND non-trivial per-exec cost (crit: >=100x at avg>=5s; warn: >=50x at avg>=1s), so it reflects real aggregate query-time load. Purely-frequent fast patterns are left to the advisory repeated-patterns info alert.
…stworthy - Resolve side effects now fire from EVERY resolve path, not just the reconcile clean-check path. Extracted notifyResolved() (PagerDuty resolve + webhook alert_resolved + ack clear); added ResolveAndNotify (UI resolve) and ResolveStaleAndNotify (heartbeat stale-sweep, via before/after diff). Wired server handleResolveAlert + handleResolveStale through them. Fixes PagerDuty incidents that leaked open forever on UI/stale resolves. - Severity escalation (warn -> critical on an already-open alert) now notifies: detected in the reconcile diff, force-refreshed so the DB severity updates immediately, and routed to notify (respecting maintenance/inhibition/snooze). Previously it rode the existing warn row as a silent touch and never paged. - Ack stops escalation: the heartbeat escalation notice is skipped when every active non-info alert on the instance is acked. - Snooze silences an ALREADY-firing alert: snoozed alerts are hidden from the Slack per-instance message and escalation (still visible in the UI, which reads the store directly). - Webhook rate limiter no longer drops resolves or escalations: only alert_firing is rate-limited, keyed by severity+dedup_key, so terminal resolve/all_clear events and genuine warn->critical escalations always deliver. - Added replication:critical -> inserts:warn inhibition (broken replication makes inserts fail downstream). Tests: pipeline_test.go covers escalation-notifies, resolve-fires-side- effects, and stale-sweep-fires-side-effects via a webhook capture server. All 21 alerter tests pass.
Slack snooze was implemented via the maintenance store, so `/ch snooze` DROPPED new alerts entirely (no DB row, invisible in the UI) — the opposite of what 'snooze' should do. The SlackApp wasn't even wired with the snooze store. - SnoozeStore gains per-instance snoozes: an entry with an empty DedupKey and Instance set silences a whole node. IsSnoozed(dedupKey, instance) now matches either a per-alert snooze (dedup key, from the web UI) or a per-instance snooze (from Slack). Added CancelInstance for /ch unsnooze. - Threaded SnoozeStore into slackapp.App + New() + main.go wiring. - doSnooze now writes an instance-level snooze (persist-but-silent) instead of a maintenance window; cmdUnsnooze cancels it; cmdSnoozed lists real snoozes. /ch status shows a 🔇 Snoozed indicator distinct from 🔧 Maintenance. - Updated the 3 alerter IsSnoozed call sites to pass the instance. Net: /ch snooze now silences Slack/PagerDuty/webhook while still recording alerts to the DB/UI (and, via the earlier Slack-view filter, silences an already-firing alert) — genuinely distinct from /ch maintenance, which still drops alerts. Tests: instance-snooze-suppresses-notify + CancelInstance in pipeline_test.go.
… hardening The dashboard/API (SQL terminal, KILL QUERY, OAuth token setters, alert mutation) was fully unauthenticated and CH TLS verification was hardcoded off. - Opt-in API auth: security.api_token (or CH_ANALYZER_API_TOKEN env) gates every /api/* request via Authorization: Bearer / X-API-Token / cookie, constant-time compared. Empty = unchanged open default. /health, /, and /assets/* stay open. A one-time /?token=<t> visit sets an httpOnly cookie so the bundled SPA authenticates with no rebuild (same-origin fetches carry it). Startup logs a warning when auth is off. Tested in auth_middleware_test.go. - TLS verification now defaults ON: both chclient wiring sites read security.tls_skip_verify (default false) instead of a hardcoded true. Startup warns when disabled. - Slack socketmode debug logging gated behind CH_ANALYZER_SLACK_DEBUG (was hardcoded on, flooding prod stderr). - Secrets: repo-local .gitignore for staging-env.yaml / *-staging.yaml / ai-changes-md / .env* (previously only excluded by a user-global gitignore); SECURITY.md documents the threat model, auth, TLS, readonly-user guidance, and credential rotation. - Documented security block in configs/ch-analyzer.yaml. Note: /api/alerts/trigger stays enabled (it backs Run Check's promote-to-alert action) — the token gate is its protection, not a dead-by-default flag.
Removed files with zero imports anywhere (verified via grep + routing check): - views/Dashboard.tsx (803L) — superseded ancestor of Overview's inlined widget system; not in VALID_VIEWS or the sidebar. - views/Discover.tsx (899L) — feature catalog superseded by FeatureGuide. - components/EmptyState.tsx (36L), components/ThinkingSpinner.tsx (72L) — unreferenced. Also fixed a pre-existing stale test: VALID_VIEWS has 18 routes (the 'guide' view was added without updating the test, which asserted 17). tsc -b clean, 49/49 vitest pass.
- CHLogs level filter: send ALL selected levels (comma-joined) instead of just the first; the backend already filters with level IN (...), so the multi-select pills were silently dropping every selection but one. - Alerts maintenance banner: derive it from the active maintenance windows (api.maintenance.list) instead of filtering cachedInstances — a string[] of names — on .in_maintenance, which is always undefined on a string, so the banner never rendered. - Explore deep link: derive the valid-tab allowlist from the canonical TABS list so ?tab=querylog (and ?tab=connections) resolve instead of silently falling back to 'patterns' (the hand-maintained allowlist had drifted). - Charts: replace white rgba(255,255,255,…) tooltip/hover cursors with a theme-neutral slate so they're visible in light theme too (they were invisible on a light background). Rebuilt the embedded frontend bundle (internal/web/static) so the fixes ship in the binary. tsc -b clean, 49/49 vitest pass, go build embeds cleanly.
setChatSessions serialized up to 100 sessions and wrote localStorage on every call; during SSE streaming (one call per chunk) that stringified everything on the main thread per token. Now state still updates synchronously (UI needs it live) but the persist is debounced 400ms, with a flush on tab-hide/close and unmount so the last streamed content isn't lost. Rebuilt embedded bundle.
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
1 participant
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.
Deep audit + correctness overhaul
A full audit of ch-analyzer against real ClickHouse semantics, then a correctness-first overhaul. The theme: the tool had grown broad (24 collectors, 68 alerts, 18 views, ~85 endpoints) but almost none of it was validated — so ~56% of alerts were noise or outright broken, several checks queried columns that don't exist, and the alerting lifecycle leaked. This PR makes the analysis correct and trustworthy, adds a harness so it stays that way, and cleans up the UI and security posture.
All commits build clean;
go test ./...andgo vet ./...pass, and the frontend passestsc -b+ 49 vitest tests throughout.Phase 0 — Audit (
docs/audit/)Architecture map, docs/theory reconstruction, live-site-vs-repo drift, per-collector correctness findings, a 68-alert verdict table, and bloat/duplication list. Headline: the live
ch-analyzer.pages.devwas a hand-built static page not in the repo; 12 alerts were broken, 26 false-positive-prone; the whole API was unauthenticated.Correctness harness (Phase 1)
internal/testsupport/fakech— an in-process fake ClickHouse HTTP server that exercises the real client/SQL/decode path, so collectors can be driven through known states and asserted.scenario_test.go(behavior) +sql_contract_test.go(fails the build if any collector/web SQL references a ClickHouse identifier that doesn't exist). The contract test immediately caught two additional dead-query bugs in the web layer beyond the audited 12.Collector correctness — 14 dead/wrong-data checks fixed
Async-insert enum, background merge-pool metric name, Keeper (real
is_expiredsession check replacing 3 nonexistent columns), crashtrace_full, CPU*Normalized(OSS + history-chart paths were dead), cache sizes fromasynchronous_metrics, MV inner-table join,RejectedInsertsnow alerts on a per-poll delta (not the lifetime counter that fired critical forever), parts-age is metric-only (old parts aren't merge pressure), schema-drift keyed per-instance, k8s alerts routed to the real instance, the self-contradicting merges-stalled defaults, and thehas_ttl_expressionadvisor. Three dead analyzer cross-alerts removed rather than resurrected (they duplicated dedicated alerts).Alert quality — 26 FP-prone alerts reworked
Query failures/timeouts now require count floors (no more single-exception pages); long-running defaults 30s/60s → 2m/5m (OLAP-friendly);
system.errorsalerts on the per-poll delta not the lifetime total; dictionaries only alert on a real load exception (lazy-load is silent); "too many merges" demoted (active merges are the system healing); TTL/JBOD/parts-age demoted to metric-only where they couldn't be judged correctly; anomaly detection restricted to unlabeled scalar metrics; query-storm severity tied to load, not 0.67 QPS.Alerting pipeline made trustworthy
alert_resolved, ack clear) now fire from every resolve path (reconcile, UI, stale-sweep) — no more leaked PagerDuty incidents./ch snoozeis now a real snooze (persist-but-silent, per-instance) instead of a maintenance window that dropped new alerts.Health / SLO / audit correctness
Score bands are reachable (the old floor of 50 made "critical" unreachable and SLO uptime 100% by construction); health-trend uses
maxper bucket notsum; fleet-wide actions are audited;first_seen_at/fire_countare actually read back.Landing page (
web/landing/)A self-contained static page that mirrors the real app's design tokens and carries only accurate copy (real Slack commands, version floor, collector/view counts), replacing the fabricated out-of-band page. The hero is a faithful CSS rebuild of the actual Overview dashboard.
Security hardening
Opt-in API token auth (
security.api_token/CH_ANALYZER_API_TOKEN) gating/api/*; ClickHouse TLS verification now defaults on; Slack debug logging gated; repo-local.gitignorefor credential files;SECURITY.md.Frontend cleanup
Deleted 1,810 lines of verified-dead code; fixed 5 concrete bugs (CHLogs level filter, Alerts maintenance banner, Explore deep links, light-theme chart cursors, chat-session localStorage thrash); rebuilt the embedded bundle so fixes ship.
Not in this PR (needs visual QA or external infra)
tsccan't verify UI rendering, so these need a running app.all_clearwebhook; a few per-alert tunings.Review guide
Full evidence with
file:linecitations is indocs/audit/. Each commit is scoped to one concern and independently green.