From cd606b8647f5cf76c16c4d01cd35dd0831069261 Mon Sep 17 00:00:00 2001 From: wangjoshuah Date: Thu, 10 Aug 2017 13:55:49 -0700 Subject: [PATCH 01/10] abstract sending the impression event --- .../src/main/java/com/optimizely/ab/Optimizely.java | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/core-api/src/main/java/com/optimizely/ab/Optimizely.java b/core-api/src/main/java/com/optimizely/ab/Optimizely.java index 0362c4be3..4981935b4 100644 --- a/core-api/src/main/java/com/optimizely/ab/Optimizely.java +++ b/core-api/src/main/java/com/optimizely/ab/Optimizely.java @@ -175,6 +175,16 @@ Variation activate(@Nonnull ProjectConfig projectConfig, return null; } + sendImpression(projectConfig, experiment, userId, filteredAttributes, variation); + + return variation; + } + + private void sendImpression(@Nonnull ProjectConfig projectConfig, + @Nonnull Experiment experiment, + @Nonnull String userId, + Map filteredAttributes, + Variation variation) { if (experiment.isRunning()) { LogEvent impressionEvent = eventBuilder.createImpressionEvent( projectConfig, @@ -196,8 +206,6 @@ Variation activate(@Nonnull ProjectConfig projectConfig, } else { logger.info("Experiment has \"Launched\" status so not dispatching event during activation."); } - - return variation; } //======== track calls ========// From 737b5f750409ba81e89b93cf9735f79707bd2829 Mon Sep 17 00:00:00 2001 From: wangjoshuah Date: Thu, 10 Aug 2017 14:54:31 -0700 Subject: [PATCH 02/10] support isFeatureEnabled API. add variationIdToExperiment mapping in project config --- .../java/com/optimizely/ab/Optimizely.java | 32 +++++++++++++++++-- .../optimizely/ab/config/ProjectConfig.java | 15 +++++++++ 2 files changed, 44 insertions(+), 3 deletions(-) diff --git a/core-api/src/main/java/com/optimizely/ab/Optimizely.java b/core-api/src/main/java/com/optimizely/ab/Optimizely.java index 4981935b4..5e65a5fce 100644 --- a/core-api/src/main/java/com/optimizely/ab/Optimizely.java +++ b/core-api/src/main/java/com/optimizely/ab/Optimizely.java @@ -183,8 +183,8 @@ Variation activate(@Nonnull ProjectConfig projectConfig, private void sendImpression(@Nonnull ProjectConfig projectConfig, @Nonnull Experiment experiment, @Nonnull String userId, - Map filteredAttributes, - Variation variation) { + @Nonnull Map filteredAttributes, + @Nonnull Variation variation) { if (experiment.isRunning()) { LogEvent impressionEvent = eventBuilder.createImpressionEvent( projectConfig, @@ -324,7 +324,33 @@ public void track(@Nonnull String eventName, public @Nullable Boolean isFeatureEnabled(@Nonnull String featureKey, @Nonnull String userId, @Nonnull Map attributes) { - return getFeatureVariableBoolean(featureKey, "", userId, attributes); + FeatureFlag featureFlag = projectConfig.getFeatureKeyMapping().get(featureKey); + if (featureFlag == null) { + logger.info("No feature flag was found for key \"" + featureKey + "\"."); + return null; + } + + Map filteredAttributes = filterAttributes(projectConfig, attributes); + + Variation variation = decisionService.getVariationForFeature(featureFlag, userId, filteredAttributes); + + if (variation != null) { + Experiment experiment = projectConfig.getExperimentForVariationId(variation.getId()); + if (experiment != null) { + sendImpression( + projectConfig, + experiment, + userId, + filteredAttributes, + variation); + } + logger.info("Feature flag \"" + featureKey + "\" is enabled for user \"" + userId + "\"."); + return true; + } + else { + logger.info("Feature flag \"" + featureKey + "\" is not enabled for user \"" + userId + "\"."); + return false; + } } /** diff --git a/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java b/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java index 256472461..ffb891e29 100644 --- a/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java +++ b/core-api/src/main/java/com/optimizely/ab/config/ProjectConfig.java @@ -20,9 +20,11 @@ import com.optimizely.ab.config.audience.Audience; import com.optimizely.ab.config.audience.Condition; +import javax.annotation.Nullable; import javax.annotation.concurrent.Immutable; import java.util.ArrayList; import java.util.Collections; +import java.util.HashMap; import java.util.List; import java.util.Map; @@ -80,6 +82,7 @@ public String toString() { // other mappings private final Map> liveVariableIdToExperimentsMapping; private final Map> variationToLiveVariableUsageInstanceMapping; + private final Map variationIdToExperimentMapping; // v2 constructor public ProjectConfig(String accountId, String projectId, String version, String revision, List groups, @@ -146,6 +149,14 @@ public ProjectConfig(String accountId, allExperiments.addAll(aggregateGroupExperiments(groups)); this.experiments = Collections.unmodifiableList(allExperiments); + Map variationIdToExperimentMap = new HashMap(); + for (Experiment experiment : this.experiments) { + for (Variation variation: experiment.getVariations()) { + variationIdToExperimentMap.put(variation.getId(), experiment); + } + } + this.variationIdToExperimentMapping = Collections.unmodifiableMap(variationIdToExperimentMap); + // generate the name mappers this.attributeKeyMapping = ProjectConfigUtils.generateNameMapping(attributes); this.eventNameMapping = ProjectConfigUtils.generateNameMapping(this.events); @@ -172,6 +183,10 @@ public ProjectConfig(String accountId, } } + public @Nullable Experiment getExperimentForVariationId(String variationId) { + return this.variationIdToExperimentMapping.get(variationId); + } + private List aggregateGroupExperiments(List groups) { List groupExperiments = new ArrayList(); for (Group group : groups) { From 5683338938ceba2aa7d594337bc83c9060f4fdd8 Mon Sep 17 00:00:00 2001 From: wangjoshuah Date: Fri, 11 Aug 2017 10:36:04 -0700 Subject: [PATCH 03/10] update log messages --- core-api/src/main/java/com/optimizely/ab/Optimizely.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/core-api/src/main/java/com/optimizely/ab/Optimizely.java b/core-api/src/main/java/com/optimizely/ab/Optimizely.java index 5e65a5fce..a39f63214 100644 --- a/core-api/src/main/java/com/optimizely/ab/Optimizely.java +++ b/core-api/src/main/java/com/optimizely/ab/Optimizely.java @@ -344,11 +344,15 @@ public void track(@Nonnull String eventName, filteredAttributes, variation); } - logger.info("Feature flag \"" + featureKey + "\" is enabled for user \"" + userId + "\"."); + else { + logger.info("The user \"" + userId + + "\" is not being experimented on in feature \"" + featureKey + "\"."); + } + logger.info("Feature \"" + featureKey + "\" is enabled for user \"" + userId + "\"."); return true; } else { - logger.info("Feature flag \"" + featureKey + "\" is not enabled for user \"" + userId + "\"."); + logger.info("Feature \"" + featureKey + "\" is not enabled for user \"" + userId + "\"."); return false; } } From 12b5a7550c0eca28b66b77de7b4ea320e35c7db9 Mon Sep 17 00:00:00 2001 From: wangjoshuah Date: Fri, 11 Aug 2017 11:06:51 -0700 Subject: [PATCH 04/10] add comment describing why experiment would be null -- in a rollout --- core-api/src/main/java/com/optimizely/ab/Optimizely.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core-api/src/main/java/com/optimizely/ab/Optimizely.java b/core-api/src/main/java/com/optimizely/ab/Optimizely.java index a39f63214..e9791c91f 100644 --- a/core-api/src/main/java/com/optimizely/ab/Optimizely.java +++ b/core-api/src/main/java/com/optimizely/ab/Optimizely.java @@ -336,7 +336,7 @@ public void track(@Nonnull String eventName, if (variation != null) { Experiment experiment = projectConfig.getExperimentForVariationId(variation.getId()); - if (experiment != null) { + if (experiment != null) { // user is in a rollout not an experiment sendImpression( projectConfig, experiment, From 9c3088b07523e5cda1815d7e7fe1aac5cd6804f7 Mon Sep 17 00:00:00 2001 From: wangjoshuah Date: Fri, 11 Aug 2017 16:02:04 -0700 Subject: [PATCH 05/10] change behavior of isFeatureEnabled APIs --- .../src/main/java/com/optimizely/ab/Optimizely.java | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/core-api/src/main/java/com/optimizely/ab/Optimizely.java b/core-api/src/main/java/com/optimizely/ab/Optimizely.java index e9791c91f..ed3c3aa72 100644 --- a/core-api/src/main/java/com/optimizely/ab/Optimizely.java +++ b/core-api/src/main/java/com/optimizely/ab/Optimizely.java @@ -301,10 +301,9 @@ public void track(@Nonnull String eventName, * @param userId The ID of the user. * @return True if the feature is enabled. * False if the feature is disabled. - * Will always return True if toggling the feature is disabled. - * Will return Null if the feature is not found. + * False if the feature is not found. */ - public @Nullable Boolean isFeatureEnabled(@Nonnull String featureKey, + public @Nonnull Boolean isFeatureEnabled(@Nonnull String featureKey, @Nonnull String userId) { return isFeatureEnabled(featureKey, userId, Collections.emptyMap()); } @@ -318,16 +317,15 @@ public void track(@Nonnull String eventName, * @param attributes The user's attributes. * @return True if the feature is enabled. * False if the feature is disabled. - * Will always return True if toggling the feature is disabled. - * Will return Null if the feature is not found. + * False if the feature is not found. */ - public @Nullable Boolean isFeatureEnabled(@Nonnull String featureKey, + public @Nonnull Boolean isFeatureEnabled(@Nonnull String featureKey, @Nonnull String userId, @Nonnull Map attributes) { FeatureFlag featureFlag = projectConfig.getFeatureKeyMapping().get(featureKey); if (featureFlag == null) { logger.info("No feature flag was found for key \"" + featureKey + "\"."); - return null; + return false; } Map filteredAttributes = filterAttributes(projectConfig, attributes); From dbab5e189c96009c6aae0b1a9cd1f03337dee4ba Mon Sep 17 00:00:00 2001 From: wangjoshuah Date: Fri, 11 Aug 2017 16:02:56 -0700 Subject: [PATCH 06/10] unit test to make sure isFeatureEnabled returns false when feature flag key is invalid --- .../com/optimizely/ab/OptimizelyTest.java | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java index e981704e4..58d627020 100644 --- a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java +++ b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java @@ -97,6 +97,7 @@ import static org.hamcrest.Matchers.hasEntry; import static org.hamcrest.Matchers.hasKey; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assume.assumeTrue; @@ -2371,6 +2372,41 @@ public void getFeatureVariableValueReturnsVariationValueWhenUserGetsBucketedToVa assertEquals(expectedValue, value); } + /** + * Verify {@link Optimizely#isFeatureEnabled(String, String)} calls into + * {@link Optimizely#isFeatureEnabled(String, String, Map)} and they both + * return False + * when the APIs are called with an feature Key that is not in the datafile. + * @throws Exception + */ + @Test + public void isFeatureEnabledReturnsFalseWhenFeatureFlagKeyIsInvalid() throws Exception { + + String invalidFeatureKey = "nonexistent feature key"; + + Optimizely spyOptimizely = spy(Optimizely.builder(validDatafile, mockEventHandler) + .withConfig(validProjectConfig) + .withDecisionService(mockDecisionService) + .build()); + + assertFalse(spyOptimizely.isFeatureEnabled(invalidFeatureKey, genericUserId)); + + logbackVerifier.expectMessage( + Level.INFO, + "No feature flag was found for key \"" + invalidFeatureKey + "\"." + ); + verify(spyOptimizely, times(1)).isFeatureEnabled( + eq(invalidFeatureKey), + eq(genericUserId), + eq(Collections.emptyMap()) + ); + verify(mockDecisionService, never()).getVariation( + any(Experiment.class), + anyString(), + anyMapOf(String.class, String.class)); + verify(mockEventHandler, never()).dispatchEvent(any(LogEvent.class)); + } + //======== Helper methods ========// private Experiment createUnknownExperiment() { From 04ea4ebc4db262af5896aad88811d48ab8dd46e2 Mon Sep 17 00:00:00 2001 From: wangjoshuah Date: Fri, 11 Aug 2017 16:19:03 -0700 Subject: [PATCH 07/10] unit test to ensure isFeatureEnabled returns false when the user is not bucketed into any variation for the feature --- .../com/optimizely/ab/OptimizelyTest.java | 47 ++++++++++++++++++- 1 file changed, 46 insertions(+), 1 deletion(-) diff --git a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java index 58d627020..f6e28627c 100644 --- a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java +++ b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java @@ -23,6 +23,7 @@ import com.optimizely.ab.config.Attribute; import com.optimizely.ab.config.EventType; import com.optimizely.ab.config.Experiment; +import com.optimizely.ab.config.FeatureFlag; import com.optimizely.ab.config.LiveVariable; import com.optimizely.ab.config.LiveVariableUsageInstance; import com.optimizely.ab.config.ProjectConfig; @@ -2376,7 +2377,7 @@ public void getFeatureVariableValueReturnsVariationValueWhenUserGetsBucketedToVa * Verify {@link Optimizely#isFeatureEnabled(String, String)} calls into * {@link Optimizely#isFeatureEnabled(String, String, Map)} and they both * return False - * when the APIs are called with an feature Key that is not in the datafile. + * when the APIs are called with an feature key that is not in the datafile. * @throws Exception */ @Test @@ -2407,6 +2408,50 @@ public void isFeatureEnabledReturnsFalseWhenFeatureFlagKeyIsInvalid() throws Exc verify(mockEventHandler, never()).dispatchEvent(any(LogEvent.class)); } + /** + * Verify {@link Optimizely#isFeatureEnabled(String, String)} calls into + * {@link Optimizely#isFeatureEnabled(String, String, Map)} and they both + * return False + * when the user is not bucketed into any variation for the feature. + * @throws Exception + */ + @Test + public void isFeatureEnabledReturnsFalseWhenUserIsNotBucketedIntoAnyVariation() throws Exception { + assumeTrue(datafileVersion >= Integer.parseInt(ProjectConfig.Version.V4.toString())); + + String validFeatureKey = FEATURE_MULTI_VARIATE_FEATURE_KEY; + + Optimizely spyOptimizely = spy(Optimizely.builder(validDatafile, mockEventHandler) + .withConfig(validProjectConfig) + .withDecisionService(mockDecisionService) + .build()); + + doReturn(null).when(mockDecisionService).getVariationForFeature( + any(FeatureFlag.class), + anyString(), + anyMapOf(String.class, String.class) + ); + + assertFalse(spyOptimizely.isFeatureEnabled(validFeatureKey, genericUserId)); + + logbackVerifier.expectMessage( + Level.INFO, + "Feature \"" + validFeatureKey + + "\" is not enabled for user \"" + genericUserId + "\"." + ); + verify(spyOptimizely).isFeatureEnabled( + eq(validFeatureKey), + eq(genericUserId), + eq(Collections.emptyMap()) + ); + verify(mockDecisionService).getVariationForFeature( + eq(FEATURE_FLAG_MULTI_VARIATE_FEATURE), + eq(genericUserId), + eq(Collections.emptyMap()) + ); + verify(mockEventHandler, never()).dispatchEvent(any(LogEvent.class)); + } + //======== Helper methods ========// private Experiment createUnknownExperiment() { From b34a8b01afb106f8cbe95bd2462a5da39196b6b8 Mon Sep 17 00:00:00 2001 From: wangjoshuah Date: Fri, 11 Aug 2017 16:45:39 -0700 Subject: [PATCH 08/10] unit test for isFeatureEnabled to verify that no event is sent when the user is bucketed into a variation that is not part of an experiment for a feature --- .../com/optimizely/ab/OptimizelyTest.java | 51 +++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java index f6e28627c..88de576ae 100644 --- a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java +++ b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java @@ -37,6 +37,7 @@ import com.optimizely.ab.event.LogEvent; import com.optimizely.ab.event.internal.EventBuilder; import com.optimizely.ab.event.internal.EventBuilderV2; +import com.optimizely.ab.event.internal.payload.Feature; import com.optimizely.ab.internal.LogbackVerifier; import com.optimizely.ab.notification.NotificationListener; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; @@ -2452,6 +2453,56 @@ public void isFeatureEnabledReturnsFalseWhenUserIsNotBucketedIntoAnyVariation() verify(mockEventHandler, never()).dispatchEvent(any(LogEvent.class)); } + /** + * Verify {@link Optimizely#isFeatureEnabled(String, String)} calls into + * {@link Optimizely#isFeatureEnabled(String, String, Map)} and they both + * return True + * when the user is bucketed into a variation for the feature. + * An impression event should not be dispatched since the user was not bucketed into an Experiment. + * @throws Exception + */ + @Test + public void isFeatureEnabledReturnsTrueButDoesNotSendWhenUserIsBucketedIntoVariationWithoutExperiment() throws Exception { + assumeTrue(datafileVersion >= Integer.parseInt(ProjectConfig.Version.V4.toString())); + + String validFeatureKey = FEATURE_MULTI_VARIATE_FEATURE_KEY; + + Optimizely spyOptimizely = spy(Optimizely.builder(validDatafile, mockEventHandler) + .withConfig(validProjectConfig) + .withDecisionService(mockDecisionService) + .build()); + + doReturn(new Variation("variationId", "variationKey")).when(mockDecisionService).getVariationForFeature( + eq(FEATURE_FLAG_MULTI_VARIATE_FEATURE), + eq(genericUserId), + eq(Collections.emptyMap()) + ); + + assertTrue(spyOptimizely.isFeatureEnabled(validFeatureKey, genericUserId)); + + logbackVerifier.expectMessage( + Level.INFO, + "The user \"" + genericUserId + + "\" is not being experimented on in feature \"" + validFeatureKey + "\"." + ); + logbackVerifier.expectMessage( + Level.INFO, + "Feature \"" + validFeatureKey + + "\" is enabled for user \"" + genericUserId + "\"." + ); + verify(spyOptimizely).isFeatureEnabled( + eq(validFeatureKey), + eq(genericUserId), + eq(Collections.emptyMap()) + ); + verify(mockDecisionService).getVariationForFeature( + eq(FEATURE_FLAG_MULTI_VARIATE_FEATURE), + eq(genericUserId), + eq(Collections.emptyMap()) + ); + verify(mockEventHandler, never()).dispatchEvent(any(LogEvent.class)); + } + //======== Helper methods ========// private Experiment createUnknownExperiment() { From f272f73a2eba9b5c1336450a381cf0f5c5a44b89 Mon Sep 17 00:00:00 2001 From: wangjoshuah Date: Fri, 11 Aug 2017 17:23:25 -0700 Subject: [PATCH 09/10] add integration-ish test for isFeatureEnabled when bucketed into an experiment and make sure we send an impression event --- .../com/optimizely/ab/OptimizelyTest.java | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java index 88de576ae..92ffcddcc 100644 --- a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java +++ b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java @@ -2503,6 +2503,37 @@ public void isFeatureEnabledReturnsTrueButDoesNotSendWhenUserIsBucketedIntoVaria verify(mockEventHandler, never()).dispatchEvent(any(LogEvent.class)); } + /** Integration Test + * Verify {@link Optimizely#isFeatureEnabled(String, String, Map)} + * returns True + * when the user is bucketed into a variation for the feature. + * The user is also bucketed into an experiment, so we verify that an event is dispatched. + * @throws Exception + */ + @Test + public void isFeatureEnabledReturnsTrueAndDispatchesEventWhenUserIsBucketedIntoAnExperiment() throws Exception { + assumeTrue(datafileVersion >= Integer.parseInt(ProjectConfig.Version.V4.toString())); + + String validFeatureKey = FEATURE_MULTI_VARIATE_FEATURE_KEY; + + Optimizely optimizely = Optimizely.builder(validDatafile, mockEventHandler) + .withConfig(validProjectConfig) + .build(); + + assertTrue(optimizely.isFeatureEnabled( + validFeatureKey, + genericUserId, + Collections.singletonMap(ATTRIBUTE_HOUSE_KEY, AUDIENCE_GRYFFINDOR_VALUE) + )); + + logbackVerifier.expectMessage( + Level.INFO, + "Feature \"" + validFeatureKey + + "\" is enabled for user \"" + genericUserId + "\"." + ); + verify(mockEventHandler, times(1)).dispatchEvent(any(LogEvent.class)); + } + //======== Helper methods ========// private Experiment createUnknownExperiment() { From afe018d7c1d6ed47032d65f649ebf9e5d53e8f26 Mon Sep 17 00:00:00 2001 From: wangjoshuah Date: Mon, 14 Aug 2017 15:14:06 -0700 Subject: [PATCH 10/10] fix confusing commit --- core-api/src/main/java/com/optimizely/ab/Optimizely.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core-api/src/main/java/com/optimizely/ab/Optimizely.java b/core-api/src/main/java/com/optimizely/ab/Optimizely.java index ed3c3aa72..36d91f77d 100644 --- a/core-api/src/main/java/com/optimizely/ab/Optimizely.java +++ b/core-api/src/main/java/com/optimizely/ab/Optimizely.java @@ -334,7 +334,8 @@ public void track(@Nonnull String eventName, if (variation != null) { Experiment experiment = projectConfig.getExperimentForVariationId(variation.getId()); - if (experiment != null) { // user is in a rollout not an experiment + if (experiment != null) { + // the user is in an experiment for the feature sendImpression( projectConfig, experiment,