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

fix(events-explorer): events explorer improvements #14648

Merged
merged 14 commits into from
Mar 9, 2023

Conversation

mariusandra
Copy link
Collaborator

@mariusandra mariusandra commented Mar 9, 2023

Problem

  • The events explorer table is slow in its default view for our team (10sec for 24h of events), and OOMs for some of our largest users due to the persons table join.
  • Without the join results always come back in under a second.

Changes

  • tldr: makes the events explorer table usable
  • Uses a separate query to fetch persons, just like we did in the old events table.
  • This applies just to selecting a special column "person" in "EventsQuery" that powers the events table, not to HogQL itself.
  • Person property filters would still rely on a join like they did before
  • Also flyby updates some example queries, limiting their timeranges.
  • Also flyby changes the events table "live update" interval from 5 sec to 30sec.

How did you test this code?

Added a test and clicked around locally.

@mariusandra mariusandra requested a review from a team March 9, 2023 09:46
Copy link
Member

@pauldambra pauldambra left a comment

Choose a reason for hiding this comment

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

Do I need to do some setup to make this work?

where-they-be

for column_index, column in enumerate(select_input_raw):
if column != "person":
continue
for index, result in enumerate(query_result.results):
Copy link
Member

Choose a reason for hiding this comment

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

I'm not super clear on what all is going on in these things yet 🙈

This is an in-memory loop not an N+1 query?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are all in memory loops.

To get it to work, just select more than "last 24h"

Copy link
Member

Choose a reason for hiding this comment

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

To get it to work, just select more than "last 24h"

🙈 🤣

Then I get

Screenshot 2023-03-09 at 10 28 49

Copy link
Member

Choose a reason for hiding this comment

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

return !!personRecord.distinct_ids.length ? (

when

{
    "uuid": "01869ed9-85a3-0000-f0dc-c6dbf095aef6",
    "created_at": "2023-03-01T20:24:59.852Z",
    "properties": {
        "name": null,
        "email": "paul@posthog.com"
    },
    "distinct_id": "a0JNkovhz54zqTBN2pBKf8jqA488qudRnugUzVshN2E"
}

Copy link
Member

Choose a reason for hiding this comment

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

Dumping in

else if (key === 'person' && isEventsQuery(query.source)) {
        const personRecord = value as PersonType
        let distinctId = undefined
        if (personRecord.distinct_ids && personRecord.distinct_ids.length > 0) {
            distinctId = personRecord.distinct_ids[0]
        } else if (personRecord.distinct_id) {
            distinctId = personRecord.id
        }
        return distinctId ? (
            <Link to={urls.person(distinctId)}>
                <PersonHeader noLink withIcon person={personRecord} />
            </Link>
        ) : (
            <PersonHeader noLink withIcon person={value} />
        )
    }

and the table displays (although the type system is unhappy)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

whoops. Should be fixed now 🙈

Copy link
Member

@pauldambra pauldambra left a comment

Choose a reason for hiding this comment

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

other than the distinct_id vs distinct_ids kerfuffle :shipit:

@posthog-bot
Copy link
Contributor

📸 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.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

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

  • chromium: 0 added, 1 modified, 0 deleted (wasn't pushed!)
  • 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.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

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

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

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@mariusandra mariusandra enabled auto-merge (squash) March 9, 2023 11:04
@mariusandra mariusandra merged commit 0fca570 into master Mar 9, 2023
@mariusandra mariusandra deleted the events-table-interval branch March 9, 2023 11:15
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.

3 participants