Add analytics page telemetry + QA data correctness runbook#244
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds PostHog event telemetry instrumentation to the Analytics page. It introduces four new analytics events (page viewed, date range changed, refresh clicked, source error shown) to the event taxonomy, implements event emission in the frontend controller with deduplication logic, and provides QA documentation and test coverage for the new telemetry. Changes
Sequence DiagramsequenceDiagram
participant User
participant Controller as analytics_dashboard_controller.js
participant API as Analytics API
participant PostHog
User->>Controller: Load analytics page
Controller->>Controller: Initialize state & compute default preset
Controller->>PostHog: Emit analytics_page_viewed
Controller->>API: load() with triggerSource
API-->>Controller: API response
User->>Controller: Change date range (preset or custom)
Controller->>Controller: Check if range fingerprint changed
Controller->>PostHog: Emit analytics_date_range_changed (if changed)
Controller->>API: load() with triggerSource
User->>Controller: Click refresh
Controller->>PostHog: Emit analytics_refresh_clicked
Controller->>API: load() with triggerSource
API-->>Controller: Error or source health data
alt API Error or Source Error
Controller->>Controller: Attach telemetry metadata to error
Controller->>PostHog: Emit analytics_source_error_shown (deduplicated per fingerprint)
Controller->>Controller: renderError() with telemetry context
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Greptile SummaryThis PR instruments four PostHog telemetry events on the Analytics page ( Key issues found:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Controller as analytics_dashboard_controller.js
participant PostHog as window.posthog
participant API as /api/projects/.../analytics/aggregation
User->>Controller: connect() [page load]
Controller->>PostHog: analytics_page_viewed {project_id, date_range_start, date_range_end, range_days}
Controller->>API: fetch(start_date, end_date)
API-->>Controller: payload or error
alt API success
Controller->>Controller: render(payload) → renderSourceHealth(rows)
loop Each source row with last_error (deduplicated by fingerprint)
Controller->>PostHog: analytics_source_error_shown {project_id, source, source_status, error_message, result_status}
end
else API failure
Controller->>PostHog: analytics_source_error_shown {project_id, source=dashboard_api, source_status, error_message, result_status}
end
User->>Controller: applyPreset(event)
Controller->>PostHog: analytics_date_range_changed {project_id, date_range_start, date_range_end, range_days, change_source=preset_click}
Controller->>API: fetch(start_date, end_date)
User->>Controller: refresh(event)
Controller->>PostHog: analytics_refresh_clicked {project_id, date_range_start, date_range_end, range_days}
Note over Controller,PostHog: analytics_date_range_changed fires here only if range changed
Controller->>PostHog: analytics_date_range_changed {change_source=custom_date_refresh}
Controller->>API: fetch(start_date, end_date)
Last reviewed commit: "Add analytics page t..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| rangeDays(startDate, endDate) { | ||
| if (!startDate || !endDate) { | ||
| return null; | ||
| } | ||
|
|
||
| const start = new Date(startDate); | ||
| const end = new Date(endDate); | ||
| if (Number.isNaN(start.getTime()) || Number.isNaN(end.getTime())) { | ||
| return null; | ||
| } | ||
|
|
||
| return Math.floor((end.getTime() - start.getTime()) / DAY) + 1; | ||
| } |
There was a problem hiding this comment.
rangeDays can return 0 or a negative value
When endDate is earlier than startDate, Math.floor((end - start) / DAY) + 1 yields 0 or a negative integer. This value propagates into every telemetry event that spreads currentRangeTelemetry() (e.g. analytics_page_viewed, analytics_refresh_clicked, analytics_date_range_changed), quietly tainting PostHog with invalid range_days data.
While the API call would also fail for an inverted range, the telemetry fires before the API response, so bad events would be recorded. Consider clamping or nullifying the return value:
| rangeDays(startDate, endDate) { | |
| if (!startDate || !endDate) { | |
| return null; | |
| } | |
| const start = new Date(startDate); | |
| const end = new Date(endDate); | |
| if (Number.isNaN(start.getTime()) || Number.isNaN(end.getTime())) { | |
| return null; | |
| } | |
| return Math.floor((end.getTime() - start.getTime()) / DAY) + 1; | |
| } | |
| rangeDays(startDate, endDate) { | |
| if (!startDate || !endDate) { | |
| return null; | |
| } | |
| const start = new Date(startDate); | |
| const end = new Date(endDate); | |
| if (Number.isNaN(start.getTime()) || Number.isNaN(end.getTime())) { | |
| return null; | |
| } | |
| const days = Math.floor((end.getTime() - start.getTime()) / DAY) + 1; | |
| return days > 0 ? days : null; | |
| } |
| async refresh(event) { | ||
| if (event) { | ||
| event.preventDefault(); | ||
| } | ||
| this.setPresetUi("custom"); | ||
| await this.load(); | ||
|
|
||
| this.captureEvent("analytics_refresh_clicked", { | ||
| ...this.currentRangeTelemetry(), | ||
| trigger_source: "refresh_button", | ||
| }); | ||
|
|
||
| this.captureDateRangeChangedIfNeeded({ | ||
| change_source: "custom_date_refresh", | ||
| }); | ||
|
|
||
| await this.load({ triggerSource: "refresh_button" }); |
There was a problem hiding this comment.
analytics_refresh_clicked fires before analytics_date_range_changed
In the refresh() flow, analytics_refresh_clicked is emitted first (line 89), then captureDateRangeChangedIfNeeded emits analytics_date_range_changed second (line 94). The taxonomy description for analytics_date_range_changed says it fires "before a new query load", and technically both events do fire before load(), but the resulting event stream in PostHog will show refresh_clicked → date_range_changed — the reverse of the conceptual order (range changes first, then refresh is triggered).
For cleaner funnel sequencing in analytics, consider emitting date_range_changed before refresh_clicked:
| async refresh(event) { | |
| if (event) { | |
| event.preventDefault(); | |
| } | |
| this.setPresetUi("custom"); | |
| await this.load(); | |
| this.captureEvent("analytics_refresh_clicked", { | |
| ...this.currentRangeTelemetry(), | |
| trigger_source: "refresh_button", | |
| }); | |
| this.captureDateRangeChangedIfNeeded({ | |
| change_source: "custom_date_refresh", | |
| }); | |
| await this.load({ triggerSource: "refresh_button" }); | |
| async refresh(event) { | |
| if (event) { | |
| event.preventDefault(); | |
| } | |
| this.setPresetUi("custom"); | |
| this.captureDateRangeChangedIfNeeded({ | |
| change_source: "custom_date_refresh", | |
| }); | |
| this.captureEvent("analytics_refresh_clicked", { | |
| ...this.currentRangeTelemetry(), | |
| trigger_source: "refresh_button", | |
| }); | |
| await this.load({ triggerSource: "refresh_button" }); | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| "analytics_source_error_shown": { | ||
| "stage": "risk", | ||
| "description": "Analytics page surfaced a provider or API error to the user.", | ||
| "required_properties": [ | ||
| "project_id", | ||
| "source", | ||
| "error_message", | ||
| "result_status" | ||
| ] | ||
| } |
There was a problem hiding this comment.
source_status sent by controller but absent from taxonomy required_properties
The controller sends source_status in both code paths that fire analytics_source_error_shown:
renderError(...)— passessource_status(e.g., HTTP status code or"fetch_failed")renderSourceHealth(...)— passessource_status(mapped fromrow.status)
The production runbook in docs/analytics-page-data-correctness-checklist.md also explicitly lists source_status as a field to inspect when troubleshooting repeated error toasts. It is therefore a de-facto required diagnostic property, but it is not listed in required_properties here. This creates a gap between what the event actually carries and what the taxonomy documents.
Consider adding it:
| "analytics_source_error_shown": { | |
| "stage": "risk", | |
| "description": "Analytics page surfaced a provider or API error to the user.", | |
| "required_properties": [ | |
| "project_id", | |
| "source", | |
| "error_message", | |
| "result_status" | |
| ] | |
| } | |
| "analytics_source_error_shown": { | |
| "stage": "risk", | |
| "description": "Analytics page surfaced a provider or API error to the user.", | |
| "required_properties": [ | |
| "project_id", | |
| "source", | |
| "source_status", | |
| "error_message", | |
| "result_status" | |
| ] | |
| } |
| from pathlib import Path | ||
|
|
||
|
|
||
| def test_analytics_dashboard_controller_tracks_v1_analytics_events(): | ||
| controller_path = Path(__file__).resolve().parents[2] / "frontend" / "src" / "controllers" / "analytics_dashboard_controller.js" | ||
| source = controller_path.read_text(encoding="utf-8") | ||
|
|
||
| assert "window.posthog.capture" in source | ||
| assert '"analytics_page_viewed"' in source | ||
| assert '"analytics_date_range_changed"' in source | ||
| assert '"analytics_refresh_clicked"' in source | ||
| assert '"analytics_source_error_shown"' in source | ||
| assert "project_id" in source | ||
| assert "date_range_start" in source | ||
| assert "date_range_end" in source | ||
| assert "range_days" in source |
There was a problem hiding this comment.
The test verifies that event-name strings and a few property names appear somewhere in the controller source file. This means it would pass even if those strings existed in comments, and it would not catch:
analytics_date_range_changedfiring when the date range has not changed (fingerprint logic regression)analytics_source_error_shownfiring multiple times for the same error in a single render cycle (deduplication regression)analytics_page_viewedfiring more than once perconnect()call
Because these are the behaviours that matter for PostHog data quality (the whole point of this PR), consider adding at least one JS unit test (e.g. with Vitest/Jest) that mounts the controller with a stub window.posthog and exercises the fingerprint guard and deduplication logic. The current approach gives a false sense of coverage while leaving the actual runtime behaviour untested.
|
Addressed Greptile feedback in follow-up commit 87369f7:
|
1 participant
Summary
analytics_dashboard_controller.jsfor:analytics_page_viewedanalytics_date_range_changedanalytics_refresh_clickedanalytics_source_error_showncore/analytics/event_taxonomy.json) with required propertiesdocs/analytics-page-data-correctness-checklist.md)Testing
./.venv/bin/pytest -q core/tests/test_posthog_event_coverage.py core/tests/test_analytics_dashboard_telemetry.py core/tests/test_analytics_events.py./.venv/bin/pytest -q core/tests/test_project_analytics_view.py core/tests/test_project_analytics_api.py(blocked in this runtime: Postgres hostdbnot resolvable)Summary by CodeRabbit
New Features
Documentation
Tests