comment resolution

Author: Dogacel

core/src/main/java/com/linecorp/armeria/common/util/AndSampler.java

@@ -32,6 +32,6 @@ public boolean isSampled(T t) {
 
     @Override
     public String toString() {
-        return left.toString() + " and " + right.toString();
+        return left + " and " + right;
     }
 }

core/src/main/java/com/linecorp/armeria/common/util/OrSampler.java

@@ -32,6 +32,6 @@ public boolean isSampled(T t) {
 
     @Override
     public String toString() {
-        return left.toString() + " or " + right.toString();
+        return left + " or " + right;
     }
 }

core/src/main/java/com/linecorp/armeria/common/util/Sampler.java

@@ -29,6 +29,8 @@
  */
 package com.linecorp.armeria.common.util;
 
+import static java.util.Objects.requireNonNull;
+
 /**
  * Sampler is responsible for deciding if a particular trace should be "sampled", i.e. whether the
  * overhead of tracing will occur and/or if a trace will be reported to the collection tier.
@@ -47,48 +49,60 @@ public interface Sampler<T> {
      * Returns a sampler that applies logical or operator to both samplers decisions.
      */
     default Sampler<T> or(Sampler<T> other) {
-        return new OrSampler<>(this, other);
+        return new OrSampler<>(this, requireNonNull(other, "other"));
     }
 
     /**
      * Returns a sampler that applies logical and operator to both samplers decisions.
      */
     default Sampler<T> and(Sampler<T> other) {
-        return new AndSampler<>(this, other);
+        return new AndSampler<>(this, requireNonNull(other, "other"));
+    }
+
+    /**
+     * Returns a sampler that applies logical not operator to the sampler decision.
+     */
+    default Sampler<T> not() {
+        return object -> !isSampled(object);
     }
 
     /**
      * Returns a sampler that returns {@code true} if the value is greater than the given value.
      */
     static <T extends Comparable<T>> Sampler<T> greaterThan(T val) {
+        requireNonNull(val, "val");
         return object -> object.compareTo(val) > 0;
     }
 
     /**
      * Returns a sampler that returns {@code true} if the value is less than or equal to the given value.
      */
     static <T extends Comparable<T>> Sampler<T> greaterThanOrEqual(T val) {
+        requireNonNull(val, "val");
         return object -> object.compareTo(val) >= 0;
     }
 
     /**
      * Returns a sampler that returns {@code true} if the value is less than the given value.
      */
     static <T extends Comparable<T>> Sampler<T> lessThan(T val) {
+        requireNonNull(val, "val");
         return object -> object.compareTo(val) < 0;
     }
 
     /**
      * Returns a sampler that returns {@code true} if the value is less than or equal to the given value.
      */
     static <T extends Comparable<T>> Sampler<T> lessThanOrEqual(T val) {
+        requireNonNull(val, "val");
         return object -> object.compareTo(val) <= 0;
     }
 
     /**
      * Returns a sampler that returns {@code true} if the value is equal to the given value.
      */
     static <T extends Comparable<T>> Sampler<T> equal(T val) {
+        requireNonNull(val, "val");
         return object -> object.compareTo(val) == 0;
     }
 

core/src/main/java/com/linecorp/armeria/common/util/TimeWindowPercentileSampler.java

@@ -10,6 +10,9 @@
 package com.linecorp.armeria.common.util;
 
 import java.time.Duration;
+import java.util.concurrent.atomic.AtomicReference;
+
+import com.google.common.annotations.VisibleForTesting;
 
 import io.micrometer.core.instrument.Clock;
 import io.micrometer.core.instrument.distribution.DistributionStatisticConfig;
@@ -23,13 +26,19 @@ final class TimeWindowPercentileSampler implements Sampler<Long> {
 
     private final float percentile;
     private final long windowLengthMillis;
-
     private final TimeWindowPercentileHistogram histogram;
     private static final long SNAPSHOT_UPDATE_MILLIS = 1000L;
-    private long lastSnapshotMillis = 0L;
+    private long lastSnapshotMillis;
     private HistogramSnapshot histogramSnapshot;
 
+    private final AtomicReference<Boolean> isTakingSnapshot = new AtomicReference<>(false);
+
     TimeWindowPercentileSampler(float percentile, long windowLengthMillis) {
+        this(percentile, windowLengthMillis, Clock.SYSTEM);
+    }
+
+    @VisibleForTesting
+    TimeWindowPercentileSampler(float percentile, long windowLengthMillis, Clock clock) {
         this.percentile = percentile;
         this.windowLengthMillis = windowLengthMillis;
 
@@ -42,7 +51,7 @@ final class TimeWindowPercentileSampler implements Sampler<Long> {
                                            .expiry(Duration.ofMillis(windowLengthMillis))
                                            .bufferLength(3)
                                            .build();
-        this.histogram = new TimeWindowPercentileHistogram(Clock.SYSTEM, distributionStatisticConfig, true);
+        this.histogram = new TimeWindowPercentileHistogram(clock, distributionStatisticConfig, true);
         this.histogramSnapshot = histogram.takeSnapshot(0, 0, 0);
         this.lastSnapshotMillis = System.currentTimeMillis();
     }
@@ -56,17 +65,20 @@ public boolean isSampled(Long t) {
         histogram.recordLong(t);
 
         if (lastSnapshotMillis + SNAPSHOT_UPDATE_MILLIS < System.currentTimeMillis()) {
-            histogramSnapshot = histogram.takeSnapshot(0, 0, 0);
-            lastSnapshotMillis = System.currentTimeMillis();
+            if (isTakingSnapshot.compareAndSet(false, true)) {
+                histogramSnapshot = histogram.takeSnapshot(0, 0, 0);
+                lastSnapshotMillis = System.currentTimeMillis();
+                isTakingSnapshot.set(false);
+            }
         }
 
         final Double percentileValue = histogramSnapshot.percentileValues()[0].value();
-        return percentileValue.longValue() <= t;
+        return t >= percentileValue.longValue();
     }
 
     @Override
     public String toString() {
-        return "SlidingWindowPercentileSampler with " + windowLengthMillis + " ms window and " + percentile +
+        return "TimeWindowPercentileSampler with " + windowLengthMillis + " ms window and " + percentile +
                " percentile";
     }
 }

core/src/test/java/com/linecorp/armeria/common/util/SamplerTest.java

@@ -111,25 +111,15 @@ void andOr() {
             assertThat(neverAndAlwaysSampler.isSampled(i)).isFalse();
         }
 
-        final Sampler<Object> halfAndHalfSampler = Sampler.random(0.5f).and(Sampler.random(0.5f));
-        int halfAndHalfSamplerCount = 0;
-        for (int i = 0; i < 10000; i++) {
-            if (halfAndHalfSampler.isSampled(i)) {
-                halfAndHalfSamplerCount += 1;
-            }
+        final Sampler<Object> notNeverSampler = Sampler.never().not();
+        for (int i = 0; i < 10; i++) {
+            assertThat(notNeverSampler.isSampled(i)).isTrue();
         }
-        // 0.5*0.5 = 0.25
-        assertThat(halfAndHalfSamplerCount).isBetween(2000, 3000); // Should be roughly 2500 //
-
-        final Sampler<Object> halfOrHalfSampler = Sampler.random(0.5f).or(Sampler.random(0.5f));
-        int halfOrHalfSamplerCount = 0;
-        for (int i = 0; i < 10000; i++) {
-            if (halfOrHalfSampler.isSampled(i)) {
-                halfOrHalfSamplerCount += 1;
-            }
+
+        final Sampler<Object> notAlwaysSampler = Sampler.always().not();
+        for (int i = 0; i < 10; i++) {
+            assertThat(notAlwaysSampler.isSampled(i)).isFalse();
         }
-        // 1 - (0.5*0.5) = 0.75
-        assertThat(halfOrHalfSamplerCount).isBetween(7000, 8000); // Should be roughly 7500
     }
 
     private static class SampleOnce implements Sampler<Object> {