Skip to content
This repository has been archived by the owner on Jul 10, 2024. It is now read-only.

Commit

Permalink
Merge pull request #173 from indeedeng/PROC-1525
Browse files Browse the repository at this point in the history 
PROC-1525: Add logic for filtering out losing payload experiments
  • Loading branch information
@zacharygoodwin
zacharygoodwin authored Jun 28, 2024
2 parents 9d50f35 + d7ae0b6 commit c35c9e0
Show file tree
Hide file tree
Showing 5 changed files with 184 additions and 14 deletions.
20 changes: 20 additions & 0 deletions proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -490,6 +490,10 @@ protected final List<String> getLoggingTestNames() {
proctorResult.getTestDefinitions();
// following lines should preserve the order in the map to ensure logging values are stable
final Map<String, TestBucket> buckets = proctorResult.getBuckets();
final Set<String> winningPayloadExperiments =
proctorResult.getProperties().values().stream()
.map(PayloadProperty::getTestName)
.collect(Collectors.toSet());
return buckets.keySet().stream()
.filter(
testName -> {
Expand All @@ -499,6 +503,7 @@ protected final List<String> getLoggingTestNames() {
return (consumableTestDefinition == null)
|| !consumableTestDefinition.getSilent();
})
.filter(testName -> loggablePayloadExperiment(testName, winningPayloadExperiments))
// Suppress 100% allocation logging
.filter(this::loggableAllocation)
// call to getValueWithouMarkingUsage() to allow overrides of getActiveBucket, but
Expand Down Expand Up @@ -568,6 +573,21 @@ private boolean loggableAllocation(final String testName) {
return loggableAllocation(testName, td, proctorResult);
}

private boolean loggablePayloadExperiment(
final String testName, final Set<String> validPayloadExperiments) {
final ConsumableTestDefinition td = proctorResult.getTestDefinitions().get(testName);
return loggablePayloadExperiment(testName, td, validPayloadExperiments);
}

public static boolean loggablePayloadExperiment(
final String testName,
@Nullable final ConsumableTestDefinition td,
final Set<String> validPayloadExperiments) {
return td == null
|| td.getPayloadExperimentConfig() == null
|| validPayloadExperiments.contains(testName);
}

public static boolean loggableAllocation(
final String testName,
final ConsumableTestDefinition td,
Expand Down
16 changes: 14 additions & 2 deletions proctor-consumer/src/main/java/com/indeed/proctor/consumer/ProctorGroupsWriter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.function.BiPredicate;

import static com.indeed.proctor.consumer.AbstractGroups.loggableAllocation;
Expand All @@ -30,7 +31,7 @@ public class ProctorGroupsWriter {
// for historic reasons, allow more than one formatter
private final TestGroupFormatter[] formatters;
private final BiPredicate<String, ProctorResult> testFilter;

private Set<String> winningPayloadExperiment;
/**
* @param groupsSeparator how elements in logged string are separated
* @param formatters ideally only one, all groups will be logged for all formatters
Expand Down Expand Up @@ -71,7 +72,7 @@ public final String writeGroupsAsString(
initialCapacity += testName.length() + 10;
}
initialCapacity *= formatters.length;
for (String c : classifiers) {
for (final String c : classifiers) {
initialCapacity += c.length() + 1;
}

Expand Down Expand Up @@ -205,6 +206,17 @@ public ProctorGroupsWriter build() {
if (additionalFilter != null) {
return additionalFilter.test(testName, proctorResult);
}

// Do not log payload experiments which were overwritten
if (consumableTestDefinition != null
&& consumableTestDefinition.getPayloadExperimentConfig() != null
&& proctorResult.getProperties().values().stream()
.noneMatch(
property ->
property.getTestName().equals(testName))) {
return false;
}

// Suppress 100% allocation logging
return loggableAllocation(
testName, consumableTestDefinition, proctorResult);
Expand Down
52 changes: 45 additions & 7 deletions proctor-consumer/src/test/java/com/indeed/proctor/consumer/ProctorGroupStubber.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,22 @@ public class ProctorGroupStubber {
new TestBucket("control", 0, "control", new Payload("controlPayload"));
public static final TestBucket GROUP_1_BUCKET_WITH_PAYLOAD =
new TestBucket("group1", 1, "group1", new Payload("activePayload"));

public static final TestBucket JSON_BUCKET;

static {
try {
JSON_BUCKET =
new TestBucket(
"group1",
1,
"group1",
new Payload(new ObjectMapper().readTree("{\"foo\": 1, \"bar\": 2}")));
} catch (final JsonProcessingException e) {
throw new RuntimeException(e);
}
}

public static final TestBucket GROUP_1_BUCKET = new TestBucket("group1", 2, "group1");

public static final TestBucket GROUP_1_BUCKET_PROPERTY_PAYLOAD;
Expand Down Expand Up @@ -64,7 +80,7 @@ public class ProctorGroupStubber {
public static class ProctorResultStubBuilder {

private final Map<String, ConsumableTestDefinition> definitions = new TreeMap<>();
private final Map<StubTest, TestBucket> resolvedBuckets = new TreeMap<>();
private final Map<String, TestBucket> resolvedBuckets = new TreeMap<>();
private final Map<String, PayloadProperty> properties = new TreeMap<>();

public ProctorResultStubBuilder withStubTest(
Expand Down Expand Up @@ -92,7 +108,7 @@ public ProctorResultStubBuilder withStubTest(
@Nullable final TestBucket resolved,
final ConsumableTestDefinition definition) {
if (resolved != null) {
resolvedBuckets.put(stubTest, resolved);
resolvedBuckets.put(stubTest.getName(), resolved);
}
if (definition != null) {
definitions.put(stubTest.getName(), definition);
Expand All @@ -103,7 +119,7 @@ public ProctorResultStubBuilder withStubTest(
public ProctorResultStubBuilder withStubProperty(
final StubTest stubTest, @Nullable final TestBucket resolved) {
if (resolved != null) {
resolvedBuckets.put(stubTest, resolved);
resolvedBuckets.put(stubTest.getName(), resolved);
assert resolved.getPayload() != null;
assert resolved.getPayload().getJson() != null;
resolved.getPayload()
Expand All @@ -123,17 +139,39 @@ public ProctorResultStubBuilder withStubProperty(
return this;
}

public ProctorResultStubBuilder withStubProperty(
final String testName,
final ConsumableTestDefinition td,
@Nullable final TestBucket resolved) {
if (resolved != null) {
definitions.put(testName, td);
resolvedBuckets.put(testName, resolved);
assert resolved.getPayload() != null;
assert resolved.getPayload().getJson() != null;
resolved.getPayload()
.getJson()
.fields()
.forEachRemaining(
field ->
properties.put(
field.getKey(),
PayloadProperty.builder()
.testName(testName)
.value(field.getValue())
.build()));
}
return this;
}

public ProctorResult build() {
return new ProctorResult(
"0",
resolvedBuckets.entrySet().stream()
.collect(
Collectors.toMap(
e -> e.getKey().getName(), Map.Entry::getValue)),
.collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)),
resolvedBuckets.entrySet().stream()
.collect(
Collectors.toMap(
e -> e.getKey().getName(),
Map.Entry::getKey,
e ->
new Allocation(
null,
Expand Down
71 changes: 66 additions & 5 deletions proctor-consumer/src/test/java/com/indeed/proctor/consumer/ProctorGroupsWriterTest.java
View file
Original file line number Diff line number Diff line change
@@ -1,12 +1,19 @@
package com.indeed.proctor.consumer;

import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSortedMap;
import com.google.common.collect.Ordering;
import com.indeed.proctor.common.PayloadProperty;
import com.indeed.proctor.common.ProctorResult;
import com.indeed.proctor.common.model.Allocation;
import com.indeed.proctor.common.model.ConsumableTestDefinition;
import com.indeed.proctor.common.model.PayloadExperimentConfig;
import com.indeed.proctor.common.model.TestBucket;
import com.indeed.proctor.common.model.TestDefinition;
import com.indeed.proctor.common.model.TestType;
import com.indeed.proctor.consumer.logging.TestGroupFormatter;
import org.assertj.core.util.Strings;
import org.junit.Test;
Expand All @@ -25,6 +32,8 @@ public class ProctorGroupsWriterTest {
private static final String GROUP1_TEST_NAME = "d_foo_tst";
private static final String SILENT_TEST_NAME = "e_silent_tst";
private static final String FORCED_TEST_NAME = "f_forced_tst";
private static final String PAYLOAD_PROPERTY_TEST_NAME = "g_payload_tst";
private static final String WINNING_PAYLOAD_PROPERTY_TEST_NAME = "h_winning_payload_tst";

// for this test, buckets can be reused between test definitions, sorting by testname
public static final TestBucket INACTIVE_BUCKET = new TestBucket("fooname", -1, "foodesc");
Expand All @@ -46,6 +55,8 @@ public class ProctorGroupsWriterTest {
.put(GROUP1_TEST_NAME, GROUP1_BUCKET)
.put(SILENT_TEST_NAME, GROUP1_BUCKET)
.put(FORCED_TEST_NAME, GROUP1_BUCKET)
.put(WINNING_PAYLOAD_PROPERTY_TEST_NAME, GROUP1_BUCKET)
.put(PAYLOAD_PROPERTY_TEST_NAME, GROUP1_BUCKET)
.build();
private static final Map<String, Allocation> ALLOCATIONS =
new ImmutableSortedMap.Builder<String, Allocation>(Ordering.natural())
Expand All @@ -54,19 +65,65 @@ public class ProctorGroupsWriterTest {
.put(EMPTY_ALLOCID_DEFINITION_TEST_NAME, ALLOCATION_EMPTY_ID)
.put(GROUP1_TEST_NAME, ALLOCATION_A)
.put(SILENT_TEST_NAME, ALLOCATION_A)
.put(WINNING_PAYLOAD_PROPERTY_TEST_NAME, ALLOCATION_A)
.put(PAYLOAD_PROPERTY_TEST_NAME, ALLOCATION_A)
.build();

static final ConsumableTestDefinition PROPERTY_TD =
ConsumableTestDefinition.fromTestDefinition(
TestDefinition.builder()
.setTestType(TestType.RANDOM)
.setSalt("foo")
.setForceLogging(true)
.setPayloadExperimentConfig(
PayloadExperimentConfig.builder()
.priority("123")
.namespaces(ImmutableList.of("test"))
.build())
.build());
static final ConsumableTestDefinition WINNING_PROPERTY_TD =
ConsumableTestDefinition.fromTestDefinition(
TestDefinition.builder()
.setTestType(TestType.RANDOM)
.setSalt("foo")
.setForceLogging(true)
.setPayloadExperimentConfig(
PayloadExperimentConfig.builder()
.priority("20000")
.namespaces(ImmutableList.of("test"))
.build())
.build());

private static final Map<String, ConsumableTestDefinition> DEFINITIONS =
ImmutableMap.<String, ConsumableTestDefinition>builder()
.put(INACTIVE_TEST_NAME, stubDefinition(INACTIVE_BUCKET))
.put(EMPTY_ALLOCID_DEFINITION_TEST_NAME, stubDefinition(GROUP1_BUCKET))
.put(GROUP1_TEST_NAME, stubDefinition(GROUP1_BUCKET))
.put(SILENT_TEST_NAME, stubDefinition(GROUP1_BUCKET, d -> d.setSilent(true)))
.put(FORCED_TEST_NAME, stubDefinition(GROUP1_BUCKET))
.put(WINNING_PAYLOAD_PROPERTY_TEST_NAME, WINNING_PROPERTY_TD)
.put(PAYLOAD_PROPERTY_TEST_NAME, PROPERTY_TD)
.build();
private static final Map<String, PayloadProperty> PROPERTIES;

static {
try {
PROPERTIES =
ImmutableMap.<String, PayloadProperty>builder()
.put(
"foo",
PayloadProperty.builder()
.value(new ObjectMapper().readTree("1"))
.testName(WINNING_PAYLOAD_PROPERTY_TEST_NAME)
.build())
.build();
} catch (final JsonProcessingException e) {
throw new RuntimeException(e);
}
}

private static final ProctorResult PROCTOR_RESULT =
new ProctorResult(null, BUCKETS, ALLOCATIONS, DEFINITIONS);
new ProctorResult(null, BUCKETS, ALLOCATIONS, DEFINITIONS, PROPERTIES);

@Test
public void testWithEmptyResult() {
Expand All @@ -86,7 +143,7 @@ public void testWithEmptyResult() {
public void testDoubleFormattingWriter() {
// legacy Indeed behavior
final String expected =
"b_missing_definition0,c_empty_alloc_id0,d_foo_tst1,f_forced_tst1,#A:b_missing_definition0,#A:d_foo_tst1,f_forced_tst1";
"b_missing_definition0,c_empty_alloc_id0,d_foo_tst1,f_forced_tst1,h_winning_payload_tst1,#A:b_missing_definition0,#A:d_foo_tst1,f_forced_tst1,#A:h_winning_payload_tst1";

final ProctorGroupsWriter defaultWriter =
new ProctorGroupsWriter.Builder(
Expand All @@ -101,9 +158,11 @@ public void testDoubleFormattingWriter() {
EMPTY_ALLOCID_DEFINITION_TEST_NAME + 0,
GROUP1_TEST_NAME + 1,
FORCED_TEST_NAME + 1,
WINNING_PAYLOAD_PROPERTY_TEST_NAME + 1,
"#A:" + MISSING_DEFINITION_TEST_NAME + 0,
"#A:" + GROUP1_TEST_NAME + 1,
FORCED_TEST_NAME + 1)
FORCED_TEST_NAME + 1,
"#A:" + WINNING_PAYLOAD_PROPERTY_TEST_NAME + 1)
.with(","));
}

Expand All @@ -116,15 +175,17 @@ public void testCustomWriter() {
.setIncludeInactiveGroups(true)
.build();
assertThat(writerWithAllocIds.writeGroupsAsString(PROCTOR_RESULT))
.isEqualTo("#A:a_inactive_tst-1,#A:d_foo_tst1,#A:e_silent_tst1,f_forced_tst1");
.isEqualTo(
"#A:a_inactive_tst-1,#A:d_foo_tst1,#A:e_silent_tst1,f_forced_tst1,#A:h_winning_payload_tst1");

final ProctorGroupsWriter writerWithoutAllocIds =
new ProctorGroupsWriter.Builder(TestGroupFormatter.WITHOUT_ALLOC_ID)
.setIncludeSilentTests(true)
.setIncludeTestWithoutDefinition(false)
.build();
assertThat(writerWithoutAllocIds.writeGroupsAsString(PROCTOR_RESULT))
.isEqualTo("c_empty_alloc_id0,d_foo_tst1,e_silent_tst1,f_forced_tst1");
.isEqualTo(
"c_empty_alloc_id0,d_foo_tst1,e_silent_tst1,f_forced_tst1,h_winning_payload_tst1");
}

@Test
Expand Down
39 changes: 39 additions & 0 deletions proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Lists;
import com.google.common.collect.Sets;
Expand All @@ -26,6 +27,7 @@
import static com.indeed.proctor.consumer.ProctorGroupStubber.GROUP_1_BUCKET_PROPERTY_PAYLOAD;
import static com.indeed.proctor.consumer.ProctorGroupStubber.GROUP_1_BUCKET_WITH_PAYLOAD;
import static com.indeed.proctor.consumer.ProctorGroupStubber.INACTIVE_BUCKET;
import static com.indeed.proctor.consumer.ProctorGroupStubber.JSON_BUCKET;
import static com.indeed.proctor.consumer.ProctorGroupStubber.StubTest.CONTROL_SELECTED_TEST;
import static com.indeed.proctor.consumer.ProctorGroupStubber.StubTest.GROUP1_SELECTED_TEST;
import static com.indeed.proctor.consumer.ProctorGroupStubber.StubTest.GROUP_WITH_FALLBACK_TEST;
Expand Down Expand Up @@ -235,6 +237,43 @@ public void testToVerifyGroupsString() {
.isEqualTo("#A1:suppress_logging_example_tst0");
}

@Test
public void testToLoggingWithProperties() {
final ConsumableTestDefinition td =
ConsumableTestDefinition.fromTestDefinition(
TestDefinition.builder()
.setTestType(TestType.RANDOM)
.setSalt("foo")
.setForceLogging(true)
.setPayloadExperimentConfig(
PayloadExperimentConfig.builder()
.priority("123")
.namespaces(ImmutableList.of("test"))
.build())
.build());
final ConsumableTestDefinition tdWithHigherPriority =
ConsumableTestDefinition.fromTestDefinition(
TestDefinition.builder()
.setTestType(TestType.RANDOM)
.setSalt("foo")
.setForceLogging(true)
.setPayloadExperimentConfig(
PayloadExperimentConfig.builder()
.priority("20000")
.namespaces(ImmutableList.of("test"))
.build())
.build());
final ProctorResult result =
new ProctorGroupStubber.ProctorResultStubBuilder()
.withStubProperty(CONTROL_SELECTED_TEST.getName(), td, JSON_BUCKET)
.withStubProperty(
PROPERTY_TEST.getName(), tdWithHigherPriority, JSON_BUCKET)
.build();

assertThat((new AbstractGroups(result) {}).toLoggingString())
.isEqualTo("#A1:propertytest1");
}

@Test
public void testCheckRolledOutAllocation() {
final ConsumableTestDefinition td =
Expand Down

0 comments on commit c35c9e0

Please sign in to comment.