feat: Write person ID and properties to redis by tiina303 · Pull Request #9377 · PostHog/posthog

tiina303 · 2022-04-10T16:49:04Z

Problem

Storing person info in redis:

teamId, distinctId -> personId
teamId, personId -> createdAt
teamId, personId -> properties

Changes

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

How did you test this code?

Ran locally with PERSON_INFO_TO_REDIS_TEAMS="1,2,3,4,5" ./bin/start
Sent an event with posthog python >>> posthog.capture('test-id3', 'test-event', properties={ '$set': { 'userProperty': 16 } })
Verified that info was in Redis as expected:

$ redis-cli
>  keys person*
1) "person_props:1:5"
2) "person_create_at:1:5"
3) "person_id:1:test-id3"
127.0.0.1:6379> keys person*
1) "person_props:1:5"
2) "person_create_at:1:5"
3) "person_id:1:test-id3"
127.0.0.1:6379> get person_props:1:5
"\"{\\\"$initial_geoip_city_name\\\":\\\"Sydney\\\",\\\"$initial_geoip_country_name\\\":\\\"Australia\\\",\\\"$initial_geoip_country_code\\\":\\\"AU\\\",\\\"$initial_geoip_continent_name\\\":\\\"Oceania\\\",\\\"$initial_geoip_continent_code\\\":\\\"OC\\\",\\\"$initial_geoip_postal_code\\\":\\\"1148\\\",\\\"$initial_geoip_latitude\\\":-33.8715,\\\"$initial_geoip_longitude\\\":151.2006,\\\"$initial_geoip_time_zone\\\":\\\"Australia/Sydney\\\",\\\"$initial_geoip_subdivision_1_code\\\":\\\"NSW\\\",\\\"$initial_geoip_subdivision_1_name\\\":\\\"New South Wales\\\",\\\"userProperty\\\":16,\\\"$geoip_city_name\\\":\\\"Sydney\\\",\\\"$geoip_country_name\\\":\\\"Australia\\\",\\\"$geoip_country_code\\\":\\\"AU\\\",\\\"$geoip_continent_name\\\":\\\"Oceania\\\",\\\"$geoip_continent_code\\\":\\\"OC\\\",\\\"$geoip_postal_code\\\":\\\"1148\\\",\\\"$geoip_latitude\\\":-33.8715,\\\"$geoip_longitude\\\":151.2006,\\\"$geoip_time_zone\\\":\\\"Australia/Sydney\\\",\\\"$geoip_subdivision_1_code\\\":\\\"NSW\\\",\\\"$geoip_subdivision_1_name\\\":\\\"New South Wales\\\"}\""
127.0.0.1:6379> get person_create_at:1:5
"\"2022-04-12T13:24:00.825Z\""
127.0.0.1:6379> get person_id:1:test-id3
"5"

Changed the userProperty with py >>> posthog.capture('test-id3', 'test-event', properties={ '$set': { 'userProperty': 334 } })
In redis:

127.0.0.1:6379> get person_props:1:5
"\"{\\\"userProperty\\\":334,\\\"$geoip_latitude\\\":-33.8715,\\\"$geoip_city_name\\\":\\\"Sydney\\\",\\\"$geoip_longitude\\\":151.2006,\\\"$geoip_time_zone\\\":\\\"Australia/Sydney\\\",\\\"$geoip_postal_code\\\":\\\"1148\\\",\\\"$geoip_country_code\\\":\\\"AU\\\",\\\"$geoip_country_name\\\":\\\"Australia\\\",\\\"$geoip_continent_code\\\":\\\"OC\\\",\\\"$geoip_continent_name\\\":\\\"Oceania\\\",\\\"$initial_geoip_latitude\\\":-33.8715,\\\"$initial_geoip_city_name\\\":\\\"Sydney\\\",\\\"$initial_geoip_longitude\\\":151.2006,\\\"$initial_geoip_time_zone\\\":\\\"Australia/Sydney\\\",\\\"$geoip_subdivision_1_code\\\":\\\"NSW\\\",\\\"$geoip_subdivision_1_name\\\":\\\"New South Wales\\\",\\\"$initial_geoip_postal_code\\\":\\\"1148\\\",\\\"$initial_geoip_country_code\\\":\\\"AU\\\",\\\"$initial_geoip_country_name\\\":\\\"Australia\\\",\\\"$initial_geoip_continent_code\\\":\\\"OC\\\",\\\"$initial_geoip_continent_name\\\":\\\"Oceania\\\",\\\"$initial_geoip_subdivision_1_code\\\":\\\"NSW\\\",\\\"$initial_geoip_subdivision_1_name\\\":\\\"New South Wales\\\"}\""

Added second user and merged

>>> posthog.capture('test-id4', 'test-event', properties={ '$set': { 'userProperty': 600 } })
>>> posthog.alias('test-id3', 'test-id4')

redis. Note that we didn't expire the old props and created_at, but that personId isn't used anymore.

> keys person*
1) "person_props:1:5"
2) "person_create_at:1:5"
3) "person_id:1:test-id4"
4) "person_props:1:6"
5) "person_id:1:test-id3"
6) "person_create_at:1:6"
> get person_id:1:test-id4
"5"
127.0.0.1:6379> get person_id:1:test-id3
"5"
9> get person_props:1:5
"\"{\\\"userProperty\\\":334,\\\"$geoip_latitude\\\":-33.8715,\\\"$geoip_city_name\\\":\\\"Sydney\\\",\\\"$geoip_longitude\\\":151.2006,\\\"$geoip_time_zone\\\":\\\"Australia/Sydney\\\",\\\"$geoip_postal_code\\\":\\\"1148\\\",\\\"$geoip_country_code\\\":\\\"AU\\\",\\\"$geoip_country_name\\\":\\\"Australia\\\",\\\"$geoip_continent_code\\\":\\\"OC\\\",\\\"$geoip_continent_name\\\":\\\"Oceania\\\",\\\"$initial_geoip_latitude\\\":-33.8715,\\\"$initial_geoip_city_name\\\":\\\"Sydney\\\",\\\"$initial_geoip_longitude\\\":151.2006,\\\"$initial_geoip_time_zone\\\":\\\"Australia/Sydney\\\",\\\"$geoip_subdivision_1_code\\\":\\\"NSW\\\",\\\"$geoip_subdivision_1_name\\\":\\\"New South Wales\\\",\\\"$initial_geoip_postal_code\\\":\\\"1148\\\",\\\"$initial_geoip_country_code\\\":\\\"AU\\\",\\\"$initial_geoip_country_name\\\":\\\"Australia\\\",\\\"$initial_geoip_continent_code\\\":\\\"OC\\\",\\\"$initial_geoip_continent_name\\\":\\\"Oceania\\\",\\\"$initial_geoip_subdivision_1_code\\\":\\\"NSW\\\",\\\"$initial_geoip_subdivision_1_name\\\":\\\"New South Wales\\\"}\""

yakkomajuri · 2022-04-11T08:34:17Z

Thanks for submitting the WIP PR, appreciate the visibility into the work

yakkomajuri

Looking good. One comment that the buffer depends on this work, and it will use the person created_at field to determine what events should be sent there.

Could you (either in this PR or the next) add a cache for created_at (either as its own thing or bundled in the props by caching a larger subset of "person" instead of just props at the top level).

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

yakkomajuri

looks good - a few nits.

also, can we add a metric for each of the cache functions? redisSet already has metrics but I'd love to see a breakdown across each of the operations more clearly

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

tiina303 · 2022-04-13T00:02:02Z

also, can we add a metric for each of the cache functions? redisSet already has metrics but I'd love to see a breakdown across each of the operations more clearly

They are all just calling redisSet directly are you predicting it to be slower for setting properties or what would you use it for? That said this could be good for the fetching part, which might end up going to DB and updating the cache. - we'd want to know the cache misses there etc.

yakkomajuri · 2022-04-13T10:31:16Z

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

        clickhouse: ClickHouse | undefined,
-        statsd: StatsD | undefined
+        statsd: StatsD | undefined,
+        personInfoCacheTtl = 1,


would be best to have this to the same default as the config but sure

set it like that on purpose to expire fast if not set.

feat: Write person ID and properties to redis

ae98167

tiina303 marked this pull request as draft April 10, 2022 16:49

yakkomajuri reviewed Apr 11, 2022

View reviewed changes

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

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

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

tiina303 added 4 commits April 12, 2022 13:54

comments

efbca82

test

da9e7ab

fix ts usage

f45e51a

move json stringify to caching function

f5dbbe4

tiina303 marked this pull request as ready for review April 12, 2022 14:28

yakkomajuri approved these changes Apr 12, 2022

View reviewed changes

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

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

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

tiina303 force-pushed the poe-write-to-redis branch from 5ca62a2 to f5dbbe4 Compare April 12, 2022 21:32

Pull cache key creation out

ba2b986

tiina303 mentioned this pull request Apr 12, 2022

feat: Add person info to events #9404

Merged

review comments

345b7d4

yakkomajuri reviewed Apr 13, 2022

View reviewed changes

yakkomajuri merged commit 38bf3ef into master Apr 13, 2022

yakkomajuri deleted the poe-write-to-redis branch April 13, 2022 10:34

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: Write person ID and properties to redis#9377

feat: Write person ID and properties to redis#9377
yakkomajuri merged 7 commits intomasterfrom
poe-write-to-redis

tiina303 commented Apr 10, 2022 •

edited

Loading

Uh oh!

yakkomajuri commented Apr 11, 2022

Uh oh!

yakkomajuri left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yakkomajuri left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tiina303 commented Apr 13, 2022

Uh oh!

yakkomajuri Apr 13, 2022

Uh oh!

tiina303 Apr 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

tiina303 commented Apr 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Changes

How did you test this code?

Uh oh!

yakkomajuri commented Apr 11, 2022

Uh oh!

yakkomajuri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yakkomajuri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tiina303 commented Apr 13, 2022

Uh oh!

yakkomajuri Apr 13, 2022

Choose a reason for hiding this comment

Uh oh!

tiina303 Apr 13, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tiina303 commented Apr 10, 2022 •

edited

Loading