feat: Add person info to events #9404

tiina303 · 2022-04-12T22:55:11Z

Problem

For #9180

Changes

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

Depends on #9377

added UUID caching as that's what we'll want to be using in CH (based on the table types, but also I assume postgres person_id would be pretty useless there)
Query cache if the value isn't there query backing datastore and put it into cache. Designed it in a way that the user doesn't need to think about needing to updating the cache.
The person info is grouped together for efficiency as we'll be using it together (I assume), so if the value isn't in cache we'll query postgres once & can run the cache lookup in parallel (if they all separately fell back to postgres we'd either need to not run them in parallel or have multiple postgres reads & redis writes).

How did you test this code?

ran the migration from feat(persons-on-events): add required person and group columns to events table #9251
Turned off Geo IP (to have less data in properties)
Ran locally with

CLICKHOUSE_DISABLE_EXTERNAL_SCHEMAS_TEAMS="1,2,3,4,5" PERSON_INFO_TO_REDIS_TEAMS="1,2,3,4,5" ./bin/start

from py lib

>>> posthog.capture('test-buf-test-7', 'test-event-2', properties={ '$set': { 'userProperty': 997 } })
>>> posthog.capture('test-buf-test-7', 'test-event-3')

saw the event was ingested
In local CH instance

SELECT
    event,
    uuid,
    timestamp,
    distinct_id,
    person_id,
    person_properties
FROM events
WHERE distinct_id = 'test-buf-test-7'

Query id: 1d063222-2675-4655-b0de-bb3c05b839f1

┌─event────────┬─uuid─────────────────────────────────┬──────────────────timestamp─┬─distinct_id─────┬─person_id────────────────────────────┬─person_properties────┐
│ test-event-3 │ 01802553-d076-0000-6642-d0a13cc95343 │ 2022-04-13 23:48:11.064000 │ test-buf-test-7 │ 0180254f-3025-0000-26b6-ee09cce318fa │ {"userProperty":997} │
└──────────────┴──────────────────────────────────────┴────────────────────────────┴─────────────────┴──────────────────────────────────────┴──────────────────────┘
┌─event────────┬─uuid─────────────────────────────────┬──────────────────timestamp─┬─distinct_id─────┬─person_id────────────────────────────┬─person_properties────┐
│ test-event-2 │ 0180254f-301c-0000-3739-106d42db8d98 │ 2022-04-13 23:43:07.990000 │ test-buf-test-7 │ 0180254f-3025-0000-26b6-ee09cce318fa │ {"userProperty":997} │
└──────────────┴──────────────────────────────────────┴────────────────────────────┴─────────────────┴──────────────────────────────────────┴──────────────────────┘

SELECT *
FROM person_distinct_id2

Query id: 80833953-d523-4e52-9275-619a838c610b

┌─team_id─┬─distinct_id─────┬─person_id────────────────────────────┬─is_deleted─┬─version─┬──────────_timestamp─┬─_offset─┬─_partition─┐
│       1 │ test-buf-test-7 │ 0180254f-3025-0000-26b6-ee09cce318fa │          0 │       0 │ 2022-04-13 23:43:13 │      20 │          0 │
└─────────┴─────────────────┴──────────────────────────────────────┴────────────┴─────────┴─────────────────────┴─────────┴────────────┘

btw didn't see any change in the UI so we might need to change that too (note that I wasn't running on top of the branch with migration):

plugin-server/src/utils/db/db.ts

plugin-server/src/worker/ingestion/process-event.ts

yakkomajuri

looking good, some nits.

I wonder if it's a worthy project for the future to have posthog_persondistinctid map distinct id -> UUID and add an index for posthog_person to use UUID. Probably is - toss it in the backlog lol

yakkomajuri · 2022-04-14T12:38:18Z

plugin-server/src/utils/db/db.ts

+        if (personId !== null) {
+            return await this.getPersonInfoThroughCache(teamId, personId)
+        }
+        return [null, null, null]


I think I'd rather return an object as that makes it much easier to work with instead of having to remember the order of the returned values

plugin-server/src/utils/db/db.ts

plugin-server/src/worker/ingestion/process-event.ts

yakkomajuri

mostly looks good. one comment + have you tested JSON ingestion with this change?

yakkomajuri · 2022-04-25T10:57:58Z

plugin-server/src/worker/ingestion/process-event.ts

@@ -576,6 +576,9 @@ export class EventsProcessor {

        const elementsChain = elements && elements.length ? elementsToString(elements) : ''

+        // TODO: don't parse back and forth with json


let's then just add a flag getRawProperties or something. It can event be a separate value in the return object

tiina303 · 2022-04-25T11:03:54Z

mostly looks good. one comment + have you tested JSON ingestion with this change?

Yes, check the test plan.

yakkomajuri · 2022-04-25T11:12:24Z

Wanna give the Kafka Inspector a try and see what the raw message looks like? @tiina303 😉

plugin-server/src/utils/db/db.ts

tiina303 · 2022-04-25T12:12:32Z

plugin-server/src/utils/db/db.ts

@@ -663,6 +768,7 @@ export class DB {
        }

        await this.updatePersonPropertiesCache(updatedPerson.team_id, updatedPerson.id, updatedPerson.properties)
+        await this.updatePersonCreatedAtCache(updatedPerson.team_id, updatedPerson.id, updatedPerson.created_at)


forgot this earlier - on merges we might be updating create_at currently too

tiina303 · 2022-04-25T13:27:36Z

yakkomajuri · 2022-04-25T13:53:51Z

Very nice!

yakkomajuri

lgtm

let's monitor as we merge and confirm:

a) that all events are being ingested with no issues
b) what the messages look like in Kafka

tiina303 · 2022-04-25T14:24:18Z

Tested without CLICKHOUSE_DISABLE_EXTERNAL_SCHEMAS_TEAMS & PERSON_INFO_TO_REDIS_TEAMS enabled too. Only have tested on top of the schema change.

tiina303 · 2022-04-26T11:04:06Z

Tested locally without the schema change, it worked fine (DEBUG=1 ./bin/start & events got ingested)

* master: (137 commits) feat(cohorts): add cohort filter grammars (#9540) feat(cohorts): Backwards compatibility of groups and properties (#9462) perf(ingestion): unsubscribe from buffer topic while no events are produced to it (#9556) fix: Fix `Loading` positioning and `LemonButton` disabled state (#9554) test: Speed up backend tests (#9289) fix: LemonSpacer -> LemonDivider (#9549) feat(funnels): Highlight significant deviations in new funnel viz (#9536) docs(storybook): Lemon UI (#9426) feat: add support for list of teams to enable the conversion buffer for (#9542) chore(onboarding): cleanup framework grid experiment (#9527) fix(signup): domain provisioning on cloud (#9515) chore: split out async migrations ci (#9539) feat(ingestion): enable json ingestion for self-hosted by default (#9448) feat(cohort): add all cohort filter selectors to Storybook (#9492) feat(ingestion): conversion events buffer consumer (#9432) ci(run-backend-tests): remove CH version default (#9532) feat: Add person info to events (#9404) feat(ingestion): produce to buffer partitioned by team_id:distinct_id (#9518) fix: bring latest_migrations.manifest up to date (#9525) chore: removes unused feature flag (#9529) ...

tiina303 marked this pull request as draft April 12, 2022 22:58

tiina303 force-pushed the poe-person-info-to-events branch 3 times, most recently from fd7cd27 to 8d2f222 Compare April 13, 2022 00:10

Base automatically changed from poe-write-to-redis to master April 13, 2022 10:34

tiina303 force-pushed the poe-person-info-to-events branch 2 times, most recently from 6d4e322 to d873578 Compare April 13, 2022 13:46

tiina303 requested a review from yakkomajuri April 14, 2022 00:02

tiina303 marked this pull request as ready for review April 14, 2022 00:03

tiina303 commented Apr 14, 2022

View reviewed changes

plugin-server/src/utils/db/db.ts Show resolved Hide resolved

yakkomajuri reviewed Apr 14, 2022

View reviewed changes

plugin-server/src/utils/db/db.ts Outdated Show resolved Hide resolved

plugin-server/src/worker/ingestion/process-event.ts Show resolved Hide resolved

yakkomajuri approved these changes Apr 14, 2022

View reviewed changes

yakkomajuri suggested changes Apr 19, 2022

View reviewed changes

yakkomajuri reviewed Apr 25, 2022

View reviewed changes

tiina303 added 8 commits April 25, 2022 14:10

feat: Add person info to events

1e329e9

make it actually work

4a4184d

comments

1de83f1

address review

37a1ccd

typo

4344736

nit

28066bb

nit

1858840

Store raw strings in Redis for efficiency

f935606

tiina303 force-pushed the poe-person-info-to-events branch from ddcb960 to f935606 Compare April 25, 2022 12:10

tiina303 commented Apr 25, 2022

View reviewed changes

plugin-server/src/utils/db/db.ts Outdated Show resolved Hide resolved

tiina303 commented Apr 25, 2022

View reviewed changes

Revert back to using Properties typed for safety

608f37e

yakkomajuri approved these changes Apr 25, 2022

View reviewed changes

tiina303 merged commit 98619cf into master Apr 26, 2022

tiina303 deleted the poe-person-info-to-events branch April 26, 2022 11:05

yakkomajuri mentioned this pull request Apr 27, 2022

Plan for rolling out persons/groups on events to individual Cloud teams #9555

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add person info to events #9404

feat: Add person info to events #9404

tiina303 commented Apr 12, 2022 •

edited

Loading

yakkomajuri left a comment

yakkomajuri Apr 14, 2022

yakkomajuri left a comment •

edited

Loading

yakkomajuri Apr 25, 2022

tiina303 commented Apr 25, 2022

yakkomajuri commented Apr 25, 2022

tiina303 Apr 25, 2022

tiina303 commented Apr 25, 2022

yakkomajuri commented Apr 25, 2022

yakkomajuri left a comment

tiina303 commented Apr 25, 2022

tiina303 commented Apr 26, 2022 •

edited

Loading

		@@ -576,6 +576,9 @@ export class EventsProcessor {

		const elementsChain = elements && elements.length ? elementsToString(elements) : ''

		// TODO: don't parse back and forth with json

feat: Add person info to events #9404

feat: Add person info to events #9404

Conversation

tiina303 commented Apr 12, 2022 • edited Loading

Problem

Changes

How did you test this code?

yakkomajuri left a comment

Choose a reason for hiding this comment

yakkomajuri Apr 14, 2022

Choose a reason for hiding this comment

yakkomajuri left a comment • edited Loading

Choose a reason for hiding this comment

yakkomajuri Apr 25, 2022

Choose a reason for hiding this comment

tiina303 commented Apr 25, 2022

yakkomajuri commented Apr 25, 2022

tiina303 Apr 25, 2022

Choose a reason for hiding this comment

tiina303 commented Apr 25, 2022

yakkomajuri commented Apr 25, 2022

yakkomajuri left a comment

Choose a reason for hiding this comment

tiina303 commented Apr 25, 2022

tiina303 commented Apr 26, 2022 • edited Loading

tiina303 commented Apr 12, 2022 •

edited

Loading

yakkomajuri left a comment •

edited

Loading

tiina303 commented Apr 26, 2022 •

edited

Loading