diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index c1fed94f8..59a96bcfa 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -23,6 +23,7 @@ springJdbc = 'org.springframework:spring-jdbc:5.3.19' springTest = 'org.springframework:spring-test:5.3.19' springWeb = 'org.springframework:spring-web:5.3.19' springWebmvc = 'org.springframework:spring-webmvc:5.3.19' +lombok = 'org.projectlombok:lombok:1.18.32' ant = 'org.apache.ant:ant:1.10.9' svnkit = 'org.tmatesoft.svnkit:svnkit:1.8.5' tomcatElApi = 'org.apache.tomcat:tomcat-el-api:7.0.109' diff --git a/proctor-common/build.gradle b/proctor-common/build.gradle index 75729f3e7..e25c414e7 100644 --- a/proctor-common/build.gradle +++ b/proctor-common/build.gradle @@ -29,6 +29,11 @@ dependencies { testImplementation libs.annotationApi testImplementation libs.mockito testImplementation libs.jsr305 + compileOnly libs.lombok + annotationProcessor libs.lombok + + testCompileOnly libs.lombok + testAnnotationProcessor libs.lombok } // Need to remove all dependenceis that get added or will cause issues upstream when projects include diff --git a/proctor-common/src/main/java/com/indeed/proctor/common/PayloadProperty.java b/proctor-common/src/main/java/com/indeed/proctor/common/PayloadProperty.java new file mode 100644 index 000000000..e10891e83 --- /dev/null +++ b/proctor-common/src/main/java/com/indeed/proctor/common/PayloadProperty.java @@ -0,0 +1,18 @@ +package com.indeed.proctor.common; + +import com.fasterxml.jackson.databind.JsonNode; +import lombok.AllArgsConstructor; +import lombok.Builder; +import lombok.Data; +import lombok.NoArgsConstructor; +import lombok.extern.jackson.Jacksonized; + +@Data +@Builder +@NoArgsConstructor +@AllArgsConstructor +@Jacksonized +public class PayloadProperty { + private String testName; + private JsonNode value; +} diff --git a/proctor-common/src/main/java/com/indeed/proctor/common/Proctor.java b/proctor-common/src/main/java/com/indeed/proctor/common/Proctor.java index 29ee45d17..a8bfd94c5 100644 --- a/proctor-common/src/main/java/com/indeed/proctor/common/Proctor.java +++ b/proctor-common/src/main/java/com/indeed/proctor/common/Proctor.java @@ -11,6 +11,8 @@ import com.indeed.proctor.common.model.Allocation; import com.indeed.proctor.common.model.Audit; import com.indeed.proctor.common.model.ConsumableTestDefinition; +import com.indeed.proctor.common.model.Payload; +import com.indeed.proctor.common.model.PayloadExperimentConfig; import com.indeed.proctor.common.model.TestBucket; import com.indeed.proctor.common.model.TestMatrixArtifact; import com.indeed.proctor.common.model.TestType; @@ -323,6 +325,7 @@ public ProctorResult determineTestGroups( // this method final SortedMap testGroups = new TreeMap<>(); final SortedMap testAllocations = new TreeMap<>(); + final SortedMap testProperties = new TreeMap<>(); final List filteredEvaluationOrder; if (determineAllTests) { @@ -400,6 +403,9 @@ public ProctorResult determineTestGroups( if (chooseResult.getTestBucket() != null) { testGroups.put(testName, chooseResult.getTestBucket()); + if (testChooser.getTestDefinition().getPayloadExperimentConfig() != null) { + populateProperties(testName, chooseResult.getTestBucket(), testProperties); + } } if (chooseResult.getAllocation() != null) { testAllocations.put(testName, chooseResult.getAllocation()); @@ -424,7 +430,8 @@ public ProctorResult determineTestGroups( testAllocations, testDefinitions, identifiers, - inputContext); + inputContext, + testProperties); if (resultReporter != null) { resultReporter.reportMetrics(result, invalidIdentifierCount); @@ -515,4 +522,57 @@ private boolean isIncognitoEnabled(@Nonnull final Map inputConte .map(value -> value.equalsIgnoreCase("true")) .orElse(false); } + + /** + * Aggregates properties of Payload Experiments for look up in Proctor Result. Checks Priority + * of Property to handle conflicting overrides of properties within namespaces. Properties are + * the Key:Value pairs of fields stored in JsonNode Payload type. + */ + private void populateProperties( + final String testName, + final TestBucket testBucket, + final Map testProperties) { + final Payload payload = testBucket.getPayload(); + if (payload != null && payload.getJson() != null) { + payload.getJson() + .fields() + .forEachRemaining( + field -> attemptStoringProperty(field, testName, testProperties)); + } + } + + private void attemptStoringProperty( + final Map.Entry field, + final String testName, + final Map testProperties) { + final PayloadProperty curr = testProperties.get(field.getKey()); + // store property if it does not exist in map + if (curr == null) { + testProperties.put( + field.getKey(), + PayloadProperty.builder().value(field.getValue()).testName(testName).build()); + } else { + final PayloadExperimentConfig currPayloadConfig = + testChoosers + .get(curr.getTestName()) + .getTestDefinition() + .getPayloadExperimentConfig(); + final PayloadExperimentConfig newPayloadConfig = + testChoosers.get(testName).getTestDefinition().getPayloadExperimentConfig(); + // store property if it has higher priority than the currently stored property + try { + if (currPayloadConfig == null + || currPayloadConfig.isHigherPriorityThan(newPayloadConfig)) { + testProperties.put( + field.getKey(), + PayloadProperty.builder() + .value(field.getValue()) + .testName(testName) + .build()); + } + } catch (final NumberFormatException e) { + LOGGER.error("Failed to parse priority value to Long: ", e); + } + } + } } diff --git a/proctor-common/src/main/java/com/indeed/proctor/common/ProctorResult.java b/proctor-common/src/main/java/com/indeed/proctor/common/ProctorResult.java index b7f4e5363..5a1340f54 100644 --- a/proctor-common/src/main/java/com/indeed/proctor/common/ProctorResult.java +++ b/proctor-common/src/main/java/com/indeed/proctor/common/ProctorResult.java @@ -30,6 +30,7 @@ public class ProctorResult { emptySortedMap(), emptyMap(), new Identifiers(emptyMap()), + emptyMap(), emptyMap()); private final String matrixVersion; @@ -40,6 +41,8 @@ public class ProctorResult { /** maps from testname to TestDefinition */ @Nonnull private final Map testDefinitions; + @Nonnull private final Map properties; + private final Identifiers identifiers; private final Map inputContext; @@ -81,7 +84,8 @@ public ProctorResult( * @param buckets the resolved bucket for each test * @param allocations the determined allocation for each test * @param testDefinitions the original test definitions - * @deprecated this constructor creates copies of all input collections + * @deprecated this constructor creates copies of all input collections negatively effecting + * performance */ @Deprecated public ProctorResult( @@ -102,6 +106,40 @@ public ProctorResult( (testDefinitions == null) ? emptyMap() : new HashMap<>(testDefinitions)); } + /** + * Create a ProctorResult with copies of the provided collections + * + * @param matrixVersion any string, used for debugging + * @param buckets the resolved bucket for each test + * @param allocations the determined allocation for each test + * @param testDefinitions the original test definitions + * @param properties the properties + * @deprecated this constructor creates copies of all input collections negatively effecting + * performance + */ + @Deprecated + public ProctorResult( + final String matrixVersion, + @Nonnull final Map buckets, + @Nonnull final Map allocations, + // allowing null for historical reasons + @Nullable final Map testDefinitions, + @Nonnull final Map properties) { + // Potentially client applications might need to build ProctorResult instances in each + // request, and some apis + // have large proctorResult objects, so if teams use this constructor, this may have a + // noticeable + // impact on latency and GC, so ideally clients should avoid this constructor. + this( + matrixVersion, + new TreeMap<>(buckets), + new TreeMap<>(allocations), + (testDefinitions == null) ? emptyMap() : new HashMap<>(testDefinitions), + new Identifiers(emptyMap()), + emptyMap(), + (properties == null) ? emptyMap() : new HashMap<>(properties)); + } + /** * Plain constructor, not creating TreeMaps. * @@ -132,6 +170,8 @@ public ProctorResult( * @param buckets the resolved bucket for each test * @param allocations the determined allocation for each test * @param testDefinitions the original test definitions + * @param identifiers the identifiers used for determine groups + * @param inputContext the context variables */ public ProctorResult( @Nonnull final String matrixVersion, @@ -140,6 +180,35 @@ public ProctorResult( @Nonnull final Map testDefinitions, @Nonnull final Identifiers identifiers, @Nonnull final Map inputContext) { + this( + matrixVersion, + buckets, + allocations, + testDefinitions, + identifiers, + inputContext, + emptyMap()); + } + + /** + * Plain constructor, not creating TreeMaps. + * + * @param matrixVersion any string, used for debugging + * @param buckets the resolved bucket for each test + * @param allocations the determined allocation for each test + * @param testDefinitions the original test definitions + * @param identifiers the identifiers used for determine groups + * @param inputContext the context variables + * @param properties the aggregated properties of payload experiments + */ + public ProctorResult( + @Nonnull final String matrixVersion, + @Nonnull final SortedMap buckets, + @Nonnull final SortedMap allocations, + @Nonnull final Map testDefinitions, + @Nonnull final Identifiers identifiers, + @Nonnull final Map inputContext, + @Nonnull final Map properties) { this.matrixVersion = matrixVersion; this.buckets = buckets; this.allocations = allocations; @@ -147,6 +216,7 @@ public ProctorResult( this.identifiers = identifiers; this.inputContext = inputContext; this.hasLoggedTests = new HashSet<>(); + this.properties = properties; } /** @@ -161,7 +231,8 @@ public static ProctorResult unmodifiableView(final ProctorResult proctorResult) Collections.unmodifiableSortedMap(proctorResult.allocations), Collections.unmodifiableMap(proctorResult.testDefinitions), proctorResult.identifiers, - Collections.unmodifiableMap(proctorResult.inputContext)); + Collections.unmodifiableMap(proctorResult.inputContext), + Collections.unmodifiableMap(proctorResult.properties)); } @SuppressWarnings("UnusedDeclaration") @@ -198,6 +269,11 @@ public Map getInputContext() { return inputContext; } + @Nonnull + public Map getProperties() { + return properties; + } + public boolean markTestAsLogged(final String test) { return this.hasLoggedTests.add(test); } diff --git a/proctor-common/src/main/java/com/indeed/proctor/common/dynamic/DynamicFilters.java b/proctor-common/src/main/java/com/indeed/proctor/common/dynamic/DynamicFilters.java index 83bf84075..73398ee7b 100644 --- a/proctor-common/src/main/java/com/indeed/proctor/common/dynamic/DynamicFilters.java +++ b/proctor-common/src/main/java/com/indeed/proctor/common/dynamic/DynamicFilters.java @@ -32,7 +32,8 @@ public class DynamicFilters implements JsonSerializable { TestNamePrefixFilter.class, TestNamePatternFilter.class, MetaTagsFilter.class, - MatchAllFilter.class)); + MatchAllFilter.class, + NamespacesFilter.class)); private final List filters; diff --git a/proctor-common/src/main/java/com/indeed/proctor/common/dynamic/NamespacesFilter.java b/proctor-common/src/main/java/com/indeed/proctor/common/dynamic/NamespacesFilter.java new file mode 100644 index 000000000..0073b91a0 --- /dev/null +++ b/proctor-common/src/main/java/com/indeed/proctor/common/dynamic/NamespacesFilter.java @@ -0,0 +1,59 @@ +package com.indeed.proctor.common.dynamic; + +import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.annotation.JsonTypeName; +import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableSet; +import com.indeed.proctor.common.model.ConsumableTestDefinition; +import com.indeed.proctor.common.model.PayloadExperimentConfig; +import org.springframework.util.CollectionUtils; + +import java.util.Collections; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; + +@JsonTypeName("namespaces_filter") +public class NamespacesFilter implements DynamicFilter { + private final Set namespaces; + + public NamespacesFilter(@JsonProperty("namespaces") final Set namespaces) { + Preconditions.checkArgument( + !CollectionUtils.isEmpty(namespaces), + "namespaces should be non-empty string list."); + this.namespaces = ImmutableSet.copyOf(namespaces); + } + + @JsonProperty("namespaces") + public Set getNamespaces() { + return this.namespaces; + } + + @Override + public boolean matches(final String testName, final ConsumableTestDefinition testDefinition) { + final boolean isMatched = + Optional.ofNullable(testDefinition.getPayloadExperimentConfig()) + .map(PayloadExperimentConfig::getNamespaces).orElse(Collections.emptyList()) + .stream() + .anyMatch(this.namespaces::contains); + testDefinition.setDynamic(isMatched); + return isMatched; + } + + @Override + public boolean equals(final Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + final NamespacesFilter that = (NamespacesFilter) o; + return namespaces.equals(that.namespaces); + } + + @Override + public int hashCode() { + return Objects.hash(namespaces); + } +} diff --git a/proctor-common/src/main/java/com/indeed/proctor/common/model/ConsumableTestDefinition.java b/proctor-common/src/main/java/com/indeed/proctor/common/model/ConsumableTestDefinition.java index d73088fee..db33c9704 100644 --- a/proctor-common/src/main/java/com/indeed/proctor/common/model/ConsumableTestDefinition.java +++ b/proctor-common/src/main/java/com/indeed/proctor/common/model/ConsumableTestDefinition.java @@ -38,6 +38,8 @@ public class ConsumableTestDefinition { private boolean containsUnitlessAllocation = false; private boolean forceLogging = false; + @Nullable PayloadExperimentConfig payloadExperimentConfig; + public ConsumableTestDefinition() { /* intentionally empty */ } @@ -141,7 +143,8 @@ private ConsumableTestDefinition( final boolean evaluateForIncognitoUsers, final boolean enableUnitlessAllocations, final boolean containsUnitlessAllocation, - final boolean forceLogging) { + final boolean forceLogging, + final PayloadExperimentConfig payloadExperimentConfig) { this.constants = constants; this.version = version; this.salt = salt; @@ -157,6 +160,7 @@ private ConsumableTestDefinition( this.enableUnitlessAllocations = enableUnitlessAllocations; this.containsUnitlessAllocation = containsUnitlessAllocation; this.forceLogging = forceLogging; + this.payloadExperimentConfig = payloadExperimentConfig; } @Nonnull @@ -298,6 +302,15 @@ public void setForceLogging(final boolean forceLogging) { this.forceLogging = forceLogging; } + @Nullable + public PayloadExperimentConfig getPayloadExperimentConfig() { + return payloadExperimentConfig; + } + + public void setPayloadExperimentConfig(final PayloadExperimentConfig payloadExperimentConfig) { + this.payloadExperimentConfig = payloadExperimentConfig; + } + @Nonnull public static ConsumableTestDefinition fromTestDefinition(@Nonnull final TestDefinition td) { final Map specialConstants = td.getSpecialConstants(); @@ -354,6 +367,7 @@ public static ConsumableTestDefinition fromTestDefinition(@Nonnull final TestDef td.getEvaluateForIncognitoUsers(), td.getEnableUnitlessAllocations(), containsUnitlessAllocation, - td.getForceLogging()); + td.getForceLogging(), + td.getPayloadExperimentConfig()); } } diff --git a/proctor-common/src/main/java/com/indeed/proctor/common/model/PayloadExperimentConfig.java b/proctor-common/src/main/java/com/indeed/proctor/common/model/PayloadExperimentConfig.java new file mode 100644 index 000000000..83e54019e --- /dev/null +++ b/proctor-common/src/main/java/com/indeed/proctor/common/model/PayloadExperimentConfig.java @@ -0,0 +1,43 @@ +package com.indeed.proctor.common.model; + +import lombok.AllArgsConstructor; +import lombok.Builder; +import lombok.Data; +import lombok.NoArgsConstructor; +import lombok.extern.jackson.Jacksonized; + +import javax.annotation.Nonnull; +import javax.annotation.Nullable; +import java.util.Collections; +import java.util.List; + +@Data +@Builder +@NoArgsConstructor +@AllArgsConstructor +@Jacksonized +public class PayloadExperimentConfig { + @Nullable private String priority; + @Nonnull private List namespaces = Collections.emptyList(); + + @Override + public String toString() { + return "PayloadExperimentConfig{" + + "priority='" + + priority + + '\'' + + ", namespaces=" + + namespaces + + '}'; + } + + public boolean isHigherPriorityThan(final PayloadExperimentConfig otherPayloadConfig) { + final long currPriority = + this.getPriority() == null ? Long.MIN_VALUE : Long.parseLong(this.getPriority()); + final long otherPriority = + otherPayloadConfig == null || otherPayloadConfig.getPriority() == null + ? Long.MIN_VALUE + : Long.parseLong(otherPayloadConfig.getPriority()); + return currPriority < otherPriority; + } +} diff --git a/proctor-common/src/main/java/com/indeed/proctor/common/model/TestDefinition.java b/proctor-common/src/main/java/com/indeed/proctor/common/model/TestDefinition.java index 414c09017..75646932d 100644 --- a/proctor-common/src/main/java/com/indeed/proctor/common/model/TestDefinition.java +++ b/proctor-common/src/main/java/com/indeed/proctor/common/model/TestDefinition.java @@ -57,6 +57,8 @@ public class TestDefinition { private boolean forceLogging; + @Nullable PayloadExperimentConfig payloadExperimentConfig; + public TestDefinition() { /* intentionally empty */ } @@ -141,6 +143,7 @@ public TestDefinition( this.metaTags = metaTags; this.evaluateForIncognitoUsers = false; this.forceLogging = false; + payloadExperimentConfig = null; } public TestDefinition(@Nonnull final TestDefinition other) { @@ -163,6 +166,7 @@ private TestDefinition(@Nonnull final Builder builder) { evaluateForIncognitoUsers = builder.evaluateForIncognitoUsers; enableUnitlessAllocations = builder.enableUnitlessAllocations; forceLogging = builder.forceLogging; + payloadExperimentConfig = builder.payloadExperimentConfig; } public static Builder builder() { @@ -332,6 +336,11 @@ public boolean getForceLogging() { return forceLogging; } + @Nullable + public PayloadExperimentConfig getPayloadExperimentConfig() { + return payloadExperimentConfig; + } + @Override public String toString() { return "TestDefinition{" @@ -369,6 +378,8 @@ public String toString() { + enableUnitlessAllocations + ", forceLogging=" + forceLogging + + ", payloadExperimentConfig=" + + payloadExperimentConfig + '}'; } @@ -403,7 +414,8 @@ public int hashCode() { dependsOn, evaluateForIncognitoUsers, enableUnitlessAllocations, - forceLogging); + forceLogging, + payloadExperimentConfig); } /** @@ -436,7 +448,8 @@ && bucketListEqual(buckets, that.buckets) && Objects.equals(dependsOn, that.dependsOn) && Objects.equals(evaluateForIncognitoUsers, that.evaluateForIncognitoUsers) && Objects.equals(enableUnitlessAllocations, that.enableUnitlessAllocations) - && Objects.equals(forceLogging, that.forceLogging); + && Objects.equals(forceLogging, that.forceLogging) + && Objects.equals(payloadExperimentConfig, that.payloadExperimentConfig); } @VisibleForTesting @@ -478,6 +491,7 @@ public static class Builder { private boolean evaluateForIncognitoUsers; private boolean enableUnitlessAllocations; private boolean forceLogging; + private PayloadExperimentConfig payloadExperimentConfig; public Builder from(@Nonnull final TestDefinition other) { setVersion(other.version); @@ -495,6 +509,7 @@ public Builder from(@Nonnull final TestDefinition other) { setEvaluateForIncognitoUsers(other.evaluateForIncognitoUsers); setEnableUnitlessAllocations(other.enableUnitlessAllocations); setForceLogging(other.forceLogging); + setPayloadExperimentConfig(other.payloadExperimentConfig); return this; } @@ -608,6 +623,12 @@ public Builder setForceLogging(final boolean forceLogging) { return this; } + public Builder setPayloadExperimentConfig( + final PayloadExperimentConfig payloadExperimentConfig) { + this.payloadExperimentConfig = payloadExperimentConfig; + return this; + } + public TestDefinition build() { return new TestDefinition(this); } diff --git a/proctor-common/src/test/java/com/indeed/proctor/common/TestProctor.java b/proctor-common/src/test/java/com/indeed/proctor/common/TestProctor.java index dc82a7f38..8239eb410 100644 --- a/proctor-common/src/test/java/com/indeed/proctor/common/TestProctor.java +++ b/proctor-common/src/test/java/com/indeed/proctor/common/TestProctor.java @@ -1,5 +1,6 @@ package com.indeed.proctor.common; +import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.base.Splitter; @@ -10,6 +11,8 @@ import com.indeed.proctor.common.model.Allocation; import com.indeed.proctor.common.model.Audit; import com.indeed.proctor.common.model.ConsumableTestDefinition; +import com.indeed.proctor.common.model.Payload; +import com.indeed.proctor.common.model.PayloadExperimentConfig; import com.indeed.proctor.common.model.Range; import com.indeed.proctor.common.model.TestBucket; import com.indeed.proctor.common.model.TestDefinition; @@ -919,6 +922,216 @@ public void testDetermineTestGroups_OnlyEvaluateIncognitoTestsWhenEnabled() { .containsEntry("ignore2", testDefinition_nonIncognito); } + @Test + public void testDetermineTestGroups_withProperty() throws JsonProcessingException { + final JsonNode p1 = new ObjectMapper().readTree("{\"key1\": 1, \"key2\": \"abc\"}"); + final JsonNode p2 = + new ObjectMapper() + .readTree("{\"some.property\": {}, \"another.property\": [\"abc\"]}"); + final TestBucket inactiveBucket = new TestBucket("inactive", -1, "", new Payload(p1)); + final TestBucket controlBucket = new TestBucket("control", 0, "", new Payload(p2)); + final TestBucket activeBucket = new TestBucket("active", 1, "", new Payload(p1)); + final Allocation allocationX = new Allocation("", ImmutableList.of(new Range(1, 1.0))); + final Allocation allocationY = new Allocation("", ImmutableList.of(new Range(0, 1.0))); + + final ConsumableTestDefinition testDefinition1 = + ConsumableTestDefinition.fromTestDefinition( + TestDefinition.builder() + .setSalt("&X") + .setTestType(TestType.ANONYMOUS_USER) + .addBuckets(inactiveBucket, controlBucket, activeBucket) + .addAllocations(allocationX) + .setPayloadExperimentConfig( + PayloadExperimentConfig.builder() + .namespaces(ImmutableList.of("foobar")) + .priority("1") + .build()) + .build()); + final ConsumableTestDefinition testDefinition2 = + ConsumableTestDefinition.fromTestDefinition( + TestDefinition.builder() + .setSalt("&Y") + .setTestType(TestType.ANONYMOUS_USER) + .addBuckets(inactiveBucket, controlBucket, activeBucket) + .addAllocations(allocationY) + .setPayloadExperimentConfig( + PayloadExperimentConfig.builder() + .namespaces(ImmutableList.of("foobar")) + .priority("1") + .build()) + .build()); + + final Map tests = + ImmutableMap.of( + "foo", testDefinition1, + "bar", testDefinition2); + final TestMatrixArtifact matrix = new TestMatrixArtifact(); + matrix.setTests(tests); + matrix.setAudit(new Audit()); + + final Proctor proctor = + Proctor.construct( + matrix, + ProctorLoadResult.emptyResult(), + RuleEvaluator.defaultFunctionMapperBuilder().build()); + + final ProctorResult proctorResult = + proctor.determineTestGroups( + Identifiers.of(TestType.ANONYMOUS_USER, "cookie"), + Collections.emptyMap(), + ForceGroupsOptions.builder().build(), + Collections.emptyList()); + + assertThat(proctorResult.getBuckets()) + .containsOnlyKeys("foo", "bar") + .containsEntry("foo", activeBucket) + .containsEntry("bar", controlBucket); + assertThat(proctorResult.getAllocations()).containsOnlyKeys("foo", "bar"); + assertThat(proctorResult.getTestDefinitions()) + .containsOnlyKeys("foo", "bar") + .containsEntry("foo", testDefinition1) + .containsEntry("bar", testDefinition2); + assertThat(proctorResult.getProperties()) + .containsOnlyKeys("key1", "key2", "some.property", "another.property") + .containsEntry( + "key1", + PayloadProperty.builder().value(p1.get("key1")).testName("foo").build()) + .containsEntry( + "key2", + PayloadProperty.builder().value(p1.get("key2")).testName("foo").build()) + .containsEntry( + "some.property", + PayloadProperty.builder() + .value(p2.get("some.property")) + .testName("bar") + .build()) + .containsEntry( + "another.property", + PayloadProperty.builder() + .value(p2.get("another.property")) + .testName("bar") + .build()); + ; + } + + @Test + public void testDetermineTestGroups_withPropertyAndPriority() throws JsonProcessingException { + final JsonNode p1 = new ObjectMapper().readTree("{\"key1\": 1, \"key2\": \"abc\"}"); + final JsonNode p2 = + new ObjectMapper() + .readTree( + "{\"key1\": 2, \"some.property\": {}, \"another.property\": [\"abc\"]}"); + final JsonNode p3 = new ObjectMapper().readTree("{\"some.property\": {\"sub\": 123}}"); + final TestBucket inactiveBucket = new TestBucket("inactive", -1, "", new Payload(p3)); + final TestBucket controlBucket = new TestBucket("control", 0, "", new Payload(p2)); + final TestBucket activeBucket = new TestBucket("active", 1, "", new Payload(p1)); + final Allocation allocation1 = new Allocation("", ImmutableList.of(new Range(1, 1.0))); + final Allocation allocation2 = new Allocation("", ImmutableList.of(new Range(0, 1.0))); + final Allocation allocation3 = new Allocation("", ImmutableList.of(new Range(-1, 1.0))); + + final ConsumableTestDefinition testDefinition1 = + ConsumableTestDefinition.fromTestDefinition( + TestDefinition.builder() + .setSalt("&X") + .setTestType(TestType.ANONYMOUS_USER) + .addBuckets(inactiveBucket, controlBucket, activeBucket) + .addAllocations(allocation1) + .setPayloadExperimentConfig( + PayloadExperimentConfig.builder() + .namespaces(ImmutableList.of("foobar")) + .priority("1") + .build()) + .build()); + final ConsumableTestDefinition testDefinition2 = + ConsumableTestDefinition.fromTestDefinition( + TestDefinition.builder() + .setSalt("&Y") + .setTestType(TestType.ANONYMOUS_USER) + .addBuckets(inactiveBucket, controlBucket, activeBucket) + .addAllocations(allocation2) + .setPayloadExperimentConfig( + PayloadExperimentConfig.builder() + .namespaces(ImmutableList.of("foobar")) + .priority("2") + .build()) + .build()); + + final ConsumableTestDefinition testDefinition3 = + ConsumableTestDefinition.fromTestDefinition( + TestDefinition.builder() + .setSalt("&Y") + .setTestType(TestType.ANONYMOUS_USER) + .addBuckets(inactiveBucket, controlBucket, activeBucket) + .addAllocations(allocation3) + .setPayloadExperimentConfig( + PayloadExperimentConfig.builder() + .namespaces(ImmutableList.of("foobar")) + .priority("3") + .build()) + .build()); + + final Map tests = + ImmutableMap.of( + "foo", testDefinition1, + "bar", testDefinition2, + "baz", testDefinition3); + final TestMatrixArtifact matrix = new TestMatrixArtifact(); + matrix.setTests(tests); + matrix.setAudit(new Audit()); + + final Proctor proctor = + Proctor.construct( + matrix, + ProctorLoadResult.emptyResult(), + RuleEvaluator.defaultFunctionMapperBuilder().build()); + + final ProctorResult proctorResult = + proctor.determineTestGroups( + Identifiers.of(TestType.ANONYMOUS_USER, "cookie"), + Collections.emptyMap(), + ForceGroupsOptions.builder().build(), + Collections.emptyList()); + + assertThat(proctorResult.getBuckets()) + .containsOnlyKeys("foo", "bar", "baz") + .containsEntry("foo", activeBucket) + .containsEntry("bar", controlBucket) + .containsEntry("baz", inactiveBucket); + assertThat(proctorResult.getAllocations()).containsOnlyKeys("foo", "bar", "baz"); + assertThat(proctorResult.getTestDefinitions()) + .containsOnlyKeys("foo", "bar", "baz") + .containsEntry("foo", testDefinition1) + .containsEntry("bar", testDefinition2) + .containsEntry("baz", testDefinition3); + assertThat(proctorResult.getProperties()) + .containsOnlyKeys("key1", "key2", "some.property", "another.property") + .containsEntry( + "key1", + PayloadProperty.builder() + .value(new ObjectMapper().readTree("2")) + .testName("bar") + .build()) + .containsEntry( + "key2", + PayloadProperty.builder() + .value(new ObjectMapper().readTree("\"abc\"")) + .testName("foo") + .build()) + .containsEntry( + "some.property", + PayloadProperty.builder() + .value(new ObjectMapper().readTree("{\"sub\": 123}")) + .testName("baz") + .build()) + .containsEntry( + "another.property", + PayloadProperty.builder() + .value(new ObjectMapper().readTree("[\"abc\"]")) + .testName("bar") + .build()); + ; + } + private static TestMatrixArtifact createTestMatrixWithOneRandomTest(final String testName) { final TestMatrixArtifact matrix = new TestMatrixArtifact(); final ConsumableTestDefinition testDefinition = new ConsumableTestDefinition(); diff --git a/proctor-common/src/test/java/com/indeed/proctor/common/dynamic/TestNamespacesFilter.java b/proctor-common/src/test/java/com/indeed/proctor/common/dynamic/TestNamespacesFilter.java new file mode 100644 index 000000000..748da5ad9 --- /dev/null +++ b/proctor-common/src/test/java/com/indeed/proctor/common/dynamic/TestNamespacesFilter.java @@ -0,0 +1,89 @@ +package com.indeed.proctor.common.dynamic; + +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.indeed.proctor.common.Serializers; +import com.indeed.proctor.common.model.ConsumableTestDefinition; +import com.indeed.proctor.common.model.PayloadExperimentConfig; +import com.indeed.proctor.common.model.TestType; +import org.junit.Test; + +import java.util.List; + +import static java.util.Collections.EMPTY_LIST; +import static java.util.Collections.emptyList; +import static java.util.Collections.emptyMap; +import static java.util.Collections.emptySet; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.junit.Assert.assertEquals; + +public class TestNamespacesFilter { + private static final ObjectMapper OBJECT_MAPPER = Serializers.lenient(); + + @Test + public void testMatchEmptyNamespaces() { + assertThatThrownBy(() -> new NamespacesFilter(emptySet())) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("namespaces should be non-empty string list."); + } + + @Test + public void testMatchSingleNamespaces() { + final NamespacesFilter filter = new NamespacesFilter(ImmutableSet.of("test")); + + assertFilterMatchesEquals(true, filter, ImmutableList.of("test")); + assertFilterMatchesEquals(true, filter, ImmutableList.of("foo", "test")); + + assertFilterMatchesEquals(false, filter, emptyList()); + assertFilterMatchesEquals(false, filter, ImmutableList.of("foo_test")); + } + + @Test + public void testMatchNamespaces() { + final NamespacesFilter filter = new NamespacesFilter(ImmutableSet.of("test1", "test2")); + + assertFilterMatchesEquals(true, filter, ImmutableList.of("test1")); + assertFilterMatchesEquals(true, filter, ImmutableList.of("test2")); + assertFilterMatchesEquals(true, filter, ImmutableList.of("test1", "test2")); + assertFilterMatchesEquals(true, filter, ImmutableList.of("test2", "test1")); + assertFilterMatchesEquals(true, filter, ImmutableList.of("foo", "test1", "test2")); + + assertFilterMatchesEquals(false, filter, emptyList()); + } + + private void assertFilterMatchesEquals( + final boolean expected, + final NamespacesFilter filter, + final List testNamespaces) { + final ConsumableTestDefinition definition = + new ConsumableTestDefinition( + "", + null, + TestType.EMAIL_ADDRESS, + null, + emptyList(), + emptyList(), + false, + emptyMap(), + null, + EMPTY_LIST); + definition.setPayloadExperimentConfig( + PayloadExperimentConfig.builder().namespaces(testNamespaces).build()); + + assertEquals(expected, filter.matches("", definition)); + assertEquals(expected, definition.getDynamic()); + } + + @Test + public void testSerializeMetaTagsFilter() throws JsonProcessingException { + final NamespacesFilter filter = new NamespacesFilter(ImmutableSet.of("test1", "test2")); + + final String serializedFilter = OBJECT_MAPPER.writeValueAsString(filter); + + assertEquals( + "{\"type\":\"namespaces_filter\",\"namespaces\":[\"test1\",\"test2\"]}", + serializedFilter); + } +} diff --git a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java index 7588867a3..8238c3f05 100644 --- a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java +++ b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/AbstractGroups.java @@ -1,7 +1,10 @@ package com.indeed.proctor.consumer; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.base.Strings; import com.google.common.collect.Maps; +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; @@ -39,6 +42,7 @@ */ public abstract class AbstractGroups { private static final Logger LOGGER = LogManager.getLogger(AbstractGroups.class); + private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper(); private final ProctorResult proctorResult; // Option using injected Observer @@ -319,6 +323,23 @@ final TestBucket getTestBucketWithValue(final String testName, final int bucketV return getTestBucketWithValueOptional(proctorResult, testName, bucketValue).orElse(null); } + public @Nullable JsonNode getProperty(final String propertyName) { + final Optional payloadProperty = + Optional.ofNullable(proctorResult.getProperties().get(propertyName)); + + payloadProperty.ifPresent(p -> markTestUsed(p.getTestName())); + + return payloadProperty.map(PayloadProperty::getValue).orElse(null); + } + + public @Nullable T getProperty(final String propertyName, final Class propertyClazz) { + try { + return OBJECT_MAPPER.convertValue(getProperty(propertyName), propertyClazz); + } catch (final IllegalArgumentException e) { + return null; + } + } + /** @return a comma-separated String of {testname}-{active-bucket-name} for ALL tests */ public String toLongString() { if (isEmpty()) { @@ -469,6 +490,10 @@ protected final List getLoggingTestNames() { proctorResult.getTestDefinitions(); // following lines should preserve the order in the map to ensure logging values are stable final Map buckets = proctorResult.getBuckets(); + final Set winningPayloadExperiments = + proctorResult.getProperties().values().stream() + .map(PayloadProperty::getTestName) + .collect(Collectors.toSet()); return buckets.keySet().stream() .filter( testName -> { @@ -478,6 +503,7 @@ protected final List 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 @@ -547,6 +573,21 @@ private boolean loggableAllocation(final String testName) { return loggableAllocation(testName, td, proctorResult); } + private boolean loggablePayloadExperiment( + final String testName, final Set 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 validPayloadExperiments) { + return td == null + || td.getPayloadExperimentConfig() == null + || validPayloadExperiments.contains(testName); + } + public static boolean loggableAllocation( final String testName, final ConsumableTestDefinition td, @@ -666,7 +707,8 @@ public ProctorResult getAsProctorResult() { (SortedMap) proctorResult.getAllocations()), Collections.unmodifiableMap(proctorResult.getTestDefinitions()), proctorResult.getIdentifiers(), - Collections.unmodifiableMap(proctorResult.getInputContext())); + Collections.unmodifiableMap(proctorResult.getInputContext()), + Collections.unmodifiableMap(proctorResult.getProperties())); } /** diff --git a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/ProctorGroupsWriter.java b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/ProctorGroupsWriter.java index 6380f74ba..838c754fd 100644 --- a/proctor-consumer/src/main/java/com/indeed/proctor/consumer/ProctorGroupsWriter.java +++ b/proctor-consumer/src/main/java/com/indeed/proctor/consumer/ProctorGroupsWriter.java @@ -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; @@ -30,7 +31,7 @@ public class ProctorGroupsWriter { // for historic reasons, allow more than one formatter private final TestGroupFormatter[] formatters; private final BiPredicate testFilter; - + private Set winningPayloadExperiment; /** * @param groupsSeparator how elements in logged string are separated * @param formatters ideally only one, all groups will be logged for all formatters @@ -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; } @@ -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); diff --git a/proctor-consumer/src/test/java/com/indeed/proctor/consumer/ProctorGroupStubber.java b/proctor-consumer/src/test/java/com/indeed/proctor/consumer/ProctorGroupStubber.java index 94e0fdc2b..460fbade0 100644 --- a/proctor-consumer/src/test/java/com/indeed/proctor/consumer/ProctorGroupStubber.java +++ b/proctor-consumer/src/test/java/com/indeed/proctor/consumer/ProctorGroupStubber.java @@ -1,5 +1,8 @@ package com.indeed.proctor.consumer; +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.ObjectMapper; +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; @@ -27,7 +30,42 @@ 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; + + static { + try { + GROUP_1_BUCKET_PROPERTY_PAYLOAD = + new TestBucket( + "group1", + 2, + "group1", + new Payload( + new ObjectMapper() + .readTree( + "{\"some.property\": {}, \"another.property\": [\"abc\"]}"))); + } catch (final JsonProcessingException e) { + throw new RuntimeException(e); + } + } + public static final TestBucket FALLBACK_TEST_BUCKET = new TestBucket( "fallbackBucket", @@ -42,7 +80,8 @@ public class ProctorGroupStubber { public static class ProctorResultStubBuilder { private final Map definitions = new TreeMap<>(); - private final Map resolvedBuckets = new TreeMap<>(); + private final Map resolvedBuckets = new TreeMap<>(); + private final Map properties = new TreeMap<>(); public ProctorResultStubBuilder withStubTest( final StubTest stubTest, @@ -69,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); @@ -77,17 +116,62 @@ public ProctorResultStubBuilder withStubTest( return this; } + public ProctorResultStubBuilder withStubProperty( + final StubTest stubTest, @Nullable final TestBucket resolved) { + if (resolved != null) { + resolvedBuckets.put(stubTest.getName(), resolved); + assert resolved.getPayload() != null; + assert resolved.getPayload().getJson() != null; + resolved.getPayload() + .getJson() + .fields() + .forEachRemaining( + field -> + properties.put( + field.getKey(), + PayloadProperty.builder() + .testName(stubTest.getName()) + .value(field.getValue()) + .build())); + definitions.put( + stubTest.getName(), stubDefinitionWithVersion(false, "v1", resolved)); + } + 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, @@ -96,7 +180,8 @@ public ProctorResult build() { e.getValue().getValue(), 1.0)), "#A1"))), - definitions); + definitions, + properties); } } @@ -150,7 +235,8 @@ public enum StubTest implements com.indeed.proctor.consumer.Test { MISSING_DEFINITION_TEST("no_definition_tst", -1), SILENT_TEST("silent_tst", -1), - NO_BUCKETS_WITH_FALLBACK_TEST("nobucketfallbacktst", -1); + NO_BUCKETS_WITH_FALLBACK_TEST("nobucketfallbacktst", -1), + PROPERTY_TEST("propertytest", -1); private final String name; private final int fallbackValue; diff --git a/proctor-consumer/src/test/java/com/indeed/proctor/consumer/ProctorGroupsWriterTest.java b/proctor-consumer/src/test/java/com/indeed/proctor/consumer/ProctorGroupsWriterTest.java index ca7dc529b..b9e5f3299 100644 --- a/proctor-consumer/src/test/java/com/indeed/proctor/consumer/ProctorGroupsWriterTest.java +++ b/proctor-consumer/src/test/java/com/indeed/proctor/consumer/ProctorGroupsWriterTest.java @@ -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; @@ -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"); @@ -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 ALLOCATIONS = new ImmutableSortedMap.Builder(Ordering.natural()) @@ -54,8 +65,35 @@ 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 DEFINITIONS = ImmutableMap.builder() .put(INACTIVE_TEST_NAME, stubDefinition(INACTIVE_BUCKET)) @@ -63,10 +101,29 @@ public class ProctorGroupsWriterTest { .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 PROPERTIES; + + static { + try { + PROPERTIES = + ImmutableMap.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() { @@ -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( @@ -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(",")); } @@ -116,7 +175,8 @@ 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) @@ -124,7 +184,8 @@ public void testCustomWriter() { .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 diff --git a/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java b/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java index 303b70e6f..983c32ebc 100644 --- a/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java +++ b/proctor-consumer/src/test/java/com/indeed/proctor/consumer/TestAbstractGroups.java @@ -1,5 +1,8 @@ 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.ImmutableSet; import com.google.common.collect.Lists; import com.google.common.collect.Sets; @@ -21,14 +24,17 @@ import static com.indeed.proctor.consumer.ProctorGroupStubber.FALLBACK_NOPAYLOAD_BUCKET; import static com.indeed.proctor.consumer.ProctorGroupStubber.FALLBACK_TEST_BUCKET; import static com.indeed.proctor.consumer.ProctorGroupStubber.GROUP_1_BUCKET; +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; 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.PROPERTY_TEST; import static com.indeed.proctor.consumer.ProctorGroupStubber.StubTest.SUPPRESS_LOGGING_TST; import static java.util.Collections.emptyMap; import static java.util.Collections.singleton; @@ -93,6 +99,7 @@ public void setUp() { ProctorGroupStubber.StubTest.MISSING_DEFINITION_TEST, GROUP_1_BUCKET, (ConsumableTestDefinition) null) + .withStubProperty(PROPERTY_TEST, GROUP_1_BUCKET_PROPERTY_PAYLOAD) .build(); observer = new TestMarkingObserver(proctorResult); sampleGroups = new AbstractGroups(proctorResult, observer) {}; @@ -163,6 +170,16 @@ public void testGetPayload() { assertThat(emptyGroup.getPayload("notexist")).isEqualTo(Payload.EMPTY_PAYLOAD); } + @Test + public void testGetProperty() throws JsonProcessingException { + final ObjectMapper ob = new ObjectMapper(); + assertThat(sampleGroups.getProperty("another.property", String[].class)) + .isEqualTo(new String[] {"abc"}); + assertThat(sampleGroups.getProperty("another.property")) + .isEqualTo(ob.readTree("[\"abc\"]")); + assertThat(sampleGroups.getProperty("some.property")).isEqualTo(ob.readTree("{}")); + } + @Test public void testIsEmpty() { assertThat(emptyGroup.isEmpty()).isTrue(); @@ -174,7 +191,7 @@ public void testToLongString() { assertThat(emptyGroup.toLongString()).isEmpty(); assertThat(sampleGroups.toLongString()) .isEqualTo( - "abtst-group1,bgtst-control,btntst-inactive,groupwithfallbacktst-group1,no_definition_tst-group1,suppress_logging_example_tst-control"); + "abtst-group1,bgtst-control,btntst-inactive,groupwithfallbacktst-group1,no_definition_tst-group1,propertytest-group1,suppress_logging_example_tst-control"); } @Test @@ -220,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 = @@ -362,7 +416,7 @@ public void testAppendTestGroupsWithAllocations() { @Test public void testAppendTestGroups() { - StringBuilder builder = new StringBuilder(); + final StringBuilder builder = new StringBuilder(); sampleGroups.appendTestGroups(builder, ','); assertThat(builder.toString().split(",")) .containsExactlyInAnyOrder( @@ -378,11 +432,12 @@ public void testGetJavaScriptConfig() { assertThat(emptyGroup.getJavaScriptConfig()).hasSize(0); assertThat(sampleGroups.getJavaScriptConfig()) - .hasSize(5) + .hasSize(6) .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(PROPERTY_TEST.getName(), 2) .containsEntry(SUPPRESS_LOGGING_TST.getName(), 0); }