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 3 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
21 changes: 21 additions & 0 deletions proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java
View file
Open in desktop
Expand Up @@ -417,6 +417,8 @@ protected final List<String> 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)
Expand Down Expand Up @@ -454,6 +456,25 @@ protected final void appendTestGroupsWithAllocations(
}
}

private boolean filterRolledOutAllocations(final String testName) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I'm terrible with names, but I'd prefer this to indicate the return type of boolean. Something like isTestRolledOut

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking about this more. isTestRolledOut is not actually descriptive of what this method is doing. It is checking if the test should be logged when the Allocation is fully rolled out to 100%. A better name might be something like shouldLogRolledOutTest or loggableRolledOutAllocation. Thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That makes sense. I tend to name methods more for their content than where they're used. So, I like the second option more since it indicates a boolean and what it is doing. Maybe even isRolledOutAllocation. Either way, I trust your judgement, so I'll leave it up to you.

final ConsumableTestDefinition td = proctorResult.getTestDefinitions().get(testName);
return shouldLogRolledOutAllocation(testName, td, proctorResult);
}

public static boolean shouldLogRolledOutAllocation(
zacharygoodwin marked this conversation as resolved.
Show resolved Hide resolved
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.shouldLogRolledOutAllocation;

/**
* 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 shouldLogRolledOutAllocation(
testName, consumableTestDefinition, proctorResult);
});
}
}
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
59 changes: 56 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.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;
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,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() {

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