Convert analytics read models to bounded microbatches - #1185
Conversation
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? |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Automated Checks (advisory, non-blocking)✅ All checks passed. Surmado Code Review — Free tier limit reachedYou've used all 10 free reviews this month. Deterministic checks (secrets, model strings) still ran above. Upgrade to the Paid plan for 100 reviews/month + $15 per additional 100: https://app.surmado.com/checkout?plan=pr_review_starter Or wait until your next monthly window for 10 more free reviews. Surmado Code Review (v1.2-mt) |
📝 WalkthroughWalkthroughThis PR introduces a bounded microbatch architecture for analytics read models: three new incremental models bound sensor and location data to sleep/activity windows, two summary aggregation models compute per-activity metrics from those bounded inputs, two downstream models refactor to consume the precomputed summaries instead of raw streams, the safe model set expands to include all new models, and dependency-order validation tests are added. ChangesAnalytics Microbatch Architecture Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
|
Storybook previews for This comment updates automatically on each PR push. |
There was a problem hiding this comment.
Pull request overview
This PR converts ClickHouse analytics read models to bounded microbatch intermediates and chained aggregates so scheduled production dbt builds stay reliable and fast, and re-enables previously excluded models in the production safe-model selection.
Changes:
- Added bounded microbatch intermediates for sleep heart-rate, activity sensor samples, and activity location samples.
- Added compact per-activity aggregate intermediates and rewired
activity_summary_rows+resting_heart_rate_sleep_windowto read from bounded inputs. - Expanded
DBT_SAFE_MODELS, added SQL-structure tests, and updated analytics/ops documentation.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Updates architecture overview to reflect bounded microbatches and chained aggregates. |
| entrypoint.sh | Expands DBT_SAFE_MODELS to include new intermediates and re-enabled models. |
| docs/production-incident-baseline.md | Documents the “resting heart rate chart tail stale” incident and mitigation plan. |
| analytics/README.md | Updates analytics build docs to describe new bounded microbatch pipeline. |
| analytics/models/read_models/sleep_heart_rate_sample.sql | New microbatch intermediary for sleep↔heart-rate membership. |
| analytics/models/read_models/resting_heart_rate_sleep_window.sql | Reworks RHR aggregation to consume bounded sleep intermediary and limits threads. |
| analytics/models/read_models/read_model_microbatch.sql.test.ts | Adds Vitest guardrails to enforce safe-model ordering and bounded-dependency boundaries. |
| analytics/models/read_models/activity_sensor_sample.sql | New microbatch intermediary for activity↔sensor sample membership. |
| analytics/models/read_models/activity_location_sample.sql | New microbatch intermediary for activity↔location sample membership + best-source selection. |
| analytics/models/read_models/activity_sensor_summary_rows.sql | New per-activity sensor aggregate intermediate. |
| analytics/models/read_models/activity_location_summary_rows.sql | New per-activity location aggregate intermediate. |
| analytics/models/read_models/activity_summary_rows.sql | Rewires summary model to join compact intermediates instead of raw streams. |
💡 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: 5
🤖 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 `@analytics/models/read_models/activity_location_sample.sql`:
- Around line 18-24: The CTE activity_members references the hardcoded
analytics.v_activity_members which breaks dbt lineage; replace that direct
schema-qualified reference with a dbt macro call—use {{
ref('v_activity_members') }} if v_activity_members is a model in this project,
or {{ source('your_source_name','v_activity_members') }} if it’s an external
source defined in sources.yml—update the SELECT in the activity_members CTE to
use the appropriate ref/source call so dbt can track the dependency.
- Around line 46-61: In provider_counts, the window ORDER BY uses count()
directly which is less clear—use the computed alias sample_count instead; update
the row_number() OVER (PARTITION BY activity_members.activity_id ORDER BY
sample_count DESC, location_rows.provider_id ASC) expression so it references
sample_count (and keep the GROUP BY and count() aggregation as-is) to make the
intent in provider_counts / row_number clearer and avoid repeating the
aggregate.
In `@analytics/models/read_models/activity_sensor_sample.sql`:
- Around line 18-25: The CTE current_activity references the hardcoded
analytics.v_activity which breaks dbt lineage; replace that raw table reference
with the appropriate dbt call—use {{ ref('v_activity') }} if v_activity is a dbt
model (consistent with the existing {{ ref('deduped_sensor') }} usage) or use {{
source('analytics', 'v_activity') }} and add a sources.yml entry if it’s an
external/source table—then run dbt compile to ensure lineage is picked up.
In `@analytics/models/read_models/activity_summary_rows.sql`:
- Around line 180-188: The sensor_summary and location_summary CTEs currently
select all rows from {{ ref('activity_sensor_summary_rows') }} and {{
ref('activity_location_summary_rows') }} (using FINAL) and thus include
soft-deleted rows; update the sensor_summary and location_summary CTEs to
explicitly filter WHERE is_deleted = 0 so they only expose non-deleted summary
rows (this keeps the LEFT JOIN behavior intact but makes the intent explicit and
avoids joining to soft-deleted summary records).
In `@analytics/models/read_models/sleep_heart_rate_sample.sql`:
- Around line 25-31: Extract the duplicated multiIf nap-classification into a
dbt macro (e.g., macro name is_nap(sleep_type, duration_minutes)) that returns
the boolean expression currently used; replace the inline multiIf in
sleep_heart_rate_sample.sql (the expression labeled AS is_nap) and the identical
block in resting_heart_rate_sleep_window.sql with a call to that macro so both
models call is_nap(sleep_type, duration_minutes) instead of duplicating the
logic; ensure the macro uses the same precedence and coalesce logic (handling
null duration_minutes) so results remain identical.
🪄 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: e8c4f7d6-9e44-4070-b218-afb103a70d00
📒 Files selected for processing (12)
README.mdanalytics/README.mdanalytics/models/read_models/activity_location_sample.sqlanalytics/models/read_models/activity_location_summary_rows.sqlanalytics/models/read_models/activity_sensor_sample.sqlanalytics/models/read_models/activity_sensor_summary_rows.sqlanalytics/models/read_models/activity_summary_rows.sqlanalytics/models/read_models/read_model_microbatch.sql.test.tsanalytics/models/read_models/resting_heart_rate_sleep_window.sqlanalytics/models/read_models/sleep_heart_rate_sample.sqldocs/production-incident-baseline.mdentrypoint.sh
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 12 files
Confidence score: 2/5
- High-risk merge at the moment: the issue in
analytics/models/read_models/activity_location_sample.sql(8/10, high confidence) can drop output rows whenprovider_countsis empty, preventing tombstoning of previously materialized location samples after deletions. analytics/models/read_models/activity_summary_rows.sqlhas a NULL-handling regression risk (missingcoalesceon elevation fields) when the LEFT JOIN tosensor_summaryhas no match, which can change downstream behavior unexpectedly.analytics/models/read_models/activity_location_summary_rows.sqlmay repeatedly emit tombstones for already-deleted keys unlessexisting_summaryis filtered to non-deleted rows before derivingstale_dirty_keys.- Pay close attention to
analytics/models/read_models/activity_location_sample.sql,analytics/models/read_models/activity_summary_rows.sql,analytics/models/read_models/activity_location_summary_rows.sql- deletion/tombstone semantics and NULL defaults need correction to avoid persistent data quality issues.
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 8 files (changes from recent commits).
Requires human review: This PR refactors core analytics dbt models, adds new microbatch intermediates, and changes the production build set, which carries moderate risk of data staleness or incorrect aggregates and requires human review of the business logic and dependency changes.
Re-trigger cubic
Summary
Verification
Summary by cubic
Converted analytics read models to bounded microbatches and added compact activity aggregates to make builds reliable and fast. Re-enables scheduled resting heart rate and activity summaries in production.
New Features
sleep_heart_rate_sample,activity_sensor_sample,activity_location_sample(daily batches, short lookbacks).activity_sensor_summary_rows,activity_location_summary_rows(location picks the best provider per activity).DBT_SAFE_MODELSto include the above plusresting_heart_rate_sleep_windowandactivity_summary_rows.Refactors
activity_summary_rowsnow joins the aggregate intermediates instead of raw streams; preserves elevation defaults and usesmax_threads=1for offline builds.resting_heart_rate_sleep_windownow aggregates fromsleep_heart_rate_sampleand limits threads.analyticssources (v_activity,v_activity_members), and the production incident baseline.Written for commit 76d7387. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
Release Notes
Bug Fixes
Documentation
Tests