-
Notifications
You must be signed in to change notification settings - Fork 184
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
Various improvements #192
Various improvements #192
Conversation
- Make “num_workers” setting configurable - Add possibility to send log messages to stderr - Improve reporting about log level - Log warning message if transform_data cannot be formatted properly - Improve worker robustness and shutdown behavior re. job queue draining - Improve logging and robustness for service plugins smtp and xmpp - Add monkey patch for xmpp service plugin to mitigate problem with SSLSocket X.509 issuer
@@ -26,8 +27,13 @@ def plugin(srv, item): | |||
msg['From'] = sender | |||
msg['X-Mailer'] = srv.SCRIPTNAME | |||
|
|||
if not smtp_addresses: | |||
srv.logging.warn("Skipped sending SMTP notification to %s, " | |||
"no addresses configured: %s" % (item.target, smtp_addresses)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the second parameter here redundant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, because it'll be None
. I will remove after merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, the second parameter will be None or an empty list. Thanks for spotting this!
Looks really good Andreas - thank you! I will leave JP to comment further but I took a good look around your project and am mightily impressed - I can imagine this will be a very useful reference project for others want to setup similar monitoring applications using MQTT/mqttwarn. Thanks again for sharing your code with our little project! |
@amotl this looks excellent thank you. And thank you particularly for finally finding and fixing the suboptimal shutdown behavior. ;-) |
Merged! |
Thanks! |
Dear Jan-Piet and Ben,
we have another round of improvements to mqttwarn ready. This is the foundation of the implementation of our prototypic Hiveeyes Schwarmalarm #193 on top of mqttwarn, which is the preliminary alerting component of the Hiveeyes system. We are building upon one of our former contributions, see also Incorporate topic names into topic targets and #169, #170.
The changes are mainly about overall robustness, especially when having more pressure on the job queue. The shutdown behavior of mqttwarn hasn't been optimal before, we often had cases where mqttwarn wouldn't stop at all on
SIGTERM
because the worker (def processor
) already had been shut down whiledef cleanup
was waiting for the notification job queue to drain, resulting in deadlock situations only to be mitigated bykill -9
on our systems.Along the lines, we added and improved some log messages here and there, integrated some minor features not worth a separate pull request in our opinion and also added a conveniency routine to monkeypatch the xmpppy library code for mitigating a known and popular problem when connecting to XMPP servers with TLS support (can be used optionally, thus non-invasive).
The details:
Please let us know what you think about this. In the meanwhile, we will prepare the main PR mentioned above. Thanks in advance for your efforts!
Cheers, Andreas.