chore(recordings): Optimize recordings list query #14458

EDsCODE · 2023-02-28T18:11:57Z

Problem

addressing Improving performance of recordings list query #14072

Changes

implements suggestions from Improving performance of recordings list query #14072

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

How did you test this code?

posthog-bot · 2023-02-28T18:12:41Z

Hey @EDsCODE! 👋
This pull request seems to contain no description. Please add useful context, rationale, and/or any other information that will help make sense of this change now and in the distant Mars-based future.

EDsCODE · 2023-02-28T22:00:36Z

TODO:

handle person properties pushdown

EDsCODE · 2023-03-02T00:33:58Z

posthog/queries/session_recordings/session_recording_list.py

+        )
+
+    def run(self, *args, **kwargs) -> SessionRecordingQueryResult:
+        self._filter.hogql_context.using_person_on_events = True


This connection to hogql restricts the improvement to person_on_events enabled orgs for now.

don't get this, aren't there like only 4 poe orgs right now?

yeah. hogql allows for person properties to be filtered in the recordings event query which usually was not the case. Now, either we use person on events and only have this optimization working for the tiny subset until everything works, or I go through a refactor that splits up and does the right pushdown of person property filtering to the subquery when we detect a hogql based person property

Ugh I see, okay, good enough

posthog-bot · 2023-03-10T07:30:53Z

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

neilkakkar · 2023-03-14T11:54:29Z

posthog/queries/session_recordings/test/__snapshots__/test_session_recording_list.ambr

+     HAVING full_snapshots > 0
+     AND start_time >= '2021-01-14 00:00:00'
+     AND start_time <= '2021-01-21 20:00:00') AS session_recordings ON session_recordings.session_id = events.session_id
+  WHERE session_recordings.distinct_id == events.distinct_id


doesn't matching by distinctID lead to problems as we're randomly selecting one?

Don't think sessionIDs change between login steps, but distinctIDs can & do?

I also wonder this... Seems like a copy from the v1 query but I'm not sure why it matters... We only need one distinctId to load the Person anyways and we have that from the recording. We anyways group by session_id...

Perhaps this is to account for weird cases where there is a clash of session_id but I don't see that happening...

neilkakkar

looks good! The code here is going get more complicated as we support multiple ways of doing things, but given that it will all go away sometime with the schema changes, not too bothered.

Would like it if session-recording folks have a look too though!

benjackwhite · 2023-03-14T12:15:30Z

posthog/models/team/team.py

@@ -217,6 +217,26 @@ def _person_on_events_querying_enabled(self) -> bool:
        # on self-hosted, use the instance setting
        return get_instance_setting("PERSON_ON_EVENTS_ENABLED")

+    @property
+    def recordings_list_v2_query_enabled(self) -> bool:


Could we do this on the frontend instead and control it via a query param to the API? Makes it much easier to test back and forth in a live situation to compare results etc.

Hmm, on second thought, I don't want to add filters to the object and muddy our caching for insights. Do you think it's worth it for this?

I wasn't suggesting adding it to filters (not everything has to go in that mega object 😅 ) - just a standard query param. I pushed the change in the way I meant so now we just check a query param which makes it super easy to flip back and forth without having to change the feature flag all the time

Oh yep, we can do that. Too focused on the filter object 😬

benjackwhite · 2023-03-14T12:25:18Z

posthog/queries/session_recordings/test/__snapshots__/test_session_recording_list.ambr

+     HAVING full_snapshots > 0
+     AND start_time >= '2021-01-14 00:00:00'
+     AND start_time <= '2021-01-21 20:00:00') AS session_recordings ON session_recordings.session_id = events.session_id
+  WHERE session_recordings.distinct_id == events.distinct_id


I also wonder this... Seems like a copy from the v1 query but I'm not sure why it matters... We only need one distinctId to load the Person anyways and we have that from the recording. We anyways group by session_id...

Perhaps this is to account for weird cases where there is a clash of session_id but I don't see that happening...

benjackwhite · 2023-03-14T12:27:27Z

posthog/queries/session_recordings/session_recording_list.py

Overall I think this is worth trying but reading it still makes me think "there must be a simpler way" :D

I keep thinking why is not just

select * from recording_events_grouped where session_id in (BuildStandardEventsQuery(filters).select("session_id")

Definitely don't want to derail and at this point I'll take any improvements to the speed of this thing but these queries still confuse the hell out of me

It should be that easy... why isn't it? :D

What would this look like if you rebuilt it the same way the events query is built?

Bigger issue to wait on before spending time unnecessarily is person on events deployment. Once we're there, we can probably move this to @mariusandra's suggestion with less headache and a lot less joins to negotiate

EDsCODE · 2023-03-14T21:41:58Z

Will need this to wait on person on events because of this dependency: #14458 (comment)

mariusandra · 2023-03-14T22:07:55Z

The point of HogQL is that it'll deal with the persons on events issue for you. I'm actively working on making this actually true. Once that PR is wrapped up (tomorrow? 🤞), person joins will and won't happen as needed, for both the events and session_recording_events tables. You can just use person_id and person.properties.foo as if it's on the same table.

I also wrote up a quick guide on how to use HogQL with Python in the backend. Hopefully that can provide some inspiration. You will be free to do any joins or non-joins as you'd like, or let HogQL handle it for you. It's just a layer around ClickHouse SQL after all... as long as you stick to the implemented syntax (no arrays yet :/).

EDsCODE · 2023-03-21T21:17:48Z

I've updated the logic to not use V2 if there are hogql properties. This will be in effect until person on events is fully deployed. The updates to hogql mentioned do not solve this particular issue because the recordings query does adhoc property parsing that does not adhere to the rest of the query patterns

posthog-bot · 2023-03-22T08:32:04Z

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

chromium: 0 added, 0 modified, 0 deleted
webkit: 0 added, 1 modified, 0 deleted (diff for shard 2)
firefox: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

benjackwhite

Overall I'm happy for this to go in but it would be great if this doesn't hang around for 3 months as half-done thing 😅 Ideally we test it heavily and immediately, and then follow up with a review to swap over fully ASAP.

In the not too far future, we're going to be rebuilding the whole thing (hopefully using the new query stuff) in order to support Notebooks more effectively so it would be good to have as a clean a base to start from

posthog-bot · 2023-03-22T08:45:08Z

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

chromium: 0 added, 0 modified, 0 deleted
webkit: 0 added, 1 modified, 0 deleted (diff for shard 2)
firefox: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

* derivative class * derivative class * tests * Update query snapshots * add flag * typing * Update query snapshots * Update query snapshots * Update query snapshots * Update query snapshots * use person on events with new query * Update query snapshots * Update query snapshots * Update query snapshots * Update query snapshots * Update query snapshots * Update query snapshots * Update query snapshots * Update query snapshots * Update query snapshots * Update query snapshots * snapshots * Update query snapshots * Update query snapshots * Update query snapshots * merge master * merge master * remove unnecessary joins with poe is active * try turning off the test unless poe * revert poe changes * don't use optimized recording list on hogql queries for now * Moved flag to frontend * Update query snapshots * Update UI snapshots for `chromium` (1) * Update UI snapshots for `webkit` (2) * Update UI snapshots for `webkit` (2) --------- Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Ben White <ben@posthog.com>

EDsCODE added 3 commits February 27, 2023 18:34

derivative class

695b787

derivative class

e2add4e

tests

c7eff77

EDsCODE added feature/recordings performance Has to do with performance. For PRs, runs the clickhouse query performance suite labels Feb 28, 2023

github-actions bot and others added 4 commits February 28, 2023 18:21

Update query snapshots

9253921

add flag

bce115e

typing

5ce263f

Update query snapshots

68b86fc

github-actions bot and others added 16 commits February 28, 2023 22:02

Update query snapshots

eed5f89

Update query snapshots

f5edc76

Update query snapshots

d68ac85

use person on events with new query

034221a

Update query snapshots

f25e745

Update query snapshots

e9c559a

Update query snapshots

fc831d4

Update query snapshots

10efc7d

Update query snapshots

51893d6

Update query snapshots

a0c966e

Update query snapshots

5b1435d

Update query snapshots

36c0e01

Update query snapshots

da42388

Update query snapshots

24135c2

snapshots

4bd7535

Update query snapshots

a22cfb1

EDsCODE commented Mar 2, 2023

View reviewed changes

github-actions bot and others added 2 commits March 2, 2023 00:34

Update query snapshots

b72a7f1

merge master

37238f8

EDsCODE requested a review from neilkakkar March 2, 2023 18:30

posthog-bot added the stale label Mar 10, 2023

EDsCODE removed the stale label Mar 13, 2023

EDsCODE added 3 commits March 13, 2023 15:28

merge master

6f8f06e

merge master

e9efea6

merge master

9562ebf

neilkakkar reviewed Mar 14, 2023

View reviewed changes

neilkakkar approved these changes Mar 14, 2023

View reviewed changes

benjackwhite requested changes Mar 14, 2023

View reviewed changes

EDsCODE added 4 commits March 14, 2023 09:20

Merge branch 'master' into optimize-recordings-list-query

16d4cf5

remove unnecessary joins with poe is active

fd7a7fd

try turning off the test unless poe

6bd28d9

revert poe changes

c9d18a2

EDsCODE added 2 commits March 21, 2023 14:25

merge master

9d746ea

don't use optimized recording list on hogql queries for now

3e5dbb4

benjackwhite and others added 4 commits March 22, 2023 09:20

Moved flag to frontend

695f328

Update query snapshots

4467504

Update UI snapshots for chromium (1)

5234c22

Update UI snapshots for webkit (2)

c52d00b

benjackwhite approved these changes Mar 22, 2023

View reviewed changes

Update UI snapshots for webkit (2)

1d6e97f

EDsCODE merged commit a1a8fdd into master Mar 22, 2023

EDsCODE deleted the optimize-recordings-list-query branch March 22, 2023 14:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(recordings): Optimize recordings list query #14458

chore(recordings): Optimize recordings list query #14458

EDsCODE commented Feb 28, 2023

posthog-bot commented Feb 28, 2023

EDsCODE commented Feb 28, 2023

EDsCODE Mar 2, 2023

neilkakkar Mar 14, 2023

EDsCODE Mar 14, 2023

neilkakkar Mar 14, 2023

posthog-bot commented Mar 10, 2023

neilkakkar Mar 14, 2023

benjackwhite Mar 14, 2023

neilkakkar left a comment

benjackwhite Mar 14, 2023

EDsCODE Mar 14, 2023 •

edited

Loading

benjackwhite Mar 22, 2023

EDsCODE Mar 22, 2023

benjackwhite Mar 14, 2023

benjackwhite Mar 14, 2023

mariusandra Mar 14, 2023

EDsCODE Mar 14, 2023

EDsCODE commented Mar 14, 2023

mariusandra commented Mar 14, 2023

EDsCODE commented Mar 21, 2023

posthog-bot commented Mar 22, 2023

benjackwhite left a comment

posthog-bot commented Mar 22, 2023

chore(recordings): Optimize recordings list query #14458

chore(recordings): Optimize recordings list query #14458

Conversation

EDsCODE commented Feb 28, 2023

Problem

Changes

How did you test this code?

posthog-bot commented Feb 28, 2023

EDsCODE commented Feb 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

posthog-bot commented Mar 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neilkakkar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EDsCODE Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EDsCODE commented Mar 14, 2023

mariusandra commented Mar 14, 2023

EDsCODE commented Mar 21, 2023

posthog-bot commented Mar 22, 2023

📸 UI snapshots have been updated

benjackwhite left a comment

Choose a reason for hiding this comment

posthog-bot commented Mar 22, 2023

📸 UI snapshots have been updated

EDsCODE Mar 14, 2023 •

edited

Loading