Plugin Capabilities

[mariusandra]

Splitting this from #165

We need to cache what each plugin supports. Basically if it has support for 1) scheduled functions runEveryX 2) event processing functions processEvent / processEventBatch. This lets us separate VMs better in the future. For example having a separate pool of piscina workers for scheduled tasks, starting there only the VMs that are relevant. This caching could be run when a plugin is installed.

It would help with distributing and optimising worker pools inside the plugin server, if we knew what each plugin can do. Basically, what it exports. This knowledge can be used to help make a nicer interface (no need to reorder export plugins that run at the same time) and it'll help us split processEvent/onEvent tasks (which need to finish very fast) from the rest (scheduled jobs and other async tasks)

[neilkakkar]

Thinking this through, this is how things currently look:

We need to figure out two things:

How to represent capabilities

Capabilities ought to be a property of Plugin and not PluginConfig or the rest, since it doesn't change per teams.

We do want to search and sort plugins by capabilities, but at the same time, given a 1,000 is a reasonable upper limit on the plugins, searching via a linear scan is OK, which means we don't necessarily need an indexed column to filter by capability in a DB query. (a.k.a. capabilities can a list inside a column on the plugins table. This also keeps things extensible whenever we add new capabilities)

An alternative is a new table that links to Plugin and has rows of pluginIDs, capability in a many to many relationship. This is also extensible, and allows filtering on the DB level via joins. This is a bit more complicated when it comes to updating capabilities on source change (for example), and doesn't make sense to me, unless we foresee having extra metadata on capabilities. (I can't think of anything, but let me know if I'm mistaken)

How to infer capabilities

Now, let's say we've chosen a model to represent capabilities. Given a plugin, how do we figure out what capabilities it has?

1. We could try this on installation: unzip the archive, get the index.js file, parse the exported functions, get list of all of them to figure out the capabilities.

2. I find the above approach annoying, since the VM does exactly this to give us the __methods and __tasks list, which are the plugin capabilities. In a sense, this is too late: since we were going to use the capabilities to decide which VMs to use. However, this fits in very well with #165 : if we have a VM / worker dedicated to testing, we could run the test not just for source plugins, but for all plugins, on installation and on save. And this test, along with running the setup, testing the plugin doesn't crash, populates the capabilities.

3. Another, simpler approach is to push this onto the plugin creator: please tell us which all functions you're using. It's a bit more annoying development wise, but simple to code up, and we get this information, say, directly through plugin.json. In this case, we can probably store it as a part of pluginConfig, or copy it over to Plugin.

---

I think long term, (2) makes sense. However, I'm not yet sure how much we're going to use the capabilities information, so I'm in favour of doing (3) now: seeing how much we use this, and eventually, once we have source testing as well, and capabilities seem useful enough to not put developers through the pain of the extra config item, we can integrate them into (2).

Thoughts? cc: @yakkomajuri @macobo @Twixes

[yakkomajuri]

Thanks for writing this out @neilkakkar! I need to think about this a bit more before I can give you a strong opinion on approach for inferring capabilities. I am quite averse to asking plugin creators to do this though - I think it'd give a bad impression: "how come you don't know what this can do? you run these functions!"

Regarding representing them, I also think one extra column to the plugins table should do it.

[macobo]

My 2c:

• Agreed with yakko, pushing this onto plugin creators is a no-go. It makes the plugin creation experience worse (hence hurting this teams goals) and opens a new class of errors (why doesn't my plugin do anything).
• Hunch re this meshing with #165 is good - it might make sense to make the first steps towards solving that here.

One scheme that might work and gets us closer would be:

• During init and message on PLUGINS_RELOAD_PUBSUB_CHANNEL, before firing the public reload, instead:

        pubSub.on('message', async (channel: string, message) => {
            if (channel === server!.PLUGINS_RELOAD_PUBSUB_CHANNEL) {
                status.info('⚡', 'Reloading plugins!')

                await piscina?.runTask({ task: 'calculatePluginCapabilities' })
                await piscina?.broadcastTask({ task: 'reloadPlugins' })
                await scheduleControl?.reloadSchedule()
            }
        })

• This would work on a single thread, fetch plugins with no capabilities recorded and record them.
• Whenever a plugin gets added to the db or updated (e.g. version change) we reset the capabilities column so plugin server knows what to do.

It would be slightly racy in that it would trigger in all plugin server instances but should be correct/good enough to get started.

With #165 we can then refactor for this to happen once/outside the main flow completely.

[neilkakkar]

This is an interesting idea, too!

Following up on both your points, not pushing it onto developers makes sense, specially with regards to the extra debugging required with mistakes: yeah, that's not great.

---

This made me think of another interesting middle ground: We let things run as is, and on loading the plugin, if there's no capabilities, they get populated.

It's implicit on setup, which means it happens inside the reloadPlugins task as well iff the config / something about the plugin has changed. I feel this works better if we go the #165 route, since the plugins would need to be setup for the test invocation as well. It resolves the race condition, and ensures if a plugin is loaded, its capabilities are populated.

The only issue here is with partial upgrades: during migration to this new version, if you don't restart the plugin server, some running plugins won't have their capabilities updated. But that's easily resolvable.

---

I think this gives us the best of everything so far?

[neilkakkar]

Realised during implementation: one thing I missed above is the lazy loading, since inferring capabilities on reload means the VMs would be loaded on all threads where the plugin has changed / new plugin has been introduced :/ This won't be a problem if it were a test worker setup, but maybe does become a problem on the regular workers.

Separating it out into a task solves this above problem.

Another problem we have is that if the task runs before the reload, we're dealing with stale data, which isn't good. If it runs after the reload, the plugins object becomes stale, so we need to reload again (or dynamically modify the plugins object and defer reload to the next time something changes via the posthog API - this is messy, and doesn't percolate up to the other threads).

The crux: These issues arise because we're trying to set plugin data not on creation but on load, which means there is a load -> setCap -> load again loop; which would exist until we can somehow move setCap back to PostHog (with #165).

Hmmm, hard to say which way is better for now, since it's unclear which route we go for plugin testing. So, choosing the one that takes less work, while keeping things as separate from the rest of the code as possible, i.e. the non-task approach.

[yakkomajuri]

Here's my pragmatic view:

Let's get the ball rolling and iterate as we go - the best thing right now is probably to get a PR out and we can go from there. Will also put the discussion into context quite nicely.

Either way we'll tackle the capabilities representation "problem", even if the inference occurs "too late". We can then iterate to get the inference to happen at an earlier stage/with a better setup, probably integrating with some #165 work.

[neilkakkar]

Indeed. I'm being verbose here because I like keeping a log of what decisions we make, and how we make them. Helps me revisit when we get around to refactoring to figure out the mistakes I made.

[yakkomajuri]

Oh yeah I'm all for that!

It helps me (and everyone else) get context too.

[mariusandra]

Agree with the above and great work digging into all of it @neilkakkar

Like @yakkomajuri said, I think the first step should be to just capture the capabilities of a plugin when we can, probably after the plugin loads if no capabilities have been recorded... or if they differ from the reality. Later we'll address #165 and do this capability recording in a separate test or installation step.

As for storing it, a simple JSON object (with some structure) on Plugin makes sense for now. It lets us move fast.