Comcast / jrugged

A Java libary of robustness design patterns
Apache License 2.0
266 stars 93 forks source link

Add service name to RolledUpMonitoredService reasons. #16

Closed weggert closed 11 years ago

weggert commented 11 years ago

This addresses issue #15 in the Issues list.

If a RolledUpMonitoredService has a status that is not UP then getReasons can be used to determine the cause. Calling getReasons() enumerates the reason for each child service that is not UP. In many cases the child services are CircuitBreakers.

The current behavior for getReasons() lists only 'Open' or 'Send Probe Request' for each CircuitBreaker. It does not include any identification for which CircuitBreaker is affected.

This change adds the service name as a prefix for each reason, making it clear which service is causing the Non-UP status for the RolledUpMonitoredService.

Before Change: getReasons() might return { "Open", "Send Probe Request" }

After Chnage: getReasons() might return { "Service 1:Open", "Service 2:Send Probe Request" }

joercampbell commented 11 years ago

Wally - I am a little concerned about this:

for (String reason : serviceStatus.getReasons()) { reasons.add(serviceStatus.getName() + ":" + reason); }

How many reasons can a serviceStatus contain? I am not sure I would want to consistently do a tight loop like this with a string concat in it for object storage/usage reasons. But if it would only ever loop like 2 or 3 times then maybe I don't care...

weggert commented 11 years ago

The number of reasons is equal to the number of Non-UP child services. In general I would expect that to be a small number (0 or in the low single-digits).

I would expect that RolledUpServiceStatus.getReasons() would typically be called during a health check. This code is not executed during the CircuitBreaker.invoke() code path.

joercampbell commented 11 years ago

OK - I am going to pull this change in with two other pending requests and rev to 3.1.0. Your change and the other two are changing the expectations without changing the API. Thanks Wally.