[codex] Repair legacy ClickHouse metric stream engine - #1099
Conversation
|
Storybook previews for This comment updates automatically on each PR push. |
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds a targeted ClickHouse migration to repair a production incident where legacy ChangesLegacy Metric Stream Engine Repair
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds a targeted ClickHouse migration to detect and repair legacy postgres_fitness.metric_stream mirrors that were created with the plain MergeTree engine, which breaks newer analytics read models that query the mirror using FINAL.
Changes:
- Add a new ClickHouse migration (
0007_repair_legacy_metric_stream_engine) that detects the legacy engine, rebuilds the mirror asReplacingMergeTree(_peerdb_version), backfills it, and refreshes dependent analytics read models. - Extend ClickHouse migration test coverage to exercise the legacy-engine replacement path.
- Document the associated production deploy incident and mitigation in the incident baseline.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/db/clickhouse-migrations.ts | Adds a new migration that conditionally drops/recreates the metric stream mirror and refreshes dependent analytics models. |
| src/db/clickhouse-migrations.test.ts | Updates migration-count expectations and adds a regression test for the legacy MergeTree repair path. |
| docs/production-incident-baseline.md | Adds a dated incident entry documenting symptoms, root cause, and mitigation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| id: "0007_repair_legacy_metric_stream_engine", | ||
| run: replaceLegacyMetricStreamIfNeeded, | ||
| }, |
| "DROP VIEW IF EXISTS analytics.provider_stats", | ||
| "DROP TABLE IF EXISTS analytics.provider_stats", | ||
| "DROP VIEW IF EXISTS analytics.derived_resting_heart_rate", | ||
| "DROP TABLE IF EXISTS analytics.derived_resting_heart_rate", | ||
| "DROP VIEW IF EXISTS analytics.activity_summary", | ||
| "DROP TABLE IF EXISTS analytics.activity_summary", | ||
| "DROP VIEW IF EXISTS analytics.deduped_sensor", | ||
| "DROP TABLE IF EXISTS analytics.deduped_sensor", |
|
Review app is ready: This environment runs on a dedicated Hetzner server for PR #1099 and updates on each push. |
Summary
postgres_fitness.metric_streamtables created as plainMergeTreeReplacingMergeTree(_peerdb_version), backfill it, and refresh dependent analytics read models before provider stats runsRoot Cause
Production still had a legacy
postgres_fitness.metric_streamtable created with the plainMergeTreeengine. Newer ClickHouse read models query that raw mirror withFINAL, which requiresReplacingMergeTree(_peerdb_version). The existing bootstrap SQL usedCREATE TABLE IF NOT EXISTS, so it did not repair the table engine.Validation
pnpm install --frozen-lockfiledocker compose up -d db redisattempted; Redis started, DB bind was already occupied by the existingaloud-bike-db-1container on127.0.0.1:5435pnpm test run src/db/clickhouse-migrations.test.tspnpm lintpnpm tsc --noEmitcd packages/server && pnpm tsc --noEmitcd packages/web && pnpm tsc --noEmitpnpm test:changedSummary by CodeRabbit
Bug Fixes
Documentation