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

feat(cdp): client side destinations #26169

Open
wants to merge 67 commits into
base: master
Choose a base branch
from
Open

feat(cdp): client side destinations #26169

wants to merge 67 commits into from

Conversation

mariusandra
Copy link
Collaborator

@mariusandra mariusandra commented Nov 13, 2024

Problem

We want to do filter matching in the client side using Hog, and also replicate the Google Tag Manager product.

Requires PostHog/posthog-js#1546

Changes

Adds "client-side destinations" as a new destination type:

2024-11-28 16 41 29

Refactors "site apps" to use this same system, and migrate over both site apps (pineapple + notification). You can now modify the app code:

2024-11-28 16 42 47

There were many changes that made themselves into this PR:

  • Hog Function and Hog Function Template API endpoints now support filtering by multiple types
  • Refactor pipelineDestinations into hogFunctionsList in order to abstract it between the different destination and site app types, and parameterize the logic
  • Add "site destinations" into "destinations", and create a new "site apps" view to replace the old one.
    • If your team has any plugin-based site apps enabled, include those at the end of the page.
  • Make one extra query in /decide for teams with site apps enabled to load new hog-function-site-apps/destinations. We're moving to something more static soon anyway.
  • Add group type aliases to CDP, like "organization.properties.name", in addition to "group_0.properties.name"

How did you test this code?

Added various tests

Open questions

  • Security.

    • The JavaScript for old site apps is loaded from /site_app/:id/:token/:content_hash. The new site functions are loaded from /site_function/:uuid/:content_hash. For example the URL for a new site function is: http://localhost:8000/site_function/01935491-740c-0000-5262-5d79a1d54094/a595e3847458e6ea6795cc1a07fe5db7/
    • The content hash is just for caching, not for verification. It's just a hash over the function's updated_at.
    • The new functions use a UUID instead of a integer, but don't have the "token". Should we add one?
  • CDN

    • We now make one extra query to fetch the new site apps/functions in each /decide, in addition to one query to fetch the old site apps. Once we deprecate, migrate and remove the old site apps, it'll be back to just one query. Is that acceptable for now? We could cache the site apps into Team as well.
    • We should add some kind of caching/CDN layer in front. Some options?
      • Easiest: redis cache for the same id/hash
      • Medium?: we upload the generated JS to a S3 bucket every time it changes
      • Hard?: some caching server (a'la varnish) in front of the /site_functions URL
    • Will CORS or cross domain script loading cause problems?
    • Site apps already don't work with reverse proxies.

TODO in other PRs

  • Some way to test? Via the toolbar?
  • Put it all behind a CDN
  • Verify Autocapture selector matching
  • Log errors as PostHog events
  • Migrate existing site app users to the new system

@mariusandra mariusandra marked this pull request as draft November 13, 2024 15:29
Base automatically changed from hog-to-js to master November 18, 2024 13:38
Copy link
Contributor

github-actions bot commented Nov 20, 2024

Size Change: +362 B (+0.03%)

Total Size: 1.11 MB

ℹ️ View Unchanged
Filename Size Change
frontend/dist/toolbar.js 1.11 MB +362 B (+0.03%)

compressed-size-action

@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

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

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

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

  • chromium: 0 added, 16 modified, 0 deleted (diff for shard 1)
  • webkit: 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

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

  • chromium: 0 added, 10 modified, 0 deleted (diff for shard 1)
  • webkit: 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

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

  • chromium: 0 added, 10 modified, 0 deleted (diff for shard 1)
  • webkit: 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, 2 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@mariusandra mariusandra marked this pull request as ready for review December 2, 2024 13:52
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.

2 participants