[codex] Move provider stats fully to ClickHouse - #1098
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis pull request routes provider statistics reads from Postgres to a ClickHouse read model by adding five new raw ClickHouse tables and a PeerDB mirror, expanding analytics.provider_stats, splitting/executing PeerDB SQL statements, adding a migration, updating SyncRepository to use a ClickHouse store, wiring the tRPC endpoint, updating tests, and documenting the changes. ChangesProvider Stats ClickHouse Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
Asherlc
marked this pull request as ready for review
May 7, 2026 17:35
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 completes the migration of sync.providerStats from Postgres to the ClickHouse analytics.provider_stats read model, and expands PeerDB CDC + raw ClickHouse mirror coverage so provider inventory tables are available in ClickHouse.
Changes:
- Route
sync.providerStatsto ClickHouse-backed counts instead of per-provider Postgres subqueries. - Add new PeerDB mirror + ClickHouse raw tables for provider inventory (
food_entry,health_event,lab_panel,lab_result,journal_entry). - Add ClickHouse migration
0008_complete_provider_stats_raw_mirrorsand update operational/docs notes.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/db/peerdb/metric-stream-cdc.sql | Adds a dedicated PeerDB mirror for provider-inventory tables into postgres_fitness in ClickHouse. |
| src/db/clickhouse.test.ts | Extends bootstrap SQL assertions to include new raw tables and provider_stats logic. |
| src/db/clickhouse-read-models.ts | Updates analytics.provider_stats to count inventory tables; introduces provider_stats statement builder used by migrations/bootstrap. |
| src/db/clickhouse-raw-tables.ts | Adds ClickHouse raw table DDL for provider inventory tables under postgres_fitness.*. |
| src/db/clickhouse-migrations.ts | Adds migration 0008_complete_provider_stats_raw_mirrors to create raw tables and rebuild provider_stats. |
| src/db/clickhouse-migrations.test.ts | Updates expected migration output and migration count. |
| src/db/clickhouse-cdc.ts | Adds provider inventory tables to CDC publication/table lists. |
| src/db/clickhouse-cdc.test.ts | Updates CDC setup expectations to include the new mirror and updated query counts. |
| packages/server/src/routers/sync.ts | Passes ctx.sensorStore into SyncRepository for providerStats. |
| packages/server/src/routers/sync.test.ts | Updates providerStats tests to validate ClickHouse path and that Postgres isn’t queried. |
| packages/server/src/routers/clickhouse-integration-test-helpers.ts | Adds raw-table sync support for new provider inventory tables in ClickHouse integration tests. |
| packages/server/src/routers/clickhouse-integration-test-helpers.test.ts | Extends assertions for new truncate/insert commands. |
| packages/server/src/repositories/sync-repository.ts | Replaces Postgres provider stats query with ClickHouse analytics.provider_stats query via injected store. |
| packages/server/src/repositories/sync-repository.test.ts | Updates unit tests to validate ClickHouse stats path and mapping behavior. |
| docs/production-incident-baseline.md | Adds incident write-up documenting Postgres pool starvation and the mitigation. |
| docs/clickhouse-metric-stream.md | Documents provider inventory counts being sourced from analytics.provider_stats and new mirrors. |
| deploy/README.md | Updates deploy step docs to reflect multiple mirrors applied during CDC setup. |
💡 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: 1
🧹 Nitpick comments (2)
src/db/clickhouse-read-models.ts (1)
771-788: ⚡ Quick winCollapse the two
food_entryaggregations into one scan.
food_entry_countsandnutrition_daily_countsboth readpostgres_fitness.food_entry FINAL, so everyanalytics.provider_statsrefresh scans that table twice. Since this view is refreshed continuously, it’s cheaper to computecount()anduniqExact(date)in the same CTE and join once.♻️ Suggested shape
-food_entry_counts AS ( - SELECT user_id, provider_id, count() AS count +food_entry_counts AS ( + SELECT + user_id, + provider_id, + count() AS food_entries, + uniqExact(date) AS nutrition_daily FROM postgres_fitness.food_entry FINAL WHERE _peerdb_is_deleted = 0 GROUP BY user_id, provider_id ), health_event_counts AS ( SELECT user_id, provider_id, count() AS count FROM postgres_fitness.health_event FINAL WHERE _peerdb_is_deleted = 0 GROUP BY user_id, provider_id ), -nutrition_daily_counts AS ( - SELECT user_id, provider_id, uniqExact(date) AS count - FROM postgres_fitness.food_entry FINAL - WHERE _peerdb_is_deleted = 0 - GROUP BY user_id, provider_id -), lab_panel_counts AS ( SELECT user_id, provider_id, count() AS count FROM postgres_fitness.lab_panel FINAL WHERE _peerdb_is_deleted = 0 GROUP BY user_id, provider_id ) ... - coalesce(food_entry_counts.count, 0) AS food_entries, + coalesce(food_entry_counts.food_entries, 0) AS food_entries, coalesce(health_event_counts.count, 0) AS health_events, coalesce(metric_stream_counts.count, 0) AS metric_stream, - coalesce(nutrition_daily_counts.count, 0) AS nutrition_daily, + coalesce(food_entry_counts.nutrition_daily, 0) AS nutrition_daily, coalesce(lab_panel_counts.count, 0) AS lab_panels, coalesce(lab_result_counts.count, 0) AS lab_results, coalesce(journal_entry_counts.count, 0) AS journal_entries ... -LEFT JOIN nutrition_daily_counts - ON nutrition_daily_counts.user_id = providers.user_id - AND nutrition_daily_counts.provider_id = providers.provider_id LEFT JOIN lab_panel_counts ON lab_panel_counts.user_id = providers.user_id AND lab_panel_counts.provider_id = providers.provider_idAlso applies to: 817-818, 843-845
packages/server/src/routers/clickhouse-integration-test-helpers.test.ts (1)
129-136: ⚡ Quick winExpand provider-inventory INSERT coverage in this helper test.
Only
.lab_resultis asserted in the new insert-path checks;.health_eventand.lab_panelregressions could slip through here.Proposed test assertion additions
expect( commands.some( (command) => command.includes("INSERT INTO postgres_fitness_test_") && command.includes(".lab_result") && command.includes("FROM postgresql('db:5432', 'health', 'lab_result'"), ), ).toBe(true); + expect( + commands.some( + (command) => + command.includes("INSERT INTO postgres_fitness_test_") && + command.includes(".health_event") && + command.includes("FROM postgresql('db:5432', 'health', 'health_event'"), + ), + ).toBe(true); + expect( + commands.some( + (command) => + command.includes("INSERT INTO postgres_fitness_test_") && + command.includes(".lab_panel") && + command.includes("FROM postgresql('db:5432', 'health', 'lab_panel'"), + ), + ).toBe(true);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server/src/routers/clickhouse-integration-test-helpers.test.ts` around lines 129 - 136, The test currently only asserts that commands.some(...) includes an INSERT referencing ".lab_result"; update the assertion in clickhouse-integration-test-helpers.test.ts (the commands.some check) to also verify INSERTs for ".health_event" and ".lab_panel" by adding similar includes checks for ".health_event" and ".lab_panel" (alongside the existing ".lab_result") and ensure the postgresql(...) source strings match the corresponding table names (e.g., postgresql('db:5432', 'health', 'health_event') and postgresql('db:5432', 'health', 'lab_panel')); keep the overall commands.some(...) structure so the test fails if any of the three INSERT paths are missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/server/src/routers/sync.ts`:
- Around line 315-317: The providerStats procedure (created via
cachedProtectedQuery with CacheTTL.SHORT) is missing a Zod output schema; update
the procedure chain for providerStats to call .output(...) with a Zod schema
that matches the shape returned by SyncRepository.getProviderStats() (e.g., an
array/object fields returned from getProviderStats) so the tRPC output is
validated; ensure the schema is imported from zod and attached to the same
providerStats definition where cachedProtectedQuery(...)...query(...) is
declared.
---
Nitpick comments:
In `@packages/server/src/routers/clickhouse-integration-test-helpers.test.ts`:
- Around line 129-136: The test currently only asserts that commands.some(...)
includes an INSERT referencing ".lab_result"; update the assertion in
clickhouse-integration-test-helpers.test.ts (the commands.some check) to also
verify INSERTs for ".health_event" and ".lab_panel" by adding similar includes
checks for ".health_event" and ".lab_panel" (alongside the existing
".lab_result") and ensure the postgresql(...) source strings match the
corresponding table names (e.g., postgresql('db:5432', 'health', 'health_event')
and postgresql('db:5432', 'health', 'lab_panel')); keep the overall
commands.some(...) structure so the test fails if any of the three INSERT paths
are missing.
🪄 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: 8a654b39-2c16-439a-b5a2-e9977bee22ed
📒 Files selected for processing (17)
deploy/README.mddocs/clickhouse-metric-stream.mddocs/production-incident-baseline.mdpackages/server/src/repositories/sync-repository.test.tspackages/server/src/repositories/sync-repository.tspackages/server/src/routers/clickhouse-integration-test-helpers.test.tspackages/server/src/routers/clickhouse-integration-test-helpers.tspackages/server/src/routers/sync.test.tspackages/server/src/routers/sync.tssrc/db/clickhouse-cdc.test.tssrc/db/clickhouse-cdc.tssrc/db/clickhouse-migrations.test.tssrc/db/clickhouse-migrations.tssrc/db/clickhouse-raw-tables.tssrc/db/clickhouse-read-models.tssrc/db/clickhouse.test.tssrc/db/peerdb/metric-stream-cdc.sql
Contributor
|
Review app is ready: This environment runs on a dedicated Hetzner server for PR #1098 and updates on each push. |
Asherlc
enabled auto-merge (squash)
May 7, 2026 18:00
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/server/src/routers/sync.test.ts (1)
1163-1209: ⚡ Quick winAssert the ClickHouse query stays user-scoped.
This test proves the mapping, but it doesn't protect the
userIdfilter. Please also assert thatsensorStore.queryis called with the current user param so a future regression can't drop the per-user boundary and still pass.Suggested test hardening
- it("maps ClickHouse rows to provider stats", async () => { - const execute = vi.fn().mockResolvedValue([]); + it("maps ClickHouse rows to provider stats", async () => { + const execute = vi.fn().mockResolvedValue([]); + const query = vi.fn().mockResolvedValue([ + { + provider_id: "wahoo", + activities: 10, + daily_metrics: 5, + sleep_sessions: 3, + body_measurements: 2, + food_entries: 8, + health_events: 1, + metric_stream: 100, + nutrition_daily: 7, + lab_panels: 2, + lab_results: 4, + journal_entries: 6, + }, + ]); const caller = createCaller({ db: { execute, }, sensorStore: { - query: vi.fn().mockResolvedValue([ - { - provider_id: "wahoo", - activities: 10, - daily_metrics: 5, - sleep_sessions: 3, - body_measurements: 2, - food_entries: 8, - health_events: 1, - metric_stream: 100, - nutrition_daily: 7, - lab_panels: 2, - lab_results: 4, - journal_entries: 6, - }, - ]), + query, }, userId: "user-1", timezone: "UTC", }); @@ expect(result).toEqual([ { providerId: "wahoo", @@ }, ]); expect(execute).not.toHaveBeenCalled(); + expect(query).toHaveBeenCalledWith( + expect.anything(), + expect.stringContaining("FROM analytics.provider_stats"), + { userId: "user-1" }, + ); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server/src/routers/sync.test.ts` around lines 1163 - 1209, The test currently verifies mapping but not that the ClickHouse query is scoped to the current user; update the "maps ClickHouse rows to provider stats" test to assert that sensorStore.query was invoked with the current user parameter (e.g. userId "user-1") so the per-user filter is preserved — locate the caller constructed by createCaller and add an assertion that sensorStore.query (the mock passed into caller) was called with an argument object containing the userId (or equivalent key used by providerStats) to ensure providerStats enforces user scoping.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/server/src/routers/sync.test.ts`:
- Around line 1163-1209: The test currently verifies mapping but not that the
ClickHouse query is scoped to the current user; update the "maps ClickHouse rows
to provider stats" test to assert that sensorStore.query was invoked with the
current user parameter (e.g. userId "user-1") so the per-user filter is
preserved — locate the caller constructed by createCaller and add an assertion
that sensorStore.query (the mock passed into caller) was called with an argument
object containing the userId (or equivalent key used by providerStats) to ensure
providerStats enforces user scoping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 81170ce1-7ef8-476c-b68b-eb64e5236208
📒 Files selected for processing (4)
packages/server/src/repositories/sync-repository.tspackages/server/src/routers/sync.test.tspackages/server/src/routers/sync.tssrc/db/clickhouse-read-models.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/server/src/routers/sync.ts
This was referenced May 7, 2026
This was referenced May 31, 2026
This was referenced Jun 18, 2026
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
sync.providerStatsentirely onto ClickHouseanalytics.provider_stats0008_complete_provider_stats_raw_mirrorsand update docs/incident notesRoot Cause
Production Postgres pool starvation made cheap auth/session queries fail while expensive provider-stat and dashboard reads held pool clients. The provider stats endpoint was still doing per-provider Postgres count subqueries over large raw tables.
Validation
pnpm lintpnpm tsc --noEmitcd packages/server && pnpm tsc --noEmitcd packages/web && pnpm tsc --noEmitpnpm test run packages/server/src/routers/router-sql.integration.test.ts --project integrationpnpm test:changedSummary by CodeRabbit
Documentation
Performance Improvements
Tests