Generalise exponential bucket histogram, add percentile calculation #127597
Conversation
Closes: ES-10531
…ool_queue_latency_metric
…ool_queue_latency_metric
…ool_queue_latency_metric
…ool_queue_latency_metric
…ialBucketHistogram.java Co-authored-by: Yang Wang <[email protected]>
…ool_queue_latency_metric
…ialBucketHistogram.java Co-authored-by: Dianna Hohensee <[email protected]>
nicktindall
added
:Distributed Coordination/Network
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
elasticsearchmachine
added
Team:Distributed Coordination
nicktindall
changed the title
| * @return An array of frequencies of handling times in buckets with upper bounds as returned by {@link #getBucketUpperBounds()}, plus | ||
| * an extra bucket for handling times longer than the longest upper bound. | ||
| */ | ||
| public long[] getSnapshot() { |
There was a problem hiding this comment.
I changed this from getHistogram to getSnapshot because histogram.getHistogram() would look weird.
server/src/main/java/org/elasticsearch/common/metrics/ExponentialBucketHistogram.java
Outdated
Show resolved
Hide resolved
| if (percentile < 0 || percentile > 1) { | ||
| throw new IllegalArgumentException("Requested percentile must be in [0, 1.0], percentile=" + percentile); | ||
| } |
There was a problem hiding this comment.
Might be good to add a few quick expectThrows() tests for these?
There was a problem hiding this comment.
Addressed in 221e079
| public void clear() { | ||
| for (int i = 0; i < BUCKET_COUNT; i++) { | ||
| buckets[i].reset(); | ||
| } | ||
| } |
There was a problem hiding this comment.
And a test for clear()?
There was a problem hiding this comment.
Addressed in 221e079
| if (percentile < 0 || percentile > 1) { | ||
| throw new IllegalArgumentException("Requested percentile must be in [0, 1.0], percentile=" + percentile); | ||
| } |
| public void clear() { | ||
| for (int i = 0; i < BUCKET_COUNT; i++) { | ||
| buckets[i].reset(); | ||
| } | ||
| } |
|
|
||
| /** | ||
| * Tracks how long message handling takes on a transport thread as a histogram with fixed buckets. | ||
| */ | ||
| public class HandlingTimeTracker { | ||
| public class HandlingTimeTracker extends ExponentialBucketHistogram { |
There was a problem hiding this comment.
I decided to keep the HandlingTimeTracker because the decision to have 17 + 1 buckets is contextual and should be fixed because it's used in serialization. It also reduces the size of the change.
HandlingTimeTracker is now just an ExponentialBucketHistogram of a specific size.
| public ExponentialBucketHistogram(int bucketCount) { | ||
| if (bucketCount < 2 || bucketCount > Integer.SIZE) { | ||
| throw new IllegalArgumentException("Bucket count must be in [2, " + Integer.SIZE + "], got " + bucketCount); | ||
| } |
There was a problem hiding this comment.
The observations are long but the bucket bounds are integers, I suppose because the scale of the numbers we've been putting in here are easily contained in the integer range. Perhaps we could revisit this if we need histograms to hold larger values, but that's not the case for the queue latency metric.
I want to re-use the exponential-bucket histogram implemented as
HandlingTimeTrackerfor our queue latency metric.This PR renames it, and references to it, to make it more suitable for re-use and adds
getPercentilewhich can be used to calculate a percentile value from its contents.See #120488
Relates ES-10531
Note there's a massive amount of commits because I just took the other PR and removed the metrics bits. It should all get squashed away anyway.