diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/experiment/Experiment.java b/service/src/main/java/org/whispersystems/textsecuregcm/experiment/Experiment.java index 163729671..57ea3c701 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/experiment/Experiment.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/experiment/Experiment.java @@ -15,35 +15,49 @@ import java.util.function.Supplier; */ public class Experiment { - private final Timer matchTimer; - private final Timer mismatchTimer; - private final Timer errorTimer; + private final Timer matchTimer; + private final Timer errorTimer; - static final String OUTCOME_TAG = "outcome"; - static final String MATCH_OUTCOME = "match"; - static final String MISMATCH_OUTCOME = "mismatch"; - static final String ERROR_OUTCOME = "error"; + private final Timer bothPresentMismatchTimer; + private final Timer controlNullMismatchTimer; + private final Timer experimentNullMismatchTimer; + + private static final String OUTCOME_TAG = "outcome"; + private static final String MATCH_OUTCOME = "match"; + private static final String MISMATCH_OUTCOME = "mismatch"; + private static final String ERROR_OUTCOME = "error"; + + private static final String MISMATCH_TYPE_TAG = "mismatchType"; + private static final String BOTH_PRESENT_MISMATCH = "bothPresent"; + private static final String CONTROL_NULL_MISMATCH = "controlResultNull"; + private static final String EXPERIMENT_NULL_MISMATCH = "experimentResultNull"; public Experiment(final String... names) { - this(buildTimer(MATCH_OUTCOME, names), buildTimer(MISMATCH_OUTCOME, names), buildTimer(ERROR_OUTCOME, names)); + this(buildTimer(MATCH_OUTCOME, names).register(Metrics.globalRegistry), + buildTimer(ERROR_OUTCOME, names).register(Metrics.globalRegistry), + buildTimer(MISMATCH_OUTCOME, names).tag(MISMATCH_TYPE_TAG, BOTH_PRESENT_MISMATCH).register(Metrics.globalRegistry), + buildTimer(MISMATCH_OUTCOME, names).tag(MISMATCH_TYPE_TAG, CONTROL_NULL_MISMATCH).register(Metrics.globalRegistry), + buildTimer(MISMATCH_OUTCOME, names).tag(MISMATCH_TYPE_TAG, EXPERIMENT_NULL_MISMATCH).register(Metrics.globalRegistry)); } - private static Timer buildTimer(final String outcome, final String... names) { + private static Timer.Builder buildTimer(final String outcome, final String... names) { if (names == null || names.length == 0) { throw new IllegalArgumentException("Experiments must have a name"); } return Timer.builder(MetricRegistry.name(Experiment.class, names)) .tag(OUTCOME_TAG, outcome) - .publishPercentileHistogram() - .register(Metrics.globalRegistry); + .publishPercentileHistogram(); } @VisibleForTesting - Experiment(final Timer matchTimer, final Timer mismatchTimer, final Timer errorTimer) { + Experiment(final Timer matchTimer, final Timer errorTimer, final Timer bothPresentMismatchTimer, final Timer controlNullMismatchTimer, final Timer experimentNullMismatchTimer) { this.matchTimer = matchTimer; - this.mismatchTimer = mismatchTimer; this.errorTimer = errorTimer; + + this.bothPresentMismatchTimer = bothPresentMismatchTimer; + this.controlNullMismatchTimer = controlNullMismatchTimer; + this.experimentNullMismatchTimer = experimentNullMismatchTimer; } public void compareFutureResult(final T expected, final CompletionStage experimentStage) { @@ -78,11 +92,18 @@ public class Experiment { } private void recordResult(final long durationNanos, final T expected, final T actual) { - final boolean shouldIgnore = actual == null && expected != null; + final Timer timer; - if (!shouldIgnore) { - final Timer timer = Objects.equals(expected, actual) ? matchTimer : mismatchTimer; - timer.record(durationNanos, TimeUnit.NANOSECONDS); + if (Objects.equals(expected, actual)) { + timer = matchTimer; + } else if (expected == null) { + timer = controlNullMismatchTimer; + } else if (actual == null) { + timer = experimentNullMismatchTimer; + } else { + timer = bothPresentMismatchTimer; } + + timer.record(durationNanos, TimeUnit.NANOSECONDS); } } diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/experiment/ExperimentTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/experiment/ExperimentTest.java index c8db7e890..424339d91 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/experiment/ExperimentTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/experiment/ExperimentTest.java @@ -1,47 +1,36 @@ package org.whispersystems.textsecuregcm.experiment; -import io.micrometer.core.instrument.Meter; -import io.micrometer.core.instrument.MeterRegistry; -import io.micrometer.core.instrument.Tag; import io.micrometer.core.instrument.Timer; -import io.micrometer.core.instrument.distribution.DistributionStatisticConfig; -import org.LatencyUtils.PauseDetector; import org.junit.Before; import org.junit.Test; -import org.mockito.ArgumentCaptor; -import org.mockito.ArgumentMatchers; -import java.util.HashSet; -import java.util.Set; import java.util.concurrent.CompletableFuture; import java.util.concurrent.TimeUnit; -import static org.junit.Assert.*; -import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyLong; -import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.verifyNoMoreInteractions; -import static org.mockito.Mockito.when; public class ExperimentTest { private Timer matchTimer; - private Timer mismatchTimer; private Timer errorTimer; + private Timer bothPresentMismatchTimer; + private Timer controlNullMismatchTimer; + private Timer experimentNullMismatchTimer; private Experiment experiment; @Before public void setUp() { matchTimer = mock(Timer.class); - mismatchTimer = mock(Timer.class); errorTimer = mock(Timer.class); + bothPresentMismatchTimer = mock(Timer.class); + controlNullMismatchTimer = mock(Timer.class); + experimentNullMismatchTimer = mock(Timer.class); - experiment = new Experiment(matchTimer, mismatchTimer, errorTimer); + experiment = new Experiment(matchTimer, errorTimer, bothPresentMismatchTimer, controlNullMismatchTimer, experimentNullMismatchTimer); } @Test @@ -51,9 +40,21 @@ public class ExperimentTest { } @Test - public void compareFutureResultMismatch() { + public void compareFutureResultMismatchBothPresent() { experiment.compareFutureResult(12, CompletableFuture.completedFuture(77)); - verify(mismatchTimer).record(anyLong(), eq(TimeUnit.NANOSECONDS)); + verify(bothPresentMismatchTimer).record(anyLong(), eq(TimeUnit.NANOSECONDS)); + } + + @Test + public void compareFutureResultMismatchControlNull() { + experiment.compareFutureResult(null, CompletableFuture.completedFuture(77)); + verify(controlNullMismatchTimer).record(anyLong(), eq(TimeUnit.NANOSECONDS)); + } + + @Test + public void compareFutureResultMismatchExperimentNull() { + experiment.compareFutureResult(12, CompletableFuture.completedFuture(null)); + verify(experimentNullMismatchTimer).record(anyLong(), eq(TimeUnit.NANOSECONDS)); } @Test @@ -62,12 +63,6 @@ public class ExperimentTest { verify(errorTimer).record(anyLong(), eq(TimeUnit.NANOSECONDS)); } - @Test - public void compareFutureResultNoExperimentData() { - experiment.compareFutureResult(12, CompletableFuture.completedFuture(null)); - verifyNoMoreInteractions(matchTimer, mismatchTimer, errorTimer); - } - @Test public void compareSupplierResultMatch() { experiment.compareSupplierResult(12, () -> 12); @@ -75,9 +70,21 @@ public class ExperimentTest { } @Test - public void compareSupplierResultMismatch() { + public void compareSupplierResultMismatchBothPresent() { experiment.compareSupplierResult(12, () -> 77); - verify(mismatchTimer).record(anyLong(), eq(TimeUnit.NANOSECONDS)); + verify(bothPresentMismatchTimer).record(anyLong(), eq(TimeUnit.NANOSECONDS)); + } + + @Test + public void compareSupplierResultMismatchControlNull() { + experiment.compareSupplierResult(null, () -> 77); + verify(controlNullMismatchTimer).record(anyLong(), eq(TimeUnit.NANOSECONDS)); + } + + @Test + public void compareSupplierResultMismatchExperimentNull() { + experiment.compareSupplierResult(12, () -> null); + verify(experimentNullMismatchTimer).record(anyLong(), eq(TimeUnit.NANOSECONDS)); } @Test @@ -85,10 +92,4 @@ public class ExperimentTest { experiment.compareSupplierResult(12, () -> { throw new RuntimeException("OH NO"); }); verify(errorTimer).record(anyLong(), eq(TimeUnit.NANOSECONDS)); } - - @Test - public void compareSupplierResultNoExperimentData() { - experiment.compareSupplierResult(12, () -> null); - verifyNoMoreInteractions(mismatchTimer, mismatchTimer, errorTimer); - } }