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: RemoteConfig logic (decide replacement) #26348

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

Conversation

benjackwhite
Copy link
Contributor

@benjackwhite benjackwhite commented Nov 22, 2024

Problem

Needs #26534

See https://github.com/PostHog/product-internal/pull/680 for more info.

Generally the idea is that the new flags service will replace the "decide" django endpoint with a much more scalable solution. The thing it won't do necessarily is handle the "remote config" side of things which is currently the other main use for decide.

My idea is to generate the "config" parts into a dedicated helper model which takes care of creating this config derived from the various settings allowing us to do a few cool things:

  1. Serve the config as a static file via a CDN - no hot servers needed
  2. Back support /decide filling in from the same highly cached concept
  3. Build an additional file which is a token-scoped file containing all of the config and all of the posthog-js (one load to rule them all)
  4. Allow us to GTM style things for the destinations to inject extra JS loading in a hyper optimal way

Megaissue here

Changes

This PR is just part 1 - generate the RemoteConfigs as and when they should be, with new endpoints to serve them (endpoints that will eventually be served by the CDN instead).

  • RemoteConfig class that generates this config as and when it is needed
  • Saving writes the data to S3 and invalidates the CDN (at least in a flagged way)
  • Tracks a metric so we can alarm on it
  • Generates a second file that actually includes the site_apps code to be executed
  • Generates a third file that is a personalised array.js loader
  • Fix loading of array.js in code...

Follow up work

  • Figure out what to do about the one or two replay things that are "dynamic" (like checking the domain...)
  • Bundle dependencies automatically based on configs (i.e. surveys if surveys are enabled, recordings etc.)
  • Cronjob that takes care of ensuring things are in sync (just in case)
    • Maybe do this with a hash or something so we can detect whether changes are necessary or not
  • Decide endpoint having an option to use this RemoteConfig instead of the existing team access (with tests to cover it all)
  • Hook up to CDN and add cache invalidation

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

Does this work well for both Cloud and self-hosted?

How did you test this code?

Copy link
Contributor

github-actions bot commented Nov 28, 2024

Size Change: +30 B (0%)

Total Size: 1.11 MB

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

compressed-size-action

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

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

  • chromium: 0 added, 18 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.

@benjackwhite benjackwhite mentioned this pull request Dec 2, 2024
18 tasks
# Conflicts:
#	frontend/__snapshots__/scenes-other-settings--settings-project--dark.png
#	frontend/__snapshots__/scenes-other-settings--settings-project--light.png
#	frontend/__snapshots__/scenes-other-settings--settings-project-with-replay-features--dark.png
#	frontend/__snapshots__/scenes-other-settings--settings-project-with-replay-features--light.png
#	frontend/__snapshots__/scenes-other-settings--settings-session-timeout-all-options--dark.png
#	frontend/__snapshots__/scenes-other-settings--settings-session-timeout-all-options--light.png
#	frontend/__snapshots__/scenes-other-settings--settings-session-timeout-password-only--dark.png
#	frontend/__snapshots__/scenes-other-settings--settings-session-timeout-password-only--light.png
#	frontend/__snapshots__/scenes-other-settings--settings-session-timeout-sso-enforced-github--dark.png
#	frontend/__snapshots__/scenes-other-settings--settings-session-timeout-sso-enforced-github--light.png
#	frontend/__snapshots__/scenes-other-settings--settings-session-timeout-sso-enforced-google--dark.png
#	frontend/__snapshots__/scenes-other-settings--settings-session-timeout-sso-enforced-google--light.png
#	frontend/__snapshots__/scenes-other-settings--settings-session-timeout-sso-enforced-saml--dark.png
#	frontend/__snapshots__/scenes-other-settings--settings-session-timeout-sso-enforced-saml--light.png
#	frontend/__snapshots__/scenes-other-settings--settings-session-timeout-sso-only--dark.png
#	frontend/__snapshots__/scenes-other-settings--settings-session-timeout-sso-only--light.png
#	frontend/src/lib/constants.tsx
#	posthog/migrations/max_migration.txt
@benjackwhite benjackwhite marked this pull request as ready for review December 2, 2024 13:00
@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.

Copy link
Collaborator

@mariusandra mariusandra left a comment

Choose a reason for hiding this comment

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

Cool stuff

benjackwhite and others added 4 commits December 4, 2024 13:46
# Conflicts:
#	frontend/__snapshots__/scenes-other-settings--settings-project--dark.png
#	frontend/__snapshots__/scenes-other-settings--settings-project--light.png
#	frontend/__snapshots__/scenes-other-settings--settings-project-with-replay-features--dark.png
#	frontend/__snapshots__/scenes-other-settings--settings-project-with-replay-features--light.png
#	frontend/__snapshots__/scenes-other-settings--settings-session-timeout-all-options--dark.png
#	frontend/__snapshots__/scenes-other-settings--settings-session-timeout-all-options--light.png
#	frontend/__snapshots__/scenes-other-settings--settings-session-timeout-password-only--dark.png
#	frontend/__snapshots__/scenes-other-settings--settings-session-timeout-password-only--light.png
#	frontend/__snapshots__/scenes-other-settings--settings-session-timeout-sso-enforced-github--dark.png
#	frontend/__snapshots__/scenes-other-settings--settings-session-timeout-sso-enforced-github--light.png
#	frontend/__snapshots__/scenes-other-settings--settings-session-timeout-sso-enforced-google--dark.png
#	frontend/__snapshots__/scenes-other-settings--settings-session-timeout-sso-enforced-google--light.png
#	frontend/__snapshots__/scenes-other-settings--settings-session-timeout-sso-enforced-saml--dark.png
#	frontend/__snapshots__/scenes-other-settings--settings-session-timeout-sso-enforced-saml--light.png
#	frontend/__snapshots__/scenes-other-settings--settings-session-timeout-sso-only--dark.png
#	frontend/__snapshots__/scenes-other-settings--settings-session-timeout-sso-only--light.png
#	posthog/api/site_app.py
#	posthog/migrations/max_migration.txt
#	posthog/plugins/site.py
#	posthog/session_recordings/test/__snapshots__/test_session_recordings.ambr
#	posthog/urls.py
@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.

@@ -257,7 +257,7 @@
LOGOUT_URL = "/logout"
LOGIN_REDIRECT_URL = "/"
APPEND_SLASH = False
CORS_URLS_REGEX = r"^(/site_app/|/api/(?!early_access_features|surveys|web_experiments).*$)"
CORS_URLS_REGEX = r"^(/site_app/|/array/|/api/(?!early_access_features|surveys|web_experiments).*$)"
Copy link
Member

Choose a reason for hiding this comment

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

there's a config somewhere in this file iirc to enable gzip for these new endpoints

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.

4 participants