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): Add web analytics tabs #17981

Merged
merged 13 commits into from
Oct 17, 2023
Merged

Conversation

robbie-c
Copy link
Member

@robbie-c robbie-c commented Oct 14, 2023

Problem

Want to be able to breakdown by more properties

Ticks off 4 boxes here https://docs.google.com/document/d/1LsK70o5CTPKHWEeB_8txmJHmOMhNPCc5GnHhw87nLYY/edit

Changes

  • Add a new query type which does pageviews, uniques, and bounce rate, with a breakdown_by enum. This is the basis for most web analytics tables now.
  • Remove the unused top_sources and top_pages query types
  • Add a change to property_to_expr to support nested properties
Screenshot 2023-10-14 at 10 58 31 Screenshot 2023-10-14 at 11 00 02

How did you test this code?

(Behind FF) ran it manually

@robbie-c robbie-c changed the title Add web analytics tabs feat(web-analytics): Add web analytics tabs Oct 14, 2023
@@ -582,15 +569,28 @@ export interface WebTopClicksQueryResponse extends QueryResponse {
columns?: unknown[]
}

export interface WebTopPagesQuery extends WebAnalyticsQueryBase {
kind: NodeKind.WebTopPagesQuery
export enum WebStatsBreakdown {
Copy link
Member Author

@robbie-c robbie-c Oct 14, 2023

Choose a reason for hiding this comment

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

I used an enum here because some properties will take special handling on the query side, so I can't just pass a property name (well I could, but I'd have to handle it with cases in the same way)

@@ -129,7 +129,7 @@ def property_to_expr(
return ast.Or(exprs=exprs)

chain = ["person", "properties"] if property.type == "person" and scope != "person" else ["properties"]
field = ast.Field(chain=chain + [property.key])
field = ast.Field(chain=chain + property.key.split("."))
Copy link
Member Author

@robbie-c robbie-c Oct 14, 2023

Choose a reason for hiding this comment

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

Drawing attention to this, theoretically could be breaking as it unescapes any ..

i.e. a.b was previously "properties"."a.b" and now will be "properties"."a"."b"

Copy link
Collaborator

@mariusandra mariusandra Oct 16, 2023

Choose a reason for hiding this comment

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

Hmm, sorry, got to block this, as it's a global change in the hogql folder. We have users who have actual dots in their property name strings, and this will break some filter somewhere for them. The event/feature/property filter is made to work with the current taxonomic filter, which presents just the first layer of properties and has never supported more.

You can use the hogql template helper like

hogql`SELECT properties.${hogql.identifier('odd property')} FROM events`

if you want to cleanly escape multiple properties into a hogql string.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is fixed by creating hogql filters on the front end

@robbie-c robbie-c requested a review from mariusandra October 16, 2023 08:11
@robbie-c robbie-c marked this pull request as ready for review October 16, 2023 08:11
@robbie-c robbie-c requested a review from Gilbert09 October 16, 2023 08:12
Copy link
Collaborator

@mariusandra mariusandra left a comment

Choose a reason for hiding this comment

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

All good for most of the changes, but the one in hogql/property.py will break the app for some existing users, so we're going to have to find another way.

@@ -129,7 +129,7 @@ def property_to_expr(
return ast.Or(exprs=exprs)

chain = ["person", "properties"] if property.type == "person" and scope != "person" else ["properties"]
field = ast.Field(chain=chain + [property.key])
field = ast.Field(chain=chain + property.key.split("."))
Copy link
Collaborator

@mariusandra mariusandra Oct 16, 2023

Choose a reason for hiding this comment

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

Hmm, sorry, got to block this, as it's a global change in the hogql folder. We have users who have actual dots in their property name strings, and this will break some filter somewhere for them. The event/feature/property filter is made to work with the current taxonomic filter, which presents just the first layer of properties and has never supported more.

You can use the hogql template helper like

hogql`SELECT properties.${hogql.identifier('odd property')} FROM events`

if you want to cleanly escape multiple properties into a hogql string.

@robbie-c robbie-c force-pushed the add-web-analytics-tabs branch from 206fb13 to 2e3aa89 Compare October 16, 2023 11:06
@robbie-c robbie-c force-pushed the add-web-analytics-tabs branch from 2e9cac2 to 30babc3 Compare October 16, 2023 12:02
@robbie-c robbie-c force-pushed the add-web-analytics-tabs branch from 30babc3 to 7c89fd6 Compare October 16, 2023 12:07
@robbie-c robbie-c requested a review from mariusandra October 16, 2023 12:42
@robbie-c
Copy link
Member Author

I switched the parser over to CPP and moved the navbar icon to the right section

@robbie-c robbie-c enabled auto-merge (squash) October 16, 2023 14:27
Copy link
Collaborator

@mariusandra mariusandra left a comment

Choose a reason for hiding this comment

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

One thought regarding escaping strings, but LGTM.

export const initialWebAnalyticsFilter = [] as WebAnalyticsPropertyFilters

const setOncePropertyNames = ['$initial_pathname', '$initial_referrer', '$initial_utm_source', '$initial_utm_campaign']
const hogqlForSetOnceProperty = (key: string, value: string): string => `properties.$set_once.${key} = '${value}'`
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will work for a limited subset of keys, but you can use hogql.identifier(key) (from import { hogql } from './utils') to actually safely escape all keys.

@robbie-c robbie-c merged commit 1fb0548 into master Oct 17, 2023
73 checks passed
@robbie-c robbie-c deleted the add-web-analytics-tabs branch October 17, 2023 13:05
daibhin pushed a commit that referenced this pull request Oct 23, 2023
* Add web analytics tabs

* Add stats table query runner and use it for pathname

* Add stats query runner, replacing page and source

* Remove classes for now-unused queries

* Add remaining breakdowns

* Prefer unknown to any

* Remove unused ctes

* Fix typing issues

* Fix python typing

* Switch to hogql filters for set_once properties

* Use cpp parser

* Move web analytics to the right navbar section

* Fix typing issues
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