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

Refactor Session Recording Proxy Configuration #475

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

Conversation

chrisknu
Copy link

Summary
This PR changes how Mixpanel session recording determines whether to use a proxy when sending data. It adds a semantically clear session_recording_use_proxy parameter to direct the heavy usage/recording payloads to the proxy with a default of false. If true, it ships to the proxy; else default behaviour.

Changes
Added session_recording_use_proxy configuration parameter (default: false)
Defaults to the direct route to Mixpanel (which is the optimal path for most users)

Why this change?
When using the api_host proxy for capturing/forwarding usage/* events, issues were had trying to pass through the relatively large usage/record event/payloads. Instead of incurring the throughput, its simpler/cleaner to just ship usage/record events straight to mixpanel.

Copy link
Member

@tdumitrescu tdumitrescu left a comment

Choose a reason for hiding this comment

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

I think the move here would be to allow each endpoint to specify its own api host, the way we currently allow them to specify their own paths with the api_routes config. You might do this by adding an api_hosts config parallel to the api_routes one (and let those take precedence over the api_host config if present).

@chrisknu
Copy link
Author

Good idea. I'll take a swag at that in a few

@chrisknu
Copy link
Author

@tdumitrescu
I should probably also mention the below config snippet was problematic in a slightly related way. To get the non-proxy/proxy usage/record URL changes to work, I had to preload the mixpanel-recorder snippet (with api_host changes) in my app to supercede the CDN version (no api_host changes)

'recorder_src': 'https://cdn.mxpnl.com/libs/mixpanel-recorder.min.js',

If/when merged, the config URL shouldn't be a problem but I wanted to see if there was an easier way to config/point to a source (CDN or local in-app asset)

@tdumitrescu
Copy link
Member

I had to preload the mixpanel-recorder snippet (with api_host changes) in my app to supercede the CDN version

I'm not sure what that means. The way to switch the location of the mixpanel-recorder bundle for development is to use that config option, e.g.:

mixpanel.init('my token', {recorder_src: 'https://url.of.local.dev.sdk/build/mixpanel-recorder.js'});

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
@chrisknu
Copy link
Author

@tdumitrescu Refactored out the single use config override; added this._mixpanel._getApiHost(<route/type>)

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