refactor(plugin-server): Run ingestion only on worker threads #9738
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yakkomajuri
reviewed
May 11, 2022
Not sure what do you mean - what tests do we need here beyond what we already have? Note the intent here (as discussed on tuesday) is 0 behavioral changes besides moving some code from outside worker to inside so we know it's safe (can run it in production). Actual refactoring and behavioral changes will happen once we have verified that it works and are happy with it. |
d82584e to
d5e0d05
Compare
This takes the function from 13-17s to 4s locally. Not great but not terrible either.
|
This is now ready for review/merge. @yakkomajuri please take a look and let's merge it sometime today :) I needed to unfortunately include some test refactorings - there were some bugs in the test suite causing pain getting this green. |
yakkomajuri
approved these changes
May 13, 2022
| console.log("Error creating topics. Probably they're already created.\n", error) | ||
|
|
||
| const existingTopics = await admin.listTopics() | ||
| const topicsToCreate = topics.filter((topic) => !existingTopics.includes(topic)).map((topic) => ({ topic })) |
| @@ -109,5 +112,9 @@ function shouldSendEventToBuffer(hub: Hub, event: PreIngestionEvent, person?: Pe | |||
| hub.statsd?.increment('conversion_events_buffer_size', { teamId: event.teamId.toString() }) | |||
| } | |||
|
|
|||
| if (!hub.CONVERSION_BUFFER_ENABLED && !hub.conversionBufferEnabledTeams.has(teamId)) { | |||
macobo
added a commit
that referenced
this pull request
May 13, 2022
Follow-up to #9738, silly mistake
macobo
added a commit
that referenced
this pull request
May 13, 2022
Follow-up to #9738, silly mistake
fuziontech
added a commit
that referenced
this pull request
May 13, 2022
* master: fix: exclusion steps cannot be selected (#9762) feat(lemon-button): Support `status` for `primary` buttons (#9782) fix: healthcheck for kafka on plugin server (#9771) fix(billing): Update billing success message (#9739) fix(plugin-server): Set transpileOnly when importing piscina code in tests (#9777) fix(plugin-server): Remove unused kafka reset test code (#9779) fix: set kafka_skip_broken_messages on dead letter queue table (#9754) fix(plugin-server): remove dead code from worker.test.ts (#9776) refactor(plugin-server): Run ingestion only on worker threads (#9738) fix: Plugin-server tests with kafka need to have consumer stopped (#9774) chore(deps): Update posthog-js to 1.21.1 (#9773) chore(dep): upgrading rr-web (#9772) fix: ouroboros inputs (#9769)
alexkim205
pushed a commit
that referenced
this pull request
May 23, 2022
* Refactor method to do everything it's supposed to do No tests though? * Remove unused argument from a function * Remove dead code * Run event pipeline on worker threads * Remove code in way of refactor * Make metrics happy * Silence a yeetcode-related failure * Mock over stopping unneeded services * Yeetcode a test * Reset the right table between tests * Reset healthcheck consumer between tests * Silence logspam around creating topics * Significantly speed up resetting clickhouse tables between tests This takes the function from 13-17s to 4s locally. Not great but not terrible either. * Remove try-catch * Remove a dead test
alexkim205
pushed a commit
that referenced
this pull request
May 23, 2022
Follow-up to #9738, silly mistake
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
WIP. Creating this to run tests
Our ingestion logic is hard to understand and harder to split up. Doing some work in main thread and some in workers is one part of the problem.
Changes
We now run all of the previous
ingestEventin the worker thread.No behavior is dropped - most code is copy-pasta with minor callsite changes.
Once this is live we will have info whether doing ingestion purely in workers causes issues.
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
How did you test this code?
See tests.