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: Allow setting asset_host separately from api_host #942

Closed
wants to merge 4 commits into from

Conversation

frankh
Copy link
Contributor

@frankh frankh commented Dec 19, 2023

Changes

Currently we fetch static assets from the api_host. This has a bunch of issues and we'd like to separate these (so that e.g. static assets can be behind a CDN but not the API)

Checklist

Copy link
Collaborator

@benjackwhite benjackwhite left a comment

Choose a reason for hiding this comment

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

Not sure if this is the way to go... if we ever wanted it to be a universal change, we would have to get every user to add an additional config item. 🤔

Could we write out the various configurations we expect to get to and what they solve first? (Just trying to reduce the work and confusing setups that users have to implement as we end up stuck with them for basically forever...)

@benjackwhite
Copy link
Collaborator

Follow up thought - part of the thing here is to make sure that blockers don't add blocks to non-tracking domains (leading to our web app or other parts being broken).

I feel like adding a new assets host will just make it clear "oh these are more hosts we should block" which just exacerbates the problem right?

In my mind best would be to have tracking assets and tracking endpoints linked together as a default. That way tracker-blockers (that we have to live with) don't block anything they shouldn't, and if someone uses a reverse proxy, then the chances of successful tracking increase as both assets and tracking goes via that route?

Which again comes down to my general thought - how does a separate assets host actually benefit us at all? 😅

@frankh
Copy link
Contributor Author

frankh commented Dec 19, 2023

the idea is:

  1. it defaults to how it is now, so nobody has to update config for things to continue working
  2. we automatically map asset_host for our api_hosts (app.posthog.com -> app-static-prod.posthog.com)
  3. we allow people to optionally pass in asset_host if they're proxying api host (but by default we'll just use api_host like we do now)

the main reason for separate hosts is to have a CDN on static assets but not the api endpoints

@frankh frankh changed the title Allow setting asset_host separately from api_host feat: Allow setting asset_host separately from api_host Dec 19, 2023
Copy link

github-actions bot commented Dec 19, 2023

Size Change: +968 B (0%)

Total Size: 753 kB

Filename Size Change
dist/array.full.js 177 kB +242 B (0%)
dist/array.js 118 kB +242 B (0%)
dist/es.js 118 kB +242 B (0%)
dist/module.js 119 kB +242 B (0%)
ℹ️ View Unchanged
Filename Size
dist/exception-autocapture.js 12 kB
dist/recorder-v2.js 104 kB
dist/recorder.js 58.4 kB
dist/surveys.js 46.8 kB

compressed-size-action

@frankh frankh force-pushed the frank/allow-setting-asset-host branch from 17511bd to 8b4016f Compare December 19, 2023 09:55
@benjackwhite
Copy link
Collaborator

the idea is:

  1. it defaults to how it is now, so nobody has to update config for things to continue working
  2. we automatically map asset_host for our api_hosts (app.posthog.com -> app-static-prod.posthog.com)
  3. we allow people to optionally pass in asset_host if they're proxying api host (but by default we'll just use api_host like we do now)

the main reason for separate hosts is to have a CDN on static assets but not the api endpoints

  1. Sure
  2. This is the bit I feel is going to be an issue (for the reasons I outlined in the other comment)
  3. Reading the code again I see what you mean - if you reverse proxy then it won't be any different

I still feel like we need a tracking-specific CDN for those assets, otherwise eventually it will get added to a block list and then our whole app will be broken again

@posthog-bot
Copy link
Collaborator

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@posthog-bot
Copy link
Collaborator

This PR was closed due to lack of activity. Feel free to reopen if it's still relevant.

@posthog-bot posthog-bot closed this Jan 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants