add support to person profiles

thisames commented 2 months ago

:bulb: 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

:green_heart: 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.

:pencil: Checklist

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

marandaneto commented 2 months ago

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

marandaneto commented 2 months ago

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 commented 2 months ago

I will fix all that you commented, soon update

thisames commented 2 months ago

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 commented 1 month ago

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 https://github.com/PostHog/posthog-android/pull/171#issuecomment-2330727362, so everything but this part can be the same as the JS implementation.

thisames commented 1 month ago

@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.

https://github.com/thisames/posthog-android/commit/0f0c39a37799e867c2a484b6862341595a211923

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

marandaneto commented 1 month ago

@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 commented 1 month ago

About the commit https://github.com/thisames/posthog-android/commit/0f0c39a37799e867c2a484b6862341595a211923 Looks good, I'd have a few comments, lets make a new PR so we can discuss.

marandaneto commented 1 month ago

@thisames see my PR https://github.com/PostHog/posthog-ios/pull/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

marandaneto commented 1 month ago

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

PostHog / posthog-android