Add notification for third party services #7
Conversation
openreferee_server/app.py
Outdated
| @@ -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')) | |||
There was a problem hiding this comment.
Use os.environ[...] if you expect this to always be present
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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,
There was a problem hiding this comment.
Yeah, fair. I'll refactor this now then.
| if self.session is None: | ||
| self.session = setup_requests_session(self.token) | ||
|
|
||
| threading.Thread(target=self.send, args=(payload,)).start() |
There was a problem hiding this comment.
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 itno threads, no flask extension, no context etc needed ;)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Log error on exception Check logger extension is present before attempting to log
…esence of NOTIFY_URL, route the flask debugger into the service to allow logging to actually work
… class remove the extension class which really isn't used beyond initialisation notify_init is now the path for initialising the service
|
@ThiefMaster is there anything you still need me to do on this PR before it can be merged? |
| @@ -285,6 +291,17 @@ def review_editable( | |||
| "A new revision %r was submitted for contribution %r", revision_id, contrib_id | |||
| ) | |||
| resp = {} | |||
|
|
|||
| notify(current_app, { | |||
There was a problem hiding this comment.
you might want to consider calling this in some of the other methods, such as create_editable, otherwise you won't get notified of the first submission... even if you don't need it right now it might be useful in the future
isort ran remove non-pythonic empty check remove incorrect return type hinting reduce indents by returning early on notify_init
There was a problem hiding this comment.
lgtm! might wanna get rid of that merge commit tho, we usually prefer rebasing
(not sure if this matters for this repo, I'll let @ThiefMaster decide)
|
I'll squash+merge anyway, so doesn't really matter. |
This will allow downstream systems to be notified when revisions are created in indico specifically for JACoW
It is fire and forget to prevent any outage affecting indico uptime.
The hope is that will enable a downstream system "e.g." catscan to perform a scan on a change, rather than polling.
Also first python PR ever, so I welcome any feedback.
If you are happy to merge and deploy the change, we can discuss the URL / Token on slack