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

Commit

Permalink
Merge pull request #171 from indeedeng/EXP-660-Payload-Experiment-MVP
Browse files Browse the repository at this point in the history
EXP-660: Payload Experiment MVP
  • Loading branch information
zacharygoodwin authored Jul 1, 2024
2 parents 1950c8a + c35c9e0 commit 510417d
Show file tree
Hide file tree
Showing 17 changed files with 883 additions and 27 deletions.
1 change: 1 addition & 0 deletions gradle/libs.versions.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
5 changes: 5 additions & 0 deletions proctor-common/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -323,6 +325,7 @@ public ProctorResult determineTestGroups(
// this method
final SortedMap<String, TestBucket> testGroups = new TreeMap<>();
final SortedMap<String, Allocation> testAllocations = new TreeMap<>();
final SortedMap<String, PayloadProperty> testProperties = new TreeMap<>();

final List<String> filteredEvaluationOrder;
if (determineAllTests) {
Expand Down Expand Up @@ -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());
Expand All @@ -424,7 +430,8 @@ public ProctorResult determineTestGroups(
testAllocations,
testDefinitions,
identifiers,
inputContext);
inputContext,
testProperties);

if (resultReporter != null) {
resultReporter.reportMetrics(result, invalidIdentifierCount);
Expand Down Expand Up @@ -515,4 +522,57 @@ private boolean isIncognitoEnabled(@Nonnull final Map<String, Object> 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<String, PayloadProperty> 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<String, com.fasterxml.jackson.databind.JsonNode> field,
final String testName,
final Map<String, PayloadProperty> 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);
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ public class ProctorResult {
emptySortedMap(),
emptyMap(),
new Identifiers(emptyMap()),
emptyMap(),
emptyMap());

private final String matrixVersion;
Expand All @@ -40,6 +41,8 @@ public class ProctorResult {
/** maps from testname to TestDefinition */
@Nonnull private final Map<String, ConsumableTestDefinition> testDefinitions;

@Nonnull private final Map<String, PayloadProperty> properties;

private final Identifiers identifiers;

private final Map<String, Object> inputContext;
Expand Down Expand Up @@ -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(
Expand All @@ -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<String, TestBucket> buckets,
@Nonnull final Map<String, Allocation> allocations,
// allowing null for historical reasons
@Nullable final Map<String, ConsumableTestDefinition> testDefinitions,
@Nonnull final Map<String, PayloadProperty> 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.
*
Expand Down Expand Up @@ -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,
Expand All @@ -140,13 +180,43 @@ public ProctorResult(
@Nonnull final Map<String, ConsumableTestDefinition> testDefinitions,
@Nonnull final Identifiers identifiers,
@Nonnull final Map<String, Object> 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<String, TestBucket> buckets,
@Nonnull final SortedMap<String, Allocation> allocations,
@Nonnull final Map<String, ConsumableTestDefinition> testDefinitions,
@Nonnull final Identifiers identifiers,
@Nonnull final Map<String, Object> inputContext,
@Nonnull final Map<String, PayloadProperty> properties) {
this.matrixVersion = matrixVersion;
this.buckets = buckets;
this.allocations = allocations;
this.testDefinitions = testDefinitions;
this.identifiers = identifiers;
this.inputContext = inputContext;
this.hasLoggedTests = new HashSet<>();
this.properties = properties;
}

/**
Expand All @@ -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")
Expand Down Expand Up @@ -198,6 +269,11 @@ public Map<String, Object> getInputContext() {
return inputContext;
}

@Nonnull
public Map<String, PayloadProperty> getProperties() {
return properties;
}

public boolean markTestAsLogged(final String test) {
return this.hasLoggedTests.add(test);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ public class DynamicFilters implements JsonSerializable {
TestNamePrefixFilter.class,
TestNamePatternFilter.class,
MetaTagsFilter.class,
MatchAllFilter.class));
MatchAllFilter.class,
NamespacesFilter.class));

private final List<DynamicFilter> filters;

Expand Down
Original file line number Diff line number Diff line change
@@ -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<String> namespaces;

public NamespacesFilter(@JsonProperty("namespaces") final Set<String> namespaces) {
Preconditions.checkArgument(
!CollectionUtils.isEmpty(namespaces),
"namespaces should be non-empty string list.");
this.namespaces = ImmutableSet.copyOf(namespaces);
}

@JsonProperty("namespaces")
public Set<String> 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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ public class ConsumableTestDefinition {
private boolean containsUnitlessAllocation = false;
private boolean forceLogging = false;

@Nullable PayloadExperimentConfig payloadExperimentConfig;

public ConsumableTestDefinition() {
/* intentionally empty */
}
Expand Down Expand Up @@ -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;
Expand All @@ -157,6 +160,7 @@ private ConsumableTestDefinition(
this.enableUnitlessAllocations = enableUnitlessAllocations;
this.containsUnitlessAllocation = containsUnitlessAllocation;
this.forceLogging = forceLogging;
this.payloadExperimentConfig = payloadExperimentConfig;
}

@Nonnull
Expand Down Expand Up @@ -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<String, Object> specialConstants = td.getSpecialConstants();
Expand Down Expand Up @@ -354,6 +367,7 @@ public static ConsumableTestDefinition fromTestDefinition(@Nonnull final TestDef
td.getEvaluateForIncognitoUsers(),
td.getEnableUnitlessAllocations(),
containsUnitlessAllocation,
td.getForceLogging());
td.getForceLogging(),
td.getPayloadExperimentConfig());
}
}
Loading

0 comments on commit 510417d

Please sign in to comment.