Calculate metrics using buckets. #18
Conversation
Rewrite the metrics calculation. It replaces the window array which a data sample for every operation with a fixed size list of buckets. This resolves vert-x3#17 which describes this memory leak.
| } | ||
|
|
||
| public void add(Operation operation) { | ||
| getBucket(operation.end).add(operation); |
There was a problem hiding this comment.
In the original implementation, one looked at the begin timestamps:
window.removeIf(operation -> operation.begin < beginningOfTheWindow);
This causes operations which take more than 10 seconds to not being taken into account in the rolling statistics. So that is why I decided to look at .end.
|
|
||
| private Summary(long bucketIndex) { | ||
| this.bucketIndex = bucketIndex; | ||
| statistics = new Histogram(2); |
There was a problem hiding this comment.
Propose to use 2 significant digits to use less space. (4 kilobyte/histogram <> 64kilobyte per histogram)
No problem for me to change it back to 3.
| } else { | ||
| json.put("rollingSuccessPercentage", ((double) rollingSuccess / window.size()) * 100); | ||
| json.put("rollingErrorPercentage", | ||
| ((double) (rollingException + rollingFailure + rollingTimeout + rollingShortCircuited) / window.size()) * 100); |
There was a problem hiding this comment.
This shortCircuits were only seen as errors for for the rollingErrorPercentage, not for the totalErrorPercentage.
Removed it for consistency. But do not mind to regard them as errors.
|
It looks great. Can you add a test to check it. Also, I believe you did, but did you try with the Hystrix dashboard? |
|
@cescoffier thanks for your review. I validated it with the Hystrix dashboard. There are already some extensive tests on the |
|
Great, thanks! |
Rewrite the metrics calculation. It replaces the window array which
a data sample for every operation with a fixed size list of buckets.
This resolves #17 which describes this memory leak.