added sliding window histogram #200
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| package io.reactivesocket.loadbalancer.servo.internal; | ||
|
|
||
| import org.HdrHistogram.ConcurrentHistogram; | ||
| import org.HdrHistogram.Histogram; | ||
|
|
||
| import java.io.PrintStream; | ||
| import java.util.ArrayDeque; | ||
| import java.util.concurrent.TimeUnit; | ||
|
|
||
| /** | ||
| * Wraps HdrHistogram to create a sliding window of n histogramQueue. Default window number is five. | ||
| */ | ||
| public class SlidingWindowHistogram { | ||
| private volatile Histogram liveHistogram; | ||
|
|
||
| private final ArrayDeque<Histogram> histogramQueue; | ||
|
|
||
| private final Object LOCK = new Object(); | ||
|
|
||
| public SlidingWindowHistogram() { | ||
| this(5); | ||
| } | ||
|
|
||
| public SlidingWindowHistogram(final int numOfWindows) { | ||
| if (numOfWindows < 2) { | ||
| throw new IllegalArgumentException("number of windows must be greater than 1"); | ||
| } | ||
| this.histogramQueue = new ArrayDeque<>(numOfWindows - 1); | ||
| this.liveHistogram = createHistogram(); | ||
|
|
||
| for (int i = 0; i < numOfWindows - 1; i++) { | ||
| histogramQueue.offer(createHistogram()); | ||
| } | ||
| } | ||
|
|
||
| private static Histogram createHistogram() { | ||
| ConcurrentHistogram histogram = new ConcurrentHistogram(TimeUnit.MINUTES.toNanos(1), 2); | ||
| histogram.setAutoResize(true); | ||
| return histogram; | ||
| } | ||
|
|
||
| /** | ||
| * Records a value to the in window liveHistogram | ||
| * | ||
| * @param value value to record | ||
| */ | ||
| public void recordValue(long value) { | ||
| liveHistogram.recordValue(value); | ||
| } | ||
|
|
||
| /** | ||
| * Slides the Histogram window. Pops a Histogram off a queue, resets it, and places the old | ||
| * on in the queue. | ||
| */ | ||
| public void rotateHistogram() { | ||
| synchronized (LOCK) { | ||
| Histogram onDeck = histogramQueue.poll(); | ||
| if (onDeck != null) { | ||
| onDeck.reset(); | ||
| Histogram old = liveHistogram; | ||
| liveHistogram = onDeck; | ||
| histogramQueue.offer(old); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Aggregates the Histograms into a single Histogram and returns it. | ||
| * | ||
| * @return Aggregated liveHistogram | ||
| */ | ||
| 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. 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 |
||
| Histogram aggregate = createHistogram(); | ||
|
|
||
| synchronized (LOCK) { | ||
| aggregate.add(liveHistogram); | ||
| histogramQueue | ||
| .forEach(aggregate::add); | ||
| } | ||
|
|
||
| return aggregate; | ||
| } | ||
|
|
||
| /** | ||
| * Prints HdrHistogram to System.out | ||
| */ | ||
| public void print() { | ||
| print(System.out); | ||
| } | ||
|
|
||
| public void print(PrintStream printStream) { | ||
| aggregateHistogram().outputPercentileDistribution(printStream, 1000.0); | ||
| } | ||
|
|
||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| package io.reactivesocket.loadbalancer.servo.internal; | ||
|
|
||
| import org.HdrHistogram.Histogram; | ||
| import org.junit.Assert; | ||
| import org.junit.Test; | ||
|
|
||
| public class SlidingWindowHistogramTest { | ||
| @Test | ||
| public void test() { | ||
| SlidingWindowHistogram slidingWindowHistogram = new SlidingWindowHistogram(2); | ||
|
|
||
| for (int i = 0; i < 100_000; i++) { | ||
| slidingWindowHistogram.recordValue(i); | ||
| } | ||
|
|
||
| slidingWindowHistogram.print(); | ||
| slidingWindowHistogram.rotateHistogram(); | ||
| Histogram histogram = | ||
| slidingWindowHistogram.aggregateHistogram(); | ||
|
|
||
| long totalCount = histogram.getTotalCount(); | ||
| Assert.assertTrue(totalCount == 100_000); | ||
|
|
||
| long p90 = histogram.getValueAtPercentile(90); | ||
| Assert.assertTrue(p90 < 100_000); | ||
|
|
||
| for (int i = 0; i < 100_000; i++) { | ||
| slidingWindowHistogram.recordValue(i * 10_000); | ||
| } | ||
|
|
||
| slidingWindowHistogram.print(); | ||
| slidingWindowHistogram.rotateHistogram(); | ||
|
|
||
| histogram = | ||
| slidingWindowHistogram.aggregateHistogram(); | ||
|
|
||
| p90 = histogram.getValueAtPercentile(90); | ||
| Assert.assertTrue(p90 >= 100_000); | ||
|
|
||
| for (int i = 0; i < 100_000; i++) { | ||
| slidingWindowHistogram.recordValue(i); | ||
| } | ||
|
|
||
| slidingWindowHistogram.print(); | ||
| slidingWindowHistogram.rotateHistogram(); | ||
|
|
||
| for (int i = 0; i < 100_000; i++) { | ||
| slidingWindowHistogram.recordValue(i); | ||
| } | ||
|
|
||
| slidingWindowHistogram.print(); | ||
|
|
||
| histogram = | ||
| slidingWindowHistogram.aggregateHistogram(); | ||
|
|
||
| totalCount = histogram.getTotalCount(); | ||
| Assert.assertTrue(totalCount == 200_000); | ||
|
|
||
| p90 = histogram.getValueAtPercentile(90); | ||
| Assert.assertTrue(p90 < 100_000); | ||
| } | ||
| } |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
slide is passed in as a closure, and that method has a 1 minute timeout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but do you intend to slide (and in the other class, today it rotates the histogram) on every
get?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes - it'll only rotate now when the the timeout expires
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, didn't see the timeout condition, thanks!