Hi Willy, On Sun, Feb 01, 2015 at 03:15:27PM +0100, Willy Tarreau wrote: > Hi Simon, > > On Fri, Jan 30, 2015 at 11:22:52AM +0900, Simon Horman wrote: > > Hi Willy, Hi All, > > > > the purpose of this email is to solicit feedback on an implementation > > of email alerts for haproxy the design of which is based on a discussion > > in this forum some months ago. > > Thanks for working on this! > > (...) > > Internally the code makes use of a dummy check using the tcpcheck > > code to set up a simple sequence of expect/send rules. This, > > along with the notion of mailers, is my interpretation of the > > earlier discussion in this forum. > > Yes I have some memories about this discussion. > > > The current code has only been lightly tested and has a number of > > limitations including: > > > > * No options to control sending email alerts based on the event > > that has occurred. That is, its probably way to noisy for most > > people's tastes. > > Most likely, yes. I think that having a minimum level, just like > we do for logs, would solve that problem, given that messages are > arranged by log levels to avoid that verbosity as well. Thus maybe > having a statement like "email-alert level <syslog-level>" would > elegantly solve this.
Yes, I agree. That seems like a nice solution. > > * No options to configure the format of the email alerts > > You know, even if we make this format very flexible, some users > will complain that they cannot send it in html and attach graphs :-) Haha, yes indeed. One reason I choose not to tackle that problem at all was that it seems to have some similarity to Pandora's box. The best idea I have had so far is to allow a template, with some meta-fields that are filled in at run-time, E.g. %p might mean "insert the name of the proxy here". And for all other text to be treated as literals. This may be flexible enough for many use-cases. Though as you point out, its hard to cover all the bases. I'd also be happy to let this be and see what requests people have to enhance or configure the messages. > > * No options to configure delays associated with the checks > > used to send alerts. > > Maybe an improvement could be to later implement a small queue > and group messages that are too close together. It would require > a timeout as well so that queued messages are sent once the max > aggregation delay is elapsed. Do you mean include multiple alerts in a single email message? If so, yes that sounds reasonable to me and it is something that had crossed my mind while working on this series. I suspect it would not be terribly difficult to implement on top of this series. > > * No Documentation This one is not so difficult to fix and I will see about doing so. > > * No support for STLS. This one will be a little tricky. > > I don't think it is a big problem. Users may very well route to > the local relay if they absolutely don't want to send a clear-text > alert over a shared infrastructure. But given that they are the > same people who read their e-mails from their smartphone without > ever entering a password, I'd claim that there are a lot of things > to improve on their side before STLS becomes their first reasonable > concern. Yes, I agree. I think it would be nice to have. But I also think it is best classified as future work. > > Again the purpose is to solicit feedback on the code so far, > > in particular the design, before delving into further implementation > > details. > > I've reviewed the whole patch set and have nothing to say about > it, it's clean and does what you explained here. I feel like you're > not very satisfied with abusing the check infrastructure to send > an e-mail, but I wouldn't worry about that now given that nowhere > in the configuration these checks are visible, so once we find it > easier to handle the mailers using a regular task managing its own > session, it shouldn't be that hard to convert the code. I was a little apprehensive about reusing checks in this way. But the code turned out to be a lot cleaner than I expected. And I am not comfortable with this approach. > So for now I'm totally positive about your work. Please tell me > if you want me to merge it now in case that makes things easier > for you to continue. Some breakage is expected to happen soon on > the buffer management, requiring some painful rebases, so at least > we'll have to sync on this to limit the painful work. Thanks for your positive review. I would like to take your offer and ask for the code to be merged. I'll see about working on some of the lower hanging fruit discussed above. Especially providing some documentation.
