In-memory action definitions synced with Django #403

Twixes · 2021-05-21T08:13:25Z

Changes

Part of #235.

Checklist

Jest tests

Twixes · 2021-05-21T08:29:12Z

This is a diff of almost 500 lines already, would you prefer to merge just this action syncing feature alone (with it tested, but unused in ingestion), or merge all of action matching at once (will be a big diff)?

mariusandra

Few thoughts inline. Otherwise, I think it's good to merge this feature chunk by chunk.

mariusandra · 2021-05-21T08:57:20Z

src/worker/tasks.ts

-    hello: (server, args) => {
-        return `hello ${args}!`
-    },


Finally you got rid of it 😁

mariusandra · 2021-05-21T09:00:26Z

src/worker/ingestion/process-event.ts

+        this.pubSub = new PubSub(pluginsServer, {
+            'fetch-action': async (message) => await this.actionManager.fetchAction(parseInt(message)),
+            'delete-action': (message) => this.actionManager.deleteAction(parseInt(message)),
+        })


The only problem with this is that it'll add another redis connection per worker thread, which can add up. I guess even if we have officially given up on Heroku free redis tiers, it might be good to reload these inside the workers with a similar broadcastTask system that plugin reloads use.

Agreed, used similar system as reloadPlugins, this way avoiding increase of number ofRedis connections with per-thread pubsub instances

mariusandra · 2021-05-21T09:07:52Z

src/worker/ingestion/action-manager.ts

+        return this.actionCache[id]
+    }
+
+    public async fetchAction(id: Action['id']): Promise<void> {


The name of this function is misleading. It's more reloadAction than "fetch (and return) from the database"

Yeah, was definitely on the fence about this, went for reloadAction and dropAction

mariusandra · 2021-05-21T09:08:38Z

tests/worker/ingestion/action-manager.test.ts

+            )
+
+            // This is normally done by Django async in such a situation
+            await actionManager.fetchAction(67)


Yup, echoing my point from above, the name is misleading here. What happens to the fetched action? Not obvious it actually caches it in a map... :)

Other than calling this here, should we also have a bigger and more E2E test to make sure actions actually reload in the workers? Fine to push that for a later PR.

At this stage it's hard to test this E2E with the pubsub only in the main thread (whole startPluginsServer needed instead of just createServer), I don't know about reaching into actionManagers in worker threads, and there's no actual functionality (like onAction) to test here yet. But definitely a goal to have E2E tests when more parts are in place.

mariusandra

I'm sure this will work well (once all code paths are tested/covered, see below :P), however as a general observation, I'm afraid of "eventual inconsistency" here. In case we miss some drop/reload signal from the app, we'll never reach consistency.

This dropped signal might seem unlikely, but experience shows that things breaking randomly is more likely than not, given enough time :). Even if not today, then someone could accidentally introduce a 30sec "await" after the actions load, but before the pubsub started, or ... whatever imaginary case.

With reloads, this is less of an issue since reloads happen frequently, and always read in all plugins and diff changes. However here I'd add a "every 15min do a manual sync" just in case.

mariusandra · 2021-05-25T11:21:51Z

src/main/pluginsServer.ts

+            'reload-action': async (message) =>
+                await piscina?.broadcastTask({ task: 'reloadAction', args: { actionId: parseInt(message) } }),
+            'drop-action': async (message) =>
+                await piscina?.broadcastTask({ task: 'dropAction`', args: { actionId: parseInt(message) } }),


There's some "`" here after "dropAction". Can you make sure this code path is also tested?

mariusandra · 2021-05-25T11:27:08Z

src/worker/tasks.ts

+    reloadAction: async (server, args: { actionId: Action['id'] }) => {
+        return await server.eventsProcessor.actionManager.reloadAction(args.actionId)
+    },
+    dropAction: (server, args: { actionId: Action['id'] }) => {
+        return server.eventsProcessor.actionManager.dropAction(args.actionId)
+    },


Can you add these to worker.test.ts

src/types.ts

src/utils/db/db.ts

src/utils/status.ts

neilkakkar · 2021-05-25T13:36:41Z

src/worker/ingestion/action-manager.ts

+    }
+
+    public async prepare(): Promise<void> {
+        this.actionCache = await this.db.fetchAllActionsMap()


Not sure how exactly action manager will be used yet, so:

If something goes wrong with fetching actions, such that this cache isn't populated, do we want to stop ingestion completely? or continue?

Well, in that case an error will be thrown in this method (if e.g. Postgres connection fails) and indeed ingestion will be stopped

Right, I'm asking if that's what we ought to do, or not?

Ah, yes, we do, since otherwise data integrity will be at risk (action matching wouldn't work). Besides, this is not likely to fail for internal reasons, and if Postgres (the external dependency) fails, then we are in catastrophic failure territory anyway

tests/helpers/sql.ts

Twixes · 2021-05-25T17:06:21Z

Done @mariusandra resyncing all every 5 minutes + added taskRunner tests.

mariusandra

That's a nice bit of code! Let's see what it does 👀

I added a few comments still inline, feel free to fix or defer.

mariusandra · 2021-05-25T18:08:38Z

src/utils/db/db.ts

@@ -727,10 +727,16 @@ export class DB {

    public async fetchAllActionsMap(): Promise<Record<Action['id'], Action>> {
        const rawActions: RawAction[] = (
-            await this.postgresQuery(`SELECT * FROM posthog_action`, undefined, 'fetchActions')
+            await this.postgresQuery(`SELECT * FROM posthog_action WHERE deleted = FALSE`, undefined, 'fetchActions')


nice catch!

src/utils/status.ts

mariusandra · 2021-05-25T18:22:23Z

src/utils/status.ts

+        if (process.env.NODE_ENV?.toLowerCase() === 'test') {
+            // TODO: use determineNodeEnv() here
+            return () => {} // eslint-disable-line @typescript-eslint/no-empty-function
+        }


I get the frustration with this, though sometimes when coding locally logging is good to have. It's somewhat unintuitive to find this piece of code to re-enable it.

I think there's on alternative to this, adding LOG_LEVEL=none as a default for test mode.

It might have a bug, in that the existing console patching mechanism will also swallow messages sent by jest that would be good to still see. If so, the solution would be to just add this log level filtering inside status.

Eh, removed this path altogether as it had circular import problems with config, maybe another PR

mariusandra · 2021-05-25T18:29:02Z

src/utils/pubsub.ts

+                throw new Error(
+                    `Received a pubsub message for unassociated channel ${channel}! Associated channels are: ${Object.keys(
+                        this.taskMap
+                    ).join(', ')}`
+                )


I wonder if this can backfire? E.g. through some upgrade django starts sending runCommand via pubsub before the plugin servers have restarted to receive it. Failing silently to sentry would be better probably.

Possibly, yeah, moved to just captureException

neilkakkar · 2021-05-26T10:56:40Z

src/main/pluginsServer.ts


        if (hub.jobQueueManager) {
            const queueString = hub.jobQueueManager.getJobQueueTypesAsString()
            await hub!.db!.redisSet('@posthog-plugin-server/enabled-job-queues', queueString)
        }

+        // every 5 minutes all ActionManager caches are reloaded for eventual consistency
+        pingJob = schedule.scheduleJob('*/5 * * * *', async () => {


think this is overridden by the pingJob below?

…Hog/plugin-server#403) * Add ActionManager * Refactor ActionManager * Remove hello * Adjust ActionManager method names and use single PubSub * Touch tests up * Make some adjustments * Disable `status` stdout logs in test mode * Fix `status` * Fix test problems * Fix dropAction typo * Reload all ActionManager caches every 5 min * Fix duplicate RawAction * Don't stringify JSONB column for `insertRow` * It's a hub now * Filter by Action.deleted * Enhance ActionManager tests * Add Action-syncing task runner tests * Use `LOG_LEVEL=warn` in tests * Don't `throw` error on unassociated channel pubsub * Don't use defaultConfig in Status.buildMethod due to circular import * Fix actions reload job var name

Twixes added 3 commits May 21, 2021 03:53

Add ActionManager

42eb852

Refactor ActionManager

bb45010

Remove hello

580ffc1

Twixes mentioned this pull request May 21, 2021

Syncing action definition changes with plugin server PostHog/posthog#4436

Merged

mariusandra suggested changes May 21, 2021

View reviewed changes

Twixes added 3 commits May 21, 2021 11:59

Adjust ActionManager method names and use single PubSub

755e5dd

Touch tests up

54f254a

Merge branch 'master' into 235-action-matching

797d56f

Twixes marked this pull request as ready for review May 21, 2021 11:03

Twixes changed the title ~~Action matching in ingestion~~ In-memory action definitions synced with Django May 21, 2021

Twixes added 4 commits May 24, 2021 22:47

Make some adjustments

a87a0a0

Disable status stdout logs in test mode

ff0013e

Fix status

e12d648

Fix test problems

f3415c8

Twixes requested a review from mariusandra May 25, 2021 00:26

mariusandra suggested changes May 25, 2021

View reviewed changes

Twixes added 3 commits May 25, 2021 13:43

Merge branch 'master' into 235-action-matching

a42f6c8

Fix dropAction typo

3f5553a

Reload all ActionManager caches every 5 min

63785f1

neilkakkar reviewed May 25, 2021

View reviewed changes

Twixes added 8 commits May 25, 2021 15:45

Merge branch 'master' into 235-action-matching

e27c039

Fix duplicate RawAction

1590d3c

Don't stringify JSONB column for insertRow

7f0dc18

It's a hub now

d768f11

Filter by Action.deleted

a5be2e4

Enhance ActionManager tests

5dde8df

Add Action-syncing task runner tests

4af1ba7

Merge branch 'master' into 235-action-matching

bb606db

Twixes requested a review from mariusandra May 25, 2021 17:06

mariusandra approved these changes May 25, 2021

View reviewed changes

Twixes added 3 commits May 26, 2021 12:04

Use LOG_LEVEL=warn in tests

8304827

Don't throw error on unassociated channel pubsub

c132a53

Don't use defaultConfig in Status.buildMethod due to circular import

4082003

neilkakkar reviewed May 26, 2021

View reviewed changes

Fix actions reload job var name

6212419

Twixes merged commit c8f211f into master May 26, 2021

Twixes deleted the 235-action-matching branch May 26, 2021 12:35

posthog-bot mentioned this pull request May 26, 2021

Update plugin server to 0.21.9 PostHog/posthog#4508

Merged

This was referenced May 26, 2021

Reorient ActionManager to group by teamId for practicality #433

Merged

Action matching juice #436

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

In-memory action definitions synced with Django #403

In-memory action definitions synced with Django #403

Twixes commented May 21, 2021 •

edited

Loading

Twixes commented May 21, 2021 •

edited

Loading

mariusandra left a comment

mariusandra May 21, 2021

mariusandra May 21, 2021

Twixes May 21, 2021 •

edited

Loading

mariusandra May 21, 2021

Twixes May 21, 2021

mariusandra May 21, 2021

mariusandra May 21, 2021

Twixes May 21, 2021

mariusandra left a comment

mariusandra May 25, 2021

mariusandra May 25, 2021

neilkakkar May 25, 2021

Twixes May 25, 2021 •

edited

Loading

neilkakkar May 25, 2021

Twixes May 25, 2021

Twixes commented May 25, 2021

mariusandra left a comment

mariusandra May 25, 2021

mariusandra May 25, 2021

Twixes May 26, 2021

mariusandra May 25, 2021

Twixes May 26, 2021

neilkakkar May 26, 2021

Twixes May 26, 2021

In-memory action definitions synced with Django #403

In-memory action definitions synced with Django #403

Conversation

Twixes commented May 21, 2021 • edited Loading

Changes

Checklist

Twixes commented May 21, 2021 • edited Loading

mariusandra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Twixes May 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariusandra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Twixes May 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Twixes commented May 25, 2021

mariusandra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Twixes commented May 21, 2021 •

edited

Loading

Twixes commented May 21, 2021 •

edited

Loading

Twixes May 21, 2021 •

edited

Loading

Twixes May 25, 2021 •

edited

Loading