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

add support to person profiles #171

Merged
merged 17 commits into from
Sep 11, 2024

Conversation

thisames
Copy link
Contributor

@thisames thisames commented Sep 4, 2024

💡 Motivation and Context

This PR adds support for the personProfiles configuration option in the PostHog SDK. The personProfiles option allows users to control how person profile data is processed during various events.

reference
https://github.com/PostHog/product-internal/pull/637
https://github.com/PostHog/posthog-js/blob/0b40e5af5381c7d356de31fc0458e91afbfa9a6b/src/types.ts#L206-L213

fix #159

💚 How did you test it?

  • Manually tested by sending multiple events to the PostHog dashboard with all three personProfiles settings ("always", "never", "identified_only") to confirm that the events are processed according to the expected behavior for each setting.

  • Added unit tests in PostHogTest to verify the behavior of the personProfiles setting:

    • Ensured that all events are captured when personProfiles is set to "always".
    • Validated that no profile processing occurs when personProfiles is set to "never".
    • Checked that non-identification events do not process profiles when personProfiles is set to "identified_only".
    • Confirmed that identification events process profiles correctly when personProfiles is set to "identified_only".

⚠️ Observations

  • Noticed a potential issue: when personProfiles is set to "never" or "identified_only", the flag process_person_profile is added to the "screen" event, but the event still appears on the dashboard. This behavior is unexpected, as the flag should filter out the event, preventing it from showing up on the dashboard. It seems that additional logic may be needed to properly handle "screen" events according to the personProfiles setting.

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • I updated the docs if needed.
  • No breaking change or entry added to the changelog.

@marandaneto
Copy link
Member

Add a TODO somewhere that we need to implement createPersonProfile and setPersonProperties, which can be a separate PR, another time.

@marandaneto
Copy link
Member

I think this check is also missing somewhere https://github.com/PostHog/posthog-js/blob/34204f4aff107162bdb259d2a7c855d67bf0260c/src/posthog-core.ts#L829 which is about setting userPropertiesSetOnce

@thisames
Copy link
Contributor Author

thisames commented Sep 5, 2024

I will fix all that you commented, soon update

@thisames
Copy link
Contributor Author

thisames commented Sep 6, 2024

I'm sorry @marandaneto, I thought the implementation of this feature was based solely on the documentation. I didn't realize there was already a reference implementation for it, which is why my code looks confusing, as I developed it from my own logic.

I now understand exactly how to do it correctly, but I still have a few questions.

In the identify_only documentation, it states that it will only process events related to identifying:

'identified_only' - we will only process persons when you call posthog.identify, posthog.alias, posthog.setPersonProperties, posthog.group, posthog.setPersonPropertiesForFlags, or posthog.setGroupPropertiesForFlags. Anonymous users won't get person profiles.

But in the actual posthog-js implementation, it seems to process any type of event, or am I mistaken?

Additionally, in this PR, is it necessary to implement the persistence mechanism, like for dataSetOnce, for example?

@marandaneto
Copy link
Member

I'm sorry @marandaneto, I thought the implementation of this feature was based solely on the documentation. I didn't realize there was already a reference implementation for it, which is why my code looks confusing, as I developed it from my own logic.

I now understand exactly how to do it correctly, but I still have a few questions.

In the identify_only documentation, it states that it will only process events related to identifying:

'identified_only' - we will only process persons when you call posthog.identify, posthog.alias, posthog.setPersonProperties, posthog.group, posthog.setPersonPropertiesForFlags, or posthog.setGroupPropertiesForFlags. Anonymous users won't get person profiles.

But in the actual posthog-js implementation, it seems to process any type of event, or am I mistaken?

Additionally, in this PR, is it necessary to implement the persistence mechanism, like for dataSetOnce, for example?

the flag is set for any type of event, the same as the JS implementation.
the dataSetOnce bits can be done later such as #171 (comment), so everything but this part can be the same as the JS implementation.

@thisames
Copy link
Contributor Author

thisames commented Sep 9, 2024

@marandaneto I updated the PR with only what's necessary to make the personProfiles config feature work. Just the hasPersonProcessing function solves the problem.

However, I introduced a complete version with the dependencies that complete the personProfile feature. If possible, take a look at this version and see if it makes sense. Based on my tests in core-js and in this version's code, everything seems fine.

thisames@0f0c39a

Please analyze whether it makes sense to include this in the PR or leave it for later.

@marandaneto
Copy link
Member

@marandaneto I updated the PR with only what's necessary to make the personProfiles config feature work. Just the hasPersonProcessing function solves the problem.

However, I introduced a complete version with the dependencies that complete the personProfile feature. If possible, take a look at this version and see if it makes sense. Based on my tests in core-js and in this version's code, everything seems fine.

thisames@0f0c39a

Please analyze whether it makes sense to include this in the PR or leave it for later.

you can run make api, and make format just to be sure its passing CI.
It'd be cool to add tests for this flag as well, if $process_person_profile is being set true or false correctly.

@marandaneto
Copy link
Member

About the commit thisames@0f0c39a
Looks good, I'd have a few comments, lets make a new PR so we can discuss.

@marandaneto
Copy link
Member

marandaneto commented Sep 10, 2024

@thisames see my PR PostHog/posthog-ios#187
I'd copy what is there (I copied what was in JS).
Also, it's missing the requirePersonProcessing bits, see my PR as well

thisames and others added 5 commits September 10, 2024 18:03
- Introduced 'ENABLE_PERSON_PROCESSING' to control person processing logic.
- Added `requirePersonProcessing` method to validate if person processing is enabled before capturing events like 'alias', 'identify', and 'group'.
- Modified `capture` method to enforce person processing when necessary properties are provided.
- Enhanced `hasPersonProcessing` to check against the new 'ENABLE_PERSON_PROCESSING' preference.
- Updated methods `alias`, `identify`, and `group` to respect person processing rules.
- Improved logging for clearer debugging.
@marandaneto
Copy link
Member

@thisames I pushed a few changes to fix and improve a few things, thanks for the PR.

@marandaneto marandaneto merged commit 1dca934 into PostHog:main Sep 11, 2024
4 checks passed
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.

Support person_profiles
2 participants