Retire Postgres metric stream (P1): heart-rate reads from ClickHouse - #1268
Merged
Merged
Conversation
Sequenced plan (P0 rename CH table -> P1 migrate scalar readers to ClickHouse -> P2 IMU debug pages to R2 coverage -> P3 drop fitness.metric_stream + retire PeerDB mirror + delete backfill script). Reader inventory, naming decision, and per-phase steps + validation gates. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Move the heart-rate router off `fitness.metric_stream` (Postgres, now frozen) onto the Redpanda-fed ClickHouse mirror. Extract the query into a HeartRateRepository (server convention: no raw SQL in routers) that reads via the ActivitySensorStore `query()` escape hatch. Rows are version-deduplicated (`FINAL` + `_peerdb_is_deleted = 0`) but NOT collapsed by provider priority, so every source is still returned for the per-source overlay. Day window uses `toDate(recorded_at, timezone)`; minute bins formatted to ISO-8601 Z to preserve the existing client contract. Tests: repository unit test (grouping), router unit test (sensorStore wiring + empty-when-unavailable), and a ClickHouse integration test asserting per-source separation, version dedup (v1 supersedes v0), and exclusion of deleted / other-day / zero / other-channel rows. Part of docs/metric-stream-postgres-retirement.md (P1). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Contributor
Reviewer's GuideMigrates the heart-rate dailyBySource reader from Postgres fitness.metric_stream to the Redpanda-fed ClickHouse mirror via a new HeartRateRepository, adds ClickHouse-backed integration coverage, and documents the full Postgres metric_stream retirement plan. Sequence diagram for heartRateRouter dailyBySource using ClickHousesequenceDiagram
actor User
participant HeartRateRouter
participant HeartRateRepository
participant ActivitySensorStore as MetricStreamClickHouseReader
participant ClickHouseMetricStream as postgres_fitness_metric_stream
User->>HeartRateRouter: dailyBySource({ date })
alt sensorStore unavailable
HeartRateRouter-->>User: []
else sensorStore available
HeartRateRouter->>HeartRateRepository: new HeartRateRepository(sensorStore, userId, timezone)
HeartRateRouter->>HeartRateRepository: dailyBySource(date)
HeartRateRepository->>ActivitySensorStore: query(heartRateRowSchema, SELECT ... FROM postgres_fitness.metric_stream ..., params)
ActivitySensorStore->>ClickHouseMetricStream: execute SELECT
ClickHouseMetricStream-->>ActivitySensorStore: rows
ActivitySensorStore-->>HeartRateRepository: parsed rows
HeartRateRepository-->>HeartRateRouter: HeartRateSourceSeries[]
HeartRateRouter-->>User: HeartRateSourceSeries[]
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughThis PR migrates the heart-rate scalar reader endpoint from inline Postgres/Drizzle SQL to a ClickHouse-backed repository pattern, adds a runbook for Postgres ChangesHeart-rate reader ClickHouse migration
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant HeartRateRepository
participant ClickHouse
Client->>Router: POST /heart-rate.dailyBySource {date}
Router->>HeartRateRepository: dailyBySource(date, userId, timezone)
HeartRateRepository->>ClickHouse: query(metric_stream.events, params)
ClickHouse-->>HeartRateRepository: rows
HeartRateRepository-->>Router: HeartRateSourceSeries[]
Router-->>Client: 200 OK {series}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
Storybook previews for This comment updates automatically on each PR push. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="packages/server/src/repositories/heart-rate-repository.ts" line_range="54" />
<code_context>
+ heartRateRowSchema,
+ `SELECT
+ provider_id,
+ formatDateTime(minute_bucket, '%Y-%m-%dT%H:%i:%SZ') AS recorded_at,
+ toInt32(round(avg(scalar))) AS heart_rate
+ FROM (
</code_context>
<issue_to_address>
**issue (bug_risk):** The recorded_at format omits milliseconds but tests and schemas expect millisecond precision.
Here `recorded_at` is formatted without milliseconds, but tests and `timestampStringSchema` expect millisecond precision (e.g. `2026-04-12T10:00:00.000Z`). This mismatch will cause test failures and potential client parsing differences. Please switch to a millisecond-precise format (e.g. `'%Y-%m-%dT%H:%i:%s.000Z'`) or use a native `DateTime64` and let the client/schema handle ISO stringification consistently.
</issue_to_address>
### Comment 2
<location path="docs/metric-stream-postgres-retirement.md" line_range="102" />
<code_context>
+
+## P3 — Drop Postgres metric_stream + cleanup
+
+Only after P0–P2 merged + verified, and the historical backfill is complete.
+
+1. Retire PeerDB metric_stream CDC mirror (so CH is fed only by Redpanda).
</code_context>
<issue_to_address>
**suggestion (typo):** Slightly awkward grammar; consider adding a verb after “P0–P2”.
For example: `Only after P0–P2 are merged and verified, and the historical backfill is complete.`
```suggestion
Only after P0–P2 are merged and verified, and the historical backfill is complete.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/repositories/heart-rate-repository.integration.test.ts`:
- Around line 97-106: The test expectations include timestamps with millisecond
precision (".000") whereas the repository's formatDateTime function emits
timestamps without milliseconds, causing failures; either update formatDateTime
to include milliseconds (e.g., adjust its format string to produce ".SSS") or
remove the ".000" from the expected timestamps in the test (see the whoop_ble
expectation array and formatDateTime) so both formats match.
In `@packages/server/src/repositories/heart-rate-repository.ts`:
- Line 54: The SQL timestamp formatting in the query uses
formatDateTime(minute_bucket, '%Y-%m-%dT%H:%i:%SZ') AS recorded_at which omits
milliseconds and conflicts with tests that expect a '.000Z' suffix; update that
format string to include fractional seconds (e.g., '%Y-%m-%dT%H:%i:%S.%fZ') so
recorded_at includes milliseconds, or alternatively update the integration test
expectations to remove the '.000' if milliseconds are not desired.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ef9a725e-3c75-43aa-8b60-fd8307df2399
📒 Files selected for processing (6)
docs/metric-stream-postgres-retirement.mdpackages/server/src/repositories/heart-rate-repository.integration.test.tspackages/server/src/repositories/heart-rate-repository.test.tspackages/server/src/repositories/heart-rate-repository.tspackages/server/src/routers/heart-rate.test.tspackages/server/src/routers/heart-rate.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 6 files
Confidence score: 3/5
- There is a concrete correctness risk in
packages/server/src/routers/heart-rate.ts: treating a missing ClickHouse store as empty data can mask infrastructure failures and return misleading heart-rate results to users. packages/server/src/repositories/heart-rate-repository.tsusestoDate(recorded_at)in the day filter, which can prevent key range pruning and cause heavier daily scans, creating noticeable performance risk under load.- Given a medium-severity user-facing behavior issue plus a query-efficiency concern, this carries some merge risk and is worth tightening before release.
- Pay close attention to
packages/server/src/routers/heart-rate.tsandpackages/server/src/repositories/heart-rate-repository.ts- avoid silent fallback on store failures and restore efficient date-range filtering.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Auto-approval blocked by 2 unresolved issues from previous reviews.
Re-trigger cubic
- Hardcode `.000` in the recorded_at format (`%Y-%m-%dT%H:%i:%S.000Z`) so the ISO-8601 contract is explicit and version-independent (minute buckets always have zero sub-seconds). Resolves the format/expectation ambiguity flagged by sourcery + coderabbit. - Replace `toDate(recorded_at, tz) = date` with a raw-column range (`recorded_at >= toDateTime(date, tz) AND < + INTERVAL 1 DAY`) so the ORDER BY key can prune instead of scanning (cubic perf finding). - Docs grammar fix in the retirement plan. Integration + unit tests still green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
CR responses (
|
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Auto-approved: The PR migrates the heart-rate dailyBySource reader from Postgres to ClickHouse with a well-structured repository, comprehensive tests (unit, router wiring, and real-ClickHouse integration), and the same API contract, making it low-risk and safe to auto-approve.
Re-trigger cubic
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: This PR migrates heart-rate reads from Postgres to ClickHouse, a significant refactor of core fitness data retrieval that requires careful human review to verify query correctness, dedup logic, and client contract preservation.
Re-trigger cubic
…nal deps Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Migrating heart-rate dailyBySource from Postgres to ClickHouse changes a core production data path with potential for data freshness and correctness issues, requiring human review of the SQL, deduplication logic, and timezone handling.
Re-trigger cubic
… branch Per the new "no test/optional-only branches" rule: the heart-rate router's `if (!ctx.sensorStore) return []` guard was dead in production (createApp always constructs the sensor store) and only reachable in tests, where it silently returned empty data instead of surfacing a real ClickHouse failure. - `Context.sensorStore` and `createApp`'s `sensorStore` param are now required (non-optional). createApp always wraps it in LimitedActivitySensorStore. - heart-rate router calls the repository directly with `ctx.sensorStore`; guard removed, and the "returns [] when unavailable" test deleted. - Integration-test callers that previously omitted the store (whoop-auth, food, settings, health-kit-sync) now pass `makeMockSensorStore()`. A real ClickHouse failure now throws from `query()` and is handled by the tRPC error path + reported to Sentry, instead of being masked as empty data. (The ~14 other routers still carry the same guard; they're now dead branches on a required field and will be removed in a follow-up sweep.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 8 files (changes from recent commits).
Auto-approved: This PR migrates the heart-rate dailyBySource reader from Postgres to ClickHouse with a clean repository pattern, comprehensive tests (unit, router wiring, real-ClickHouse integration), and no changes to the client API contract, while also hardening the dependency by making the sensor store...
Re-trigger cubic
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.
First step of retiring
fitness.metric_streamfrom Postgres. Full sequenced plan indocs/metric-stream-postgres-retirement.md(P0 rename CH table → P1 migrate scalar readers → P2 IMU debug to R2 coverage → P3 drop PG table + retire PeerDB mirror + delete backfill script).Context
Writers already publish only to Redpanda;
fitness.metric_streamis frozen. Readers still on Postgres serve stale data. This migrates them to the Redpanda-fed ClickHouse mirror.This PR
dailyBySource→ ClickHouse:HeartRateRepository(server convention: no raw SQL in routers), reads via theActivitySensorStore.query()escape hatch.FINAL+_peerdb_is_deleted = 0), notdeduped_sensor— preserves per-source rows for the overlay (priority-dedup would collapse sources).toDate(recorded_at, timezone); minute bins formatted to ISO-8601 Z to keep the client contract.Remaining readers (provider-detail, spo2/skin-temp), the
v_metric_streamdrop, P2 (IMU), P0 (rename), and P3 (drop table) follow per the doc.🤖 Generated with Claude Code
Summary by Sourcery
Migrate the heart-rate dailyBySource API from Postgres metric_stream to the ClickHouse-backed metric stream mirror and document the full Postgres metric_stream retirement plan.
Enhancements:
Documentation:
Tests:
Summary by cubic
Serve heart-rate dailyBySource from the Redpanda-fed ClickHouse mirror instead of Postgres to restore freshness and kick off Postgres
metric_streamretirement. Client contract stays the same; per-source overlays are preserved.docs/metric-stream-postgres-retirement.md; updatedAGENTS.mdto ban test/optional-only branches and require non-optional deps.HeartRateRepositoryreading ClickHouse viaActivitySensorStore.query(); removed raw SQL from the router.postgres_fitness.metric_streamwithFINALand_peerdb_is_deleted = 0, filtersscalar > 0, bins to 1 minute with ISO.000Ztimestamps; day window is[toDateTime(date, tz), +1 day)for ORDER BY pruning; no provider-priority collapse.ctx.sensorStore(no guard);createAppand tRPCContextnow require a sensor store; failures bubble via tRPC/Sentry.makeMockSensorStore().Written for commit 1f785bf. Summary will update on new commits.
Summary by CodeRabbit
Documentation
Refactor
Tests