feat(feature-flags): Enable experience continuity with feature flags #10196
Conversation
…nto feature-flag-distincts
…nto feature-flag-distincts
|
I put together an idea in figma for how we can communicate this functionality to users. https://www.figma.com/file/gQBj9YnNgD8YW4nBwCVLZf?node-id=5400:35869#208945086
|
| "posthog_featureflagoverride"."override_value", | ||
| "posthog_featureflag"."id", | ||
| "posthog_featureflag"."key" | ||
| FROM "posthog_featureflagoverride" |
There was a problem hiding this comment.
this cruft from existing overrides makes me sad.
Specially, since this applies to a very small minority of users: Those who have a posthog account, while being called for every user coming to the client's website.
At minimum, we should just check if this distinct ID belongs to a posthog user, and then use the value inside this query. Hmm, but then, if featureflagoverride table is small, not sure it makes a difference.
There was a problem hiding this comment.
Could we kill this then? This won't affect any real operations and we could if we wanted to.
There was a problem hiding this comment.
Good question. I think killing this will remove the toolbar feature flags functionality. Don't think it's used anywhere else, is it? (cc: @mariusandra @rcmarron )
At the very least, I suppose we could go down this code path only when asking for /my_flags or /decide has a parameter used by the toolbar to make it happen.
This way, we can pretty much get the best of both worlds, by keeping this functionality, and removing cruft completely for most users.
Although I do question if the toolbar override functionality is used at all, @marcushyett-ph ?
|
Asking for ingestion side reviews as well, since there's an update when merging two persons together. I think it's fine to do as I have, since we're in a transaction, but lmk if I'm missing something, @tiina303 , @macobo . Edit: Also, If I'm not mistaken, the deploy will ensure that migrations run before django & plugin server updates, right? Or do I need to split out the plugin-server part to a separate PR? |
| @@ -1475,6 +1476,184 @@ test('distinct with anonymous_id which was already created', async () => { | |||
| expect(person.is_identified).toEqual(true) | |||
| }) | |||
|
|
|||
| test('distinct with anonymous_id which was already created triggers foreign key updates', async () => { | |||
There was a problem hiding this comment.
hmm, should add a test in django-land for where old user is identified, which means we won't reach this code path, and no updates would occur. Will the override django code handle that?
|
|
||
| // For FeatureFlagHashKeyOverrides | ||
| const allOverrides = await this.db.postgresQuery( | ||
| 'SELECT id, person_id, feature_flag_key FROM posthog_featureflaghashkeyoverride WHERE team_id = $1 AND person_id = ANY($2)', |
There was a problem hiding this comment.
(didn't look further into this PR): Why do we select + do post-processing + update instead of a single update query? We're introducing a lot of new failure modes here.
There was a problem hiding this comment.
A https://stackoverflow.com/questions/5874453/postgresql-way-to-insert-row-with-on-conflict-clause-semantics might work better if you're looking for UPDATE but avoids conflicts case. - just add ON CONFLICT DO NOTHING to the INSERT.
If you go this route, please also move this query to db.ts and give it proper unit tests - integration tests are nice but also really frustrating/can leave gaps.
There was a problem hiding this comment.
ooh, that's clever!
| from django.db import migrations, models | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): |
There was a problem hiding this comment.
Migration to a separate PR - otherwise this is unrevertable.
| @@ -150,7 +150,9 @@ def get_decide(request: HttpRequest): | |||
| team = user.teams.get(id=project_id) | |||
|
|
|||
| if team: | |||
| feature_flags = get_overridden_feature_flags(team.pk, data["distinct_id"], data.get("groups", {})) | |||
| feature_flags = get_overridden_feature_flags( | |||
There was a problem hiding this comment.
If we're touching /decide endpoint, that should be a separate PR to ensure easy control if something goes wrong. This single endpoint breaking can bring down all of posthog-js ingestion in nasty ways.
| @@ -798,4 +792,62 @@ export class EventsProcessor { | |||
| ) | |||
| } | |||
| } | |||
|
|
|||
| private async handleTablesDependingOnPersonID( | |||
There was a problem hiding this comment.
Note: This should be a separate PR given the risk of accidentally breaking ingestion.
| ) -> Dict[str, Union[bool, str, None]]: | ||
| feature_flags = get_active_feature_flags(team_id, distinct_id, groups) |
There was a problem hiding this comment.
Is this feature flag still used anywhere? If so, it's a sign that maybe it shouldn't be?
There was a problem hiding this comment.
not sure I get this. Do you mean overrides only applying to active feature flags? Or something else?
There was a problem hiding this comment.
I mean get_active_feature_flags as a function
There was a problem hiding this comment.
oh, lovely! Good shout
…nto feature-flag-distincts
| except IndexError: | ||
| person_id = None | ||
|
|
||
| if person_id is None and hash_key_override is not None: |
There was a problem hiding this comment.
NIT: Can we move if hash_key_override to a top level if and make the person_id if/else more obvious. Was going back and forth when looking at this sequence of checks
| # existing person. If, because of race conditions, a person merge is called for later, | ||
| # then https://github.com/PostHog/posthog/blob/master/plugin-server/src/worker/ingestion/process-event.ts#L480 | ||
| # will take care of it^. | ||
| try: |
There was a problem hiding this comment.
I think you can use .first() here
Ref: https://docs.djangoproject.com/en/dev/ref/models/querysets/#first
There was a problem hiding this comment.
Note: I have not worked with the flag matching logic in a while so I may not be 100% aware of the potholes in all the related areas that are being affected here. But, rolling out according to @macobo's should help a lot
Good work!
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
Problem
Have a look here: #9547 for context. We're going with option 3!
The code in
posthog/models/feature_flag.pymight seem a bit wrangly, but there's a good reason for it: My goal here was to make sure:decideendpoint takes exactly the same number of queries as before.There's some cases we still don't handle well, specially to do with ingestion delays, but I don't think there's a simple way around that. We're borked anyway in that case, since person properties filtering won't work, which means the feature flag, even if it had experience continuity, wouldn't have worked.
Changes
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
How did you test this code?