-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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(web-analytics): Use person properties for web analytics source #18423
feat(web-analytics): Use person properties for web analytics source #18423
Conversation
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
de0f28e
to
467c145
Compare
467c145
to
9389bdd
Compare
Hey @robbie-c! 👋 |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
ab235c8
to
79bcea2
Compare
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One big blocker inline, but otherwise looks good.
e4529f1
to
ea6da9f
Compare
📸 UI snapshots have been updated4 snapshot changes in total. 0 added, 4 modified, 0 deleted:
Triggered by this commit. |
Problem
Ideally we would use these client-session params, however due to PostHog/posthog-js#876 these don't work very well at the moment. Instead, just use person properties for now. This will be slower, but acceptable for the time being, and unblocks releasing this to more people.
Changes
How did you test this code?
Manually tested, I also did #18424