Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Inline plugins (plugins without running VM2) #23443

Merged
merged 14 commits into from
Jul 24, 2024

Conversation

oliverb123
Copy link
Contributor

@oliverb123 oliverb123 commented Jul 3, 2024

Problem

Plugins run inside a VM, which incurs memory and cpu overhead, and code complexity. As many plugins are now considered trusted-source in cloud environments, we can inline the code for commonly used plugins instead, focusing for now on processEvent plugins, to get some improved performance in the ingestion pipeline.

@oliverb123 oliverb123 requested a review from a team July 3, 2024 13:42
@oliverb123 oliverb123 changed the title Feat: add to plugin type enum feat: add to plugin type enum Jul 3, 2024
Copy link
Contributor

@tiina303 tiina303 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's build out the frontend and backend changes on top of this to make sure we don't need anything else changed in the model to minimise number of migrations.

@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@posthog-bot
Copy link
Contributor

This PR was closed due to lack of activity. Feel free to reopen if it's still relevant.

@oliverb123 oliverb123 reopened this Jul 18, 2024
@oliverb123 oliverb123 force-pushed the oliver_plugin_inlining branch from fd9f9c3 to 13545fc Compare July 18, 2024 08:57
@posthog-bot posthog-bot removed the stale label Jul 19, 2024
@oliverb123 oliverb123 force-pushed the oliver_plugin_inlining branch from b29d269 to b32c926 Compare July 19, 2024 13:32
@oliverb123 oliverb123 changed the title feat: add to plugin type enum feat: Inline plugins (plugins without running VM2) Jul 22, 2024
@oliverb123 oliverb123 force-pushed the oliver_plugin_inlining branch from 1955e54 to 61bd215 Compare July 22, 2024 12:28
@oliverb123 oliverb123 force-pushed the oliver_plugin_inlining branch 4 times, most recently from b84b06f to e0029b1 Compare July 23, 2024 08:10
Copy link
Contributor

@benjackwhite benjackwhite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is super cool. Some really clean work here (especially when working with such a messy problem!) Only one suggestion regarding tests but not blocking.

We might want to be extra careful merging this. I haven't tried it locally so worth triple checking and if there is any doubts doing a canary style deploy (with support from infra)

plugin-server/src/types.ts Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was expecting to find a test for this tbh. At least one happy path test per inline plugin would be great

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took the tests from the plugin repo and put them in inline.test.ts (the semvar-flattener one) - does this cover the kind of testing you mean, or something beyond that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if it's what he meant, but seeing a functional test that actually calls an inline plugin would be nice (maybe I'm missing one?)

@oliverb123 oliverb123 force-pushed the oliver_plugin_inlining branch from 889c186 to 05aa95a Compare July 24, 2024 11:31
@oliverb123 oliverb123 merged commit 7207da9 into master Jul 24, 2024
84 checks passed
@oliverb123 oliverb123 deleted the oliver_plugin_inlining branch July 24, 2024 11:49
thmsobrmlr pushed a commit that referenced this pull request Jul 24, 2024
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Ben White <[email protected]>
thmsobrmlr pushed a commit that referenced this pull request Jul 25, 2024
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Ben White <[email protected]>
silentninja pushed a commit to silentninja/posthog that referenced this pull request Aug 8, 2024
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Ben White <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants