fix: Inflated live-visitors chart - #601
Merged
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAnalytics now assigns generated session IDs ( ChangesSID-based analytics sessions
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Blaumaus
marked this pull request as ready for review
August 2, 2026 03:34
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/apps/cloud/src/analytics/analytics.service.ts (1)
5065-5091: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
duration_avgselectssidbut still groups bypsidin four session-summary queries. The migration changed the selected column of the inner duration subquery frompsidtosidand leftGROUP BY psidunchanged. ClickHouse rejectssidbecause it is neither an aggregate nor a grouping key, and the query fails withNOT_AN_AGGREGATE.getAnalyticsSummarythen returns a 500 for every non-allperiod in both applications.
backend/apps/cloud/src/analytics/analytics.service.ts#L5065-L5091: changeGROUP BY psidon line 5091 toGROUP BY sid.backend/apps/cloud/src/analytics/analytics.service.ts#L5130-L5156: changeGROUP BY psidon line 5156 toGROUP BY sid.backend/apps/community/src/analytics/analytics.service.ts#L3641-L3667: changeGROUP BY psidon line 3667 toGROUP BY sid.backend/apps/community/src/analytics/analytics.service.ts#L3706-L3732: changeGROUP BY psidon line 3732 toGROUP BY sid.Note that the outer
psid IN (...)predicate still scopes the rows to the filtered visitors, so grouping bysidkeeps the intended scope and produces one duration per session.🐛 Proposed fix (same shape at all four sites)
duration_avg AS ( SELECT avgOrNull(duration) as sdur FROM ( SELECT sid, dateDiff('second', min(firstSeen), max(lastSeen)) as duration FROM sessions WHERE pid = {pid:FixedString(12)} AND psid IN ( ... ) - GROUP BY psid + GROUP BY sid ) ),🤖 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 `@backend/apps/cloud/src/analytics/analytics.service.ts` around lines 5065 - 5091, Update the duration_avg inner subquery in getAnalyticsSummary so its GROUP BY uses the selected session identifier sid instead of psid. Apply this change at backend/apps/cloud/src/analytics/analytics.service.ts lines 5065-5091 and 5130-5156, and backend/apps/community/src/analytics/analytics.service.ts lines 3641-3667 and 3706-3732; leave the outer psid IN filtering unchanged.
🧹 Nitpick comments (3)
backend/migrations/clickhouse/2026_08_01_session_id.js (1)
30-30: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a failure handler for the migration promise.
queriesRunner(queries)returns a promise.queriesRunnerrethrows query errors. Without a.catch, a failure produces an unhandled rejection and the process may exit with code 0, which hides the failure from the deploy pipeline.♻️ Proposed change
-queriesRunner(queries) +queriesRunner(queries).catch((error) => { + console.error(error) + process.exit(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 `@backend/migrations/clickhouse/2026_08_01_session_id.js` at line 30, Update the migration invocation of queriesRunner(queries) to handle its rejected promise with a failure handler that reports the error and exits with a nonzero status, ensuring query failures are visible to the deploy pipeline.backend/apps/cloud/src/analytics/analytics.service.ts (1)
4151-4168: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
avg_duration, because the expression is a sum.
sum(session_duration) as avg_durationreturns the total time of all SIDs for the PSID. The alias says average. The alias then flows to thesduroutput column. Rename it tototal_durationin both CTEs to keep the query readable.Also applies to: 4319-4336
🤖 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 `@backend/apps/cloud/src/analytics/analytics.service.ts` around lines 4151 - 4168, Rename the sum-duration alias from avg_duration to total_duration in both affected CTEs, including the query section around session_duration and the corresponding earlier section, and update any downstream references that expose it as sdur. Preserve the existing sum expression and output behavior.backend/apps/community/src/analytics/analytics.service.ts (1)
2485-2528: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the class logger instead of
console.error.
AnalyticsServicedeclaresprivate readonly logger = new Logger(AnalyticsService.name)at line 516 and usesthis.logger.errorelsewhere in this file. Line 2526 writes toconsole.error, so the failure loses the Nest context and structured formatting.♻️ Proposed change
} catch (error) { - console.error('Failed to record session:', error) + this.logger.error(`[recordSessionActivity] Failed to record session: ${error}`) }🤖 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 `@backend/apps/community/src/analytics/analytics.service.ts` around lines 2485 - 2528, In AnalyticsService.recordSessionActivity, replace the console.error call in the catch block with this.logger.error, reusing the class logger declared on AnalyticsService and preserving the existing failure message and error details.
🤖 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 `@backend/apps/cloud/src/analytics/analytics.service.ts`:
- Around line 2928-2935: Update getSessionDurationFromClickHouse to calculate
duration for the replayed session rather than aggregating all sessions with
max(duration). Pass the replay sid into this query and filter the grouped
session data by that sid, preserving the existing fallback behavior in
getSessionReplaySummary.
- Around line 6633-6634: Restore the concurrency range guard in
shouldComputeConcurrency so concurrency is only computed for bounded ranges, or
only for hour-and-finer time buckets, rather than period=all. Apply the
identical guard at
backend/apps/cloud/src/analytics/analytics.service.ts:6633-6634 and
backend/apps/community/src/analytics/analytics.service.ts:5170-5171 to preserve
consistent semantics.
- Line 4905: Update the period='all' duration_avg CTE to group and calculate
durations by the same coalesced session identifier as the unique count, using
coalesce(sid, psid) instead of psid throughout its grouping and session-duration
aggregation. Preserve the existing duration_avg output and averaging behavior
for other periods.
In `@backend/apps/cloud/src/analytics/v2/registry/metrics.ts`:
- Around line 29-38: Update the sessions metric’s sqlExpr to always count
distinct sessions using coalesce(sid, psid), removing the customEVFilterApplied
branch and its count(*) path. Preserve the existing integer format and session
description.
In `@backend/apps/community/src/analytics/analytics.controller.ts`:
- Line 1620: Update the Community performance eventTransformer call in
backend/apps/community/src/analytics/analytics.controller.ts:1620-1620 to pass
psid, sid, and profileId. Update the explicit performance payload branch in
backend/apps/community/src/analytics/utils/transformers.ts:123-123 to include
and preserve those three fields in the transformed event.
In `@backend/apps/community/src/analytics/analytics.service.ts`:
- Line 3485: Update the period = 'all' duration_avg CTE to select sid instead of
psid and group by sid, matching the unique session key used by the surrounding
aggregation and the other period paths.
In `@backend/migrations/clickhouse/2026_08_01_session_id.js`:
- Around line 21-27: Update the migration sequence around the INSERT backfill
and RENAME TABLE statements to add a catch-up INSERT ... SELECT from
sessions_by_visitor_day into the newly renamed sessions table immediately after
the rename. Reuse the same column mapping and validity filters, allowing
ReplacingMergeTree to deduplicate overlapping rows by (pid, sid).
In `@docs/content/docs/visitor-identification.mdx`:
- Around line 51-56: Update the session definition in the visitor-identification
documentation to state that a session starts on a visitor’s first tracked event,
rather than their first pageview; leave the remaining session duration and
salt-rotation behavior unchanged.
---
Outside diff comments:
In `@backend/apps/cloud/src/analytics/analytics.service.ts`:
- Around line 5065-5091: Update the duration_avg inner subquery in
getAnalyticsSummary so its GROUP BY uses the selected session identifier sid
instead of psid. Apply this change at
backend/apps/cloud/src/analytics/analytics.service.ts lines 5065-5091 and
5130-5156, and backend/apps/community/src/analytics/analytics.service.ts lines
3641-3667 and 3706-3732; leave the outer psid IN filtering unchanged.
---
Nitpick comments:
In `@backend/apps/cloud/src/analytics/analytics.service.ts`:
- Around line 4151-4168: Rename the sum-duration alias from avg_duration to
total_duration in both affected CTEs, including the query section around
session_duration and the corresponding earlier section, and update any
downstream references that expose it as sdur. Preserve the existing sum
expression and output behavior.
In `@backend/apps/community/src/analytics/analytics.service.ts`:
- Around line 2485-2528: In AnalyticsService.recordSessionActivity, replace the
console.error call in the catch block with this.logger.error, reusing the class
logger declared on AnalyticsService and preserving the existing failure message
and error details.
In `@backend/migrations/clickhouse/2026_08_01_session_id.js`:
- Line 30: Update the migration invocation of queriesRunner(queries) to handle
its rejected promise with a failure handler that reports the error and exits
with a nonzero status, ensuring query failures are visible to the deploy
pipeline.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9ca66370-556b-4585-b4e0-2adda11ec162
📒 Files selected for processing (17)
backend/apps/cloud/src/analytics/analytics.controller.tsbackend/apps/cloud/src/analytics/analytics.service.tsbackend/apps/cloud/src/analytics/heartbeat.gateway.tsbackend/apps/cloud/src/analytics/utils/transformers.tsbackend/apps/cloud/src/analytics/v2/analytics-v2.service.tsbackend/apps/cloud/src/analytics/v2/registry/metrics.tsbackend/apps/cloud/src/common/constants.tsbackend/apps/community/src/analytics/analytics.controller.tsbackend/apps/community/src/analytics/analytics.service.tsbackend/apps/community/src/analytics/heartbeat.gateway.tsbackend/apps/community/src/analytics/utils/transformers.tsbackend/apps/community/src/analytics/v2/analytics-v2.service.tsbackend/apps/community/src/analytics/v2/registry/metrics.tsbackend/apps/community/src/common/constants.tsbackend/migrations/clickhouse/2026_08_01_session_id.jsdocs/content/docs/api/stats-v2.mdxdocs/content/docs/visitor-identification.mdx
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 `@backend/migrations/clickhouse/2026_08_01_session_id.js`:
- Around line 27-40: The migration sequence around the sessions table rename and
post-rename replay must support retries after the rename succeeds but the replay
fails. Add resume handling that detects the already-renamed state and skips or
safely reconstructs the pre-rename setup before rerunning the replay INSERT,
ensuring retries do not fail on the existing sessions_v2 rename target and the
replay remains idempotent.
- Around line 33-40: Replace the fixed one-day cutoff in the migration’s replay
INSERT with a migration-start watermark captured before the initial backfill,
and filter replay rows using that timestamp so all sessions written during the
migration are included. Keep the existing validity filters and session column
mapping unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 541aafdf-a818-4e8f-9d58-038d89c03d2e
📒 Files selected for processing (9)
backend/apps/cloud/src/analytics/analytics.service.tsbackend/apps/cloud/src/analytics/v2/registry/metrics.tsbackend/apps/community/src/analytics/analytics.controller.tsbackend/apps/community/src/analytics/analytics.service.tsbackend/apps/community/src/analytics/utils/transformers.tsbackend/apps/community/src/analytics/v2/registry/metrics.tsbackend/migrations/clickhouse/2026_08_01_session_id.jsdocs/content/docs/api/stats-v2.mdxdocs/content/docs/visitor-identification.mdx
🚧 Files skipped from review as they are similar to previous changes (6)
- docs/content/docs/visitor-identification.mdx
- docs/content/docs/api/stats-v2.mdx
- backend/apps/community/src/analytics/analytics.controller.ts
- backend/apps/community/src/analytics/utils/transformers.ts
- backend/apps/community/src/analytics/analytics.service.ts
- backend/apps/cloud/src/analytics/analytics.service.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@backend/migrations/clickhouse/2026_08_01_session_id.js`:
- Line 32: Replace the multi-table RENAME TABLE statement with an atomic
active-table swap between sessions and sessions_v2, then archive the previous
sessions table in a separate rename. Add resume detection in this migration for
both the pre-swap and post-swap/crash states so reruns select the correct
remaining operation without renaming or losing the active sessions table.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d7bf9a71-0bef-48c6-8fb1-0f10d796f5e8
📒 Files selected for processing (1)
backend/migrations/clickhouse/2026_08_01_session_id.js
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
1 participant
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.
Changes
If applicable, please describe what changes were made in this pull request.
Community Edition support
Database migrations
Documentation
Summary by CodeRabbit
New Features
Documentation