Skip to content

Josh/persistent bucketer #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 15 commits into from
Aug 29, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@
.classpath
.project

# android studio config files
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dont think it makes sense for Java SDK. Probably for Android SDK.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will need to expense IntelliJ Ultimate then. Android Studio adds the file automatically. It's also useful to work on the android modules and java module at the same time in Android Studio.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Makes sense.

local.properties

**/build
out
classes
Expand Down
3 changes: 2 additions & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ before_script:
- git config user.email "noreply@travis-ci.org"
- git checkout $TRAVIS_BRANCH
script:
- "./gradlew release -Prelease.useAutomaticVersion=true"
- "./gradlew clean build check"
# - '[ "${TRAVIS_PULL_REQUEST}" != "false" ] || ./gradlew release -Prelease.useAutomaticVersion=true'
env:
global:
- TERM=dumb
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
Optimizely Java SDK
===================
[![Build Status](https://travis-ci.com/optimizely/optimizely-ab-java-sdk.svg?token=qN2PbasvLidLamENwDAs&branch=master)](https://travis-ci.com/optimizely/java-sdk)
[![Build Status](https://travis-ci.org/optimizely/java-sdk.svg?branch=master)](https://travis-ci.org/optimizely/java-sdk)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jophde you may have to rebase.

cc @vraja2

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

those are the latest links, public repos use .org not .com for Travis

[![Apache 2.0](https://img.shields.io/github/license/nebula-plugins/gradle-extra-configurations-plugin.svg)](http://www.apache.org/licenses/LICENSE-2.0)

This repository houses the Java SDK for Optimizely's server-side testing product, which is currently in private beta.
Expand Down
36 changes: 24 additions & 12 deletions core-api/src/main/java/com/optimizely/ab/Optimizely.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this annotation is pretty cool

bucketer.cleanUserExperimentRecords();
}

//======== activate calls ========//

public @Nullable Variation activate(@Nonnull String experimentKey,
Expand Down Expand Up @@ -180,21 +186,21 @@ public void track(@Nonnull String eventName,

public void track(@Nonnull String eventName,
@Nonnull String userId,
long revenue) throws UnknownEventTypeException {
track(eventName, userId, Collections.<String, String>emptyMap(), revenue);
long eventValue) throws UnknownEventTypeException {
track(eventName, userId, Collections.<String, String>emptyMap(), eventValue);
}

public void track(@Nonnull String eventName,
@Nonnull String userId,
@Nonnull Map<String, String> attributes,
long revenue) throws UnknownEventTypeException {
track(eventName, userId, attributes, (Long)revenue);
long eventValue) throws UnknownEventTypeException {
track(eventName, userId, attributes, (Long)eventValue);
}

private void track(@Nonnull String eventName,
@Nonnull String userId,
@Nonnull Map<String, String> attributes,
@CheckForNull Long revenue) throws UnknownEventTypeException {
@CheckForNull Long eventValue) throws UnknownEventTypeException {

ProjectConfig currentConfig = getProjectConfig();

Expand All @@ -212,14 +218,14 @@ private void track(@Nonnull String eventName,
// create the conversion event request parameters, then dispatch
String endpointUrl = eventBuilder.getEndpointUrl(currentConfig.getProjectId());
Map<String, String> conversionParams;
if (revenue == null) {
if (eventValue == null) {
conversionParams = eventBuilder.createConversionParams(currentConfig, bucketer, userId,
eventType.getId(), eventType.getKey(),
attributes);
} else {
conversionParams = eventBuilder.createConversionParams(currentConfig, bucketer, userId,
eventType.getId(), eventType.getKey(), attributes,
revenue);
eventValue);
}

if (conversionParams == null) {
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -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) {
Expand All @@ -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;
}
}
}
71 changes: 69 additions & 2 deletions core-api/src/main/java/com/optimizely/ab/bucketing/Bucketer.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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) {
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: move this comment directly above where the salting happens on line 113

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();

Expand All @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use experimentKey here

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;
}

Expand Down Expand Up @@ -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()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

space between Map types

Copy link
Contributor

@vraja2 vraja2 Aug 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can you add some spaces between the Map types

for (String experimentKey : record.getValue().keySet()) {
Experiment experiment = projectConfig.getExperimentKeyMapping().get(experimentKey);
if (experiment == null || !experiment.isRunning()) {
userExperimentRecord.remove(record.getKey(), experimentKey);
}
}
}
}
}
}
}
Original file line number Diff line number Diff line change
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit. Empty line above this.

* @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();

}
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public class EventBuilder {
private static final String GOAL_NAME_PARAM = "n";
private static final String PPID_PARAM = "p";
private static final String PROJECT_ID_PARAM = "a";
private static final String REVENUE_PARAM = "v";
private static final String EVENT_VALUE_PARAM = "v";
private static final String SEGMENT_PARAM_PREFIX = "s";
private static final String SOURCE_PARAM = "src";
private static final String TIME_PARAM = "time";
Expand Down Expand Up @@ -89,8 +89,8 @@ public Map<String, String> createConversionParams(@Nonnull ProjectConfig project
@Nonnull String eventId,
@Nonnull String eventName,
@Nonnull Map<String, String> attributes,
long revenue) {
return createConversionParams(projectConfig, bucketer, userId, eventId, eventName, attributes, (Long)revenue);
long eventValue) {
return createConversionParams(projectConfig, bucketer, userId, eventId, eventName, attributes, (Long)eventValue);
}

private Map<String, String> createConversionParams(@Nonnull ProjectConfig projectConfig,
Expand All @@ -99,7 +99,7 @@ private Map<String, String> createConversionParams(@Nonnull ProjectConfig projec
@Nonnull String eventId,
@Nonnull String eventName,
@Nonnull Map<String, String> attributes,
@CheckForNull Long revenue) {
@CheckForNull Long eventValue) {

Map<String, String> requestParams = new HashMap<String, String>();
List<Experiment> addedExperiments =
Expand All @@ -110,7 +110,7 @@ private Map<String, String> createConversionParams(@Nonnull ProjectConfig projec
}

addCommonRequestParams(requestParams, projectConfig, userId, attributes);
addConversionGoal(requestParams, projectConfig, eventId, eventName, revenue);
addConversionGoal(requestParams, projectConfig, eventId, eventName, eventValue);

return requestParams;
}
Expand Down Expand Up @@ -256,15 +256,15 @@ private void addImpressionGoal(Map<String, String> requestParams, Experiment act
* @param projectConfig the project config
* @param eventId the goal being converted on
* @param eventName the name of the custom event goal
* @param revenue the optional revenue for the event
* @param eventValue the optional event value for the event
*/
private void addConversionGoal(Map<String, String> requestParams, ProjectConfig projectConfig, String eventId,
String eventName, @CheckForNull Long revenue) {
String eventName, @CheckForNull Long eventValue) {

String eventIds = eventId;
if (revenue != null) {
requestParams.put(REVENUE_PARAM, revenue.toString());
// to ensure that the revenue value is also included in "total revenue", pull in the default revenue goal
if (eventValue != null) {
// record the event value for the total revenue goal
requestParams.put(EVENT_VALUE_PARAM, eventValue.toString());
EventType revenueGoal = projectConfig.getEventNameMapping().get(EventType.TOTAL_REVENUE_GOAL_KEY);
if (revenueGoal != null) {
eventIds = eventId + "," + revenueGoal.getId();
Expand Down
Loading