Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(web-analytics): Support screen events in web analytics queries #27242

Merged
merged 4 commits into from
Jan 6, 2025

Conversation

robbie-c
Copy link
Member

@robbie-c robbie-c commented Jan 3, 2025

Problem

See https://posthog.slack.com/archives/C05LJK1N3CP/p1735900802399899

I haven't spent enough time on this to get the quality super high - this is definitely a "ship this quickly as it'll tell us what else needs to be improved"

Changes

Change web analytics queries to accept $screen events as well as $pageview events.

Adds a feature flag to hide the normal page tiles and replace them with a screen names tile.

Does this work well for both Cloud and self-hosted?

Yes

How did you test this code?

Add some tests on the happy path of overview and stats tile. If we decided to add more mobile-specific tiles we'd need to add some test coverage there.

Copy link
Contributor

github-actions bot commented Jan 3, 2025

Size Change: +52 B (0%)

Total Size: 9.43 MB

ℹ️ View Unchanged
Filename Size Change
frontend/dist/toolbar.js 9.43 MB +52 B (0%)

compressed-size-action

@robbie-c robbie-c requested a review from a team January 3, 2025 17:34
@robbie-c robbie-c marked this pull request as ready for review January 3, 2025 17:38
Copy link
Member

@rafaeelaudibert rafaeelaudibert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Left a handful of comments but I understand this is a first step towards a lightweight mobile analytics view, so they don't need to be worked on right now

@@ -559,7 +560,7 @@ export const webAnalyticsLogic = kea<webAnalyticsLogicType>([
}

const uniqueUserSeries: EventsNode = {
event: '$pageview',
event: featureFlags[FEATURE_FLAGS.WEB_ANALYTICS_FOR_MOBILE] ? '$screen' : '$pageview',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious, what's your long term idea for this? What if someone wants to support both mobile and web? This will most likely occur, right?

Is the idea here that they should set up separate projects to handle that - one of them tracking $pageviews and the other $screens?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also want to show web analytics for a specific website, rather than across all of your sites, so I think we could combine it with that. If the "site" is a mobile app, then put it in mobile app mode.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that makes sense. This would be pretty easy to add as a toggle at the project level then :)

@@ -149,7 +149,7 @@ def to_path_scroll_bounce_query(self) -> ast.SelectQuery:
min(session.$start_timestamp ) AS start_timestamp
FROM events
WHERE and(
events.event == '$pageview',
or(events.event == '$pageview', events.event == '$screen'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a virtual/computed field to the events Clickhouse table to aid with this? Something akin to a boolean is_pageview_like?

Or, maybe, something we could add to the HogQL translation layer instead? I believe mixing these will become cumbersome

@@ -149,7 +149,7 @@ def to_path_scroll_bounce_query(self) -> ast.SelectQuery:
min(session.$start_timestamp ) AS start_timestamp
FROM events
WHERE and(
events.event == '$pageview',
or(events.event == '$pageview', events.event == '$screen'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the front end clearly separates the screen vs. pageview UIs, can we split this in the back end as well? This shouldn't be tricky, we just need to check the same FF for that user, right?

Comment on lines +175 to 184
pageview_expr = ast.Or(
exprs=[
ast.CompareOperation(
op=ast.CompareOperationOp.Eq, left=ast.Field(chain=["event"]), right=ast.Constant(value="$pageview")
),
ast.CompareOperation(
op=ast.CompareOperationOp.Eq, left=ast.Field(chain=["event"]), right=ast.Constant(value="$screen")
),
]
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, could check the FF to decide between $pageview and $screen

@robbie-c robbie-c merged commit da23d14 into master Jan 6, 2025
103 checks passed
@robbie-c robbie-c deleted the feat/improve-web-analytics-mobile branch January 6, 2025 14:30
@robbie-c robbie-c restored the feat/improve-web-analytics-mobile branch January 6, 2025 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants