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

[WIP] Simple webhook-based notification system #3944

Draft
wants to merge 1 commit into
base: master
from

Conversation

@pferreir
Copy link
Member

commented May 31, 2019

This implements a very simple webhook-based notification system that should be compatible with CERN's Push Notification service and any other web service using a REST API. Authentication is done using Bearer tokens and data is sent in JSON.

A few things to discuss:

  • Content types - I am using the usual RenderModeMixin and allowing Markdown, HTML and Plain Text. For now we're just sending out whatever type the notification claims to be in. It could be interesting to discuss Markdown-based notifications in Indico, since it can be converted to HTML easily or gracefully degraded to plaintext if needed;
  • The dispatching mechanism - right now notifications are appended in a DB table and then dispatched by a task that runs every ~1 min. This is not real time, of course, but is more than enough for this use case (notifications about starting/ending bookings);
    • I chose to store the notification in the DB instead of just sending it out to Celery for failure recovery and tracking reasons;
    • An interesting alternative would be to immediately send out the notification but also save it in the DB (it could even be saved asynchronously, post-request). Then we could have a task that just goes over notifications that have been pending for longer than X;
  • It would be interesting to have a "notification area" for users, where they can check which notifications they've received. Since we have that information in the DB, why not?
    • In that case it could be interesting to provide the PN service with a callback URL that it could optionally call to let us know that the person has read the notification;

Things to change:

  • Using Bearer token instead of proprietary field (we have to tell the PN guys to use that instead).

try:
for notification in Notification.query.filter(~Notification.is_sent):
if notification.user.email == 'pedro.ferreira@cern.ch':

This comment has been minimized.

Copy link
@pferreir

pferreir May 31, 2019

Author Member

BTW. This is intentional. It's here to avoid accidental spamming of the PN service. You'll have to change it to your e-mail if you want to test it, of course.

indico/modules/notifications/forms.py Outdated Show resolved Hide resolved
indico/modules/notifications/forms.py Outdated Show resolved Hide resolved
indico/modules/notifications/models/notifications.py Outdated Show resolved Hide resolved
indico/modules/notifications/models/notifications.py Outdated Show resolved Hide resolved
data-href="{{ url_for('notifications.send_test') }}"
data-method="POST"
data-ajax
data-update="#dummy">

This comment has been minimized.

Copy link
@ThiefMaster

ThiefMaster May 31, 2019

Member

do we actually need that? :/

This comment has been minimized.

Copy link
@pferreir

pferreir May 31, 2019

Author Member

Yes, I'm afraid so.

indico/modules/notifications/util.py Outdated Show resolved Hide resolved
indico/modules/notifications/views.py Show resolved Hide resolved
@ThiefMaster

This comment has been minimized.

Copy link
Member

commented May 31, 2019

Regarding dispatching/storing: I think the minutely task is fine for this particular case, but for something where you can get a notification as a result of your own action I think this is somewhat annoying - imagine performing some action. and then getting an email/notification about it only a minute later. Much more disrupting than getting (and then probably deleting) it immediatey.

In terms of reliability there's no need to store them - even if Celery is down, sending a task won't fail. It will still be enqueued and picked up as soon as celery is restarted. And when redis is down, then more things are broken anyway (especially sessions and celery, so very low chance of something triggering a notification).

That aside, I think persistent storage of notifications is not a bad idea (maybe delete old ones after a while though, no need to keep that stuff for a long time..), since it'd allow showing them inside indico (like on fb, github, etc.) at some point as well.

So I'd put the notification in the DB, but also immediately send it to celery. We just need to make sure that it's committed before sending it to Celery or there can be cases where celery tries to load the notification from the DB before it's been committed.

@pferreir

This comment has been minimized.

Copy link
Member Author

commented May 31, 2019

In terms of reliability there's no need to store them - even if Celery is down, sending a task won't fail. It will still be enqueued and picked up as soon as celery is restarted.

That was my initial approach, but then I thought what a mess it will be if a few thousand tasks fail and we end up with Celery retrying all of them in an interval of minutes/seconds. Or if there's a bug that causes a task to fail and we just want to get rid of it and have to manually delete it from the Celery queue...

@ThiefMaster

This comment has been minimized.

Copy link
Member

commented May 31, 2019

Or if there's a bug that causes a task to fail and we just want to get rid of it and have to manually delete it from the Celery queue...

If a task fails unexpectedly, it won't be retried. It'll only retry it if you catch the exception and raise a different one telling celery to retry (or maybe you can also configure a task to auto-retry, but I don't think we do that anywhere)

@pferreir

This comment has been minimized.

Copy link
Member Author

commented May 31, 2019

Yeah, I know. But do you really want to ignore a task that fails, right away? Imagine there's a bug and a few thousands of them just fail. That's why keeping a log isn't a bad idea IMHO.

@pferreir pferreir force-pushed the pferreir:push-notif branch from 78ea282 to 60cf029 May 31, 2019
@pferreir

This comment has been minimized.

Copy link
Member Author

commented Jun 4, 2019

I've updated the PR to use a different strategy (discussed IRL with @ThiefMaster):

  • Sending the notification right away and graciously failing if something is wrong (logging it, of course);
  • Running, every 5 minutes, a task that re-sends all notifications older than 5 minutes that haven't yet been marked as sent;
@pferreir pferreir force-pushed the pferreir:push-notif branch from 60cf029 to ccadfdf Jun 4, 2019
@pferreir pferreir force-pushed the pferreir:push-notif branch from ccadfdf to b2f52fe Jun 4, 2019
@pferreir pferreir force-pushed the pferreir:push-notif branch from b2f52fe to 676db63 Jul 17, 2019
@pferreir pferreir force-pushed the pferreir:push-notif branch from 676db63 to 189be2e Jul 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.