-
Notifications
You must be signed in to change notification settings - Fork 5
Conversation
Some things to improve still:
|
Looks okay for now. Though merge conflict and red tests. |
I added some statsd and made a new issue to track the other stuff: https://github.com/PostHog/plugin-server/issues/429 |
One more question: does the buffer get flushed when the plugins gets unloaded? |
Very good question. Answer: yes. It does. Now.. :) |
jobs: { | ||
exportEventsWithRetry: ExportEventsJobPayload | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit confusing to me: aren't jobs supposed to be callables?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed they are, but the only thing you can actually configure is the payload of a job. So in an effort to reduce boilerplate and not give any wrong ideas, in this jobs: {}
object you just specify the payload.
Basically, I think it's just nicer to write
jobs: {
exportEventsWithRetry: ExportEventsJobPayload
}
instead of something with more boilerplate like:
jobs: {
exportEventsWithRetry: (payload: ExportEventsJobPayload) => Promise<void>
}
A question about the general flow of data here:
This is separate from the runAt / runWhenever functionality which is redlocked. So, it's entirely possible we're sending X requests together, where X is number of threads over all instances? We may have only 1 instance processing a job at a time, but that doesn't stop the buffer flushes on timeout, since this is separate from the jobs locking mechanism? *until it gets to a retry (Correct me if I'm wrong somewhere please) |
@neilkakkar indeed, |
Agreed, just wanted to (1) ensure I understood correctly, and (2) make my concern explicit. It's probably not worrisome right now, and definitely better to have a buffer than not, but as you say, something to keep in mind :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work!
* export events via vm upgrade * cleaner exportEvents upgrade, add RetryError * add basic tests * test more things, fix buffer length issue * fix type * add missing vm method * add plugin scaffold to imports * Use JSDoc style for tooltips and fix onEvent typing * stringClamp * remove dead code * add consts * log locally * better log * it's a hub now * less events in benchmark to hopefully deflake a test * fix type bug * fix awkward bug * add statsd for export event jobs * typefix and rename * fix ! in test * flush on teardown * config as a string, as it should Co-authored-by: Michael Matloka <[email protected]>
Changes
exportEvents
function in plugins that abstracts away all the queueing, batching, retrying logic #404exportEvents
by using existing plugin server tools as building blocks.await fetch
, etc) or throwsRetryError
. If the latter happens, we retry exporting the entire batch again.exportEvents
function is exported, this creates abuffer
viacreateBuffer
, and adds or patches an existingonEvent
function to add incoming events to that buffer.exportEventsWithRetry
job to retry events.meta.config
. As a plugin author, you can ask these from your users, or stick to the defaults:meta.config
parsing boilerplate insetupPlugin
.Please rip and nit this to shreds :).
Tests
mightwill still fail, will fix if so.Checklist