From bbca4b5adca2de915ddd083fad2e1ff4f94b5cb1 Mon Sep 17 00:00:00 2001 From: Zack Goodwin Date: Mon, 3 Jun 2024 12:31:38 -0400 Subject: [PATCH] PROC-1522: Use Priorty for populating aggregate payload properties --- .../com/indeed/proctor/common/Proctor.java | 47 +++++-- .../indeed/proctor/common/TestProctor.java | 118 ++++++++++++++++++ 2 files changed, 157 insertions(+), 8 deletions(-) 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 2bebbd64..92bf8e97 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 @@ -12,6 +12,7 @@ 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; @@ -403,7 +404,7 @@ public ProctorResult determineTestGroups( if (chooseResult.getTestBucket() != null) { testGroups.put(testName, chooseResult.getTestBucket()); if (testChooser.getTestDefinition().getPayloadExperimentConfig() != null) { - testProperties.putAll(getProperties(testName, chooseResult.getTestBucket())); + populateProperties(testName, chooseResult.getTestBucket(), testProperties); } } if (chooseResult.getAllocation() != null) { @@ -522,23 +523,53 @@ private boolean isIncognitoEnabled(@Nonnull final Map inputConte .orElse(false); } - private Map getProperties( - final String testName, final TestBucket testBucket) { + private void populateProperties( + final String testName, + final TestBucket testBucket, + final Map testProperties) { final Payload payload = testBucket.getPayload(); if (payload != null && payload.getJson() != null) { - final SortedMap testProperties = new TreeMap<>(); payload.getJson() .fields() .forEachRemaining( - field -> + field -> { + final PayloadProperty curr = testProperties.get(field.getKey()); + if (curr == null) { testProperties.put( field.getKey(), PayloadProperty.builder() .value(field.getValue()) .testName(testName) - .build())); - return testProperties; + .build()); + } else { + final PayloadExperimentConfig currPayloadConfig = + testChoosers + .get(curr.getTestName()) + .getTestDefinition() + .getPayloadExperimentConfig(); + final PayloadExperimentConfig newPayloadConfig = + testChoosers + .get(testName) + .getTestDefinition() + .getPayloadExperimentConfig(); + if (currPayloadConfig != null + && currPayloadConfig.getPriority() != null + && newPayloadConfig != null + && newPayloadConfig.getPriority() != null + && currPayloadConfig + .getPriority() + .compareTo( + newPayloadConfig.getPriority()) + < 0) { + testProperties.put( + field.getKey(), + PayloadProperty.builder() + .value(field.getValue()) + .testName(testName) + .build()); + } + } + }); } - return Collections.emptyMap(); } } 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 c7065177..641a08d3 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 @@ -1010,6 +1010,124 @@ public void testDetermineTestGroups_withProperty() throws JsonProcessingExceptio ; } + @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();