Add ClickHouse metric stream CDC - #1080
Merged
Merged
Conversation
Contributor
|
Storybook previews for This comment updates automatically on each PR push. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR replaces the broken ClickHouse MaterializedPostgreSQL replication approach for the TimescaleDB fitness.metric_stream hypertable with a ClickHouse-native MergeTree table + chunk-range backfill migration, and introduces internal PeerDB services plus a one-shot setup command to configure ongoing CDC into ClickHouse.
Changes:
- Switch ClickHouse bootstrap to create
postgres_fitness.metric_streamas a nativeMergeTreetable (noMaterializedPostgreSQL), and extend ClickHouse table-wait logic. - Add ClickHouse migration logic to rebuild and backfill
metric_streamusing Timescale chunk ranges via ClickHousepostgresql(...)table function. - Add PeerDB CDC setup (SQL builders + env-driven runner), Swarm services, deploy workflow orchestration, and supporting docs.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/db/setup-clickhouse-cdc.ts | Adds a one-shot CLI entrypoint to configure PeerDB peers/mirror for ClickHouse CDC. |
| src/db/clickhouse.ts | Updates ClickHouse bootstrap DDL to use a native MergeTree metric stream table and increases table-wait attempts. |
| src/db/clickhouse.test.ts | Updates bootstrap statement expectations and adds a longer wait-for-table test. |
| src/db/clickhouse-migrations.ts | Adds new ClickHouse migrations and implements chunk-based backfill into native postgres_fitness.metric_stream. |
| src/db/clickhouse-migrations.test.ts | Expands migration tests, mocks pg, and adds backfill verification tests. |
| src/db/clickhouse-cdc.ts | Implements PeerDB SQL statement builders and env-driven setup that configures Postgres/ClickHouse peers + mirror. |
| src/db/clickhouse-cdc.test.ts | Adds unit tests for generated PeerDB SQL statements. |
| drizzle/0009_metric_stream_id_not_null_primary_key.sql | Ensures fitness.metric_stream.id is backfilled, set NOT NULL, and used in a Timescale-compatible PK. |
| drizzle/0010_oauth_token_primary_key.sql | Ensures oauth_token.user_id is NOT NULL before creating PK from existing unique index. |
| docs/superpowers/plans/2026-05-01-peerdb-clickhouse-cdc.md | Adds an implementation plan outlining PeerDB services + CDC setup workflow. |
| docs/production-incident-baseline.md | Adds detailed incident timeline/lessons learned leading to this approach (native backfill + PeerDB). |
| docs/clickhouse-metric-stream.md | Updates architecture explanation: native backfill + PeerDB CDC (no MaterializedPostgreSQL). |
| docs/README.md | Updates docs index entry to reflect new ClickHouse/PeerDB design. |
| deploy/stack.yml | Adds PeerDB/Temporal/MinIO services and increases ClickHouse memory limit to 2G; updates app CLICKHOUSE_URL to use encoded password. |
| deploy/server.tf | Updates data-volume provisioning to create PeerDB bind-mount directories and bumps replacement triggers. |
| deploy/README.md | Documents PeerDB role, bind mounts, and deploy workflow steps for CDC setup. |
| README.md | Updates repo architecture overview to reflect new ClickHouse backfill + PeerDB CDC approach. |
| .github/workflows/deploy-web-stack.yml | Adds ClickHouse password encoding, ClickHouse resource-limit enforcement, improved migration execution, PeerDB readiness wait, and CDC setup step. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
drizzle/0009_metric_stream_id_not_null_primary_key.sql (1)
39-149:⚠️ Potential issue | 🟠 MajorAdd exception handling to ensure session_replication_role is restored.
Session parameters set with
set_config()are not transactional—they persist even if the transaction rolls back. If the procedure fails before reaching line 148, the restore does not execute. When theCALLstatement fails, the entire migration transaction rolls back, so theRESET session_replication_roleat line 162 never runs. The session remains in replica mode, leaving triggers and foreign key checks disabled for the rest of the session.Wrap the procedure body in an
EXCEPTIONblock to guarantee the role is restored:Example pattern
BEGIN previous_replication_role := current_setting('session_replication_role'); PERFORM set_config('session_replication_role', 'replica', false); -- ... procedure body ... EXCEPTION WHEN OTHERS THEN PERFORM set_config('session_replication_role', previous_replication_role, false); RAISE; END;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@drizzle/0009_metric_stream_id_not_null_primary_key.sql` around lines 39 - 149, The procedure currently sets session_replication_role to 'replica' using previous_replication_role and restores it only at the end, but if an error occurs the restore is skipped; wrap the main body (the code between the initial set_config(...) and the final PERFORM set_config(...)) inside a PL/pgSQL BEGIN ... EXCEPTION WHEN OTHERS THEN ... END block so that on any error you call PERFORM set_config('session_replication_role', previous_replication_role, false) in the EXCEPTION handler and then RAISE to re-throw the error; keep the existing final PERFORM set_config(...) for the normal path and ensure you reference the same variables (previous_replication_role, session_replication_role, set_config) and preserve commits/COMMIT semantics inside the try block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deploy/stack.yml`:
- Around line 224-226: Replace the hardcoded MinIO credentials by switching
MINIO_ROOT_USER and MINIO_ROOT_PASSWORD to require secret-backed env vars (use
the ${VAR_NAME:?is required} form) and reference those same vars wherever the
credentials are used (e.g., in the peerdb-minio service and the flow service env
block); also convert the bucket name PEERDB_CLICKHOUSE_AWS_S3_BUCKET_NAME to a
configurable env var if it must differ per environment and update any duplicate
occurrences noted around lines 251-253 to reuse the same secret-backed env var
names.
- Around line 229-235: The Docker entrypoint uses a fixed "sleep 2" before
running "mc alias set" which can race if MinIO isn't ready; replace that sleep
with a bounded readiness-retry loop that polls MinIO (e.g. via "mc admin info",
"mc admin health", or an HTTP health/readiness endpoint against
http://localhost:9000) and only runs "mc alias set peerdb-minio ..." once the
server responds OK; implement a retry count and short backoff to avoid infinite
wait and surface a clear error if MinIO never becomes ready, keeping the rest of
the entrypoint (minio server, mc mb, wait) unchanged and referencing the
entrypoint/minio server/mc alias set/mc mb steps.
In `@docs/production-incident-baseline.md`:
- Line 1806: The duplicate heading "### Remaining Risk" causes an MD024
duplicate heading violation; rename this instance to a section-specific title
(for example "### Remaining Risk — Service Availability" or "### Remaining Risk
(Post-incident Mitigations)") so it no longer conflicts with the other sibling
heading named "### Remaining Risk"; update the heading text where it appears to
the new unique title.
In `@docs/superpowers/plans/2026-05-01-peerdb-clickhouse-cdc.md`:
- Around line 13-62: The markdown task headings jump from top-level to H3;
change each "### Task 1: Add PeerDB CDC Setup Logic", "### Task 2: Add PeerDB
Services To Swarm", and "### Task 3: Verify And Document" to H2 (replace "###"
with "##") or insert a parent H2 above them so heading levels increment
correctly; ensure the sublists under each task remain at H3/HH4 as appropriate
and re-run the markdown linter to confirm the heading hierarchy is valid.
In `@src/db/clickhouse-cdc.ts`:
- Around line 75-89: In buildRuntimeConfig(), explicitly validate the parsed
DATABASE_URL components (databaseUrl.username, databaseUrl.password, and
databaseUrl.pathname/database name) before using them to build postgresPeer and
peerDbUrl; if any component is missing or empty, throw a clear hard-fail error
naming the missing key (e.g., "Missing DATABASE_URL username", "Missing
DATABASE_NAME in DATABASE_URL", "Missing DATABASE_URL password") so PeerDB setup
never proceeds with incomplete credentials; update buildRuntimeConfig to perform
these checks immediately after const databaseUrl = new
URL(requireEnv("DATABASE_URL")) and before composing postgresPeer/peerDbUrl.
In `@src/db/clickhouse-migrations.ts`:
- Around line 227-247: The fetchMetricStreamBackfillChunks function currently
runs a raw pg query and returns result.rows directly; change it to call
executeWithSchema() (from typed-sql.ts) with a Zod schema that validates each
row has lower_bound and upper_bound strings (or the expected types) and use the
validated output instead of result.rows; ensure you replace the direct
Client.query usage inside fetchMetricStreamBackfillChunks with
executeWithSchema(postgresClient, sqlString, schema) (or the project's
executeWithSchema signature) and propagate/return the typed
MetricStreamBackfillChunkRow[] produced by the schema validation.
In `@src/db/setup-clickhouse-cdc.ts`:
- Around line 16-18: The catch block in src/db/setup-clickhouse-cdc.ts currently
only calls logger.error and exits; update the handler to report the error to
Sentry before exiting by calling captureException(error) (ensure
captureException is imported from your Sentry client, e.g., `@sentry/node` or your
app's sentry wrapper) and then call process.exit(1); keep the existing
logger.error call so the sequence is: logger.error(...),
captureException(error), process.exit(1).
---
Outside diff comments:
In `@drizzle/0009_metric_stream_id_not_null_primary_key.sql`:
- Around line 39-149: The procedure currently sets session_replication_role to
'replica' using previous_replication_role and restores it only at the end, but
if an error occurs the restore is skipped; wrap the main body (the code between
the initial set_config(...) and the final PERFORM set_config(...)) inside a
PL/pgSQL BEGIN ... EXCEPTION WHEN OTHERS THEN ... END block so that on any error
you call PERFORM set_config('session_replication_role',
previous_replication_role, false) in the EXCEPTION handler and then RAISE to
re-throw the error; keep the existing final PERFORM set_config(...) for the
normal path and ensure you reference the same variables
(previous_replication_role, session_replication_role, set_config) and preserve
commits/COMMIT semantics inside the try block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b8af05f5-ec28-426a-a51f-332b1f587379
📒 Files selected for processing (18)
.github/workflows/deploy-web-stack.ymlREADME.mddeploy/README.mddeploy/server.tfdeploy/stack.ymldocs/README.mddocs/clickhouse-metric-stream.mddocs/production-incident-baseline.mddocs/superpowers/plans/2026-05-01-peerdb-clickhouse-cdc.mddrizzle/0009_metric_stream_id_not_null_primary_key.sqldrizzle/0010_oauth_token_primary_key.sqlsrc/db/clickhouse-cdc.test.tssrc/db/clickhouse-cdc.tssrc/db/clickhouse-migrations.test.tssrc/db/clickhouse-migrations.tssrc/db/clickhouse.test.tssrc/db/clickhouse.tssrc/db/setup-clickhouse-cdc.ts
Contributor
|
Review app is ready: This environment runs on a dedicated Hetzner server for PR #1080 and updates on each push. |
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/db/clickhouse-cdc.test.ts (1)
10-34: ⚡ Quick winExtract repeated test setup into a local
test-helpers.ts.The same
templateValuesobject and mock-client scaffolding are repeated across tests. Please move shared fixtures/builders intosrc/db/test-helpers.tsand reuse them to keep these cases focused on assertions.♻️ Suggested refactor sketch
+import { + createCdcTestHarness, + defaultPeerDbTemplateValues, +} from "./test-helpers.ts"; ... -const peerDbQueries: string[] = []; -const clickHouseCommands: string[] = []; +const { peerDbQueries, clickHouseCommands, peerDbClient, clickHouseClient } = + createCdcTestHarness(); ... - peerDbClient: { - async query(queryText) { - peerDbQueries.push(queryText); - }, - }, - clickHouseClient: { - async command(options) { - clickHouseCommands.push(options.query); - }, - }, + peerDbClient, + clickHouseClient, ... - templateValues: { ...repeated values... }, + templateValues: defaultPeerDbTemplateValues,As per coding guidelines:
Shared test utilities: When multiple unit or integration tests need to share mock setups, utility functions, or test data, extract them into a local test-helpers.ts file within the same directory.Also applies to: 42-63, 70-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db/clickhouse-cdc.test.ts` around lines 10 - 34, Tests repeat the same templateValues and mock client scaffolding for setupClickHouseCdc; extract them into src/db/test-helpers.ts by exporting a getDefaultTemplateValues() returning the templateValues object and helper builders like createPeerDbMock(peerDbQueries) and createClickHouseMock(clickHouseCommands) that implement async query/command and push calls; then update the failing test to import getDefaultTemplateValues, createPeerDbMock and createClickHouseMock and pass their results into setupClickHouseCdc instead of inlining templateValues/peerDbClient/clickHouseClient (also replace the same repeated code in the other test blocks referenced).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/db/clickhouse-cdc.test.ts`:
- Around line 10-34: Tests repeat the same templateValues and mock client
scaffolding for setupClickHouseCdc; extract them into src/db/test-helpers.ts by
exporting a getDefaultTemplateValues() returning the templateValues object and
helper builders like createPeerDbMock(peerDbQueries) and
createClickHouseMock(clickHouseCommands) that implement async query/command and
push calls; then update the failing test to import getDefaultTemplateValues,
createPeerDbMock and createClickHouseMock and pass their results into
setupClickHouseCdc instead of inlining
templateValues/peerDbClient/clickHouseClient (also replace the same repeated
code in the other test blocks referenced).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c2073cde-ece1-4c86-866e-a529a14b1e17
📒 Files selected for processing (8)
cspell.jsondeploy/README.mddocs/clickhouse-metric-stream.mddocs/production-incident-baseline.mdknip.jsonsrc/db/clickhouse-cdc.test.tssrc/db/clickhouse-cdc.tssrc/db/peerdb/metric-stream-cdc.sql
✅ Files skipped from review due to trivial changes (2)
- knip.json
- cspell.json
Asherlc
force-pushed
the
Asherlc/clickhouse-metric-stream
branch
3 times, most recently
from
May 1, 2026 22:39
f666964 to
6b3a426
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deploy/README.md`:
- Line 143: The example deploy command hard-codes the stack name "dofek"; change
it to use the placeholder "<stack>" so the README's shared staging/production
flow is parameterized. Locate the line containing the command string "docker
stack deploy -c deploy/stack.yml --with-registry-auth --prune --detach=false
dofek" and replace the trailing "dofek" with "<stack>" (matching the surrounding
steps), preserving all flags and spacing.
In `@src/db/clickhouse-cdc.ts`:
- Around line 35-36: The peerDbStringLiteral function currently escapes single
quotes and backslashes with backslashes, which is not PostgreSQL standard;
change peerDbStringLiteral to escape single quotes by doubling them (replace "'"
with "''") and do not double backslashes (leave "\" unchanged), then wrap the
result in single quotes for the CREATE PEER ... WITH clause; also update the
failing test in clickhouse-cdc.test.ts (the case with postgresCredential:
"pa'ss\\word") to expect the PostgreSQL-correct literal `'pa''ss\word'` instead
of the current escaped form.
In `@src/db/clickhouse-migrations.test.ts`:
- Around line 267-299: The test expects no backfill tracking table created but
production function backfillNativeMetricStream (in
src/db/clickhouse-migrations.ts) creates analytics.metric_stream_backfill_chunks
before checking for chunks; fix by either updating the test to expect the table
creation or change the implementation: inside backfillNativeMetricStream, call
the chunk-count/query that determines whether Timescale has metric stream chunks
first, and only create analytics.metric_stream_backfill_chunks (and perform any
INSERT INTO postgres_fitness.metric_stream work) if that count is > 0; adjust
runClickHouseMigrations-related logic accordingly so the table creation is
guarded by the empty-chunk check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5594d646-4748-44fd-b5a5-f20ef77dcbf2
📒 Files selected for processing (9)
cspell.jsondeploy/README.mddocs/clickhouse-metric-stream.mddocs/production-incident-baseline.mdknip.jsonsrc/db/clickhouse-cdc.test.tssrc/db/clickhouse-cdc.tssrc/db/clickhouse-migrations.test.tssrc/db/peerdb/metric-stream-cdc.sql
✅ Files skipped from review due to trivial changes (3)
- cspell.json
- knip.json
- src/db/peerdb/metric-stream-cdc.sql
🚧 Files skipped from review as they are similar to previous changes (2)
- src/db/clickhouse-cdc.test.ts
- docs/clickhouse-metric-stream.md
Asherlc
force-pushed
the
Asherlc/clickhouse-metric-stream
branch
from
May 1, 2026 22:51
6b3a426 to
1dff79b
Compare
Asherlc
force-pushed
the
Asherlc/clickhouse-metric-stream
branch
from
May 1, 2026 22:52
1dff79b to
ecd51a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/db/clickhouse-cdc.ts`:
- Line 98: The POSTGRES_PORT value is incorrectly wrapped with
peerDbStringLiteral() causing a quoted string in the SQL template; update the
assignment for POSTGRES_PORT (the symbol POSTGRES_PORT in
src/db/clickhouse-cdc.ts) to mirror CLICKHOUSE_PORT by using
String(values.postgresPort) without peerDbStringLiteral(), so the generated
template emits a numeric port value for PeerDB's port = {{POSTGRES_PORT}}
placeholder.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 414d0351-d89d-4f83-b3f1-b60a40b34485
📒 Files selected for processing (10)
cspell.jsondeploy/README.mddocs/clickhouse-metric-stream.mddocs/production-incident-baseline.mdknip.jsonsrc/db/clickhouse-cdc.test.tssrc/db/clickhouse-cdc.tssrc/db/clickhouse-migrations.test.tssrc/db/peerdb/metric-stream-cdc.sqlsrc/db/setup-clickhouse-cdc.test.ts
✅ Files skipped from review due to trivial changes (5)
- cspell.json
- knip.json
- src/db/peerdb/metric-stream-cdc.sql
- src/db/clickhouse-cdc.test.ts
- deploy/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/clickhouse-metric-stream.md
This was referenced May 1, 2026
This was referenced May 22, 2026
This was referenced May 31, 2026
Merged
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
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.
Summary
Test Plan
Summary by CodeRabbit
New Features
Infrastructure & Deployment
Database
Documentation
Tests