feat(person-on-events): Revamp pagination to handle 'lost' personIDs #11500
Conversation
| offset = filter.offset | ||
| if len(actors) > 100 and next_url: |
There was a problem hiding this comment.
This was basically just an optimistic query earlier: We'd query 200 people, and hope atleast 100 are valid, so we could paginate. Not great.
Got rid of this logic completely & using filter.limit to govern this now, just like rest of insight actors.
|
Also cc: @benjackwhite , I notice you're creating V2 of the Persons modal & planning to run an experiment for it, from the looks of it 👀 . Just a heads up. |
|
cc: @tiina303 do you know which link I can point this modal to? (It needs to explain why persons might be lost) |
| @@ -257,7 +257,7 @@ def funnel(self, request: request.Request, **kwargs) -> response.Response: | |||
| if not results_package: | |||
There was a problem hiding this comment.
Is this new pattern not applicable to the regular person list?
There was a problem hiding this comment.
.. What do you mean? 🤔
There was a problem hiding this comment.
Oh it only happens in one place but posthog/api/cohort.py line 217 we're still using the postgres result and not paginating based on clickhouse raw count result
There was a problem hiding this comment.
Nice catch, have deleted the should_paginate function and replaced all usage
https://posthog.com/docs/how-posthog-works/queries#insights-counting-unique-persons - is the section for it |
Problem
See #11488 (comment)
This is WIP. But wanted to get a PoC out before fixing everything.Note that this fixes pagination for pre- person-on-events too, whenever deletes occur.
Now, instead of relying on the filtered result to tell us whether to paginate or not, we use the raw query output. If the query output >= limit, then paginate; otherwise don't. Note that this happens before any filtering of orphaned/deleted persons.
I'm also thinking of sending the raw query count, which would allow us to determine when to show a banner stating 'this went wrong because of person-on-events, etc. etc.'
Link pending, but the modal now looks like this: (Retention fixes in a different PR)
Changes
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
How did you test this code?