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

Add $plugins_deferred - #444

Merged
Twixes merged 3 commits into
masterfrom
plugins-run-after
May 28, 2021
Merged

Add $plugins_deferred#444
Twixes merged 3 commits into
masterfrom
plugins-run-after

Conversation

@mariusandra

Copy link
Copy Markdown
Collaborator

PRs over issues, but still things to discuss here.

Changes

    const expectedReturnEvent = {
        ...event,
        properties: {
            $plugins_failed: [],
            $plugins_succeeded: [],
            $plugins_deferred: ['test-maxmind-plugin (39)'],
        },
    }
    expect(returnedEvent).toEqual(expectedReturnEvent)
  • Why deferred? It kind of means that, though not "immediately after". It's one word though, so easier with the capitalization-Zoo we have going around. Please suggest something better 🤞 😁 . "Enqueued"? Too many queues...
  • Alternatively, we could do something like this:
    const expectedReturnEvent = {
        ...event,
        properties: {
            $plugin_34_test_maxmind_plugin: 'succeeded',
            $plugin_39_bigquery_export_plugin: 'deferred',
        },
    }
    expect(returnedEvent).toEqual(expectedReturnEvent)

This flat structure most likely allows for better filtering on the db level.

Checklist

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

@mariusandra
mariusandra marked this pull request as ready for review May 28, 2021 07:32
@mariusandra
mariusandra requested review from Twixes and neilkakkar May 28, 2021 07:32
@Twixes

Twixes commented May 28, 2021

Copy link
Copy Markdown
Member

So this is because $plugins_succeeded/$plugins_failed only works with processEvent, while onEvent is a whole separate async thing and we can only know that we are going to send the event to that method? $plugins_deferred is not super intuitive to me, but it's pretty logical if you think about it, so this makes sense.

@neilkakkar

Copy link
Copy Markdown
Contributor

Hmm, I don't quite get it.

Let's say my plugin is:

onEvent( event, meta ) {
    // web hook to somewhere else and then
   console.log(event)
}

Now, if I want to export all events, how does $plugin_deferred for an event help me? - I don't think I need to care about this, or $plugins_succeeded/$plugins_failed because this happens before an event is ingested? While our export from beginning cares about what's in the DB, and running through those events?

@mariusandra

Copy link
Copy Markdown
Collaborator Author

The intended feature I'm trying to achieve is that you install for example the BigQuery plugin. It starts exporting events immediately. Then you can click "export from the beginning" somewhere and it'll upload all the events that happened before the plugin ran.

We can't filter by select events from the_dawn_of_time until installed_at because of rolling updates, some events might still get processed after the plugin is installed.

The $plugins_succeeded prop saves just plugins that modify the event like processEvent. Plugins that run after, like onEvent, were not recorded in the event.

Hope this clarifies a bit more :)

@neilkakkar

Copy link
Copy Markdown
Contributor

That makes sense now!

So, when it comes time to "export from beginning", you'd check all three $plugin_* , and skip this event if you find the plugin in the $plugin_succeeded and $plugin_deferred category?

Hmm, this is clearer, but I'm still not sure it makes sense to do this via $plugin_deferred (sorry) - because we don't know if the deferred task actually succeeded or failed?

@mariusandra

Copy link
Copy Markdown
Collaborator Author

Indeed a "real" solution is to have some dynamic key-value database that for each plugin/event pair states if that's successful or not. However that's way too complex and storage intensive for this need.

As a proxy, we can just assume that all deferred (I still think there could be a better name) events were processed/exported successfully. We do have a separate "retry for the 48h" mechanism in place for such exportEvents plugins anyway. Thus basically we can assume, if a plugin has been deferred, it was either successful or its retries were handled outside of our scope.

@neilkakkar

Copy link
Copy Markdown
Contributor

I guess that's fair, and a reasonable compromise! I'll just mention this in our docs as well, whenever that happens.

re: naming -> $plugins_stalled, $plugins_to_retry, $plugins_triggered. I like triggered and deferred.

.. and then all we need is a trigger warning 😂

@mariusandra mariusandra self-assigned this May 28, 2021
@Twixes
Twixes merged commit b5323e7 into master May 28, 2021
@Twixes
Twixes deleted the plugins-run-after branch May 28, 2021 11:48
@mariusandra

Copy link
Copy Markdown
Collaborator Author

Ooh... not sure about triggered. If there's any word that triggers everybody, it's that one... so let's not do that :D.

fuziontech pushed a commit to PostHog/posthog that referenced this pull request Oct 12, 2021
* add $plugins_deferred

* remove this, as we will remove the entire batching code ASAP as well

Co-authored-by: Michael Matloka <dev@twixes.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants