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

First and last seen event #4641

Closed
wants to merge 1 commit into from
Closed

First and last seen event #4641

wants to merge 1 commit into from

Conversation

liyiy
Copy link
Contributor

@liyiy liyiy commented Jun 8, 2021

Changes

Return the first/last seen event with last_seen=true or first_seen=true for #4267

url example: ?properties=%7B%7D&event=watched_movie&orderBy=%5B"timestamp"%5D&last_seen=true

Checklist

  • All querysets/queries filter by Organization, by Team, and by User
  • Django backend tests
  • Jest frontend tests
  • Cypress end-to-end tests
  • Migrations are safe to run at scale (e.g. PostHog Cloud) – present proof if not obvious
  • Frontend/CSS is usable at 320px (iPhone SE) and decent at 360px (most phones)

(cherry picked from commit 55c468218e23b9c56dacdc107fe5bae10acf2d09)
@liyiy liyiy requested a review from EDsCODE June 8, 2021 16:42
@timgl timgl temporarily deployed to posthog-pr-4641 June 8, 2021 16:44 Inactive
@liyiy liyiy requested a review from paolodamico June 8, 2021 16:45
@liyiy
Copy link
Contributor Author

liyiy commented Jun 8, 2021

@EDsCODE I don't have clickhouse on my computer I might need some help with the clickhouse version of this even though it seems pretty straightforward 🤔

@mariusandra
Copy link
Collaborator

I wonder if it wouldn't be better in the long run if we add first_seen_at and last_seen_at fields to the event definition table, and update them in the plugin server on ingestion (with some throttling built in).

Saying this because it might be especially costly with Clickhouse to go back to the beginning of your billions of events, and find the first occurrence of something. Or it might not be, I don't know to be honest :).

@EDsCODE
Copy link
Member

EDsCODE commented Jun 8, 2021

I wonder if it wouldn't be better in the long run if we add first_seen_at and last_seen_at fields to the event definition table, and update them in the plugin server on ingestion (with some throttling built in).

Was about to suggest this because the first seen timestamp will never change so we don't need to keep calculating it

Does the last seen timestamp need to be completely up to date?

@liyiy
Copy link
Contributor Author

liyiy commented Jun 8, 2021

I wonder if it wouldn't be better in the long run if we add first_seen_at and last_seen_at fields to the event definition table, and update them in the plugin server on ingestion (with some throttling built in).

Was about to suggest this because the first seen timestamp will never change so we don't need to keep calculating it

Does the last seen timestamp need to be completely up to date?

Yes I think the last seen timestamp should also be completely up to date. This would still be updated on the plugin-server, correct? Seems like all I should do here is just add those fields to the event definition model instead of this then

@mariusandra
Copy link
Collaborator

There are a few complexities here:

  • We have existing posthog installations that need these values populated. However setting these in a migration is a no-go since this can take approximately forever, and that will crash most upgrade deployments.
  • The plugin server should write the last_seen_at fields. These writes shouldn't happen once per event, as at this volume we're talking a lot of postgres traffic. They should somehow be buffered and synced every so often.

Nothing some code won't fix though :)

@paolodamico
Copy link
Contributor

Seems like this will change based on Eric's & Marius's feedback. Tag me if you need another review 😉

@paolodamico paolodamico removed their request for review June 9, 2021 02:27
@liyiy
Copy link
Contributor Author

liyiy commented Jun 9, 2021

Closing while I update

@liyiy liyiy closed this Jun 9, 2021
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.

5 participants