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: swap the default to identified_only #1468

Merged
merged 11 commits into from
Oct 17, 2024
4 changes: 2 additions & 2 deletions src/__tests__/personProcessing.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ describe('person processing', () => {
}

describe('init', () => {
it("should default to 'always' person_profiles", async () => {
it("should default to 'identified_only' person_profiles", async () => {
// arrange
const token = uuidv7()

Expand All @@ -65,7 +65,7 @@ describe('person processing', () => {
})

// assert
expect(posthog.config.person_profiles).toEqual('always')
expect(posthog.config.person_profiles).toEqual('identified_only')
})
it('should read person_profiles from init config', async () => {
// arrange
Expand Down
23 changes: 19 additions & 4 deletions src/__tests__/posthog-core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,20 @@ describe('posthog core', () => {

expect(posthog.compression).toEqual('gzip-js')
})
it('uses defaultIdentifiedOnly from decide response', () => {
const posthog = posthogWith({})

posthog._afterDecideResponse({ defaultIdentifiedOnly: true } as DecideResponse)
expect(posthog.config.person_profiles).toEqual('identified_only')

posthog._afterDecideResponse({ defaultIdentifiedOnly: false } as DecideResponse)
expect(posthog.config.person_profiles).toEqual('always')
})
it('defaultIdentifiedOnly does not override person_profiles if already set', () => {
const posthog = posthogWith({ person_profiles: 'always' })
posthog._afterDecideResponse({ defaultIdentifiedOnly: true } as DecideResponse)
expect(posthog.config.person_profiles).toEqual('always')
})

it('enables compression from decide response when only one received', () => {
const posthog = posthogWith({})
Expand Down Expand Up @@ -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: {} },
Copy link
Member Author

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn 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

} as unknown as PostHogPersistence,
sessionPersistence: {
properties: () => ({ distinct_id: 'abc', persistent: 'prop' }),
Expand Down Expand Up @@ -425,7 +440,7 @@ describe('posthog core', () => {
$window_id: 'windowId',
$session_id: 'sessionId',
$is_identified: false,
$process_person_profile: true,
$process_person_profile: false,
})
})

Expand All @@ -447,7 +462,7 @@ describe('posthog core', () => {
$session_id: 'sessionId',
$lib_custom_api_host: 'https://custom.posthog.com',
$is_identified: false,
$process_person_profile: true,
$process_person_profile: false,
})
})

Expand All @@ -465,7 +480,7 @@ describe('posthog core', () => {
posthog._calculate_event_properties('custom_event', { event: 'prop' }, new Date())[
'$process_person_profile'
]
).toEqual(true)
).toEqual(false)
})

it('only adds token and distinct_id if event_name is $snapshot', () => {
Expand Down Expand Up @@ -496,7 +511,7 @@ describe('posthog core', () => {
expect(posthog._calculate_event_properties('custom_event', { event: 'prop' }, new Date())).toEqual({
event_name: 'custom_event',
token: 'testtoken',
$process_person_profile: true,
$process_person_profile: false,
})
})

Expand Down
16 changes: 15 additions & 1 deletion src/posthog-core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Copy link
Member Author

Choose a reason for hiding this comment

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

the main change for swapping the default

__add_tracing_headers: false,
})

Expand Down Expand Up @@ -272,6 +272,7 @@ export class PostHog {
decideEndpointWasHit: boolean
analyticsDefaultEndpoint: string
version = Config.LIB_VERSION
_initialPersonProfilesConfig: 'always' | 'never' | 'identified_only' | null

SentryIntegration: typeof SentryIntegration
sentryIntegration: (options?: SentryIntegrationOptions) => ReturnType<typeof sentryIntegration>
Expand All @@ -294,6 +295,7 @@ export class PostHog {
this.__loaded = false
this.analyticsDefaultEndpoint = '/e/'
this._initialPageviewCaptured = false
this._initialPersonProfilesConfig = null

this.featureFlags = new PostHogFeatureFlags(this)
this.toolbar = new Toolbar(this)
Expand Down Expand Up @@ -390,6 +392,10 @@ export class PostHog {
this.config = {} as PostHogConfig // will be set right below
this._triggered_notifs = []

if (config.person_profiles) {
this._initialPersonProfilesConfig = config.person_profiles
}
Comment on lines +395 to +397
Copy link
Member Author

Choose a reason for hiding this comment

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

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.


this.set_config(
extend({}, defaultConfig(), configRenames(config), {
name: name,
Expand All @@ -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?
Copy link
Member Author

Choose a reason for hiding this comment

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

question

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

TLDR no I don't think so :D

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

No this is great, thanks!

const initialPersistenceProps = { ...this.persistence.props }
const initialSessionProps = { ...this.sessionPersistence.props }

Expand Down Expand Up @@ -538,6 +546,12 @@ export class PostHog {
this.analyticsDefaultEndpoint = response.analytics.endpoint
}

this.config.person_profiles = this._initialPersonProfilesConfig
? this._initialPersonProfilesConfig
: response['defaultIdentifiedOnly']
? 'identified_only'
: 'always'
raquelmsmith marked this conversation as resolved.
Show resolved Hide resolved

this.sessionRecording?.afterDecideResponse(response)
this.autocapture?.afterDecideResponse(response)
this.heatmaps?.afterDecideResponse(response)
Expand Down
1 change: 1 addition & 0 deletions src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -398,6 +398,7 @@ export interface DecideResponse {
isAuthenticated: boolean
siteApps: { id: number; url: string }[]
heatmaps?: boolean
defaultIdentifiedOnly?: boolean
Copy link
Member Author

Choose a reason for hiding this comment

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

this will be returned with /decide

}

export type FeatureFlagsCallback = (
Expand Down
Loading