search

Comcast / jrugged

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

Performance Monitor long/double value confusion #7

Closed weggert closed 11 years ago

weggert commented 11 years ago

We have an interesting mix of 'long' and 'double' values being returned in the PerformanceMonitors. The Success/Failure latencies are longs in millis. The Averages are doubles (also in millis).

We should standardize on one or the other.

michajlo commented 11 years ago

The divide is mainly along the metrics that come from MovingAverages and PercentileTrackers, and rightfully so. The simplest solution is probably to just cast the result of the moving average to a long in the PerformanceMonitor.

The question stands though if we are willing to forgo the accuracy in favor of convenience. On one hand the moving average itself may not be completely accurate, but on the other we have in the past preferred exact accuracy (referring to an issue around RequestCounters and synchronization some time back on google code).

joercampbell commented 11 years ago

I think that we could go either way on this converting the longs to doubles or the doubles to longs and be 'safe' about the end result. I would have to do a little mind reading to understand the specific use case for the long vs the double as it is currently implemented - but my guess would be original writer desired calculation precision and 'leaked' that desire out in the form of a double representation of time in millis. I don't believe that we should ever have the case that latency of any sort (calculated or otherwise) exceeding the top positive java value for a long.

Does anyone feel that it would matter one way or the other?

michajlo commented 11 years ago

I can probably provide a little more history on double vs long. If I remember properly, back when (pre 3.x?) only averge's and request counts were exposed. Then everything was double (except request count stuff, for obvious reasons). This makes sense, because a moving average is calculated as a double (see MovingAverage.java). Percentiles were introduced, which returned longs, which also makes sense, because for a percentile there is no division, just grabbing a value from the set of recent latencies.

In the end, having doubles and longs is completely justified. Perhaps instead of converting everything one way or the other, we just provide better documentation for the reason?

joercampbell commented 11 years ago

Wally - I sort of agree with Mishu on this one, does the standardization buy us something specific?

joercampbell commented 11 years ago

Pinging to see if my above comment closes out the issue.

weggert commented 11 years ago

I concur. The current implementation seems fine to me. I believe this issue can be closed.