perf(insights): Speed up loading properties #11037
Conversation
posthog/api/property_definition.py
Outdated
| @@ -115,6 +115,9 @@ def get_queryset(self): | |||
| **search_kwargs, | |||
| } | |||
|
|
|||
| limit = self.paginator.get_limit(self.request) | |||
There was a problem hiding this comment.
Are these params being passed already?
posthog/api/property_definition.py
Outdated
| @@ -130,6 +133,7 @@ def get_queryset(self): | |||
| WHERE team_id = %(team_id)s AND name NOT IN %(excluded_properties)s | |||
| {name_filter} {numerical_filter} {search_query} {event_property_filter} | |||
| ORDER BY is_event_property DESC, query_usage_30_day DESC NULLS LAST, name ASC | |||
| LIMIT {limit} OFFSET {offset} | |||
There was a problem hiding this comment.
flyby....
I think by setting limit and offset manually here we only ever get one page.
DRF LimitOffsetPagination uses the queryset to count the results here https://github.com/encode/django-rest-framework/blob/master/rest_framework/pagination.py#L387
And then uses that count to decide whether there are more pages.
It slices the queryset before loading the results.
So if we add limit and offset to the queryset the count is always equal to one page.
The TTFB on cloud for this endpoint is wild though even so
|
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 |
| @@ -4,7 +4,7 @@ describe('a11y', () => { | |||
| it('home should have no accessibility violations', () => { | |||
| cy.get('[data-attr="menu-item-projecthomepage"]').click() | |||
| cy.injectAxe() | |||
| reportA11y({ includedImpacts: ['critical'] }, 'home-page-critical', false) | |||
There was a problem hiding this comment.
This silences an accessibility failure that I thought was resolved.
I'll follow this up in separate work.
The change to false here still runs the accessibility test but doesn't fail the test run when violations are found
|
@neilkakkar I think this covers the conversation here https://posthog.slack.com/archives/C0368RPHLQH/p1660727404221419 I'd be interested in releasing this so we can measure it without trying to cache the count |
posthog/api/property_definition.py
Outdated
| "type": "string", | ||
| "nullable": True, | ||
| "format": "uri", | ||
| "example": "http://api.example.org/accounts/?{offset_param}=400&{limit_param}=100".format( |
There was a problem hiding this comment.
Minor nit, same below!
| "example": "http://api.example.org/accounts/?{offset_param}=400&{limit_param}=100".format( | |
| "example": f"http://api.example.org/accounts/?{self.offset_query_param}=400&{self.limit_query_param}=100" |
posthog/api/property_definition.py
Outdated
| }, | ||
| } | ||
|
|
||
| def get_next_link(self) -> Optional[str]: |
There was a problem hiding this comment.
you'll probably need to override get_previous_link as well?
There was a problem hiding this comment.
Actually, I'm not quite sure what this is adding over the default implementation? - as long as count is set, the default seems to be fine?
There was a problem hiding this comment.
Ah yes! I'm not removing count any more so can override way less of this.
The default implementation asks the queryset for the count and slices the results. But that's being done externally.
Problem
Loading properties is painful, takes about 7 seconds on our team. Reason is that we didn't set an offset/limit on the query, so it was always going through 100Ks of properties for each query.
In general the way we do properties needs work. We now spend ~5 minutes loading all properties for our team into browser memory, which you really start to notice. We should probably send property definitions along with properties wherever applicable instead
Changes
set limit/offset
also bump up the breakpoint for loading properties (and other stuff), as you very often end up loading lots in parallel.
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
How did you test this code?