Merge pull request #1 from optimizely/josh/persistent-bucketer

jophde · web-flow · commit 45c775ebbfa7 · 2016-08-29T09:07:16.000-07:00
Josh/persistent bucketer
diff --git a/.gitignore b/.gitignore
@@ -14,6 +14,9 @@
 .classpath
 .project
 
+# android studio config files
+local.properties
+
 **/build
 out
 classes
diff --git a/core-api/src/main/java/com/optimizely/ab/Optimizely.java b/core-api/src/main/java/com/optimizely/ab/Optimizely.java
@@ -18,6 +18,7 @@
 
 import com.optimizely.ab.annotations.VisibleForTesting;
 import com.optimizely.ab.bucketing.Bucketer;
+import com.optimizely.ab.bucketing.UserExperimentRecord;
 import com.optimizely.ab.config.Attribute;
 import com.optimizely.ab.config.EventType;
 import com.optimizely.ab.config.Experiment;
@@ -93,6 +94,11 @@ private Optimizely(@Nonnull ProjectConfig projectConfig,
         this.errorHandler = errorHandler;
     }
 
+    // Do work here that should be done once per Optimizely lifecycle
+    @VisibleForTesting void initialize() {
+        bucketer.cleanUserExperimentRecords();
+    }
+
     //======== activate calls ========//
 
     public @Nullable Variation activate(@Nonnull String experimentKey,
@@ -419,6 +425,7 @@ public static class Builder {
 
         private String datafile;
         private Bucketer bucketer;
+        private UserExperimentRecord userExperimentRecord;
         private ErrorHandler errorHandler;
         private EventHandler eventHandler;
         private EventBuilder eventBuilder;
@@ -435,6 +442,11 @@ public Builder withErrorHandler(ErrorHandler errorHandler) {
             return this;
         }
 
+        public Builder withUserExperimentRecord(UserExperimentRecord userExperimentRecord) {
+            this.userExperimentRecord = userExperimentRecord;
+            return this;
+        }
+
         protected Builder withBucketing(Bucketer bucketer) {
             this.bucketer = bucketer;
             return this;
@@ -445,9 +457,7 @@ protected Builder withEventBuilder(EventBuilder eventBuilder) {
             return this;
         }
 
-        /**
-         * Helper function for making testing easier
-         */
+        // Helper function for making testing easier
         protected Builder withConfig(ProjectConfig projectConfig) {
             this.projectConfig = projectConfig;
             return this;
@@ -460,7 +470,7 @@ public Optimizely build() {
 
             // use the default bucketer and event builder, if no overrides were provided
             if (bucketer == null) {
-                bucketer = new Bucketer(projectConfig);
+                bucketer = new Bucketer(projectConfig, userExperimentRecord);
             }
 
             if (eventBuilder == null) {
@@ -471,7 +481,9 @@ public Optimizely build() {
                 errorHandler = new NoOpErrorHandler();
             }
 
-            return new Optimizely(projectConfig, bucketer, eventHandler, eventBuilder, errorHandler);
+            Optimizely optimizely = new Optimizely(projectConfig, bucketer, eventHandler, eventBuilder, errorHandler);
+            optimizely.initialize();
+            return optimizely;
         }
     }
 }
diff --git a/core-api/src/main/java/com/optimizely/ab/bucketing/Bucketer.java b/core-api/src/main/java/com/optimizely/ab/bucketing/Bucketer.java
@@ -47,6 +47,8 @@ public class Bucketer {
 
     private final ProjectConfig projectConfig;
 
+    @Nullable private final UserExperimentRecord userExperimentRecord;
+
     private static final Logger logger = LoggerFactory.getLogger(Bucketer.class);
 
     private static final int MURMUR_HASH_SEED = 1;
@@ -58,7 +60,12 @@ public class Bucketer {
     static final int MAX_TRAFFIC_VALUE = 10000;
 
     public Bucketer(ProjectConfig projectConfig) {
+        this(projectConfig, null);
+    }
+
+    public Bucketer(ProjectConfig projectConfig, @Nullable UserExperimentRecord userExperimentRecord) {
         this.projectConfig = projectConfig;
+        this.userExperimentRecord = userExperimentRecord;
     }
 
     private String bucketToEntity(int bucketValue, List<TrafficAllocation> trafficAllocations) {
@@ -101,8 +108,29 @@ private Experiment bucketToExperiment(@Nonnull Group group,
     private Variation bucketToVariation(@Nonnull Experiment experiment,
                                         @Nonnull String userId) {
         // "salt" the bucket id using the experiment id
-        String combinedBucketId = userId + experiment.getId();
+        String experimentId = experiment.getId();
         String experimentKey = experiment.getKey();
+        String combinedBucketId = userId + experimentId;
+
+        // If a user experiment record instance is present then check it for a saved variation
+        if (userExperimentRecord != null) {
+            String variationKey = userExperimentRecord.lookup(userId, experimentKey);
+            if (variationKey != null) {
+                logger.info("Returning previously activated variation \"{}\" of experiment \"{}\" "
+                            + "for user \"{}\" from user experiment record.",
+                            variationKey, experimentKey, userId);
+                // A variation is stored for this combined bucket id
+                return projectConfig
+                    .getExperimentIdMapping()
+                    .get(experimentId)
+                    .getVariationKeyToVariationMap()
+                    .get(variationKey);
+            } else {
+                logger.info("No previously activated variation of experiment \"{}\" "
+                            + "for user \"{}\" found in user experiment record.",
+                            experimentKey, userId);
+            }
+        }
 
         List<TrafficAllocation> trafficAllocations = experiment.getTrafficAllocation();
 
@@ -113,8 +141,22 @@ private Variation bucketToVariation(@Nonnull Experiment experiment,
         String bucketedVariationId = bucketToEntity(bucketValue, trafficAllocations);
         if (bucketedVariationId != null) {
             Variation bucketedVariation = experiment.getVariationIdToVariationMap().get(bucketedVariationId);
-            logger.info("User \"{}\" is in variation \"{}\" of experiment \"{}\".", userId, bucketedVariation.getKey(),
+            String variationKey = bucketedVariation.getKey();
+                logger.info("User \"{}\" is in variation \"{}\" of experiment \"{}\".", userId, variationKey,
                         experimentKey);
+
+            // If a user experiment record is present give it a variation to store
+            if (userExperimentRecord != null) {
+                boolean saved = userExperimentRecord.save(userId, experiment.getKey(), variationKey);
+                if (saved) {
+                    logger.info("Saved variation \"{}\" of experiment \"{}\" for user \"{}\".",
+                                variationKey, experimentKey, userId);
+                } else {
+                    logger.warn("Failed to save variation \"{}\" of experiment \"{}\" for user \"{}\".",
+                                variationKey, experimentKey, userId);
+                }
+            }
+
             return bucketedVariation;
         }
 
@@ -180,4 +222,29 @@ int generateBucketValue(int hashCode) {
         double ratio = (double)(hashCode & 0xFFFFFFFFL) / Math.pow(2, 32);
         return (int)Math.floor(MAX_TRAFFIC_VALUE * ratio);
     }
+
+    @Nullable
+    public UserExperimentRecord getUserExperimentRecord() {
+        return userExperimentRecord;
+    }
+
+    /**
+     * Gives implementations of {@link UserExperimentRecord} a chance to remove records
+     * of experiments that are deleted or not running.
+     */
+    public void cleanUserExperimentRecords() {
+        if (userExperimentRecord != null) {
+            Map<String, Map<String,String>> records = userExperimentRecord.getAllRecords();
+            if (records != null) {
+                for (Map.Entry<String,Map<String,String>> record : records.entrySet()) {
+                    for (String experimentKey : record.getValue().keySet()) {
+                        Experiment experiment = projectConfig.getExperimentKeyMapping().get(experimentKey);
+                        if (experiment == null || !experiment.isRunning()) {
+                            userExperimentRecord.remove(record.getKey(), experimentKey);
+                        }
+                    }
+                }
+            }
+        }
+    }
 }
diff --git a/core-api/src/main/java/com/optimizely/ab/bucketing/UserExperimentRecord.java b/core-api/src/main/java/com/optimizely/ab/bucketing/UserExperimentRecord.java
@@ -0,0 +1,70 @@
+/**
+ *
+ *    Copyright 2016, Optimizely
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+package com.optimizely.ab.bucketing;
+
+import java.util.Map;
+
+/**
+ * Gives implementors a chance to override {@link Bucketer} bucketing on subsequent activations.
+ *
+ * Overriding bucketing for subsequent activations is useful in order to prevent changes to
+ * user experience after changing traffic allocations.  Also, this interface gives users
+ * a hook to keep track of activation history.
+ */
+public interface UserExperimentRecord {
+
+    /**
+     * Called when implementors should save an activation
+     *
+     * @param userId the user id of the activation
+     * @param experimentKey the experiment key of the activation
+     * @param variationKey the variation key of the activation
+     * @return true if saving of the record was successful
+     */
+    boolean save(String userId, String experimentKey, String variationKey);
+
+    /**
+     * Called by the bucketer to check for a record of the previous activation
+     *
+     * @param userId the user is id of the next activation
+     * @param experimentKey the experiment id of the next activation
+     * @return the variation key of the next activation, or null if no record exists
+     */
+    String lookup(String userId, String experimentKey);
+
+    /**
+     * Called when user experiment record should be removed
+     *
+     * Records should be removed when an experiment is not running or when an experiment has been
+     * deleted.
+     *
+     * @param userId the user id of the record to remove
+     * @param experimentKey the experiment key of the record to remove
+     * @return true if the record was removed
+     */
+    boolean remove(String userId, String experimentKey);
+
+    /**
+     * Called by bucketer to get a mapping of all stored records
+     *
+     * This mapping is needed so that the bucketer can {@link #remove(String, String)} outdated
+     * records.
+     * @return a map of userIds to a map of experiment keys to variation keys
+     */
+    Map<String, Map<String,String>> getAllRecords();
+
+}
diff --git a/core-api/src/main/java/com/optimizely/ab/event/EventHandler.java b/core-api/src/main/java/com/optimizely/ab/event/EventHandler.java
@@ -19,7 +19,7 @@
 import java.util.Map;
 
 /**
- * Implementations are responsible for dispatching {@link LogEvent}s to the Optimizely event end-point.
+ * Implementations are responsible for dispatching event's to the Optimizely event end-point.
  */
 public interface EventHandler {
 
diff --git a/core-api/src/test/java/com/optimizely/ab/OptimizelyBuilderTest.java b/core-api/src/test/java/com/optimizely/ab/OptimizelyBuilderTest.java
@@ -16,6 +16,7 @@
  */
 package com.optimizely.ab;
 
+import com.optimizely.ab.bucketing.UserExperimentRecord;
 import com.optimizely.ab.config.ProjectConfigTestUtils;
 import com.optimizely.ab.error.ErrorHandler;
 import com.optimizely.ab.error.NoOpErrorHandler;
@@ -35,6 +36,7 @@
 import static org.hamcrest.CoreMatchers.instanceOf;
 import static org.hamcrest.CoreMatchers.is;
 import static org.junit.Assert.assertThat;
+import static org.mockito.Mockito.mock;
 
 /**
  * Tests for {@link Optimizely#builder(String, EventHandler)}.
@@ -84,4 +86,14 @@ public void withDefaultErrorHandler() throws Exception {
 
         assertThat(optimizelyClient.errorHandler, instanceOf(NoOpErrorHandler.class));
     }
+
+    @Test
+    public void withUserExperimentRecord() throws Exception {
+        UserExperimentRecord userExperimentRecord = mock(UserExperimentRecord.class);
+        Optimizely optimizelyClient = Optimizely.builder(validConfigJson(), mockEventHandler)
+                .withUserExperimentRecord(userExperimentRecord)
+                .build();
+
+        assertThat(optimizelyClient.bucketer.getUserExperimentRecord(), is(userExperimentRecord));
+    }
 }
diff --git a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java
@@ -138,6 +138,26 @@ public void activateEndToEnd() throws Exception {
         verify(mockEventHandler).dispatchEvent("test_url", testParams);
     }
 
+    /**
+     * Verify that bucketer initialization happens after building the Optimizely object
+     * @throws Exception
+     */
+    @Test
+    public void initializationOccursForBucketerWhenBuildingOptly() throws Exception {
+        String datafile = validConfigJson();
+        ProjectConfig projectConfig = validProjectConfig();
+        EventBuilder mockEventBuilder = mock(EventBuilder.class);
+
+        Optimizely.builder(datafile, mockEventHandler)
+            .withBucketing(mockBucketer)
+            .withEventBuilder(mockEventBuilder)
+            .withConfig(projectConfig)
+            .withErrorHandler(mockErrorHandler)
+            .build();
+
+        verify(mockBucketer).cleanUserExperimentRecords();
+    }
+
     /**
      * Verify that the {@link Optimizely#activate(Experiment, String, Map)} DOES NOT dispatch an impression event
      * when the user isn't bucketed to a variation.
diff --git a/core-api/src/test/java/com/optimizely/ab/bucketing/BucketerTest.java b/core-api/src/test/java/com/optimizely/ab/bucketing/BucketerTest.java
diff --git a/gradle.properties b/gradle.properties
diff --git a/gradle/wrapper/gradle-wrapper.properties b/gradle/wrapper/gradle-wrapper.properties
