Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

PROC-1475: Suppress 100% allocations #164

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

PROC-1475: Suppress 100% allocations #164

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b85f3c5
PROC-1475: Suppress 100% allocations
zacharygoodwin Apr 24, 2024
091a144
PROC-1475: Refactor duplicated logic into single method
zacharygoodwin Apr 24, 2024
baa6a44
PROC-1475: Rename and add unit test
zacharygoodwin Apr 24, 2024
df1556a
PROC-1475: Rename method for clarity
zacharygoodwin Apr 24, 2024
64f7404
PROC-1475: Rename method for clarity
zacharygoodwin Apr 25, 2024
bbe788c
PROC-1475: Do not filter out 100% allocations for showGroups endpoint
zacharygoodwin May 8, 2024
cf0dafa
PROC-1475: Rename to loggableAllocation
zacharygoodwin May 16, 2024
ba17e42
PROC-1475: Add public method for rolled out allocations logging string
zacharygoodwin May 20, 2024
4933db6
PROC-1475: Rename method
zacharygoodwin May 20, 2024
547a9f9
PROC-1475: Update documentation of toVerifyGroupsString()
zacharygoodwin May 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
92 changes: 91 additions & 1 deletion proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java
View file
Open in desktop
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)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible allocation.getRanges() return null?

return td.getForceLogging();
}
}
return true;
}

/**
* Generates a Map[testname, bucketValue] for bucketValues >= 0.
*
Expand Down
6 changes: 5 additions & 1 deletion proctor-consumer/src/main/java/com/indeed/proctor/consumer/ProctorGroupsWriter.java
View file
Open in desktop
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
2 changes: 1 addition & 1 deletion proctor-consumer/src/main/java/com/indeed/proctor/consumer/spring/ShowGroupsHandler.java
View file
Open in desktop
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
18 changes: 16 additions & 2 deletions proctor-consumer/src/test/java/com/indeed/proctor/consumer/ProctorGroupStubber.java
View file
Open in desktop
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
80 changes: 77 additions & 3 deletions proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java
View file
Open in desktop
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