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

Add notification for third party services #7

Merged
merged 8 commits into from
Apr 18, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .env
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,5 @@ FLASK_ENV=development
FLASK_DEBUG=1
FLASK_APP=openreferee_server.app
DATABASE_URI = 'postgresql:///openreferee'
NOTIFY_URL=
NOTIFY_TOKEN=
3 changes: 2 additions & 1 deletion openreferee_server/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
from apispec_webframeworks.flask import FlaskPlugin
from flask import Flask, jsonify
from werkzeug.exceptions import HTTPException, UnprocessableEntity

from .notify import NotifyExtension
from . import __version__
from .db import db, register_db_cli

Expand All @@ -26,6 +26,7 @@ def create_app():
register_error_handlers(app)
db.init_app(app)
register_db_cli(app)
NotifyExtension(app, os.environ.get('NOTIFY_URL'), os.environ.get('NOTIFY_TOKEN'))
Copy link
Member

Choose a reason for hiding this comment

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

Use os.environ[...] if you expect this to always be present

Copy link
Contributor Author

@joshpme joshpme Dec 8, 2023

Choose a reason for hiding this comment

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

This will throw an exception if the keys are not present, where as get will soft return None

I explicitly handle none, plus as these variables can be configured with config instead of environmental variables if that is needed later.

Is there a reason why you prefer to use a dict lookup over get?

Copy link
Member

Choose a reason for hiding this comment

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

I generally prefer to fail early: If required config keys are missing, I want it to fail at startup time. That's something I see immediately because the app is clearly broken.

If the missing value leaks into the config and it only fails later when trying to send a request, there's a good chance I won't notice it until a real request from a user's action arrives.

That said, I think it makes sense to simply disable notifications if these settings are missing. I'd do that by not initializing the extension and silently doing nothing later when the extension is not on the app context (based on your current architecture). Without the extension structure I'd simply return early in the function to send a notification if the config values are None,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, fair. I'll refactor this now then.

app.register_blueprint(api)
return app

Expand Down
45 changes: 45 additions & 0 deletions openreferee_server/notify.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import json
import threading

from .operations import setup_requests_session


class NotifyService:
def __init__(self, url=None, token=None) -> None:
self.url = url
self.token = token
self.session = None

def send(self, payload):
try:
self.session.get(self.url, data=json.dumps({"payload": payload}))
joshpme marked this conversation as resolved.
Show resolved Hide resolved
except Exception:
pass
joshpme marked this conversation as resolved.
Show resolved Hide resolved

def notify(self, payload):
if self.url is None:
return

if self.session is None:
self.session = setup_requests_session(self.token)

threading.Thread(target=self.send, args=(payload,)).start()
Copy link
Member

Choose a reason for hiding this comment

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

I wonder... is all this extra complexity really needed? To me it sounds like the whole code in here could be done with a simple

def notify(payload):
    sess = ...
    try:
        sess.post(..., timeout=5)
    except ...:
        pass  # log it

no threads, no flask extension, no context etc needed ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure I thought I might be over-complicating this, but I thought I'd be adding the notify to each of the endpoints, or maybe extending the logic out if required.

The reason why I use an extension, I needed to load in the environmental config for the notify URL. So I figured I'd need an extension to load this in? Should I just load from process.env from anywhere?

The threading is there as I wanted this to me fire and forget so there would be no slow down to this service if our downstream system was offline or slow to response.

The two reasons I'm using a session is that I am able to config the authentication at a parent level using the method you already have, and depending on the traffic in this service, having a common session between requests reduces http overhead. I'm not sure how it cleans itself up though, so I'm happy to simply this if you do not believe there is much traffic?

Copy link
Member

Choose a reason for hiding this comment

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

The reason why I use an extension, I needed to load in the environmental config for the notify URL. So I figured I'd need an extension to load this in? Should I just load from process.env from anywhere?

Yes, you can read env vars anywhere. Or even put it in the app.config dict and then access it from there later.

The threading is there as I wanted this to me fire and forget so there would be no slow down to this service if our downstream system was offline or slow to response.

Fair enough, I don't know how reliable your downstream is. Personally I'd probably just use a low-enough timeout and then only log request-related errors so they don't break the flow.

The two reasons I'm using a session is that I am able to config the authentication at a parent level using the method you already have, and depending on the traffic in this service, having a common session between requests reduces http overhead. I'm not sure how it cleans itself up though, so I'm happy to simply this if you do not believe there is much traffic?

Actually, I'm not sure if the session actually keeps connections alive etc. or not. Using it for the token is a good idea though!

Copy link
Contributor Author

@joshpme joshpme Dec 10, 2023

Choose a reason for hiding this comment

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

Is this call to the microservice blocking, as in, will buttons take longer to load if we put in a downstream call?

I expect that the (yet-to-be-built) downstream service may be hosted on the other side of the planet, also I really didn't want to be responsible for an extra 2-5 second load times on some buttons, or prematurely timeout due to distance.

That being said, I'm also no python guru, so maybe threads are not the best tool for the job here (due to leaky threads or contention issues)? So if you feel strongly about this I will just change it.

Also regarding the use of an extension, I was just following a pattern I'm familiar with from other (non-python) frameworks, so if you want me to simplify it, no problem, I'm just not sure if you comment was more of an observation or a suggestion for change.

Copy link
Member

Choose a reason for hiding this comment

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

the buttons won't take longer to load since you're not doing anything to the get_custom_revision_actions endpoint, but still it's probably better to do it this way if you're unsure of the reliability of the service



class NotifyExtension:
def __init__(self, context=None, notify_url=None, notify_token=None):
self.url = notify_url
self.token = notify_token
self.service = None
if context is not None:
self.init_app(context)

def init_app(self, context):
joshpme marked this conversation as resolved.
Show resolved Hide resolved
context.config.setdefault('NOTIFY_URL', self.url)
context.config.setdefault('NOTIFY_TOKEN', self.token)

self.service = NotifyService(context.config['NOTIFY_URL'], context.config['NOTIFY_TOKEN'])
if not hasattr(context, 'extensions'):
context.extensions = {}
joshpme marked this conversation as resolved.
Show resolved Hide resolved

context.extensions['notifier'] = self.service
16 changes: 16 additions & 0 deletions openreferee_server/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

from .app import register_spec
from .db import db
from .notify import NotifyService
from .defaults import DEFAULT_EDITABLES, PROCESS_EDITABLE_FILES, SERVICE_INFO
from .models import Event
from .operations import (
Expand Down Expand Up @@ -44,6 +45,10 @@
)


def get_notifier(context) -> NotifyService:
return context.extensions.get('notifier')
joshpme marked this conversation as resolved.
Show resolved Hide resolved


def require_event_token(fn):
@wraps(fn)
def wrapper(*args, **kwargs):
Expand Down Expand Up @@ -285,6 +290,17 @@ def review_editable(
"A new revision %r was submitted for contribution %r", revision_id, contrib_id
)
resp = {}

get_notifier(current_app).notify({
"event": event.identifier,
"contrib_id": contrib_id,
"revision_id": revision_id,
"action": action,
"editable_type": editable_type,
"user": user,
"request": request.json
})

if action == "accept":
resp = process_accepted_revision(event, revision)

Expand Down
Loading