Plugin Capabilities implementation #384

neilkakkar · 2021-05-17T13:07:30Z

Changes

Adds support for Plugins Capabilities. Resolves #379

Companion PostHog/posthog PR (PostHog/posthog#4371) to be merged first.

~~Will write tests now, but just wanted to get a PoC out.~~

Checklist

Updated Settings section in README.md, if settings are affected
Jest tests

…o capabilities

mariusandra · 2021-05-18T09:25:03Z

Having given it a quick glance, I think we should store a bit more metadata with the capabilities, mainly adding the type of the capability. Basically storing this in a JSON like: { jobs: ['runSomthing'], tasks: ['processEvent', 'setupPlugin'] } or something like that.

mariusandra

Very cool! Few inline comments plus probably could use a merge from master.

tests/helpers/plugins.ts

mariusandra · 2021-05-18T13:50:15Z

src/utils/db/sql.ts

+    await server.db.createPluginLogEntry(
+        pluginConfig,
+        PluginLogEntrySource.System,
+        PluginLogEntryType.Info,
+        `Set plugin capabilities (instance ID ${server.instanceId}).`,
+        server.instanceId
+    )


I think if the capabilities haven't changed, there's no need to update them nor log that we set them. That would be needless logspam :)

TBH I don't believe these logs are valuable user-side at all. Plugin loaded/unloaded events can be important because of plugin setup/teardown methods – while capabilities are just an internal optimization of ours.

mariusandra · 2021-05-18T13:53:11Z

src/worker/plugins/loadPlugin.ts

+    // infer on load implies there's no lazy loading, but all workers get
+    // these properties loaded


We could lazy load them. We won't do anything with the loaded capabilities for the moment, and eventually set them in a separate "install" step anyway. No need to block for that.

Hmm, I could unblock them, but that still doesn't mean the VMs are lazy loaded right? since inferPluginCapabilities() will load the VM on startup, before the worker gets any event.

Maybe I should just remove this comment, just realised it's confusing w.r.t what is being lazy loaded.

The funny code here is this:

void pluginConfig.vm?.initialize!(server, pluginConfig, plugin.source, pluginDigest(plugin)) await inferPluginCapabilities(server, pluginConfig, prevConfig)

The first function is run in the background, the second one we await for. The second function awaits for a promise (resolveInternalVm) that gets set when the first function that's running in the background finishes.

In essence, the plugin is not lazy anymore, but we block until the VM setup has completed.

To get around it, we could just void the inferPluginCapabilities call and let them run in the background, however that also feels slightly hacky.

A possibly better idea is to put this inferPluginCapabilities inside the LazyVM.initialize function. There you probably won't need prevConfig either. Just check if the current pluginConfig.plugin.capabilities matches reality or not. If not, update.

that.. makes a lot of sense. Ha, would've been nice if I made this leap myself when thinking about the implementation.

Come to think of it, I don't need the prevConfig in the current setup either, for similar reasons - I already have a pluginConfig.plugin.capabilities to check.

mariusandra

Few more flyby comments and suggestions :)

src/types.ts

src/worker/plugins/loadPlugin.ts

mariusandra · 2021-05-18T21:04:41Z

src/worker/plugins/loadPlugin.ts

+        await setPluginCapabilities(server, pluginConfig, capabilities)
+    }
+
+    pluginConfig.plugin!.capabilities = capabilities


Here again the ! could cause subtle issues. After all, we're not sure this function will always be called with pluginConfigs that contain a plugin. It might be the case when we follow the current trail that leads to this function, but it might not be the case in the future.

Often the solution is to use a guard, such as if (!pluginConfig.plugin) { throw new Error('how the hell did you get here?') }, or just return, depends on the case. Guards are best placed at the top of a function as well.

src/worker/plugins/run.ts

Twixes

Nice! Just some minor comments

src/worker/plugins/loadPlugin.ts

Twixes · 2021-05-19T23:16:51Z

src/utils/db/sql.ts

+    await server.db.createPluginLogEntry(
+        pluginConfig,
+        PluginLogEntrySource.System,
+        PluginLogEntryType.Info,
+        `Set plugin capabilities (instance ID ${server.instanceId}).`,
+        server.instanceId
+    )


TBH I don't believe these logs are valuable user-side at all. Plugin loaded/unloaded events can be important because of plugin setup/teardown methods – while capabilities are just an internal optimization of ours.

src/types.ts

Twixes

Stamp of approval

Twixes · 2021-05-20T14:21:36Z

tests/plugins.test.ts

+    expect(pluginConfig.plugin!.capabilities!.methods!.sort()).toEqual([
+        'onSnapshot',
+        'processEvent',
+        'processEventBatch',


It seems a bit quirky that processEventBatch is listed as a capability when it's only an autogenerated version based on actually provided processEvent, but that seems OK

mariusandra

Still some inline comments to check :)

src/worker/plugins/loadPlugin.ts

mariusandra · 2021-05-20T19:13:44Z

src/worker/plugins/loadPlugin.ts

+    // infer on load implies there's no lazy loading, but all workers get
+    // these properties loaded


The funny code here is this:

void pluginConfig.vm?.initialize!(server, pluginConfig, plugin.source, pluginDigest(plugin)) await inferPluginCapabilities(server, pluginConfig, prevConfig)

The first function is run in the background, the second one we await for. The second function awaits for a promise (resolveInternalVm) that gets set when the first function that's running in the background finishes.

In essence, the plugin is not lazy anymore, but we block until the VM setup has completed.

To get around it, we could just void the inferPluginCapabilities call and let them run in the background, however that also feels slightly hacky.

A possibly better idea is to put this inferPluginCapabilities inside the LazyVM.initialize function. There you probably won't need prevConfig either. Just check if the current pluginConfig.plugin.capabilities matches reality or not. If not, update.

mariusandra · 2021-05-20T19:16:45Z

tests/plugins.test.ts

+    await setupPlugins(mockServer)
+    const newPluginConfig = mockServer.pluginConfigs.get(39)!
+
+    expect(newPluginConfig.plugin!.capabilities).toBe(pluginConfig.plugin!.capabilities)


Let's also check that:

expect(newPluginConfig.plugin).not.toBe(pluginConfig.plugin)

otherwise the capabilities are the same because it's literally the same object :)

…o capabilities

neilkakkar · 2021-05-24T08:34:58Z

tests/plugins.test.ts

@@ -38,7 +39,7 @@ afterEach(async () => {
 })

 test('setupPlugins and runProcessEvent', async () => {
-    getPluginRows.mockReturnValueOnce([plugin60])
+    getPluginRows.mockReturnValueOnce([{ ...plugin60 }])


it took me more than an hour on Friday night to figure out this was the issue with tests! :/ (since it affected plugin60 directly, and wouldn't show up if I simply ran the tests in isolation) - only when things run in sequence did this bork things up. 🤦 .

Ooh... that's a nice rabbit hole :D

mariusandra

Looks good to me!

…rver#384) * add tests for plugin capabilities * address comments, update tests * clean up * update capabilities type definition

neilkakkar added 3 commits May 17, 2021 14:04

WIP: capabilities implementation

7c1fbc2

Merge branch 'master' of https://github.com/PostHog/plugin-server int…

7804942

…o capabilities

add tests for plugin capabilities

bf03525

neilkakkar mentioned this pull request May 17, 2021

Add Capabilities to Plugin model PostHog/posthog#4371

Merged

5 tasks

address comments, update tests

95670b5

neilkakkar changed the title ~~WIP: Capabilities implementation~~ Capabilities implementation May 18, 2021

neilkakkar changed the title ~~Capabilities implementation~~ Plugin Capabilities implementation May 18, 2021

neilkakkar marked this pull request as ready for review May 18, 2021 11:12

neilkakkar added 2 commits May 18, 2021 12:43

fix tests

a04a197

clean up

7cf3b44

neilkakkar requested review from mariusandra and yakkomajuri May 18, 2021 11:50

neilkakkar mentioned this pull request May 18, 2021

Fix Tests: Capabilities is a new non-nullable field #390

Merged

2 tasks

merge master

e5bd9d1

mariusandra reviewed May 18, 2021

View reviewed changes

neilkakkar added 4 commits May 18, 2021 15:08

fix tests relevant to new types

063a7e5

update capabilities type definition

8dcb16c

update tests, only set capabilities if theyve changed

1697073

fix remaining tests

b2fd679

mariusandra reviewed May 18, 2021

View reviewed changes

neilkakkar added 3 commits May 19, 2021 14:47

address comments

378f5bc

merge master

227aa19

more typing fixes

a6837d5

Twixes suggested changes May 20, 2021

View reviewed changes

neilkakkar added 3 commits May 20, 2021 12:04

address comments

56e0bee

merge master

902db6e

Merge branch 'master' into capabilities

f9fc935

neilkakkar requested a review from Twixes May 20, 2021 12:29

Twixes approved these changes May 20, 2021

View reviewed changes

mariusandra suggested changes May 20, 2021

View reviewed changes

neilkakkar added 2 commits May 21, 2021 14:35

Merge branch 'master' of https://github.com/PostHog/plugin-server int…

c72b680

…o capabilities

address comments new tests

c1d4d95

neilkakkar commented May 24, 2021

View reviewed changes

neilkakkar added 2 commits May 24, 2021 09:46

prettify

8d7f160

fix failing test

caf3cde

neilkakkar requested a review from mariusandra May 24, 2021 09:45

mariusandra approved these changes May 25, 2021

View reviewed changes

mariusandra added the bump patch Bump patch version when this PR gets merged label May 25, 2021

mariusandra enabled auto-merge (squash) May 25, 2021 11:20

neilkakkar and others added 4 commits May 25, 2021 12:23

Merge branch 'master' into capabilities

56fda74

Merge branch 'master' into capabilities

81bc013

merge master

a4ee3ab

few more server->hub

f560a8f

neilkakkar disabled auto-merge May 25, 2021 12:52

neilkakkar enabled auto-merge (squash) May 25, 2021 12:55

neilkakkar merged commit 09f1c34 into master May 25, 2021

neilkakkar deleted the capabilities branch May 25, 2021 13:02

posthog-bot mentioned this pull request May 25, 2021

Update plugin server to 0.21.7 PostHog/posthog#4473

Merged

mariusandra mentioned this pull request May 26, 2021

Plugins Epic PostHog/posthog#1896

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plugin Capabilities implementation #384

Plugin Capabilities implementation #384

neilkakkar commented May 17, 2021 •

edited

Loading

mariusandra commented May 18, 2021

mariusandra left a comment

mariusandra May 18, 2021

Twixes May 19, 2021

mariusandra May 18, 2021

neilkakkar May 19, 2021

mariusandra May 20, 2021

neilkakkar May 21, 2021

mariusandra left a comment

mariusandra May 18, 2021

Twixes left a comment

Twixes May 19, 2021

Twixes left a comment

Twixes May 20, 2021

mariusandra left a comment

mariusandra May 20, 2021

mariusandra May 20, 2021

neilkakkar May 24, 2021

mariusandra May 25, 2021

mariusandra left a comment

		// infer on load implies there's no lazy loading, but all workers get
		// these properties loaded

Plugin Capabilities implementation #384

Plugin Capabilities implementation #384

Conversation

neilkakkar commented May 17, 2021 • edited Loading

Changes

Checklist

mariusandra commented May 18, 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

mariusandra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Twixes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Twixes left a 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

Choose a reason for hiding this comment

mariusandra left a comment

Choose a reason for hiding this comment

neilkakkar commented May 17, 2021 •

edited

Loading