Sort events and properties by volume first, A-Z second

[mariusandra]

Changes

• Moves events and properties in the taxonomic filter to order by popularity

• Closes Sort infinite list events & properties by popularity #5309

• I think this makes the experience nicer, as things you're more likely to use are higher up.

• Events before & after:
  
  

• Properties before & after:
  
  

How did you test this code?

• Locally in the browser

[paolodamico]

lgtm! fixed merge conflicts and moved to query volume as discussed on Slack. Only thing I'm not 100% sure is that creating this indices won't stall instances with a ton of events/properties (e.g. this customer)

[paolodamico]

I'm curious how indexing works with the -? Do you know any good literature on this?

[mariusandra]

It should just add desc in the index as well, and should actually make no difference.

[paolodamico]

Postgres-level does that just mean better performance because we would order desc too? What's the impact of not having the desc in there?

[mariusandra]

Sorry, forgot a link: https://www.postgresql.org/docs/14/indexes-ordering.html

I copied this from a different model in the system, and left the - in place since we mostly do order DESC here. If I read the doc correctly, there shouldn't be any real difference between the two. This will be truly useful if we'd sort by multiple columns. We don't here, and postgres can read indexes backwards, so it's probably better to skip the - here for clarity.

At least I should run a few tests on it to make sure it works well, but now it's gone, so :).

[mariusandra]

You are right there. I'll remove the migration.

These indices (indexes?) are just for performance. Users with a sensible number of event names probably won't notice much difference, and the user you linked will certainly experience problems. Currently this user can't even use the event stats page because of the sheer number of events, because we still need to load all of them into memory before displaying anything 🤯

Perhaps this should be one of the first uses for the "Special Migrations API"? @yakkomajuri

[mariusandra]

To add, even without the indexes we can't merge this, as with a few dozen million event names, the taxonomic filter will get really slow. So we're blocked.

[paolodamico]

Well we already didn't have an index for the event name yet we already ordered based on that, will performance be that bad with these changes?

[mariusandra]

We have an index for the event name:

posthog/posthog/models/event_definition.py

Line 23 in 60cbf2e

GinIndex(name="index_event_definition_name", fields=["name"], opclasses=["gin_trgm_ops"]),

... but seems like it's indeed not used.

[mariusandra]

Performance is really poor for this user:

But you're right again. We can try just the ordering without the index.

[paolodamico]

lgtm! let's try this out, won't merge cause we'll all be asleep if something goes wrong, but feel free to merge once you see this

[yakkomajuri]

Perhaps this should be one of the first uses for the "Special Migrations API"? @yakkomajuri

"Get in line" lol

On a more serious note, I was thinking of running a change to Replicated dead letter queue tables, and ofc we have the events migration too.

However, I'd be happy to have you rather than me as a "Beta" tester for writing a special migration, if you're willing. Will probably be good for feedback.