Hi Simon, On Mon, Feb 02, 2015 at 11:16:09AM +0900, Simon Horman wrote: > > > * 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.
Absolutely! > 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. Yes and it quickly becomes a mess where we see some %[] appear again because all possible tags have been used and are not enough :-/ > I'd also be happy to let this be and see what requests people have > to enhance or configure the messages. I agree on this! > > > * 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? Yes that was it, to preserve the reader's patience. > 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. Possible indeed. > > > * No Documentation > > This one is not so difficult to fix and I will see about doing so. OK. > > > * 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. It might be harder now than what it could be later (eg: switch the transport protocol in the middle of a connection), so let's forget about it for now. > > > 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. I know, the idea is that it's in order to provide our beloved users with the features they want right now, while we'll be the ones having to scratch our heads behind the curtain later. What you did didn't destroy health checks so in the worst case it could simply be undone, and I don't think there would be any reason for this. You're just reusing the check infrastructure because it's the only one capable of initiating a standalone connection outside for now. > > 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. So I've merged it now, all 8 patches. Now I trust you to provide the doc so that users can quickly start testing it. Do not hesitate either if you want to fix issues or change your mind about certain points you are not happy with, this is still in development, no worries! Thanks! Willy
