This repository has been archived by the owner on Nov 4, 2021. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 5
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
mariusandra
reviewed
Apr 12, 2021
mariusandra
approved these changes
Apr 12, 2021
2 tasks
fuziontech
pushed a commit
to PostHog/posthog
that referenced
this pull request
Oct 12, 2021
* Add INTERNAL_MMDB_SERVER_PORT setting * Upgrade @maxmind/geoip2-node and @posthog/plugin-scaffold * Rework MMDB feature to only run in the main thread with a TCP server * Rework MMDB tests for TCP server * Improve pluginsServer style * Use random port MMDB server by default * Clean server.ts up * Simplify createMmdbServer return type * Slightly increase mmdb.test.ts timeout
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Changes
This relocates the built-in MMDB feature to the main thread only, by running a lightweight internal TCP server responding to GeoIP requests on localhost. Resolves PostHog/posthog#3888.
Story
Initially I looked into sharing the
mmdb
(ReaderModel
instance) object between threads somehow. Turns out that some raw data can be shared nicely withSharedArrayBuffer
, but that's way too low-level to help here, database reader instantiation would nullify this optimization completely.I stumbled upon the mmap-object package. It uses
mmap
to share actual JS objects between Node processes. I attempted implementing it in #301: had to fork the package and make some changes to make it even work (allenluce/mmap-object@master...Twixes:master). Memory usage did drop somewhat (anecdotal result: with 16 cores reduced memory usage from 2.3 GB to 1.7 GB), but not nearly as much as would be optimal – a proper solution would really be embedded in mmdb-lib instead of high-level in here, but that is not really feasible.After that I searched for a way to use intraprocess
MessagePort
s, and it's a great way to communicate between main thread and worker threads, but the way Piscina works makes it unrealistic here (without significant upstream changes).Then I considered passing the GeoIP data to EVERY event right before
runPlugins
, butHaving done all of the above, a local TCP (not even HTTP) server emerged as a workable solution. It's quite elegant and lightweight. I settled for port communication - a Unix socket could possibly be slightly more performant, but wanted to avoid creating a sock file. Accepted request data is one IP address string per
socket.write
. Response data is then sent persocket.write
, until the connection is closed (in fact it's closed after one response). Initially response data format was a JSON string, but since this is purely internal (onlylocalhost
), actuallyBuffer
-based and within a single plugin server instance, I switched to low-levelv8.[de]serialize
.As with all solutions here, some downsides exist, though they should be menageable:
can't run multiple MMDB-enabled plugin server instances on one machine due to the port being blockednot a problem with a randomized portChecklist