diff --git a/micrometer-core/src/main/java/io/micrometer/core/instrument/internal/DefaultLongTaskTimer.java b/micrometer-core/src/main/java/io/micrometer/core/instrument/internal/DefaultLongTaskTimer.java index 7f56455158..d598ef6429 100644 --- a/micrometer-core/src/main/java/io/micrometer/core/instrument/internal/DefaultLongTaskTimer.java +++ b/micrometer-core/src/main/java/io/micrometer/core/instrument/internal/DefaultLongTaskTimer.java @@ -27,8 +27,7 @@ import java.util.*; import java.util.concurrent.ArrayBlockingQueue; -import java.util.concurrent.ConcurrentLinkedDeque; -import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.ConcurrentSkipListSet; import java.util.concurrent.TimeUnit; import java.util.function.Consumer; import java.util.stream.Collectors; @@ -37,18 +36,23 @@ public class DefaultLongTaskTimer extends AbstractMeter implements LongTaskTimer { /** - * Preferring {@link ConcurrentLinkedDeque} over {@link CopyOnWriteArrayList} here + * Preferring {@link ConcurrentSkipListSet} over other concurrent collections * because... *

* Retrieval of percentile values will be O(N) but starting/stopping tasks will be - * O(1). Starting/stopping tasks happen in the same thread as the main application - * code, where publishing generally happens in a separate thread. Also, shipping - * client-side percentiles should be relatively uncommon. + * O(log(N)). Starting/stopping tasks happen in the same thread as the main + * application code, where publishing generally happens in a separate thread. Also, + * shipping client-side percentiles should be relatively uncommon. + *

+ * Tasks are naturally ordered at time of insertion, but task completion/removal can + * happen out-of-order, that causes removal of tasks from the middle of the list. A + * queue would provide O(1) insertion, but O(N) removal in average. A skip-list + * provides O(log(N)) average across all tasks. *

* Histogram creation is O(N) for both the queue and list options, because we have to * consider which bucket each active task belongs. */ - private final Deque activeTasks = new ConcurrentLinkedDeque<>(); + private final NavigableSet activeTasks = new ConcurrentSkipListSet<>(); private final Clock clock; @@ -92,7 +96,9 @@ public DefaultLongTaskTimer(Id id, Clock clock, TimeUnit baseTimeUnit, @Override public Sample start() { SampleImpl sample = new SampleImpl(); - activeTasks.add(sample); + if (!activeTasks.add(sample)) { + throw new IllegalStateException(); + } return sample; } @@ -108,8 +114,13 @@ public double duration(TimeUnit unit) { @Override public double max(TimeUnit unit) { - Sample oldest = activeTasks.peek(); - return oldest == null ? 0.0 : oldest.duration(unit); + try { + Sample oldest = activeTasks.first(); + return oldest.duration(unit); + } + catch (NoSuchElementException e) { + return 0.0; + } } @Override @@ -215,7 +226,7 @@ public HistogramSnapshot takeSnapshot() { (ps, scaling) -> ps.print("Summary output for LongTaskTimer histograms is not supported.")); } - class SampleImpl extends Sample { + class SampleImpl extends Sample implements Comparable { private final long startTime; @@ -249,6 +260,15 @@ public String toString() { + ", " + "duration(nanos)=" + durationInNanoseconds + ", " + "startTimeNanos=" + startTime + '}'; } + @Override + public int compareTo(DefaultLongTaskTimer.SampleImpl o) { + int startCompare = Long.compare(startTime, o.startTime); + if (startCompare == 0) { + return Long.compare(hashCode(), o.hashCode()); + } + return startCompare; + } + } }