Add DDSketch as an option for Metric Aggregation. #2063
Conversation
|
|
||
| /** Must be called under lock. */ | ||
| private void drain() { | ||
| assert Thread.holdsLock(lock); |
There was a problem hiding this comment.
We don't generally use assert in this codebase. Can you use the @GuardedBy("lock") annotation on the method instead?
| import org.openjdk.jmh.annotations.Warmup; | ||
|
|
||
| @State(Scope.Benchmark) | ||
| public class DDSketchAggregatorBenchmark { |
There was a problem hiding this comment.
Can you publish some benchmarks? What kind of throughput can this implementation handle, with the lock on every recording?
| sum, | ||
| Arrays.asList( | ||
| ValueAtPercentile.create(0.0, current.getMinValue()), | ||
| ValueAtPercentile.create(100.0, current.getMaxValue()))); |
There was a problem hiding this comment.
I would have thought we would get more percentiles out of the sketch. Is that for a later PR?
|
I'm pretty worried about requiring a lock on every metric recording that goes through this aggregator. Could we drop recordings if we can't keep up, rather than adding another lock in that path? |
|
|
||
| // Since DDSketch is not thread safe, this queue is used to buffer calls to record, reducing | ||
| // the need to take a lock for every call. (With the downside of having to box the double.) | ||
| private final ArrayBlockingQueue<Double> pendingValues = new ArrayBlockingQueue<>(64); |
There was a problem hiding this comment.
Doesn't a blocking queue also imply a taking a lock on every call? (it's a blocking queue :) ).
Is the idea that it would be less time under lock than if always calling current.accept?
|
Had a discussion in the metrics SIG today, and everyone pretty much agrees we should revert to just doing a simple synchronized lock around the recordings and ditch the queue in front of it. We can benchmark that and see how it performs before we do something fancier. |
|
@tylerbenson I'm going to close this for now, unless you're going to have time to update it for the latest changes to the metrics SDK. Please re-open if appropriate. |
|
Good call. Sorry for leaving this open for so long. |
Will require #2037 (or something like it) in order to actually be usable.
Current iteration also doesn't map to any protobuf format.