-
Notifications
You must be signed in to change notification settings - Fork 7
Conversation
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 also looking good! I didn't have time to test with real accounts though, so tell me if I should.
I'd improve this by still adding the latest scaffold and types, like for S3:
PostHog/s3-export-plugin@2c751b6
Also, I noticed one difference between this and the S3 plugin. The S3 plugin in onFlush
tries to do the upload to S3, whereas this one runs a job in the onFlush
, and tries to upload only after the job has been dispatched (so the payload moves to postgres and back somewhere).
Any specific reason for one or the other? In the near future I do want to also add a simple memory queue, meaning all runNow()
tasks will just run now directly in the same server instance, without going through any postgres queue. So performance wise it shouldn't even matter.
@mariusandra would appreciate you testing this actually, so that I don't have to spin up another BQ instance. The way I tested it was by making sure the same payloads as before reached the BQ upload call. As for the approach, the S3 plugin originally did things like this one, where the job enqueues the next job. However, after your point about the callback I restructured it so the upload function enqueues the next job. Should be able to consolidate approaches though if you prefer. |
"Crazy how fast plugins have been moving", huh? While trying to improve this, I went ahead and built the |
I added a bunch more stuff and tested that it all still works. The only problem: locally the |
Crazy how fast plugins have been moving. This plugin's code already felt so old.
onEvent