Updated handling for stats page audience filters#22571
Conversation
ref https://linear.app/ghost/issue/ANAL-144/ - reverted previous change - updated data files to use simpler aggregate fn Type errors from the query params have been worked around by using a simpler aggregate function that should also be more performant. Still more to learn here - so far my understanding is this change is both appropriate and better, and at least works for the moment.
WalkthroughThis pull request introduces several version updates and schema adjustments across multiple files. In the Ghost admin utility ( Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
ghost/admin/app/utils/stats.jsOops! Something went wrong! :( ESLint: 8.44.0 Error: Failed to load parser '@babel/eslint-parser' declared in 'ghost/admin/.eslintrc.js': Cannot find module '@babel/eslint-parser'
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
9larsons
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ghost/web-analytics/pipes/_parsed_hits.incl (1)
1-4: Version Declaration Update and TAGS Consistency
The version declaration has been updated toVERSION 1, which aligns with the overall branch update. Please verify that retainingTAGS "v0"is intentional and consistent with your versioning strategy.ghost/web-analytics/pipes/api_top_locations.pipe (1)
1-4: Version and Aggregate Function Consistency
The version number has been updated toVERSION 1, and the query now uses functions likeuniqMerge(visits)alongsidecountMerge(pageviews). Ensure that usinguniqMerge(visits)(as opposed to, for example,uniq(session_id)in other parts) is the desired approach for simplifying aggregation. Consistency across similar queries is key to the expected audience filter handling improvements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
ghost/admin/app/utils/stats.js(2 hunks)ghost/web-analytics/datasources/_mv_pages.datasource(2 hunks)ghost/web-analytics/datasources/_mv_sessions.datasource(1 hunks)ghost/web-analytics/datasources/_mv_sources.datasource(2 hunks)ghost/web-analytics/pipes/_hits.incl(1 hunks)ghost/web-analytics/pipes/_parsed_hits.incl(1 hunks)ghost/web-analytics/pipes/api_kpis.pipe(1 hunks)ghost/web-analytics/pipes/api_top_browsers.pipe(1 hunks)ghost/web-analytics/pipes/api_top_devices.pipe(1 hunks)ghost/web-analytics/pipes/api_top_locations.pipe(1 hunks)ghost/web-analytics/pipes/api_top_os.pipe(1 hunks)ghost/web-analytics/pipes/api_top_pages.pipe(1 hunks)ghost/web-analytics/pipes/api_top_sources.pipe(1 hunks)ghost/web-analytics/pipes/mv_pages.pipe(2 hunks)ghost/web-analytics/pipes/mv_sessions.pipe(2 hunks)ghost/web-analytics/pipes/mv_sources.pipe(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Tinybird Tests (Web Analytics)
- GitHub Check: Unit tests (Node 22.13.1)
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Unit tests (Node 18.12.1)
- GitHub Check: Admin tests - Chrome
🔇 Additional comments (21)
ghost/web-analytics/pipes/_hits.incl (1)
1-6: Consistent Version Bump and Inclusion Check
The version is now set toVERSION 1and the file includes_parsed_hits.inclas expected. Ensure that the keptTAGS "v0"accurately reflects the intended tag version, and that no additional updates are needed with respect to the integration of audience filters.ghost/web-analytics/pipes/api_top_browsers.pipe (1)
1-4: Version Update and SQL Template Verification
The file’s version update from 0 to 1 is applied correctly. The SQL query—with its Jinja templating for date ranges, member status, device, browser, and source filtering—appears to remain functionally intact. Please double-check that these aggregation and filtering functions are now properly simplified and resolving the previous type errors noted in ANAL-144.ghost/web-analytics/pipes/api_top_os.pipe (1)
1-4: Version Increment Confirmation
Updating the version toVERSION 1is consistent with the project-wide schema changes. The filtering and aggregation logic for operating systems remains unchanged, so please confirm through integration tests that the intended performance and type improvements are effective.ghost/web-analytics/pipes/api_top_pages.pipe (1)
1-1: Version increment aligns with audience filter changesThis version update from 0 to 1 is part of the coordinated versioning across multiple files to support the new audience filtering approach.
ghost/web-analytics/pipes/api_top_devices.pipe (1)
1-1: Version increment aligns with audience filter changesThis version update from 0 to 1 is part of the coordinated versioning across multiple files to support the new audience filtering approach.
ghost/web-analytics/pipes/api_kpis.pipe (1)
1-1: Version increment aligns with audience filter changesThis version update from 0 to 1 is part of the coordinated versioning across multiple files to support the new audience filtering approach.
ghost/web-analytics/pipes/api_top_sources.pipe (1)
1-1: Version increment aligns with audience filter changesThis version update from 0 to 1 is part of the coordinated versioning across multiple files to support the new audience filtering approach.
ghost/admin/app/utils/stats.js (2)
3-3: Version increment supports the audience filter format changeThis version bump to
TB_VERSIONaligns with the changes in the .pipe files and supports the modified audience filter handling.
186-188:Details
✅ Verification successful
Good fix: Converting audience array to comma-separated string
This change resolves the type error mentioned in the PR description by converting the audience array to a comma-separated string format. This ensures that the member_status parameter is properly formatted for the endpoint and matches the updated SimpleAggregateFunction approach in the backend.
🏁 Script executed:
#!/bin/bash # Description: Check if the revised audience handling is used consistently in the codebase # Look for any other direct usage of member_status parameter rg -A 2 -B 2 "member_status" --type jsLength of output: 1521
Actionable Confirmation: Audience Array Conversion is Correct and Consistent
The change converting the audience array to a comma-separated string (via
audience.join(',')) is correctly implemented. Verification confirms that:
- The intended update in
ghost/admin/app/utils/stats.jsproperly transforms the array into the required string format.- Other code references handling
member_status(in both frontend and migration files) are consistent with this format expectation.No further modifications are needed.
ghost/web-analytics/pipes/mv_pages.pipe (2)
1-1: Version increment is appropriate for schema changesThe version bump from 0 to 1 correctly reflects the significant change in the aggregation method for member_status.
21-21: Simplified aggregation logic for member_statusThe change from conditional filtering (
maxIfStatewith an IN condition) to a simple maximum (maxSimpleState) aligns well with the PR objective to simplify audience filter handling and resolve type errors. This more straightforward approach should improve query performance while maintaining the necessary functionality.ghost/web-analytics/datasources/_mv_sources.datasource (2)
1-1: Version increment matches pipe file changesThe version bump to 1 properly aligns with the corresponding pipe file version changes, maintaining consistency across the system.
15-15: Schema update for simplified member_status aggregationThe change from
AggregateFunction(maxIf, String, UInt8)toSimpleAggregateFunction(max, String)correctly updates the schema to match the simplified aggregation approach used in the pipe files. This ensures that the datasource definition is compatible with the new query implementation.ghost/web-analytics/pipes/mv_sources.pipe (2)
1-1: Version increment consistent with related filesThe version bump to 1 maintains consistency with the other pipe and datasource file changes in this PR.
21-21: Simplified aggregation in sessions subqueryThe change from conditional filtering (
maxIfStatewith member status IN conditions) to a simple maximum (maxSimpleState) appropriately simplifies the logic in the sessions subquery. This is consistent with the changes in other pipe files and supports the PR goal of simplifying audience filter handling.ghost/web-analytics/datasources/_mv_pages.datasource (2)
1-1: Version increment for schema changeThe version bump to 1 correctly reflects the schema modification and maintains consistency with other file changes in this PR.
16-16: Consistent schema update for member_statusThe change from
AggregateFunction(maxIf, String, UInt8)toSimpleAggregateFunction(max, String)properly updates the schema definition to match the simplified aggregation approach. This change is consistent with the other datasource updates and aligns with the pipe file query modifications.ghost/web-analytics/pipes/mv_sessions.pipe (2)
1-1: Version increment reflects schema change.The version number increment from 0 to 1 appropriately signals the structural change in how member status is aggregated. This follows good versioning practices when modifying data schemas.
14-14: Simplified member status aggregation improves filter handling.The change from
maxIfState(which likely had a condition filtering specific values) tomaxSimpleStatealigns with the PR objective of simplifying audience filters. This approach removes conditional filtering and should help resolve the type errors mentioned in the PR description.Would you like to verify that this change works correctly with all possible member status values? If some values were previously being filtered out intentionally, you may want to ensure this change doesn't introduce unexpected behavior.
ghost/web-analytics/datasources/_mv_sessions.datasource (2)
1-1: Version increment properly tracks schema change.The version number increment from 0 to 1 appropriately reflects the schema modification. This maintains consistency with the corresponding pipe file and follows good versioning practices.
9-9: Schema change supports simplified member status aggregation.The modification of the
member_statusfield type fromAggregateFunction(maxIf, String, UInt8)toSimpleAggregateFunction(max, String)aligns with:
- The simplified aggregation in the pipe file
- The PR objective of resolving type errors with query parameters
- Potential performance improvements mentioned in the PR description
This change is an appropriate implementation of the stated PR goals for simplifying audience filter handling.
1 participant
ref https://linear.app/ghost/issue/ANAL-144/
Type errors from the query params have been worked around by using a simpler aggregate function that should also be more performant. Still more to learn here - so far my understanding is this change is both appropriate and better, and at least works for the moment.