From b85f3c5d19e756a2e3a857a3a433ad01909dfe82 Mon Sep 17 00:00:00 2001 From: Zack Goodwin Date: Wed, 24 Apr 2024 12:54:46 -0400 Subject: [PATCH 01/11] PROC-1475: Suppress 100% allocations --- .../proctor/consumer/AbstractGroups.java | 16 ++++++++++++++++ .../proctor/consumer/ProctorGroupsWriter.java | 11 +++++++++++ .../proctor/consumer/ProctorGroupStubber.java | 18 ++++++++++++++++-- .../proctor/consumer/TestAbstractGroups.java | 15 ++++++++++++--- 4 files changed, 55 insertions(+), 5 deletions(-) diff --git a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java index 68f3be266..fed9cb77c 100644 --- a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java +++ b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java @@ -417,6 +417,8 @@ protected final List getLoggingTestNames() { return (consumableTestDefinition == null) || !consumableTestDefinition.getSilent(); }) + // Suppress 100% allocation logging + .filter(this::filterRolledOutAllocations) // call to getValueWithouMarkingUsage() to allow overrides of getActiveBucket, but // avoid marking .filter(testName -> getValueWithoutMarkingUsage(testName, -1) >= 0) @@ -454,6 +456,20 @@ protected final void appendTestGroupsWithAllocations( } } + private boolean filterRolledOutAllocations(final String testName) { + final ConsumableTestDefinition td = proctorResult.getTestDefinitions().get(testName); + if (td == null) { + return true; + } + final boolean forceLogging = td.getForceLogging(); + final Allocation allocation = proctorResult.getAllocations().get(testName); + if (allocation != null + && allocation.getRanges().stream().anyMatch(range -> range.getLength() == 1)) { + return forceLogging; + } + return true; + } + /** * Generates a Map[testname, bucketValue] for bucketValues >= 0. * diff --git a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/ProctorGroupsWriter.java b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/ProctorGroupsWriter.java index af97f5011..8cd329d90 100644 --- a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/ProctorGroupsWriter.java +++ b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/ProctorGroupsWriter.java @@ -203,6 +203,17 @@ public ProctorGroupsWriter build() { if (additionalFilter != null) { return additionalFilter.test(testName, proctorResult); } + // Suppress 100% allocation logging + if (consumableTestDefinition != null) { + final Allocation allocation = + proctorResult.getAllocations().get(testName); + if (allocation != null + && allocation.getRanges().stream() + .anyMatch(range -> range.getLength() == 1)) { + return consumableTestDefinition.getForceLogging(); + } + } + return true; }); } diff --git a/proctor-consumer/src/test/java/com/indeed/proctor/consumer/ProctorGroupStubber.java b/proctor-consumer/src/test/java/com/indeed/proctor/consumer/ProctorGroupStubber.java index 419fcbb16..94e0fdc2b 100644 --- a/proctor-consumer/src/test/java/com/indeed/proctor/consumer/ProctorGroupStubber.java +++ b/proctor-consumer/src/test/java/com/indeed/proctor/consumer/ProctorGroupStubber.java @@ -48,7 +48,19 @@ public ProctorResultStubBuilder withStubTest( final StubTest stubTest, @Nullable final TestBucket resolved, final TestBucket... definedBuckets) { - withStubTest(stubTest, resolved, stubDefinitionWithVersion("v1", definedBuckets)); + withStubTest(stubTest, resolved, stubDefinitionWithVersion(true, "v1", definedBuckets)); + return this; + } + + public ProctorResultStubBuilder withStubTest( + final boolean forceLogging, + final StubTest stubTest, + @Nullable final TestBucket resolved, + final TestBucket... definedBuckets) { + withStubTest( + stubTest, + resolved, + stubDefinitionWithVersion(forceLogging, "v1", definedBuckets)); return this; } @@ -89,10 +101,11 @@ public ProctorResult build() { } public static ConsumableTestDefinition stubDefinitionWithVersion( - final String version, final TestBucket... buckets) { + final boolean forceLogging, final String version, final TestBucket... buckets) { final ConsumableTestDefinition testDefinition = new ConsumableTestDefinition(); testDefinition.setVersion(version); testDefinition.setBuckets(Arrays.asList(buckets)); + testDefinition.setForceLogging(forceLogging); return testDefinition; } @@ -126,6 +139,7 @@ public enum StubTest implements com.indeed.proctor.consumer.Test { HOLDOUT_MASTER_TEST("holdout_tst", -1), CONTROL_SELECTED_TEST("bgtst", -1), + SUPPRESS_LOGGING_TST("suppress_logging_example_tst", -1), GROUP1_SELECTED_TEST("abtst", -1), GROUP_WITH_FALLBACK_TEST("groupwithfallbacktst", -1), INACTIVE_SELECTED_TEST("btntst", -1), diff --git a/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java b/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java index cf1f56735..dcdfc0f11 100644 --- a/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java +++ b/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java @@ -27,6 +27,7 @@ import static com.indeed.proctor.consumer.ProctorGroupStubber.StubTest.INACTIVE_SELECTED_TEST; import static com.indeed.proctor.consumer.ProctorGroupStubber.StubTest.MISSING_DEFINITION_TEST; import static com.indeed.proctor.consumer.ProctorGroupStubber.StubTest.NO_BUCKETS_WITH_FALLBACK_TEST; +import static com.indeed.proctor.consumer.ProctorGroupStubber.StubTest.SUPPRESS_LOGGING_TST; import static java.util.Collections.emptyMap; import static java.util.Collections.singleton; import static org.assertj.core.api.Assertions.assertThat; @@ -53,6 +54,13 @@ public void setUp() { INACTIVE_BUCKET, CONTROL_BUCKET_WITH_PAYLOAD, GROUP_1_BUCKET_WITH_PAYLOAD) + .withStubTest( + false, + ProctorGroupStubber.StubTest.SUPPRESS_LOGGING_TST, + CONTROL_BUCKET_WITH_PAYLOAD, + INACTIVE_BUCKET, + CONTROL_BUCKET_WITH_PAYLOAD, + GROUP_1_BUCKET_WITH_PAYLOAD) .withStubTest( ProctorGroupStubber.StubTest.GROUP1_SELECTED_TEST, GROUP_1_BUCKET_WITH_PAYLOAD, @@ -164,7 +172,7 @@ public void testToLongString() { assertThat(emptyGroup.toLongString()).isEmpty(); assertThat(sampleGroups.toLongString()) .isEqualTo( - "abtst-group1,bgtst-control,btntst-inactive,groupwithfallbacktst-group1,no_definition_tst-group1"); + "abtst-group1,bgtst-control,btntst-inactive,groupwithfallbacktst-group1,no_definition_tst-group1,suppress_logging_example_tst-control"); } @Test @@ -268,11 +276,12 @@ public void testGetJavaScriptConfig() { assertThat(emptyGroup.getJavaScriptConfig()).hasSize(0); assertThat(sampleGroups.getJavaScriptConfig()) - .hasSize(4) + .hasSize(5) .containsEntry(GROUP1_SELECTED_TEST.getName(), 1) .containsEntry(CONTROL_SELECTED_TEST.getName(), 0) .containsEntry(GROUP_WITH_FALLBACK_TEST.getName(), 2) - .containsEntry(MISSING_DEFINITION_TEST.getName(), 2); + .containsEntry(MISSING_DEFINITION_TEST.getName(), 2) + .containsEntry(SUPPRESS_LOGGING_TST.getName(), 0); } @Test From 091a14469fca811e0857495c1e91a117d24ba6bf Mon Sep 17 00:00:00 2001 From: Zack Goodwin Date: Wed, 24 Apr 2024 13:02:40 -0400 Subject: [PATCH 02/11] PROC-1475: Refactor duplicated logic into single method --- .../proctor/consumer/AbstractGroups.java | 21 ++++++++++++------- .../proctor/consumer/ProctorGroupsWriter.java | 15 ++++--------- 2 files changed, 17 insertions(+), 19 deletions(-) diff --git a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java index fed9cb77c..6ecc5f73a 100644 --- a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java +++ b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java @@ -458,14 +458,19 @@ protected final void appendTestGroupsWithAllocations( private boolean filterRolledOutAllocations(final String testName) { final ConsumableTestDefinition td = proctorResult.getTestDefinitions().get(testName); - if (td == null) { - return true; - } - final boolean forceLogging = td.getForceLogging(); - final Allocation allocation = proctorResult.getAllocations().get(testName); - if (allocation != null - && allocation.getRanges().stream().anyMatch(range -> range.getLength() == 1)) { - return forceLogging; + return checkRolledOutAllocation(testName, td, proctorResult); + } + + public static boolean checkRolledOutAllocation( + final String testName, + final ConsumableTestDefinition td, + final ProctorResult proctorResult) { + if (td != null) { + final Allocation allocation = proctorResult.getAllocations().get(testName); + if (allocation != null + && allocation.getRanges().stream().anyMatch(range -> range.getLength() == 1)) { + return td.getForceLogging(); + } } return true; } diff --git a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/ProctorGroupsWriter.java b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/ProctorGroupsWriter.java index 8cd329d90..016ad1eb4 100644 --- a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/ProctorGroupsWriter.java +++ b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/ProctorGroupsWriter.java @@ -14,6 +14,8 @@ import java.util.Optional; import java.util.function.BiPredicate; +import static com.indeed.proctor.consumer.AbstractGroups.checkRolledOutAllocation; + /** * Helper class to build a Strings to log, helping with analysis of experiments. * @@ -204,17 +206,8 @@ public ProctorGroupsWriter build() { return additionalFilter.test(testName, proctorResult); } // Suppress 100% allocation logging - if (consumableTestDefinition != null) { - final Allocation allocation = - proctorResult.getAllocations().get(testName); - if (allocation != null - && allocation.getRanges().stream() - .anyMatch(range -> range.getLength() == 1)) { - return consumableTestDefinition.getForceLogging(); - } - } - - return true; + return checkRolledOutAllocation( + testName, consumableTestDefinition, proctorResult); }); } } From baa6a44ef0d49b4f206f239853e44f6246cee863 Mon Sep 17 00:00:00 2001 From: Zack Goodwin Date: Wed, 24 Apr 2024 13:54:29 -0400 Subject: [PATCH 03/11] PROC-1475: Rename and add unit test --- .../proctor/consumer/AbstractGroups.java | 4 +- .../proctor/consumer/ProctorGroupsWriter.java | 4 +- .../proctor/consumer/TestAbstractGroups.java | 44 +++++++++++++++++++ 3 files changed, 48 insertions(+), 4 deletions(-) diff --git a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java index 6ecc5f73a..f15aa2f65 100644 --- a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java +++ b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java @@ -458,10 +458,10 @@ protected final void appendTestGroupsWithAllocations( private boolean filterRolledOutAllocations(final String testName) { final ConsumableTestDefinition td = proctorResult.getTestDefinitions().get(testName); - return checkRolledOutAllocation(testName, td, proctorResult); + return shouldLogRolledOutAllocation(testName, td, proctorResult); } - public static boolean checkRolledOutAllocation( + public static boolean shouldLogRolledOutAllocation( final String testName, final ConsumableTestDefinition td, final ProctorResult proctorResult) { diff --git a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/ProctorGroupsWriter.java b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/ProctorGroupsWriter.java index 016ad1eb4..8ed924150 100644 --- a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/ProctorGroupsWriter.java +++ b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/ProctorGroupsWriter.java @@ -14,7 +14,7 @@ import java.util.Optional; import java.util.function.BiPredicate; -import static com.indeed.proctor.consumer.AbstractGroups.checkRolledOutAllocation; +import static com.indeed.proctor.consumer.AbstractGroups.shouldLogRolledOutAllocation; /** * Helper class to build a Strings to log, helping with analysis of experiments. @@ -206,7 +206,7 @@ public ProctorGroupsWriter build() { return additionalFilter.test(testName, proctorResult); } // Suppress 100% allocation logging - return checkRolledOutAllocation( + return shouldLogRolledOutAllocation( testName, consumableTestDefinition, proctorResult); }); } diff --git a/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java b/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java index dcdfc0f11..ecc7f6b70 100644 --- a/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java +++ b/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java @@ -6,6 +6,8 @@ import com.indeed.proctor.common.ProctorResult; import com.indeed.proctor.common.model.ConsumableTestDefinition; import com.indeed.proctor.common.model.Payload; +import com.indeed.proctor.common.model.TestDefinition; +import com.indeed.proctor.common.model.TestType; import com.indeed.proctor.consumer.ProctorGroupStubber.FakeTest; import com.indeed.proctor.consumer.logging.TestGroupFormatter; import com.indeed.proctor.consumer.logging.TestMarkingObserver; @@ -14,6 +16,7 @@ import java.util.Arrays; +import static com.indeed.proctor.consumer.AbstractGroups.shouldLogRolledOutAllocation; import static com.indeed.proctor.consumer.ProctorGroupStubber.CONTROL_BUCKET_WITH_PAYLOAD; import static com.indeed.proctor.consumer.ProctorGroupStubber.FALLBACK_BUCKET; import static com.indeed.proctor.consumer.ProctorGroupStubber.FALLBACK_NOPAYLOAD_BUCKET; @@ -188,6 +191,47 @@ public void testToLoggingString() { "#A1:abtst1,#A1:bgtst0,#A1:groupwithfallbacktst2,#A1:no_definition_tst2"); } + @Test + public void testCheckRolledOutAllocation() { + final ConsumableTestDefinition td = + ConsumableTestDefinition.fromTestDefinition( + TestDefinition.builder() + .setTestType(TestType.RANDOM) + .setSalt("foo") + .build()); + final ConsumableTestDefinition tdWithForceLogging = + ConsumableTestDefinition.fromTestDefinition( + TestDefinition.builder() + .setTestType(TestType.RANDOM) + .setSalt("foo") + .setForceLogging(true) + .build()); + final ProctorResult result = + new ProctorGroupStubber.ProctorResultStubBuilder() + .withStubTest( + false, + ProctorGroupStubber.StubTest.SUPPRESS_LOGGING_TST, + CONTROL_BUCKET_WITH_PAYLOAD, + INACTIVE_BUCKET, + CONTROL_BUCKET_WITH_PAYLOAD, + GROUP_1_BUCKET_WITH_PAYLOAD) + .build(); + + assertThat( + shouldLogRolledOutAllocation( + "suppress_logging_example_tst", + tdWithForceLogging, + result) + ).isTrue(); + + assertThat( + shouldLogRolledOutAllocation( + "suppress_logging_example_tst", + td, + result) + ).isFalse(); + } + @Test public void testToLoggingStringWithExposureAndObserver() { From df1556ae0ca1bbd71013b1e444c2e811c70c4b7d Mon Sep 17 00:00:00 2001 From: Zack Goodwin Date: Wed, 24 Apr 2024 15:22:14 -0400 Subject: [PATCH 04/11] PROC-1475: Rename method for clarity --- .../proctor/consumer/AbstractGroups.java | 8 ++++---- .../proctor/consumer/ProctorGroupsWriter.java | 5 ++--- .../proctor/consumer/TestAbstractGroups.java | 19 +++++-------------- 3 files changed, 11 insertions(+), 21 deletions(-) diff --git a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java index f15aa2f65..55dfd5607 100644 --- a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java +++ b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java @@ -418,7 +418,7 @@ protected final List getLoggingTestNames() { || !consumableTestDefinition.getSilent(); }) // Suppress 100% allocation logging - .filter(this::filterRolledOutAllocations) + .filter(this::isTestRolledOut) // call to getValueWithouMarkingUsage() to allow overrides of getActiveBucket, but // avoid marking .filter(testName -> getValueWithoutMarkingUsage(testName, -1) >= 0) @@ -456,12 +456,12 @@ protected final void appendTestGroupsWithAllocations( } } - private boolean filterRolledOutAllocations(final String testName) { + private boolean isTestRolledOut(final String testName) { final ConsumableTestDefinition td = proctorResult.getTestDefinitions().get(testName); - return shouldLogRolledOutAllocation(testName, td, proctorResult); + return isTestRolledOut(testName, td, proctorResult); } - public static boolean shouldLogRolledOutAllocation( + public static boolean isTestRolledOut( final String testName, final ConsumableTestDefinition td, final ProctorResult proctorResult) { diff --git a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/ProctorGroupsWriter.java b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/ProctorGroupsWriter.java index 8ed924150..caf3e1d84 100644 --- a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/ProctorGroupsWriter.java +++ b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/ProctorGroupsWriter.java @@ -14,7 +14,7 @@ import java.util.Optional; import java.util.function.BiPredicate; -import static com.indeed.proctor.consumer.AbstractGroups.shouldLogRolledOutAllocation; +import static com.indeed.proctor.consumer.AbstractGroups.isTestRolledOut; /** * Helper class to build a Strings to log, helping with analysis of experiments. @@ -206,8 +206,7 @@ public ProctorGroupsWriter build() { return additionalFilter.test(testName, proctorResult); } // Suppress 100% allocation logging - return shouldLogRolledOutAllocation( - testName, consumableTestDefinition, proctorResult); + return isTestRolledOut(testName, consumableTestDefinition, proctorResult); }); } } diff --git a/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java b/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java index ecc7f6b70..6da474964 100644 --- a/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java +++ b/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java @@ -16,7 +16,7 @@ import java.util.Arrays; -import static com.indeed.proctor.consumer.AbstractGroups.shouldLogRolledOutAllocation; +import static com.indeed.proctor.consumer.AbstractGroups.isTestRolledOut; import static com.indeed.proctor.consumer.ProctorGroupStubber.CONTROL_BUCKET_WITH_PAYLOAD; import static com.indeed.proctor.consumer.ProctorGroupStubber.FALLBACK_BUCKET; import static com.indeed.proctor.consumer.ProctorGroupStubber.FALLBACK_NOPAYLOAD_BUCKET; @@ -217,19 +217,10 @@ public void testCheckRolledOutAllocation() { GROUP_1_BUCKET_WITH_PAYLOAD) .build(); - assertThat( - shouldLogRolledOutAllocation( - "suppress_logging_example_tst", - tdWithForceLogging, - result) - ).isTrue(); - - assertThat( - shouldLogRolledOutAllocation( - "suppress_logging_example_tst", - td, - result) - ).isFalse(); + assertThat(isTestRolledOut("suppress_logging_example_tst", tdWithForceLogging, result)) + .isTrue(); + + assertThat(isTestRolledOut("suppress_logging_example_tst", td, result)).isFalse(); } @Test From 64f7404e3583e96521b000ff5015640b84d75019 Mon Sep 17 00:00:00 2001 From: Zack Goodwin Date: Thu, 25 Apr 2024 12:52:51 -0400 Subject: [PATCH 05/11] PROC-1475: Rename method for clarity --- .../java/com/indeed/proctor/consumer/AbstractGroups.java | 8 ++++---- .../com/indeed/proctor/consumer/ProctorGroupsWriter.java | 5 +++-- .../com/indeed/proctor/consumer/TestAbstractGroups.java | 8 +++++--- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java index 55dfd5607..fb94816f8 100644 --- a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java +++ b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java @@ -418,7 +418,7 @@ protected final List getLoggingTestNames() { || !consumableTestDefinition.getSilent(); }) // Suppress 100% allocation logging - .filter(this::isTestRolledOut) + .filter(this::loggableRolledOutAllocation) // call to getValueWithouMarkingUsage() to allow overrides of getActiveBucket, but // avoid marking .filter(testName -> getValueWithoutMarkingUsage(testName, -1) >= 0) @@ -456,12 +456,12 @@ protected final void appendTestGroupsWithAllocations( } } - private boolean isTestRolledOut(final String testName) { + private boolean loggableRolledOutAllocation(final String testName) { final ConsumableTestDefinition td = proctorResult.getTestDefinitions().get(testName); - return isTestRolledOut(testName, td, proctorResult); + return loggableRolledOutAllocation(testName, td, proctorResult); } - public static boolean isTestRolledOut( + public static boolean loggableRolledOutAllocation( final String testName, final ConsumableTestDefinition td, final ProctorResult proctorResult) { diff --git a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/ProctorGroupsWriter.java b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/ProctorGroupsWriter.java index caf3e1d84..27cfc983f 100644 --- a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/ProctorGroupsWriter.java +++ b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/ProctorGroupsWriter.java @@ -14,7 +14,7 @@ import java.util.Optional; import java.util.function.BiPredicate; -import static com.indeed.proctor.consumer.AbstractGroups.isTestRolledOut; +import static com.indeed.proctor.consumer.AbstractGroups.loggableRolledOutAllocation; /** * Helper class to build a Strings to log, helping with analysis of experiments. @@ -206,7 +206,8 @@ public ProctorGroupsWriter build() { return additionalFilter.test(testName, proctorResult); } // Suppress 100% allocation logging - return isTestRolledOut(testName, consumableTestDefinition, proctorResult); + return loggableRolledOutAllocation( + testName, consumableTestDefinition, proctorResult); }); } } diff --git a/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java b/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java index 6da474964..a8194822b 100644 --- a/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java +++ b/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java @@ -16,7 +16,7 @@ import java.util.Arrays; -import static com.indeed.proctor.consumer.AbstractGroups.isTestRolledOut; +import static com.indeed.proctor.consumer.AbstractGroups.loggableRolledOutAllocation; import static com.indeed.proctor.consumer.ProctorGroupStubber.CONTROL_BUCKET_WITH_PAYLOAD; import static com.indeed.proctor.consumer.ProctorGroupStubber.FALLBACK_BUCKET; import static com.indeed.proctor.consumer.ProctorGroupStubber.FALLBACK_NOPAYLOAD_BUCKET; @@ -217,10 +217,12 @@ public void testCheckRolledOutAllocation() { GROUP_1_BUCKET_WITH_PAYLOAD) .build(); - assertThat(isTestRolledOut("suppress_logging_example_tst", tdWithForceLogging, result)) + assertThat( + loggableRolledOutAllocation( + "suppress_logging_example_tst", tdWithForceLogging, result)) .isTrue(); - assertThat(isTestRolledOut("suppress_logging_example_tst", td, result)).isFalse(); + assertThat(loggableRolledOutAllocation("suppress_logging_example_tst", td, result)).isFalse(); } @Test From bbe788ce1a5d96e18d5178146c64ac8767f9f356 Mon Sep 17 00:00:00 2001 From: Zack Goodwin Date: Wed, 8 May 2024 09:43:01 -0400 Subject: [PATCH 06/11] PROC-1475: Do not filter out 100% allocations for showGroups endpoint --- .../proctor/consumer/AbstractGroups.java | 53 +++++++++++++++++-- .../consumer/spring/ShowGroupsHandler.java | 2 +- .../proctor/consumer/TestAbstractGroups.java | 3 +- 3 files changed, 53 insertions(+), 5 deletions(-) diff --git a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java index fb94816f8..d1754e463 100644 --- a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java +++ b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java @@ -399,9 +399,31 @@ public void appendTestGroups(final StringBuilder sb, final char separator) { } /** - * Return test names for tests that are non-silent or doesn't have available definition, and - * have a non-negative active bucket, in a stable sort. Stable sort is beneficial for log string - * compression, for debugging, and may help in cases of size-limited output. + * For each testname returned by getLoggingTestNames(), appends each test group in the form of + * [allocation id + ":" + test name + bucket value]. For example, it appends "#A1:bgcolortst1" + * if test name is bgcolortst and allocation id is #A1 and separator is ",". + * + *

the separator should be appended after each test group added to the string builder {@link + * #toString()} {@link #buildTestGroupString()} or {@link #appendTestGroups(StringBuilder)} + * + * @param sb a string builder + * @param separator a char used as separator x appends an empty string or a x-separated, + * x-finalized list of groups + */ + public void appendTestGroups( + final StringBuilder sb, + final char separator, + final boolean filterRolledOutAllocations) { + final List testNames = + filterRolledOutAllocations ? getLoggingTestNames() : getLoggingTestNamesFullList(); + appendTestGroupsWithAllocations(sb, separator, testNames); + } + + /** + * Return test names for tests that are non-silent, doesn't have available definition, is not + * 100% rolled out, and have a non-negative active bucket, in a stable sort. Stable sort is + * beneficial for log string compression, for debugging, and may help in cases of size-limited + * output. */ protected final List getLoggingTestNames() { final Map testDefinitions = @@ -425,6 +447,31 @@ protected final List getLoggingTestNames() { .collect(Collectors.toList()); } + /** + * Return test names for tests that are non-silent or doesn't have available definition, and + * have a non-negative active bucket, in a stable sort. Stable sort is beneficial for log string + * compression, for debugging, and may help in cases of size-limited output. + */ + protected final List getLoggingTestNamesFullList() { + final Map testDefinitions = + proctorResult.getTestDefinitions(); + // following lines should preserve the order in the map to ensure logging values are stable + final Map buckets = proctorResult.getBuckets(); + return buckets.keySet().stream() + .filter( + testName -> { + final ConsumableTestDefinition consumableTestDefinition = + testDefinitions.get(testName); + // fallback to non-silent when test definition is not available + return (consumableTestDefinition == null) + || !consumableTestDefinition.getSilent(); + }) + // call to getValueWithouMarkingUsage() to allow overrides of getActiveBucket, but + // avoid marking + .filter(testName -> getValueWithoutMarkingUsage(testName, -1) >= 0) + .collect(Collectors.toList()); + } + /** * Appends test groups in the form with allocation ids as [allocation id + ":" + test name + * bucket value] for given test names. diff --git a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/spring/ShowGroupsHandler.java b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/spring/ShowGroupsHandler.java index 684db45e0..0525eae1f 100644 --- a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/spring/ShowGroupsHandler.java +++ b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/spring/ShowGroupsHandler.java @@ -29,7 +29,7 @@ public void handleRequest(HttpServletRequest request, HttpServletResponse respon writer.println("Did not determine any groups"); } else { final StringBuilder sb = new StringBuilder(); - grps.appendTestGroups(sb, '\n'); + grps.appendTestGroups(sb, '\n', false); writer.print(sb.toString()); } } diff --git a/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java b/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java index a8194822b..b3bab599c 100644 --- a/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java +++ b/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java @@ -222,7 +222,8 @@ public void testCheckRolledOutAllocation() { "suppress_logging_example_tst", tdWithForceLogging, result)) .isTrue(); - assertThat(loggableRolledOutAllocation("suppress_logging_example_tst", td, result)).isFalse(); + assertThat(loggableRolledOutAllocation("suppress_logging_example_tst", td, result)) + .isFalse(); } @Test From cf0dafa7a293e2f722aeea265f6f653718956bd4 Mon Sep 17 00:00:00 2001 From: Zack Goodwin Date: Thu, 16 May 2024 13:38:44 -0400 Subject: [PATCH 07/11] PROC-1475: Rename to loggableAllocation --- .../java/com/indeed/proctor/consumer/AbstractGroups.java | 8 ++++---- .../com/indeed/proctor/consumer/ProctorGroupsWriter.java | 4 ++-- .../com/indeed/proctor/consumer/TestAbstractGroups.java | 9 +++------ 3 files changed, 9 insertions(+), 12 deletions(-) diff --git a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java index d1754e463..478ff6e87 100644 --- a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java +++ b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java @@ -440,7 +440,7 @@ protected final List getLoggingTestNames() { || !consumableTestDefinition.getSilent(); }) // Suppress 100% allocation logging - .filter(this::loggableRolledOutAllocation) + .filter(this::loggableAllocation) // call to getValueWithouMarkingUsage() to allow overrides of getActiveBucket, but // avoid marking .filter(testName -> getValueWithoutMarkingUsage(testName, -1) >= 0) @@ -503,12 +503,12 @@ protected final void appendTestGroupsWithAllocations( } } - private boolean loggableRolledOutAllocation(final String testName) { + private boolean loggableAllocation(final String testName) { final ConsumableTestDefinition td = proctorResult.getTestDefinitions().get(testName); - return loggableRolledOutAllocation(testName, td, proctorResult); + return loggableAllocation(testName, td, proctorResult); } - public static boolean loggableRolledOutAllocation( + public static boolean loggableAllocation( final String testName, final ConsumableTestDefinition td, final ProctorResult proctorResult) { diff --git a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/ProctorGroupsWriter.java b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/ProctorGroupsWriter.java index 27cfc983f..6380f74ba 100644 --- a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/ProctorGroupsWriter.java +++ b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/ProctorGroupsWriter.java @@ -14,7 +14,7 @@ import java.util.Optional; import java.util.function.BiPredicate; -import static com.indeed.proctor.consumer.AbstractGroups.loggableRolledOutAllocation; +import static com.indeed.proctor.consumer.AbstractGroups.loggableAllocation; /** * Helper class to build a Strings to log, helping with analysis of experiments. @@ -206,7 +206,7 @@ public ProctorGroupsWriter build() { return additionalFilter.test(testName, proctorResult); } // Suppress 100% allocation logging - return loggableRolledOutAllocation( + return loggableAllocation( testName, consumableTestDefinition, proctorResult); }); } diff --git a/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java b/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java index b3bab599c..e306aba11 100644 --- a/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java +++ b/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java @@ -16,7 +16,7 @@ import java.util.Arrays; -import static com.indeed.proctor.consumer.AbstractGroups.loggableRolledOutAllocation; +import static com.indeed.proctor.consumer.AbstractGroups.loggableAllocation; import static com.indeed.proctor.consumer.ProctorGroupStubber.CONTROL_BUCKET_WITH_PAYLOAD; import static com.indeed.proctor.consumer.ProctorGroupStubber.FALLBACK_BUCKET; import static com.indeed.proctor.consumer.ProctorGroupStubber.FALLBACK_NOPAYLOAD_BUCKET; @@ -217,13 +217,10 @@ public void testCheckRolledOutAllocation() { GROUP_1_BUCKET_WITH_PAYLOAD) .build(); - assertThat( - loggableRolledOutAllocation( - "suppress_logging_example_tst", tdWithForceLogging, result)) + assertThat(loggableAllocation("suppress_logging_example_tst", tdWithForceLogging, result)) .isTrue(); - assertThat(loggableRolledOutAllocation("suppress_logging_example_tst", td, result)) - .isFalse(); + assertThat(loggableAllocation("suppress_logging_example_tst", td, result)).isFalse(); } @Test From ba17e42ff4b4c7aa1d7d7adadd49bd24d8d14d63 Mon Sep 17 00:00:00 2001 From: Zack Goodwin Date: Mon, 20 May 2024 09:34:19 -0400 Subject: [PATCH 08/11] PROC-1475: Add public method for rolled out allocations logging string --- .../proctor/consumer/AbstractGroups.java | 22 ++++++++++++++ .../proctor/consumer/TestAbstractGroups.java | 30 +++++++++++++++++++ 2 files changed, 52 insertions(+) diff --git a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java index 478ff6e87..00352a76a 100644 --- a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java +++ b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java @@ -355,6 +355,28 @@ public String toLoggingString() { return sb.toString(); } + /** + * String to be logged for debugging purposes. For historic reasons inside Indeed, contains two + * output formats per testname. This method does not filter out 100% allocations. + * + *

Additional custom groups can be added by overriding getCustomGroupsForLogging(). + * + * @return a comma-separated List of {testname}{active-bucket-VALUE} and + * {AllocationId}{testname}{active-bucket-VALUE} for all LIVE tests + */ + public String toLoggingStringNoAllocationFilter() { + if (isEmpty()) { + return ""; + } + final StringBuilder sb = new StringBuilder(proctorResult.getBuckets().size() * 10); + appendTestGroups(sb, GROUPS_SEPARATOR, false); + // remove trailing comma + if (sb.length() > 0) { + sb.deleteCharAt(sb.length() - 1); + } + return sb.toString(); + } + /** * @return an empty string or a comma-separated, comma-finalized list of groups * @deprecated use toLoggingString() diff --git a/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java b/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java index e306aba11..0099f36a2 100644 --- a/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java +++ b/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java @@ -191,6 +191,36 @@ public void testToLoggingString() { "#A1:abtst1,#A1:bgtst0,#A1:groupwithfallbacktst2,#A1:no_definition_tst2"); } + @Test + public void testToLoggingStringNoAllocationFilter() { + final ConsumableTestDefinition td = + ConsumableTestDefinition.fromTestDefinition( + TestDefinition.builder() + .setTestType(TestType.RANDOM) + .setSalt("foo") + .build()); + final ConsumableTestDefinition tdWithForceLogging = + ConsumableTestDefinition.fromTestDefinition( + TestDefinition.builder() + .setTestType(TestType.RANDOM) + .setSalt("foo") + .setForceLogging(true) + .build()); + final ProctorResult result = + new ProctorGroupStubber.ProctorResultStubBuilder() + .withStubTest( + false, + ProctorGroupStubber.StubTest.SUPPRESS_LOGGING_TST, + CONTROL_BUCKET_WITH_PAYLOAD, + INACTIVE_BUCKET, + CONTROL_BUCKET_WITH_PAYLOAD, + GROUP_1_BUCKET_WITH_PAYLOAD) + .build(); + + assertThat((new AbstractGroups(result) {}).toLoggingStringNoAllocationFilter()) + .isEqualTo("#A1:suppress_logging_example_tst0"); + } + @Test public void testCheckRolledOutAllocation() { final ConsumableTestDefinition td = From 4933db6250705b48feae067033bceb3dcbdfab02 Mon Sep 17 00:00:00 2001 From: Zack Goodwin Date: Mon, 20 May 2024 15:42:42 -0400 Subject: [PATCH 09/11] PROC-1475: Rename method --- .../main/java/com/indeed/proctor/consumer/AbstractGroups.java | 2 +- .../java/com/indeed/proctor/consumer/TestAbstractGroups.java | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java index 00352a76a..6458ea045 100644 --- a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java +++ b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java @@ -364,7 +364,7 @@ public String toLoggingString() { * @return a comma-separated List of {testname}{active-bucket-VALUE} and * {AllocationId}{testname}{active-bucket-VALUE} for all LIVE tests */ - public String toLoggingStringNoAllocationFilter() { + public String toVerifyGroupsString() { if (isEmpty()) { return ""; } diff --git a/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java b/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java index 0099f36a2..d735c0cdb 100644 --- a/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java +++ b/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java @@ -192,7 +192,7 @@ public void testToLoggingString() { } @Test - public void testToLoggingStringNoAllocationFilter() { + public void testToVerifyGroupsString() { final ConsumableTestDefinition td = ConsumableTestDefinition.fromTestDefinition( TestDefinition.builder() @@ -217,7 +217,7 @@ public void testToLoggingStringNoAllocationFilter() { GROUP_1_BUCKET_WITH_PAYLOAD) .build(); - assertThat((new AbstractGroups(result) {}).toLoggingStringNoAllocationFilter()) + assertThat((new AbstractGroups(result) {}).toVerifyGroupsString()) .isEqualTo("#A1:suppress_logging_example_tst0"); } From 547a9f91a905fd861d934769a86a442ac05599dc Mon Sep 17 00:00:00 2001 From: Zack Goodwin Date: Mon, 20 May 2024 15:44:00 -0400 Subject: [PATCH 10/11] PROC-1475: Update documentation of toVerifyGroupsString() --- .../main/java/com/indeed/proctor/consumer/AbstractGroups.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java index 6458ea045..8079ea2b2 100644 --- a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java +++ b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java @@ -356,7 +356,7 @@ public String toLoggingString() { } /** - * String to be logged for debugging purposes. For historic reasons inside Indeed, contains two + * String to be used for verification/debugging purposes. For historic reasons inside Indeed, contains two * output formats per testname. This method does not filter out 100% allocations. * *

Additional custom groups can be added by overriding getCustomGroupsForLogging(). From ca2fd53f87305ef410c8b22c079762f0ac8b02ed Mon Sep 17 00:00:00 2001 From: Zack Goodwin Date: Wed, 29 May 2024 15:26:21 -0400 Subject: [PATCH 11/11] PROC-1475: Add 50/50 test case --- .../proctor/consumer/AbstractGroups.java | 4 +- .../proctor/consumer/TestAbstractGroups.java | 45 +++++++++++++++++-- 2 files changed, 43 insertions(+), 6 deletions(-) diff --git a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java index 8079ea2b2..28b73b359 100644 --- a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java +++ b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java @@ -356,8 +356,8 @@ public String toLoggingString() { } /** - * String to be used for verification/debugging purposes. For historic reasons inside Indeed, contains two - * output formats per testname. This method does not filter out 100% allocations. + * String to be used for verification/debugging purposes. For historic reasons inside Indeed, + * contains two output formats per testname. This method does not filter out 100% allocations. * *

Additional custom groups can be added by overriding getCustomGroupsForLogging(). * diff --git a/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java b/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java index d735c0cdb..303b70e6f 100644 --- a/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java +++ b/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java @@ -4,17 +4,16 @@ import com.google.common.collect.Lists; import com.google.common.collect.Sets; import com.indeed.proctor.common.ProctorResult; -import com.indeed.proctor.common.model.ConsumableTestDefinition; -import com.indeed.proctor.common.model.Payload; -import com.indeed.proctor.common.model.TestDefinition; -import com.indeed.proctor.common.model.TestType; +import com.indeed.proctor.common.model.*; import com.indeed.proctor.consumer.ProctorGroupStubber.FakeTest; import com.indeed.proctor.consumer.logging.TestGroupFormatter; import com.indeed.proctor.consumer.logging.TestMarkingObserver; import org.junit.Before; import org.junit.Test; +import java.util.ArrayList; import java.util.Arrays; +import java.util.List; import static com.indeed.proctor.consumer.AbstractGroups.loggableAllocation; import static com.indeed.proctor.consumer.ProctorGroupStubber.CONTROL_BUCKET_WITH_PAYLOAD; @@ -253,6 +252,44 @@ public void testCheckRolledOutAllocation() { assertThat(loggableAllocation("suppress_logging_example_tst", td, result)).isFalse(); } + @Test + public void testCheckNotRolledOutAllocationAlwaysLogs() { + final ConsumableTestDefinition td = + ConsumableTestDefinition.fromTestDefinition( + TestDefinition.builder() + .setTestType(TestType.RANDOM) + .setSalt("foo") + .build()); + final ConsumableTestDefinition tdWithForceLogging = + ConsumableTestDefinition.fromTestDefinition( + TestDefinition.builder() + .setTestType(TestType.RANDOM) + .setSalt("foo") + .setForceLogging(true) + .build()); + final ProctorResult result = + new ProctorGroupStubber.ProctorResultStubBuilder() + .withStubTest( + false, + SUPPRESS_LOGGING_TST, + CONTROL_BUCKET_WITH_PAYLOAD, + INACTIVE_BUCKET, + CONTROL_BUCKET_WITH_PAYLOAD, + GROUP_1_BUCKET_WITH_PAYLOAD) + .build(); + + final List ranges = new ArrayList<>(); + ranges.add(new Range(0, 0.5)); + ranges.add(new Range(1, 0.5)); + + result.getAllocations().get("suppress_logging_example_tst").setRanges(ranges); + + assertThat(loggableAllocation("suppress_logging_example_tst", tdWithForceLogging, result)) + .isTrue(); + + assertThat(loggableAllocation("suppress_logging_example_tst", td, result)).isTrue(); + } + @Test public void testToLoggingStringWithExposureAndObserver() {