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: added session duration as a breakdown option #18056

Merged
merged 3 commits into from
Oct 18, 2023

Conversation

Gilbert09
Copy link
Member

Problem

Changes

  • Requires an extra join to achieve this
  • Looked at adding $session_duration as a lazy join on events, but, because we filter the subquery by date, it wouldn't work I think - but I may dig deeper into this and see, it would be a lot nicer letting HogQL take care of this for us

How did you test this code?

Browser clicky clicks 🙃

@Gilbert09 Gilbert09 requested a review from mariusandra October 18, 2023 10:15
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.

Some thoughts/context, but looks good. Note: I didn't run it, but trust it'll do its job.
Also: needs tests 😅. Probably the sooner you start adding them, the easier it'll be to remember all edge cases.

@@ -63,6 +63,7 @@ class EventsTable(Table):
"distinct_id": StringDatabaseField(name="distinct_id"),
"elements_chain": StringDatabaseField(name="elements_chain"),
"created_at": DateTimeDatabaseField(name="created_at"),
"$session_id": StringDatabaseField(name="$session_id"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is actually a materialized field. I think accessing properties.$session_id will direct it to access the field with this name. However fine to add it explicitly too.

"$session_id", dateDiff('second', min(timestamp), max(timestamp)) as session_duration
FROM events
WHERE
"$session_id" != '' AND
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the "materialized columns are bad with nulls" issue. If properties.$session is not set, the materialized column $session_id is ''. If it's set to an empty string, it's also '', but if it's set to null or undefined, it'll come back as null. 🤯

@Gilbert09 Gilbert09 merged commit edbd4e0 into master Oct 18, 2023
67 checks passed
@Gilbert09 Gilbert09 deleted the feat/trends-sessions-duration branch October 18, 2023 13:13
daibhin pushed a commit that referenced this pull request Oct 23, 2023
* Added session duration as a breakdown option

* Update query snapshots

* Fixed tests

---------

Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Justicea83 pushed a commit to Justicea83/posthog that referenced this pull request Oct 25, 2023
* Added session duration as a breakdown option

* Update query snapshots

* Fixed tests

---------

Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
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