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

Commit c35c9e0

Browse files
zacharygoodwinzacharygoodwin
authored
Merge pull request #173 from indeedeng/PROC-1525
PROC-1525: Add logic for filtering out losing payload experiments
2 parents 9d50f35 + d7ae0b6 commit c35c9e0

File tree

5 files changed

+184
-14
lines changed

5 files changed

+184
-14
lines changed

‎proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java

+20
Original file line numberDiff line numberDiff line change
@@ -490,6 +490,10 @@ protected final List<String> getLoggingTestNames() {
490490
proctorResult.getTestDefinitions();
491491
// following lines should preserve the order in the map to ensure logging values are stable
492492
final Map<String, TestBucket> buckets = proctorResult.getBuckets();
493+
final Set<String> winningPayloadExperiments =
494+
proctorResult.getProperties().values().stream()
495+
.map(PayloadProperty::getTestName)
496+
.collect(Collectors.toSet());
493497
return buckets.keySet().stream()
494498
.filter(
495499
testName -> {
@@ -499,6 +503,7 @@ protected final List<String> getLoggingTestNames() {
499503
return (consumableTestDefinition == null)
500504
|| !consumableTestDefinition.getSilent();
501505
})
506+
.filter(testName -> loggablePayloadExperiment(testName, winningPayloadExperiments))
502507
// Suppress 100% allocation logging
503508
.filter(this::loggableAllocation)
504509
// call to getValueWithouMarkingUsage() to allow overrides of getActiveBucket, but
@@ -568,6 +573,21 @@ private boolean loggableAllocation(final String testName) {
568573
return loggableAllocation(testName, td, proctorResult);
569574
}
570575

576+
private boolean loggablePayloadExperiment(
577+
final String testName, final Set<String> validPayloadExperiments) {
578+
final ConsumableTestDefinition td = proctorResult.getTestDefinitions().get(testName);
579+
return loggablePayloadExperiment(testName, td, validPayloadExperiments);
580+
}
581+
582+
public static boolean loggablePayloadExperiment(
583+
final String testName,
584+
@Nullable final ConsumableTestDefinition td,
585+
final Set<String> validPayloadExperiments) {
586+
return td == null
587+
|| td.getPayloadExperimentConfig() == null
588+
|| validPayloadExperiments.contains(testName);
589+
}
590+
571591
public static boolean loggableAllocation(
572592
final String testName,
573593
final ConsumableTestDefinition td,

‎proctor-consumer/src/main/java/com/indeed/proctor/consumer/ProctorGroupsWriter.java

+14-2
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
import java.util.List;
1313
import java.util.Map;
1414
import java.util.Optional;
15+
import java.util.Set;
1516
import java.util.function.BiPredicate;
1617

1718
import static com.indeed.proctor.consumer.AbstractGroups.loggableAllocation;
@@ -30,7 +31,7 @@ public class ProctorGroupsWriter {
3031
// for historic reasons, allow more than one formatter
3132
private final TestGroupFormatter[] formatters;
3233
private final BiPredicate<String, ProctorResult> testFilter;
33-
34+
private Set<String> winningPayloadExperiment;
3435
/**
3536
* @param groupsSeparator how elements in logged string are separated
3637
* @param formatters ideally only one, all groups will be logged for all formatters
@@ -71,7 +72,7 @@ public final String writeGroupsAsString(
7172
initialCapacity += testName.length() + 10;
7273
}
7374
initialCapacity *= formatters.length;
74-
for (String c : classifiers) {
75+
for (final String c : classifiers) {
7576
initialCapacity += c.length() + 1;
7677
}
7778

@@ -205,6 +206,17 @@ public ProctorGroupsWriter build() {
205206
if (additionalFilter != null) {
206207
return additionalFilter.test(testName, proctorResult);
207208
}
209+
210+
// Do not log payload experiments which were overwritten
211+
if (consumableTestDefinition != null
212+
&& consumableTestDefinition.getPayloadExperimentConfig() != null
213+
&& proctorResult.getProperties().values().stream()
214+
.noneMatch(
215+
property ->
216+
property.getTestName().equals(testName))) {
217+
return false;
218+
}
219+
208220
// Suppress 100% allocation logging
209221
return loggableAllocation(
210222
testName, consumableTestDefinition, proctorResult);

‎proctor-consumer/src/test/java/com/indeed/proctor/consumer/ProctorGroupStubber.java

+45-7
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,22 @@ public class ProctorGroupStubber {
3030
new TestBucket("control", 0, "control", new Payload("controlPayload"));
3131
public static final TestBucket GROUP_1_BUCKET_WITH_PAYLOAD =
3232
new TestBucket("group1", 1, "group1", new Payload("activePayload"));
33+
34+
public static final TestBucket JSON_BUCKET;
35+
36+
static {
37+
try {
38+
JSON_BUCKET =
39+
new TestBucket(
40+
"group1",
41+
1,
42+
"group1",
43+
new Payload(new ObjectMapper().readTree("{\"foo\": 1, \"bar\": 2}")));
44+
} catch (final JsonProcessingException e) {
45+
throw new RuntimeException(e);
46+
}
47+
}
48+
3349
public static final TestBucket GROUP_1_BUCKET = new TestBucket("group1", 2, "group1");
3450

3551
public static final TestBucket GROUP_1_BUCKET_PROPERTY_PAYLOAD;
@@ -64,7 +80,7 @@ public class ProctorGroupStubber {
6480
public static class ProctorResultStubBuilder {
6581

6682
private final Map<String, ConsumableTestDefinition> definitions = new TreeMap<>();
67-
private final Map<StubTest, TestBucket> resolvedBuckets = new TreeMap<>();
83+
private final Map<String, TestBucket> resolvedBuckets = new TreeMap<>();
6884
private final Map<String, PayloadProperty> properties = new TreeMap<>();
6985

7086
public ProctorResultStubBuilder withStubTest(
@@ -92,7 +108,7 @@ public ProctorResultStubBuilder withStubTest(
92108
@Nullable final TestBucket resolved,
93109
final ConsumableTestDefinition definition) {
94110
if (resolved != null) {
95-
resolvedBuckets.put(stubTest, resolved);
111+
resolvedBuckets.put(stubTest.getName(), resolved);
96112
}
97113
if (definition != null) {
98114
definitions.put(stubTest.getName(), definition);
@@ -103,7 +119,7 @@ public ProctorResultStubBuilder withStubTest(
103119
public ProctorResultStubBuilder withStubProperty(
104120
final StubTest stubTest, @Nullable final TestBucket resolved) {
105121
if (resolved != null) {
106-
resolvedBuckets.put(stubTest, resolved);
122+
resolvedBuckets.put(stubTest.getName(), resolved);
107123
assert resolved.getPayload() != null;
108124
assert resolved.getPayload().getJson() != null;
109125
resolved.getPayload()
@@ -123,17 +139,39 @@ public ProctorResultStubBuilder withStubProperty(
123139
return this;
124140
}
125141

142+
public ProctorResultStubBuilder withStubProperty(
143+
final String testName,
144+
final ConsumableTestDefinition td,
145+
@Nullable final TestBucket resolved) {
146+
if (resolved != null) {
147+
definitions.put(testName, td);
148+
resolvedBuckets.put(testName, resolved);
149+
assert resolved.getPayload() != null;
150+
assert resolved.getPayload().getJson() != null;
151+
resolved.getPayload()
152+
.getJson()
153+
.fields()
154+
.forEachRemaining(
155+
field ->
156+
properties.put(
157+
field.getKey(),
158+
PayloadProperty.builder()
159+
.testName(testName)
160+
.value(field.getValue())
161+
.build()));
162+
}
163+
return this;
164+
}
165+
126166
public ProctorResult build() {
127167
return new ProctorResult(
128168
"0",
129169
resolvedBuckets.entrySet().stream()
130-
.collect(
131-
Collectors.toMap(
132-
e -> e.getKey().getName(), Map.Entry::getValue)),
170+
.collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)),
133171
resolvedBuckets.entrySet().stream()
134172
.collect(
135173
Collectors.toMap(
136-
e -> e.getKey().getName(),
174+
Map.Entry::getKey,
137175
e ->
138176
new Allocation(
139177
null,

‎proctor-consumer/src/test/java/com/indeed/proctor/consumer/ProctorGroupsWriterTest.java

+66-5
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,19 @@
11
package com.indeed.proctor.consumer;
22

3+
import com.fasterxml.jackson.core.JsonProcessingException;
4+
import com.fasterxml.jackson.databind.ObjectMapper;
5+
import com.google.common.collect.ImmutableList;
36
import com.google.common.collect.ImmutableMap;
47
import com.google.common.collect.ImmutableSortedMap;
58
import com.google.common.collect.Ordering;
9+
import com.indeed.proctor.common.PayloadProperty;
610
import com.indeed.proctor.common.ProctorResult;
711
import com.indeed.proctor.common.model.Allocation;
812
import com.indeed.proctor.common.model.ConsumableTestDefinition;
13+
import com.indeed.proctor.common.model.PayloadExperimentConfig;
914
import com.indeed.proctor.common.model.TestBucket;
15+
import com.indeed.proctor.common.model.TestDefinition;
16+
import com.indeed.proctor.common.model.TestType;
1017
import com.indeed.proctor.consumer.logging.TestGroupFormatter;
1118
import org.assertj.core.util.Strings;
1219
import org.junit.Test;
@@ -25,6 +32,8 @@ public class ProctorGroupsWriterTest {
2532
private static final String GROUP1_TEST_NAME = "d_foo_tst";
2633
private static final String SILENT_TEST_NAME = "e_silent_tst";
2734
private static final String FORCED_TEST_NAME = "f_forced_tst";
35+
private static final String PAYLOAD_PROPERTY_TEST_NAME = "g_payload_tst";
36+
private static final String WINNING_PAYLOAD_PROPERTY_TEST_NAME = "h_winning_payload_tst";
2837

2938
// for this test, buckets can be reused between test definitions, sorting by testname
3039
public static final TestBucket INACTIVE_BUCKET = new TestBucket("fooname", -1, "foodesc");
@@ -46,6 +55,8 @@ public class ProctorGroupsWriterTest {
4655
.put(GROUP1_TEST_NAME, GROUP1_BUCKET)
4756
.put(SILENT_TEST_NAME, GROUP1_BUCKET)
4857
.put(FORCED_TEST_NAME, GROUP1_BUCKET)
58+
.put(WINNING_PAYLOAD_PROPERTY_TEST_NAME, GROUP1_BUCKET)
59+
.put(PAYLOAD_PROPERTY_TEST_NAME, GROUP1_BUCKET)
4960
.build();
5061
private static final Map<String, Allocation> ALLOCATIONS =
5162
new ImmutableSortedMap.Builder<String, Allocation>(Ordering.natural())
@@ -54,19 +65,65 @@ public class ProctorGroupsWriterTest {
5465
.put(EMPTY_ALLOCID_DEFINITION_TEST_NAME, ALLOCATION_EMPTY_ID)
5566
.put(GROUP1_TEST_NAME, ALLOCATION_A)
5667
.put(SILENT_TEST_NAME, ALLOCATION_A)
68+
.put(WINNING_PAYLOAD_PROPERTY_TEST_NAME, ALLOCATION_A)
69+
.put(PAYLOAD_PROPERTY_TEST_NAME, ALLOCATION_A)
5770
.build();
5871

72+
static final ConsumableTestDefinition PROPERTY_TD =
73+
ConsumableTestDefinition.fromTestDefinition(
74+
TestDefinition.builder()
75+
.setTestType(TestType.RANDOM)
76+
.setSalt("foo")
77+
.setForceLogging(true)
78+
.setPayloadExperimentConfig(
79+
PayloadExperimentConfig.builder()
80+
.priority("123")
81+
.namespaces(ImmutableList.of("test"))
82+
.build())
83+
.build());
84+
static final ConsumableTestDefinition WINNING_PROPERTY_TD =
85+
ConsumableTestDefinition.fromTestDefinition(
86+
TestDefinition.builder()
87+
.setTestType(TestType.RANDOM)
88+
.setSalt("foo")
89+
.setForceLogging(true)
90+
.setPayloadExperimentConfig(
91+
PayloadExperimentConfig.builder()
92+
.priority("20000")
93+
.namespaces(ImmutableList.of("test"))
94+
.build())
95+
.build());
96+
5997
private static final Map<String, ConsumableTestDefinition> DEFINITIONS =
6098
ImmutableMap.<String, ConsumableTestDefinition>builder()
6199
.put(INACTIVE_TEST_NAME, stubDefinition(INACTIVE_BUCKET))
62100
.put(EMPTY_ALLOCID_DEFINITION_TEST_NAME, stubDefinition(GROUP1_BUCKET))
63101
.put(GROUP1_TEST_NAME, stubDefinition(GROUP1_BUCKET))
64102
.put(SILENT_TEST_NAME, stubDefinition(GROUP1_BUCKET, d -> d.setSilent(true)))
65103
.put(FORCED_TEST_NAME, stubDefinition(GROUP1_BUCKET))
104+
.put(WINNING_PAYLOAD_PROPERTY_TEST_NAME, WINNING_PROPERTY_TD)
105+
.put(PAYLOAD_PROPERTY_TEST_NAME, PROPERTY_TD)
66106
.build();
107+
private static final Map<String, PayloadProperty> PROPERTIES;
108+
109+
static {
110+
try {
111+
PROPERTIES =
112+
ImmutableMap.<String, PayloadProperty>builder()
113+
.put(
114+
"foo",
115+
PayloadProperty.builder()
116+
.value(new ObjectMapper().readTree("1"))
117+
.testName(WINNING_PAYLOAD_PROPERTY_TEST_NAME)
118+
.build())
119+
.build();
120+
} catch (final JsonProcessingException e) {
121+
throw new RuntimeException(e);
122+
}
123+
}
67124

68125
private static final ProctorResult PROCTOR_RESULT =
69-
new ProctorResult(null, BUCKETS, ALLOCATIONS, DEFINITIONS);
126+
new ProctorResult(null, BUCKETS, ALLOCATIONS, DEFINITIONS, PROPERTIES);
70127

71128
@Test
72129
public void testWithEmptyResult() {
@@ -86,7 +143,7 @@ public void testWithEmptyResult() {
86143
public void testDoubleFormattingWriter() {
87144
// legacy Indeed behavior
88145
final String expected =
89-
"b_missing_definition0,c_empty_alloc_id0,d_foo_tst1,f_forced_tst1,#A:b_missing_definition0,#A:d_foo_tst1,f_forced_tst1";
146+
"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";
90147

91148
final ProctorGroupsWriter defaultWriter =
92149
new ProctorGroupsWriter.Builder(
@@ -101,9 +158,11 @@ public void testDoubleFormattingWriter() {
101158
EMPTY_ALLOCID_DEFINITION_TEST_NAME + 0,
102159
GROUP1_TEST_NAME + 1,
103160
FORCED_TEST_NAME + 1,
161+
WINNING_PAYLOAD_PROPERTY_TEST_NAME + 1,
104162
"#A:" + MISSING_DEFINITION_TEST_NAME + 0,
105163
"#A:" + GROUP1_TEST_NAME + 1,
106-
FORCED_TEST_NAME + 1)
164+
FORCED_TEST_NAME + 1,
165+
"#A:" + WINNING_PAYLOAD_PROPERTY_TEST_NAME + 1)
107166
.with(","));
108167
}
109168

@@ -116,15 +175,17 @@ public void testCustomWriter() {
116175
.setIncludeInactiveGroups(true)
117176
.build();
118177
assertThat(writerWithAllocIds.writeGroupsAsString(PROCTOR_RESULT))
119-
.isEqualTo("#A:a_inactive_tst-1,#A:d_foo_tst1,#A:e_silent_tst1,f_forced_tst1");
178+
.isEqualTo(
179+
"#A:a_inactive_tst-1,#A:d_foo_tst1,#A:e_silent_tst1,f_forced_tst1,#A:h_winning_payload_tst1");
120180

121181
final ProctorGroupsWriter writerWithoutAllocIds =
122182
new ProctorGroupsWriter.Builder(TestGroupFormatter.WITHOUT_ALLOC_ID)
123183
.setIncludeSilentTests(true)
124184
.setIncludeTestWithoutDefinition(false)
125185
.build();
126186
assertThat(writerWithoutAllocIds.writeGroupsAsString(PROCTOR_RESULT))
127-
.isEqualTo("c_empty_alloc_id0,d_foo_tst1,e_silent_tst1,f_forced_tst1");
187+
.isEqualTo(
188+
"c_empty_alloc_id0,d_foo_tst1,e_silent_tst1,f_forced_tst1,h_winning_payload_tst1");
128189
}
129190

130191
@Test

‎proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java

+39
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import com.fasterxml.jackson.core.JsonProcessingException;
44
import com.fasterxml.jackson.databind.ObjectMapper;
5+
import com.google.common.collect.ImmutableList;
56
import com.google.common.collect.ImmutableSet;
67
import com.google.common.collect.Lists;
78
import com.google.common.collect.Sets;
@@ -26,6 +27,7 @@
2627
import static com.indeed.proctor.consumer.ProctorGroupStubber.GROUP_1_BUCKET_PROPERTY_PAYLOAD;
2728
import static com.indeed.proctor.consumer.ProctorGroupStubber.GROUP_1_BUCKET_WITH_PAYLOAD;
2829
import static com.indeed.proctor.consumer.ProctorGroupStubber.INACTIVE_BUCKET;
30+
import static com.indeed.proctor.consumer.ProctorGroupStubber.JSON_BUCKET;
2931
import static com.indeed.proctor.consumer.ProctorGroupStubber.StubTest.CONTROL_SELECTED_TEST;
3032
import static com.indeed.proctor.consumer.ProctorGroupStubber.StubTest.GROUP1_SELECTED_TEST;
3133
import static com.indeed.proctor.consumer.ProctorGroupStubber.StubTest.GROUP_WITH_FALLBACK_TEST;
@@ -235,6 +237,43 @@ public void testToVerifyGroupsString() {
235237
.isEqualTo("#A1:suppress_logging_example_tst0");
236238
}
237239

240+
@Test
241+
public void testToLoggingWithProperties() {
242+
final ConsumableTestDefinition td =
243+
ConsumableTestDefinition.fromTestDefinition(
244+
TestDefinition.builder()
245+
.setTestType(TestType.RANDOM)
246+
.setSalt("foo")
247+
.setForceLogging(true)
248+
.setPayloadExperimentConfig(
249+
PayloadExperimentConfig.builder()
250+
.priority("123")
251+
.namespaces(ImmutableList.of("test"))
252+
.build())
253+
.build());
254+
final ConsumableTestDefinition tdWithHigherPriority =
255+
ConsumableTestDefinition.fromTestDefinition(
256+
TestDefinition.builder()
257+
.setTestType(TestType.RANDOM)
258+
.setSalt("foo")
259+
.setForceLogging(true)
260+
.setPayloadExperimentConfig(
261+
PayloadExperimentConfig.builder()
262+
.priority("20000")
263+
.namespaces(ImmutableList.of("test"))
264+
.build())
265+
.build());
266+
final ProctorResult result =
267+
new ProctorGroupStubber.ProctorResultStubBuilder()
268+
.withStubProperty(CONTROL_SELECTED_TEST.getName(), td, JSON_BUCKET)
269+
.withStubProperty(
270+
PROPERTY_TEST.getName(), tdWithHigherPriority, JSON_BUCKET)
271+
.build();
272+
273+
assertThat((new AbstractGroups(result) {}).toLoggingString())
274+
.isEqualTo("#A1:propertytest1");
275+
}
276+
238277
@Test
239278
public void testCheckRolledOutAllocation() {
240279
final ConsumableTestDefinition td =

0 commit comments

Comments
 (0)