feat: swap the default to identified_only #1468

raquelmsmith · 2024-10-15T03:46:41Z

Warning

This should only be merged after we support the field response from /decide.

Changes

We want to make the person_profiles: identified_only config the default for everyone. It will save everyone money - even if they don't change their config - and is better for our customers and our pipeline.

Instead of just pushing out a change to posthog-js for this, we want to be able to control the behavior and roll it out slowly, just in case there are any problems.

So, this PR changes the default, but also listens to a field from decide that will say if we should be defaulting people into identified_only, or using the old default always. If someone has set their config manually, then we will continue using that setting.

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!)

vercel · 2024-10-15T03:46:45Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
posthog-js	✅ Ready (Inspect)	Visit Preview	Oct 15, 2024 7:31pm

posthog-bot · 2024-10-15T03:46:54Z

Hey @raquelmsmith! 👋
This pull request seems to contain no description. Please add useful context, rationale, and/or any other information that will help make sense of this change now and in the distant Mars-based future.

raquelmsmith · 2024-10-15T03:48:10Z

src/__tests__/posthog-core.ts

@@ -387,6 +401,7 @@ describe('posthog core', () => {
 properties: () => ({ distinct_id: 'abc', persistent: 'prop', $is_identified: false }),
 remove_event_timer: jest.fn(),
 get_property: () => 'anonymous',
+ props: { $groups: {} },


adding this made most the tests pass with the new identified_only default config. Does this indicate that something else needs to happen somewhere to make sure real users always have this in their props? Or is it just a test fixture thing? (I imagine the latter because people can use identifed_only with no problem, but want to be certain)

It feels like the latter, but I'll stare at this code a little to convince myself a bit more

Ok, it was the absence of props in the mock that made the test fail, it didn't actually need $groups: {}, so I've slightly changed this line to have a more minimal mock

raquelmsmith · 2024-10-15T03:48:31Z

src/posthog-core.ts

@@ -179,7 +179,7 @@ export const defaultConfig = (): PostHogConfig => ({
 bootstrap: {},
 disable_compression: false,
 session_idle_timeout_seconds: 30 * 60, // 30 minutes
- person_profiles: 'always',
+ person_profiles: 'identified_only',


the main change for swapping the default

raquelmsmith · 2024-10-15T03:49:31Z

src/posthog-core.ts

+ if (config.person_profiles) {
+ this._initialPersonProfilesConfig = config.person_profiles
+ }


in order to roll this out slowly, we need to know if the person has already set their config - that it's not just using the default. So we need to store the initial config somewhere.

raquelmsmith · 2024-10-15T03:49:40Z

src/posthog-core.ts

@@ -408,6 +414,8 @@ export class PostHog {
 this.config.persistence === 'sessionStorage'
 ? this.persistence
 : new PostHogPersistence({ ...this.config, persistence: 'sessionStorage' })
+
+ // should I store the initial person profiles config in persistence?


The way I've handled this so far is to save whether or not they've sent an event with person processing. If they have then we persist ENABLE_PERSON_PROCESSING to true.

I'm just wondering whether saving their config gives us anything beyond this.

Here's a couple of cases:

Case 1

posthog-js starts up with the user setting 'always'

some events are sent, with person profiles, so ENABLE_PERSON_PROCESSING is set

user closes the browser

new visit, posthog-js starts up with no setting so uses the default identified_only

some events are sent, as ENABLE_PERSON_PROCESSING was set then these have person profiles

Case 2

posthog-js starts up with the user setting 'always'

no events are sent, so ENABLE_PERSON_PROCESSING is not set

user closes the browser

new visit, posthog-js starts up with no setting so uses the default identified_only

some events are sent, these are anonymous (if we'd persisted the setting from before, they'd have person profiles)

In case 2 there's a difference, but I'd argue that those events should be sent as anonymous. Wdyt?

TLDR no I don't think so :D

Ooooook so it turns out we don't actually handle this case correctly. I added a fix and some tests to this PR. I hope you don't mind me piggybacking like this, I can very much move it to its own PR if you prefer.

No this is great, thanks!

raquelmsmith · 2024-10-15T03:50:02Z

src/types.ts

@@ -398,6 +398,7 @@ export interface DecideResponse {
 isAuthenticated: boolean
 siteApps: { id: number; url: string }[]
 heatmaps?: boolean
+ defaultIdentifiedOnly?: boolean


this will be returned with /decide

github-actions · 2024-10-15T03:50:32Z

Size Change: +3.12 kB (+0.11%)

Total Size: 2.83 MB

Filename	Size	Change
`dist/array.full.js`	355 kB	+347 B (+0.1%)
`dist/array.full.no-external.js`	354 kB	+347 B (+0.1%)
`dist/array.js`	169 kB	+347 B (+0.21%)
`dist/array.no-external.js`	168 kB	+347 B (+0.21%)
`dist/main.js`	170 kB	+347 B (+0.2%)
`dist/module.full.js`	355 kB	+347 B (+0.1%)
`dist/module.full.no-external.js`	354 kB	+347 B (+0.1%)
`dist/module.js`	169 kB	+347 B (+0.21%)
`dist/module.no-external.js`	168 kB	+347 B (+0.21%)

ℹ️ View Unchanged

Filename	Size
`dist/all-external-dependencies.js`	192 kB
`dist/exception-autocapture.js`	11.1 kB
`dist/external-scripts-loader.js`	2.35 kB
`dist/recorder-v2.js`	111 kB
`dist/recorder.js`	111 kB
`dist/surveys-preview.js`	59.8 kB
`dist/surveys.js`	66 kB
`dist/tracing-headers.js`	8.36 kB
`dist/web-vitals.js`	10.3 kB

_{compressed-size-action}

src/posthog-core.ts

robbie-c · 2024-10-15T07:38:26Z

Woohoo!

A caveat I just wanted to flag - this PR does the right thing but if we did things the other way round (set always as the default and override that to identified_only in decide) then it wouldn't work. Once we send an event with $process_person_profile: true then we persist a flag called ENABLE_PERSON_PROCESSING to ensure all following events also have $process_person_profile: true. If the default was always then the first pageview (which happens before decide returns) would set that flag and we'd never send any anonymous events.

Either way, this PR does the right thing, so maybe you already knew all of this :D

Edit: I found a bug in how we were handling the logic above, so I've added a fix and some tests to this PR

…only

robbie-c

LGTM! I added some stuff, so please make sure those changes LGTY

raquelmsmith · 2024-10-15T19:45:17Z

Looks good to me! Thanks for the help @robbie-c!!

I'll wait to merge this until I get the /decide changes in today or tomorrow.

raquelmsmith · 2024-10-17T20:47:02Z

Oh @robbie-c I forgot to specify what type of version bump this is.... will it bump automatically? Or just with the next one?

raquelmsmith added 2 commits October 14, 2024 20:45

swap the default

700a639

use value from decide to override config

b72e8de

raquelmsmith commented Oct 15, 2024

View reviewed changes

vercel bot deployed to Preview October 15, 2024 03:50 View deployment

fix bug

0de33c3

raquelmsmith commented Oct 15, 2024

View reviewed changes

src/posthog-core.ts Outdated Show resolved Hide resolved

vercel bot deployed to Preview October 15, 2024 03:58 View deployment

Fix test

e7b986b

vercel bot deployed to Preview October 15, 2024 08:48 View deployment

robbie-c added 2 commits October 15, 2024 10:12

Remove comment saving process_person=true is default

6798a2e

Handle changing the person processing mode from always to identified …

51a2ed2

…only

vercel bot deployed to Preview October 15, 2024 09:44 View deployment

Fix tests

b39fb37

vercel bot deployed to Preview October 15, 2024 09:58 View deployment

Reduce mock

642b349

vercel bot deployed to Preview October 15, 2024 10:02 View deployment

robbie-c added 2 commits October 15, 2024 11:16

Add person processing tests to handle decide response

2080958

Add test

bf232be

robbie-c approved these changes Oct 15, 2024

View reviewed changes

vercel bot deployed to Preview October 15, 2024 10:29 View deployment

use set_config

745e594

vercel bot deployed to Preview October 15, 2024 19:31 View deployment

raquelmsmith mentioned this pull request Oct 17, 2024

feat: add decide field based on env var for rollout of identified_only as default PostHog/posthog#25662

Merged

raquelmsmith marked this pull request as ready for review October 17, 2024 20:31

raquelmsmith merged commit 387d96e into main Oct 17, 2024
14 checks passed

raquelmsmith deleted the feat/identified-only-default branch October 17, 2024 20:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: swap the default to identified_only #1468

feat: swap the default to identified_only #1468

raquelmsmith commented Oct 15, 2024 •

edited

Loading

vercel bot commented Oct 15, 2024 •

edited

Loading

posthog-bot commented Oct 15, 2024

raquelmsmith Oct 15, 2024

robbie-c Oct 15, 2024

robbie-c Oct 15, 2024

raquelmsmith Oct 15, 2024

raquelmsmith Oct 15, 2024

raquelmsmith Oct 15, 2024

robbie-c Oct 15, 2024

robbie-c Oct 15, 2024

robbie-c Oct 15, 2024

raquelmsmith Oct 15, 2024

raquelmsmith Oct 15, 2024

github-actions bot commented Oct 15, 2024 •

edited

Loading

robbie-c commented Oct 15, 2024 •

edited

Loading

robbie-c left a comment

raquelmsmith commented Oct 15, 2024 •

edited

Loading

raquelmsmith commented Oct 17, 2024

feat: swap the default to identified_only #1468

feat: swap the default to identified_only #1468

Conversation

raquelmsmith commented Oct 15, 2024 • edited Loading

Changes

Checklist

vercel bot commented Oct 15, 2024 • edited Loading

posthog-bot commented Oct 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 15, 2024 • edited Loading

robbie-c commented Oct 15, 2024 • edited Loading

robbie-c left a comment

Choose a reason for hiding this comment

raquelmsmith commented Oct 15, 2024 • edited Loading

raquelmsmith commented Oct 17, 2024

raquelmsmith commented Oct 15, 2024 •

edited

Loading

vercel bot commented Oct 15, 2024 •

edited

Loading

github-actions bot commented Oct 15, 2024 •

edited

Loading

robbie-c commented Oct 15, 2024 •

edited

Loading

raquelmsmith commented Oct 15, 2024 •

edited

Loading