fix(plugin-server): write out less overrides on behalf of personless … #23204
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bretthoerner
force-pushed
the
brett/less-overrides-phase-1
branch
from
7ffda0b to
70c8a39
Compare
bretthoerner
force-pushed
the
brett/less-overrides-phase-1
branch
2 times, most recently
from
1f7134f to
a98d0cb
Compare
tkaemming
approved these changes
Jun 25, 2024
plugin-server/src/utils/db/db.ts
Outdated
Comment on lines
653
to
656
| // This should never happen and is only here to catch code during development/tests | ||
| if (distinctIds.length != distinctIdVersions.length) { | ||
| throw new Error('createPerson: distinctIds and distinctIdVersions lengths must match') | ||
| } |
There was a problem hiding this comment.
It looks like you already called this out over here but it seems like maybe the signature for this should be {distinctId: string, version?: number}[] or similar instead to avoid this invariant check?
plugin-server/src/utils/db/db.ts
Outdated
| @@ -662,7 +675,12 @@ export class DB { | |||
| // `addDistinctIdPooled` | |||
| (_, index) => `, distinct_id_${index} AS ( | |||
| INSERT INTO posthog_persondistinctid (distinct_id, person_id, team_id, version) | |||
| VALUES ($${10 + index}, (SELECT id FROM inserted_person), $5, $9))` | |||
| VALUES ( | |||
| $${11 + index + distinctIdVersions!.length - 1}, | |||
There was a problem hiding this comment.
Kind of surprised that the type checker can't just infer that distinctIdVersions can't be undefined at this point.
There was a problem hiding this comment.
I was too, vscode gave me no signal that it was made but for some reason the tsc build itself failed without this...
| VALUES ($1, $2, true, now()) | ||
| ON CONFLICT (team_id, distinct_id) DO UPDATE | ||
| SET is_merged = true | ||
| RETURNING (xmax = 0) AS inserted |
Comment on lines
+559
to
+579
| // `createPerson` uses the first Distinct ID provided to generate the Person | ||
| // UUID. That means the first Distinct ID definitely doesn't need an override, | ||
| // and can always use version 0. Below, we exhaust all of the options to decide | ||
| // whether we can optimize away an override by doing a swap, or whether we | ||
| // need to actually write an override. (But mostly we're being verbose for | ||
| // documentation purposes) | ||
| let distinctId2Version = 1 // TODO: Once `posthog_personlessdistinctid` is backfilled, this should be = 0 | ||
| if (insertedDistinctId1 && insertedDistinctId2) { | ||
| // We were the first to insert both (neither was used for Personless), so we | ||
| // can use either as the primary Person UUID and create no overrides. | ||
| } else if (insertedDistinctId1 && !insertedDistinctId2) { | ||
| // We created 1, but 2 was already used for Personless. Let's swap so | ||
| // that 2 can be the primary Person UUID and no override is needed. | ||
| ;[distinctId1, distinctId2] = [distinctId2, distinctId1] | ||
| } else if (!insertedDistinctId1 && insertedDistinctId2) { | ||
| // We created 2, but 1 was already used for Personless, so we want to | ||
| // use 1 as the primary Person UUID so that no override is needed. | ||
| } else if (!insertedDistinctId1 && !insertedDistinctId2) { | ||
| // Both were used in Personless mode, so there is no more-correct choice of | ||
| // primary Person UUID to make here, and we need to drop an override by | ||
| // using version = 1 for Distinct ID 2. | ||
| distinctId2Version = 1 | ||
| } |
There was a problem hiding this comment.
Covering all the branches like this is really helpful for thinking through this, thanks for writing it all out.
bretthoerner
force-pushed
the
brett/less-overrides-phase-1
branch
from
a98d0cb to
c64694b
Compare
bretthoerner
force-pushed
the
brett/less-overrides-phase-1
branch
from
c64694b to
9c5c57e
Compare
bretthoerner
added a commit
that referenced
this pull request
Jul 1, 2024
fuziontech
pushed a commit
that referenced
this pull request
Jul 2, 2024
#23204) * fix(plugin-server): write out less overrides on behalf of personless mode * don't skip writing overrides (so we can backfill posthog_personlessdistinctid) * swap ugly double-array params for array of objects * bigint id for PersonlessDistinctId * add LRU cache for posthog_personlessdistinctid inserts * fix tests * overzealous search and replace
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…mode## Problem
This is a copy of #23164 with a commit added so that we keep writing out unnecessary overrides.
We would deploy this, then backfill
posthog_personlessdistinctid(from CH to PG), and then we'd be able to turn on the override optimizations by undoing the extra commit here.Changes
See above.
Does this work well for both Cloud and self-hosted?
Yes
How did you test this code?
Tests