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: Person processing 2 - handle group and setPersonProperties #1124

Merged
merged 4 commits into from
Apr 9, 2024

Conversation

robbie-c
Copy link
Member

@robbie-c robbie-c commented Apr 9, 2024

Changes

Calling any of the following functions will activate person processing if set to identified_only, and fail if set to never:

  • indentify
  • alias
  • setPersonProperties
  • setPersonPropertiesForFlags
  • group
  • setGroupPropertiesForFlags

Checklist

  • Tests for new code (see advice on the tests we use)
  • Accounted for the impact of any changes across different browsers
  • Accounted for backwards compatibility of any changes (no breaking changes in posthog-js!)

Copy link

github-actions bot commented Apr 9, 2024

Size Change: +2.54 kB (0%)

Total Size: 950 kB

Filename Size Change
dist/array.full.js 226 kB +635 B (0%)
dist/array.js 127 kB +634 B (+1%)
dist/es.js 127 kB +634 B (+1%)
dist/module.js 128 kB +634 B (0%)
ℹ️ View Unchanged
Filename Size
dist/exception-autocapture.js 12.2 kB
dist/recorder-v2.js 105 kB
dist/recorder.js 105 kB
dist/surveys-module-previews.js 62 kB
dist/surveys.js 58.3 kB

compressed-size-action

@robbie-c robbie-c marked this pull request as ready for review April 9, 2024 12:48
@PostHog PostHog deleted a comment from posthog-bot Apr 9, 2024
@tiina303
Copy link
Contributor

tiina303 commented Apr 9, 2024

it stores the group properties locally

why would we store them? I'm guessing we'll still want to send group selection, e.g. that it's group_0 = x or whatever, but the properties can't be used anywhere

Calling setPersonProperties now doesn't send the $set event if person processing is not active

why not make that switch into personful mode too?

@robbie-c
Copy link
Member Author

robbie-c commented Apr 9, 2024

why would we store them? I'm guessing we'll still want to send group selection, e.g. that it's group_0 = x or whatever, but the properties can't be used anywhere

My understanding from reading the code is that they can be used locally be feature flags. Tagging @neilkakkar who can answer definitively 👋

why not make that switch into personful mode too?

This way means that they can be used locally for feature flags.

Could make them both just fail if we're in never mode though, and upgrade in identified_only

@neilkakkar
Copy link
Collaborator

That is true for both person and group properties, as an optimisation when events haven't been ingested.

Unclear to me whether they'll be able to create flags like these with person-less processing, but ok to keep both props for flags as is on client side - these are independent of anything sent to capture.

@robbie-c
Copy link
Member Author

robbie-c commented Apr 9, 2024

Unclear to me whether they'll be able to create flags like these with person-less processing

It would make sense to me that they shouldn't be able to, so I'll update this PR to this effect

robbie-c added 2 commits April 9, 2024 17:37
Use for alias, identify, group, setGroupPropertiesForFlags, setPersonProperties, setPersonPropertiesForFlags
@neilkakkar
Copy link
Collaborator

Haven't been closely following the updates here, but is this flow possible?:

  1. I have person-full events, using which I create a flag on property X.
  2. I set props for an anonymous user with property X
  3. Should the flag work? It would if we allow setPersonPropertiesForFlags, but wouldn't if we don't.

Eventually on ingestion I think property X will be dropped, so if the same anon user comes back again in a fresh session (is this even possible?) the flag will be false. But in the same session the flag will be true.

@robbie-c
Copy link
Member Author

robbie-c commented Apr 9, 2024

It depends on which person processing mode you're in.

  • If always then nothing has changed.
  • If identified_only then the first time you call setPersonPropertiesForFlags (step 2 in your example) it'll enable person processing and the flag should work
  • If never then you can't call setPersonPropertiesForFlags, you just get a warning.

if the same anon user comes back again in a fresh session (is this even possible?)

This is possible, if we store the distinct id in the cookie / localstorage then it'll persist for a while, and if the user returns after gap they'll get a fresh session id

@neilkakkar
Copy link
Collaborator

reasonable, thanks!

@robbie-c robbie-c added the bump patch Bump patch version when this PR gets merged label Apr 9, 2024
@robbie-c robbie-c merged commit 82962b3 into main Apr 9, 2024
12 checks passed
@robbie-c robbie-c deleted the feat/person-processing-2 branch April 9, 2024 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump patch Bump patch version when this PR gets merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants