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

Various improvements #192

Merged
merged 1 commit into from
May 31, 2016
Merged

Conversation

amotl
Copy link
Member

@amotl amotl commented May 30, 2016

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 while def cleanup was waiting for the notification job queue to drain, resulting in deadlock situations only to be mitigated by kill -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:

  • 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

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.

- 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))
Copy link
Collaborator

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?

Copy link
Collaborator

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

Copy link
Member Author

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!

@sumnerboy12
Copy link
Collaborator

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!

@jpmens
Copy link
Collaborator

jpmens commented May 31, 2016

@amotl this looks excellent thank you. And thank you particularly for finally finding and fixing the suboptimal shutdown behavior. ;-)

@jpmens jpmens merged commit 1069b3c into mqtt-tools:master May 31, 2016
jpmens added a commit that referenced this pull request May 31, 2016
@jpmens
Copy link
Collaborator

jpmens commented May 31, 2016

Merged!

@amotl
Copy link
Member Author

amotl commented May 31, 2016

Thanks!

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

Successfully merging this pull request may close these issues.

3 participants