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(hogql): Add feature flag for opting queries into v3 persons-on-events #21150

Merged
merged 5 commits into from
Mar 27, 2024

Conversation

tkaemming
Copy link
Contributor

@tkaemming tkaemming commented Mar 26, 2024

Problem

See #20460 and PostHog/product-internal#557 for context.

Changes

This adds a feature flag for opting teams/organizations into PoE v3.

Rather than being part of the posthog.utils.PersonOnEventsMode enum which applies to all (or at least most) queries, v3 only is available in HogQL via posthog.schema.PersonsOnEventsMode, so this is handled when determining the HogQL modifiers for a team, rather than as part of Team.person_on_events_mode which might be expected.

This also means that until all queries are run via HogQL, there is the potential for queries for the same team to be run with different PoE versions depending on that team's flag and settings configuration! The differences between non-PoE results and v3 results should be relatively minor (the only known discrepancies should be due to deletions) but the differences between v3 when compared to v1 or v2 will be more significant as they either do not have any overrides (v1) or the overrides were never fully backfilled (v2), so care should be taken when opting teams into this mode.

Does this work well for both Cloud and self-hosted?

This currently shouldn't enabled in self-hosted, and will fall back to the current behavior, though it can be enabled through settings or the right configuration to enable local evaluation of flags at your own risk.

How did you test this code?

Added test, also enabled setting in local environment and clicked around a bit though it's challenging to make too many assertions from that due to caching.

@tkaemming tkaemming mentioned this pull request Mar 26, 2024
10 tasks
@tkaemming tkaemming marked this pull request as ready for review March 26, 2024 16:18
Copy link
Collaborator

@mariusandra mariusandra left a comment

Choose a reason for hiding this comment

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

Looks good to me

@tkaemming tkaemming merged commit 205376b into master Mar 27, 2024
128 of 129 checks passed
@tkaemming tkaemming deleted the poe-v3-hogql-optin branch March 27, 2024 20:24
timgl added a commit that referenced this pull request Apr 9, 2024
timgl added a commit that referenced this pull request Apr 11, 2024
* Revert "feat(hogql): Add feature flag for opting queries into v3 persons-on-events (#21150)"

This reverts commit 205376b.

* fix: Three clear options for persons on events

* Update UI snapshots for `chromium` (1)

* use UUID to target feature flag

* fix

* add missing option

* fix

* Update UI snapshots for `chromium` (2)

* Update UI snapshots for `chromium` (2)

* Update UI snapshots for `chromium` (2)

* Update posthog/models/team/team.py

Co-authored-by: ted kaemming <65315+tkaemming@users.noreply.github.com>

* Update posthog/models/team/team.py

Co-authored-by: ted kaemming <65315+tkaemming@users.noreply.github.com>

* fix schema

* fix

* Update UI snapshots for `chromium` (2)

* Update UI snapshots for `chromium` (2)

---------

Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: ted kaemming <65315+tkaemming@users.noreply.github.com>
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.

2 participants