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

PROC-1475: Suppress 100% allocations #164

Merged
merged 11 commits into from
May 29, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -355,6 +355,28 @@ public String toLoggingString() {
return sb.toString();
}

/**
* 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.
*
* <p>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 toVerifyGroupsString() {
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()
Expand Down Expand Up @@ -398,12 +420,61 @@ public void appendTestGroups(final StringBuilder sb, final char separator) {
appendTestGroupsWithAllocations(sb, separator, testNames);
}

/**
* 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 ",".
*
* <p>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<String> 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<String> getLoggingTestNames() {
final Map<String, ConsumableTestDefinition> testDefinitions =
proctorResult.getTestDefinitions();
// following lines should preserve the order in the map to ensure logging values are stable
final Map<String, TestBucket> 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();
})
// Suppress 100% allocation logging
.filter(this::loggableAllocation)
// call to getValueWithouMarkingUsage() to allow overrides of getActiveBucket, but
// avoid marking
.filter(testName -> getValueWithoutMarkingUsage(testName, -1) >= 0)
.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<String> getLoggingTestNames() {
protected final List<String> getLoggingTestNamesFullList() {
final Map<String, ConsumableTestDefinition> testDefinitions =
proctorResult.getTestDefinitions();
// following lines should preserve the order in the map to ensure logging values are stable
Expand Down Expand Up @@ -454,6 +525,25 @@ protected final void appendTestGroupsWithAllocations(
}
}

private boolean loggableAllocation(final String testName) {
final ConsumableTestDefinition td = proctorResult.getTestDefinitions().get(testName);
return loggableAllocation(testName, td, proctorResult);
}

public static boolean loggableAllocation(
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)) {
vanhaxl marked this conversation as resolved.
Show resolved Hide resolved
return td.getForceLogging();
}
}
return true;
}

/**
* Generates a Map[testname, bucketValue] for bucketValues >= 0.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
import java.util.Optional;
import java.util.function.BiPredicate;

import static com.indeed.proctor.consumer.AbstractGroups.loggableAllocation;

/**
* Helper class to build a Strings to log, helping with analysis of experiments.
*
Expand Down Expand Up @@ -203,7 +205,9 @@ public ProctorGroupsWriter build() {
if (additionalFilter != null) {
return additionalFilter.test(testName, proctorResult);
}
return true;
// Suppress 100% allocation logging
return loggableAllocation(
testName, consumableTestDefinition, proctorResult);
});
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -14,6 +16,7 @@

import java.util.Arrays;

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;
Expand All @@ -27,6 +30,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;
Expand All @@ -53,6 +57,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,
Expand Down Expand Up @@ -164,7 +175,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
Expand All @@ -180,6 +191,68 @@ public void testToLoggingString() {
"#A1:abtst1,#A1:bgtst0,#A1:groupwithfallbacktst2,#A1:no_definition_tst2");
}

@Test
public void testToVerifyGroupsString() {
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) {}).toVerifyGroupsString())
.isEqualTo("#A1:suppress_logging_example_tst0");
}

@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(loggableAllocation("suppress_logging_example_tst", tdWithForceLogging, result))
.isTrue();

assertThat(loggableAllocation("suppress_logging_example_tst", td, result)).isFalse();
}

@Test
public void testToLoggingStringWithExposureAndObserver() {

Expand Down Expand Up @@ -268,11 +341,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
Expand Down
Loading