Hi Sorry for chiming in late ...
Am 14.08.2013 um 10:12 schrieb Bertrand Delacretaz: > Hi Carsten, > > > On Tue, Aug 13, 2013 at 4:11 PM, Carsten Ziegeler <[email protected]> > wrote: >> ...I'm not sure whether the current handling of log entries and status >> between >> the result object and the log entries is a good thing.... > > Reading your comments, I agree it can be improved. > >> ...all log entries added to the result are logged to a logger - I >> think this can easily result in having the statements logged out more than >> once and it breaks the contract of having result a simple data object... > > Right, this is not really useful in the end, I agree about removing > all the slf4j stuff in the Result class. Clients can always log the > Results if they want. > . >> In addition, when a log message is added, it's level is evaluated to >> automatically set the status - if a HC check is just doing log statements >> for informational purpuse with level that leads to automatic status >> setting, the HC can't set the status to a lower level afterwards... > > Correct, that's by design, but can be improved. I'll explain below. > . >> ...I think there shouldn't be any additional logic in the result class except >> from just adding the log entries to a collection.... > > I disagree, having a minimum amount of semantics in the Result class > helps standardize the way health checks report their results. > > We could also use a Map<Object, Object> as the result...I dont think > that's what we want ;-) > > Now, seeing your comments I realize that the current Result doesn't > express my ideas clearly enough - here are the two simple rules that I > absolutely want to keep: > > Rule 1: Health checks *must* explain why they set the result to a > given state. An HC cannot just say "my result is critical", it has to > say why. > > Rule 2: Once a result has been set to a given state, you cannot set it > back to a lower state: if you have found a critical problem, whatever > happens next doesn't matter, the result is critical. This prevents > subtle mistakes in HC implementations. These make sense to me. Yet, a "RESULT" is usually at the end of some operation. The operation does not start with the result and generates the input in the end. Hence I suggest the Result constructor to take the result status and reason. Both will become final fields of the result. If we need additional information we could still add, but I doubt its usefulness -- above anything already in the log or in the status reason. Not sure about the use case of the "merge" method... If the Result class is part of the API, I would rather see this an immutable complex data holder. > > I hope you agree that these are good rules to have, and I think we can > modify the Result class to better express this: > > public class Result { > public enum Status { DEBUG, INFO, OK, WARN, CRITICAL, HC_ERROR } I doubt DEBUG and INFO status are really worth it: I think OK status says it all for green; the status message may explain. The WARN and CRITICAL levels for yellow and red sounds good, too. Interesting, but certainly helpful is HC_ERROR to indicate an error while executing the checker (such as an uncaught Throwable or illegal configuration). Regards Felix
