Skip to content
This repository has been archived by the owner on Nov 4, 2021. It is now read-only.

onEvent & onSnapshot #359

Merged
merged 10 commits into from
May 6, 2021
Merged

onEvent & onSnapshot #359

merged 10 commits into from
May 6, 2021

Conversation

mariusandra
Copy link
Collaborator

Changes

Checklist

  • Updated Settings section in README.md, if settings are affected
  • Jest tests

@mariusandra mariusandra marked this pull request as ready for review May 6, 2021 13:59
@mariusandra mariusandra added the bump patch Bump patch version when this PR gets merged label May 6, 2021
@Twixes Twixes added bump minor Bump minor version when this PR gets merged and removed bump patch Bump patch version when this PR gets merged labels May 6, 2021
src/main/ingestion-queues/kafka-queue.ts Outdated Show resolved Hide resolved
src/main/ingestion-queues/kafka-queue.ts Outdated Show resolved Hide resolved
Comment on lines +155 to +156
onEvent: __asyncFunctionGuard(__bindMeta('onEvent')),
onSnapshot: __asyncFunctionGuard(__bindMeta('onSnapshot')),
Copy link
Member

@Twixes Twixes May 6, 2021

Choose a reason for hiding this comment

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

onEvent conveys the read-only nature of this feature well, but maybe not that this is ran at the very end of processing, when the even has been processed by all plugins (as opposed to e.g. a raw event from the endpoint). So maybe something like onCompletedEvent, onFinishedEvent, onSavedEvent?

Copy link
Collaborator Author

@mariusandra mariusandra May 6, 2021

Choose a reason for hiding this comment

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

I'd really like to fight entropy here again :). If we add some modifier, then it's another thing for people to remember, and the entire on namespace will get annoyingly repetitive. onCreatedEvent, onCreatedAction, etc.

People who will know that onEvent happens after processEvent because in the interface we'll first have processEvent plugins that can be reordered, followed by onEvent plugins that can't be.

Copy link
Member

Choose a reason for hiding this comment

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

The UI is not an issue, but this may be unclear from an API perspective for plugin developers. But I don't have a strong opinion on this, so if you definitely see plain onEvent as the best option, then let's go for that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let's just document this well :).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With the UI, I was getting at the idea that if you're a plugin user, you will see from the UI that there are some plugins that happen in order and some that happen after them. This would lead to some understanding of how to write plugins. However, thinking further, indeed the users and developers won't be the same people (see rudderstack export), so scratch that reasoning. 🤷

@mariusandra mariusandra requested a review from Twixes May 6, 2021 20:29
@mariusandra mariusandra merged commit 039bbb9 into master May 6, 2021
@mariusandra mariusandra deleted the on-event branch May 6, 2021 21:01
fuziontech pushed a commit to PostHog/posthog that referenced this pull request Oct 12, 2021
* add onEvent and onSnapshot worker tasks

* run onEvent and onSnapshot in the two queues, disable processEvent for snapshots

* fix some tests

* onEvent and onSnapshot are methods in the vm

* e2e test for onEvent and onSnapshot for postgres/celery

* 3 worker jobs per event now

* on* tests for clickhouse e2e

* fix clickhouse ingestion bug

* extract "ingestEvent" and share between kafka and celery
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bump minor Bump minor version when this PR gets merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add onEvent Don't process $snapshot via plugins
2 participants