Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Event firehose #419

Closed
jesstelford opened this issue Oct 9, 2018 · 6 comments
Closed

Event firehose #419

jesstelford opened this issue Oct 9, 2018 · 6 comments
Assignees

Comments

@jesstelford
Copy link
Contributor

Related to #292 / #293.

Context

The end goal is to be able to send emails when one of the following conditions are true:

  • A particular time has passed (eg; "an event is happening in 24hrs")
  • A database mutation occurred (eg; "an event has been created")

This problem can be broken down into 2 halves:

  1. Triggering an email send
  2. Actually sending / queueing up emails

The act of sending the emails (2) can be handled by something like Zapier or ifttt for my usecase.

This issues is mostly about (1): Triggering events.

Triggers

The two triggers present different challenges, with different solutions

Mutation based

This one is mostly solved with pre/post hooks.

I think we need to expose those hooks at a list-config level, but the logic is already there for triggering them on various CRUD operations.

It then becomes a matter of making the correct call to Zapier / ifttt within a hook:

keystone.createList('Meetup', {
  fields: {
    when: { type: DateTime },
    name: { type: Text },
    info: { type: Text },
  },
  hooks: {
    postCreate: async meetup => {
      const whoToNotify = await getGroupSubscribersFromMeetup(meetup.id);
      sendToZapier('send-email', whoToNotify, `New meetup ${event.name}!!1!1one`);
    }
  }
});

Time based

This one is a bit trickier - time based triggers require some kind of state.

There's a possibility that ifttt / Zapier support handling that state (ie; we tell Zapier when to trigger something we send it). However, that adds complexity around updates / cancellations of events.

Another option is to ditch the idea of time-based and say "Hey user; if you want to send emails to your members, you'll have to set an alarm on your smart phone, then trigger an event" (See Bike Shedding for a method of triggering events via a write to an Events list, which can be done in the admin UI)

Bike Shedding

There may be a need to keep track of what events have been sent, so instead of sending to Zapier in the Event's postCreate hook, we might setup a separate list like so:

keystone.createList('Meetup', {
  fields: {
    when: { type: DateTime },
    name: { type: Text },
    info: { type: Text },
  },
  hooks: {
    postCreate: async meetup => {
      const whoToNotify = await getGroupSubscribersFromEvent(meetup.id);
      keystone.mutation(`
        createEvent(data: { whoToNotify: ${whoToNotify}, title: "New event ${event.name}!!1!1one" }, type: 'send-email') {
          id
        }
      `);
    }
  }
});

keystone.createList('Event', {
  fields: {
    triggerTime: { type: DateTime },
    type: { type: Select, options: ['send-email'] },
    data: { type: JSON },
    triggered: { type: Checkbox },
  },
  hooks: {
    preCreate: async event => {
      // Only trigger it if it's in the past, or there's no event time set (ie; immediate)
      if (!event.triggerTime || event.triggerTime < now()) {
        sendToZapier(event.type, event.data);
        event.triggered = true;
      }
      return event;
    }
  }
});

Then for time-based ones, we could run an independent cron job every 1s, that looks for unsent events:

cron.on('trigger', () => {
  const pendingEvents = await keystone.query(`
    allEvents(where: { triggered_is: false, triggerTime_lt: "${Date.now().toUTCString()}" }) {
      id
      type
      data
    }
  `);
  pendingEvents.forEach(event => {
    keystone.mutation(`
      updateEvent(where: { id: "${event.id}" }, data: { triggered: true }) {
        id
      }
    `);
    sendToZapier(event.type, event.data);
  });
});
@JedWatson
Copy link
Member

Thinking about preCreate update hooks, As these would run before the data is committed to the database, they may run when it won't be committed to the database. Examples include validation, unique constraints, or even outright database failures etc. You really wouldn't want to send an email in that case - making postCreate more appropriate.

When doing that, you end up in a double-write scenario, but maybe that's generally a better trade-off; to simply that use case we could provide an update function to the hook that re-saves the data but bypasses any further hooks / processes etc.

Like this:

    postCreate: async (event, update) => {
      // Only trigger it if it's in the past, or there's no event time set (ie; immediate)
      if (!event.triggerTime || event.triggerTime < now()) {
        sendToZapier(event.type, event.data);
        update({ triggered: true });
      }
    }

The argument could potentially be an actions object, with various things you could do like { update, rollback, ..? } if we wanted to expose more functionality.

Really most actions performed in hooks should go through the main keystone API wherever possible, but it can be helpful to have a way around infinite loop scenarios when you need to chain updates in a process with multiple async steps that could fail.

While we're on that API, another interesting feature could be passing a context object around so you can trace through the lifecycle of hooks. I am not sure if this is actually a good idea or a bad idea

    preCreate: async (event, { context }) => {
      if (condition(event)) context.something = true;
    },
    postCreate: async (event, { context, update }) => {
      if (context.something) {
        update({ definitelySaved: true });
      }
    }

This might be an interesting way to allow cross-cutting logic between hooks, potentially even across multiple instances of the lifecycle events if that's a thing (e.g because plugins, etc).

It does effectively introduce a global object, however, which could be bad if misused.

Finally - not sure which issue to link this to - but what happens if we throw in a hook? is that defined somewhere?

@jesstelford
Copy link
Contributor Author

Good points about the pre/post hooks - didn't think of those failure cases!

We already pass context into the hooks (it's the GraphQL context object), and is in use by the Relationship field to manage actions before and after the write to the DB.

what happens if we throw in a hook? is that defined somewhere?

This is something that needs to be fleshed out more. A throw in a pre hook will cause the GraphQL operation to fail (and return the error thrown).

A throw in a post hook would also report an error, but the write would still have occurred. We need transaction support to handle these cases.

@molomby
Copy link
Member

molomby commented Oct 10, 2018

@jesstelford:

@JedWatson:

  • Pretty sure all this talk of double-writes should be irrelevant; Mongo supports multi-document transactions now and all hooks/hook-like functionality should use them.
  • Hooks definitely need a way of passing context between them; eg. a pre hooks will need a way of passing data to post hooks. This was included in the initial/incomplete hook spec work we did when we were talking through ACLs.

Most of the current hook chat is in #244.

Really most actions performed in hooks should go through the main keystone API wherever possible, but it can be helpful to have a way around infinite loop scenarios when you need to chain updates in a process with multiple async steps that could fail.

If I'm understanding you correctly this gets back to the "layers" of hooks we've discussed previously. There may be hooks at the GraphQL layer, and possibly hooks at the Keystone list API layer and maybe even developer-added hook-like operations happening at the ORM or DB layer (eg. Mongoose hooks, DB triggers). However since each layer only/always operates on the layer below it shouldn't be possible to form loops.

I'm actually not sure this issue belongs in the Keystone repo. All the generic parts of this (email interface: #292, email queue: #293, workers: #307, hooks: #244) are described elsewhere (albeit not yet spec'ed or implemented). The usage of these generic tools (to build an event firehose) seems pretty project specific to me... am I wrong?

@jesstelford
Copy link
Contributor Author

Hooking up to something like Amazon's new EventBridge would be 👌 for logging and all sorts of other uses.

https://www.trek10.com/blog/amazon-eventbridge/

@stale stale bot added the needs-review label Sep 21, 2019
@keystonejs keystonejs deleted a comment from stale bot Oct 21, 2019
@stale stale bot removed the needs-review label Oct 21, 2019
@stale stale bot added the needs-review label Feb 18, 2020
@stale stale bot removed the needs-review label Jun 1, 2020
@keystonejs keystonejs deleted a comment from stale bot Jun 1, 2020
@MadeByMike
Copy link
Contributor

@jess when you get a chance could you put some eyes over this issue: #419 is this still relevant as something we want to add to Keystone? I feel like maybe it's better people BYO their own library for this. Perhaps you have patterns now from Cete that 'solve' this.

@jesstelford
Copy link
Contributor Author

Now that our list & field hooks are much more fleshed out, that gives me all the extension points I need to trigger events.

As for time-based events, that's still on my todo list. However, I no long expect Keystone to handle this for me. Instead, I might look to something like BullMQ or Bee-Queue instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants