added sliding window histogram #200
Conversation
| min = new HdrHistogramMinGauge(MonitorConfig.builder(label).withTag("value", "min").withTags(tags).build(), histogram); | ||
| max = new HdrHistogramMaxGauge(MonitorConfig.builder(label).withTag("value", "min").withTags(tags).build(), histogram); | ||
|
|
||
| p50 = new HdrHistogramGauge(MonitorConfig.builder(label).withTag("value", "p50").withTags(tags).build(), histogram, 50, this::slide); |
There was a problem hiding this comment.
This will rotate the histogram for each slide of min, max, etc. Is that what you intend?
There was a problem hiding this comment.
get min and max don't reset the histogram
There was a problem hiding this comment.
My bad. You are correct that min and max don't but 50,90,99,99_9,99_99 all do.
| } | ||
|
|
||
| long value = histogram.aggregateHistogram().getValueAtPercentile(percentile); | ||
| slide.run(); |
There was a problem hiding this comment.
This could lead to unintentional sliding of the window. eg: If a user action fetches the value (eg: from a JMX console), it will reset the histogram.
There was a problem hiding this comment.
slide is passed in as a closure, and that method has a 1 minute timeout
There was a problem hiding this comment.
Yes but do you intend to slide (and in the other class, today it rotates the histogram) on every get ?
There was a problem hiding this comment.
yes - it'll only rotate now when the the timeout expires
There was a problem hiding this comment.
My bad, didn't see the timeout condition, thanks!
| */ | ||
| public void rotateHistogram() { | ||
| histogramAtomicReference | ||
| .updateAndGet(old -> { |
There was a problem hiding this comment.
Since, the passed function to updateAndGet should be side-effect free (as per the javadoc), I don't think this is a good usage of updateAndGet.
Maybe this should be synchronized to disallow concurrent modification and simply rotate the histograms atomically.
| * | ||
| * @return Aggregated histogramAtomicReference | ||
| */ | ||
| public Histogram aggregateHistogram() { |
There was a problem hiding this comment.
Since gets will most likely not be concurrent, done by the poller, do we need threadlocal isolation of aggregation? Synchronized aggregation may be simpler and efficient? Also, you are already synchronizing additions to the aggregate.
There was a problem hiding this comment.
No it's be bad because sync would block the aggregations. One line 86 it adds the live one, and then in the sync block it adds the ones in the queue.
I can replace the thread local with a new object.
There was a problem hiding this comment.
Yes the sync will block aggregations but one histogram will be used by one metric which will have one poller and hence there would only be a single thread calling aggregate isn't?
…ontrol aggregation and rotating histrogram
Added sliding window histogram Problem Current histogram doesn't resets on every get on every dimension. This creates choppy metrics. Modifications Modified the timer to have n histrograms in a queue and rotate of over them to smooth out the histogram over a sliding window. Switched the dimensions of the histrogram to a label called value. Switched the reseting of the histrogram to a closure that is called by the timer instead so it will rotate the histrogram once per window. Result Correct metrics over a rolling window.
Added sliding window histogram Problem Current histogram doesn't resets on every get on every dimension. This creates choppy metrics. Modifications Modified the timer to have n histrograms in a queue and rotate of over them to smooth out the histogram over a sliding window. Switched the dimensions of the histrogram to a label called value. Switched the reseting of the histrogram to a closure that is called by the timer instead so it will rotate the histrogram once per window. Result Correct metrics over a rolling window.
* KEEPALIVE timing suggestions - moved into SETUP where the value is - include server-to-server and mobile-to-server commentary * remove newline * remove change * soften language for 500ms
Problem
Current histogram doesn't resets on every get on every dimension. This creates choppy metrics.
Modifications
Modified the timer to have n histrograms in a queue and rotate of over them to smooth out the histogram over a sliding window. Switched the dimensions of the histrogram to a label called value. Switched the reseting of the histrogram to a closure that is called by the timer instead so it will rotate the histrogram once per window.
Result
Correct metrics over a rolling window.