Skip to content

fix(decide): change to return reasons as a part of tuple in decision hierarchy #415

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.

Sign up for GitHub

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 10 commits into from
Jan 14, 2021
Merged

fix(decide): change to return reasons as a part of tuple in decision hierarchy #415

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
54f4bfc
change reasons to a response tuple
jaeopt Dec 17, 2020
da53eca
recover audience testing
jaeopt Dec 17, 2020
15a8a7f
fix DecisionService tests
jaeopt Dec 18, 2020
9bf45e1
fix Optimizely tests
jaeopt Dec 18, 2020
6e70659
change tests for returning reasons
jaeopt Dec 18, 2020
f17a401
fix tests
jaeopt Dec 18, 2020
de659dc
fix doc comments for reasons
jaeopt Jan 5, 2021
705db4f
change to nonnull for all DecisionResponse
jaeopt Jan 13, 2021
b65d4ef
Merge branch 'master' into jae/return-reasons
jaeopt Jan 13, 2021
5db7a24
fix null decision response
jaeopt Jan 13, 2021
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
46 changes: 23 additions & 23 deletions core-api/src/main/java/com/optimizely/ab/Optimizely.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/****************************************************************************
* Copyright 2016-2020, Optimizely, Inc. and contributors *
* Copyright 2016-2021, Optimizely, Inc. and contributors *
* *
* Licensed under the Apache License, Version 2.0 (the "License"); *
* you may not use this file except in compliance with the License. *
Expand Down Expand Up @@ -34,11 +34,7 @@
import com.optimizely.ab.optimizelyconfig.OptimizelyConfig;
import com.optimizely.ab.optimizelyconfig.OptimizelyConfigManager;
import com.optimizely.ab.optimizelyconfig.OptimizelyConfigService;
import com.optimizely.ab.optimizelydecision.DecisionMessage;
import com.optimizely.ab.optimizelydecision.DecisionReasons;
import com.optimizely.ab.optimizelydecision.DefaultDecisionReasons;
import com.optimizely.ab.optimizelydecision.OptimizelyDecideOption;
import com.optimizely.ab.optimizelydecision.OptimizelyDecision;
import com.optimizely.ab.optimizelydecision.*;
import com.optimizely.ab.optimizelyjson.OptimizelyJSON;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -427,7 +423,7 @@ private Boolean isFeatureEnabled(@Nonnull ProjectConfig projectConfig,

Map<String, ?> copiedAttributes = copyAttributes(attributes);
FeatureDecision.DecisionSource decisionSource = FeatureDecision.DecisionSource.ROLLOUT;
FeatureDecision featureDecision = decisionService.getVariationForFeature(featureFlag, userId, copiedAttributes, projectConfig);
FeatureDecision featureDecision = decisionService.getVariationForFeature(featureFlag, userId, copiedAttributes, projectConfig).getResult();
Copy link
Contributor

Choose a reason for hiding this comment

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

The decision service methods are all marked as nullable return value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A good catch. I'll change them all @nonnull.

Boolean featureEnabled = false;
SourceInfo sourceInfo = new RolloutSourceInfo();
if (featureDecision.decisionSource != null) {
Expand Down Expand Up @@ -736,7 +732,7 @@ <T> T getFeatureVariableValueForType(@Nonnull String featureKey,

String variableValue = variable.getDefaultValue();
Map<String, ?> copiedAttributes = copyAttributes(attributes);
FeatureDecision featureDecision = decisionService.getVariationForFeature(featureFlag, userId, copiedAttributes, projectConfig);
FeatureDecision featureDecision = decisionService.getVariationForFeature(featureFlag, userId, copiedAttributes, projectConfig).getResult();
Boolean featureEnabled = false;
if (featureDecision.variation != null) {
if (featureDecision.variation.getFeatureEnabled()) {
Expand Down Expand Up @@ -869,7 +865,7 @@ public OptimizelyJSON getAllFeatureVariables(@Nonnull String featureKey,
}

Map<String, ?> copiedAttributes = copyAttributes(attributes);
FeatureDecision featureDecision = decisionService.getVariationForFeature(featureFlag, userId, copiedAttributes, projectConfig);
FeatureDecision featureDecision = decisionService.getVariationForFeature(featureFlag, userId, copiedAttributes, projectConfig).getResult();
Boolean featureEnabled = false;
Variation variation = featureDecision.variation;

Expand Down Expand Up @@ -970,7 +966,7 @@ private Variation getVariation(@Nonnull ProjectConfig projectConfig,
@Nonnull String userId,
@Nonnull Map<String, ?> attributes) throws UnknownExperimentException {
Map<String, ?> copiedAttributes = copyAttributes(attributes);
Variation variation = decisionService.getVariation(experiment, userId, copiedAttributes, projectConfig);
Variation variation = decisionService.getVariation(experiment, userId, copiedAttributes, projectConfig).getResult();

String notificationType = NotificationCenter.DecisionNotificationType.AB_TEST.toString();

Expand Down Expand Up @@ -1084,7 +1080,7 @@ public Variation getForcedVariation(@Nonnull String experimentKey,
return null;
}

return decisionService.getForcedVariation(experiment, userId);
return decisionService.getForcedVariation(experiment, userId).getResult();
}

/**
Expand Down Expand Up @@ -1182,13 +1178,14 @@ OptimizelyDecision decide(@Nonnull OptimizelyUserContext user,
DecisionReasons decisionReasons = DefaultDecisionReasons.newInstance(allOptions);

Map<String, ?> copiedAttributes = new HashMap<>(attributes);
FeatureDecision flagDecision = decisionService.getVariationForFeature(
DecisionResponse<FeatureDecision> decisionVariation = decisionService.getVariationForFeature(
flag,
userId,
copiedAttributes,
projectConfig,
allOptions,
decisionReasons);
allOptions);
FeatureDecision flagDecision = decisionVariation.getResult();
decisionReasons.merge(decisionVariation.getReasons());

Boolean flagEnabled = false;
if (flagDecision.variation != null) {
Expand All @@ -1200,11 +1197,12 @@ OptimizelyDecision decide(@Nonnull OptimizelyUserContext user,

Map<String, Object> variableMap = new HashMap<>();
if (!allOptions.contains(OptimizelyDecideOption.EXCLUDE_VARIABLES)) {
variableMap = getDecisionVariableMap(
DecisionResponse<Map<String, Object>> decisionVariables = getDecisionVariableMap(
flag,
flagDecision.variation,
flagEnabled,
decisionReasons);
flagEnabled);
variableMap = decisionVariables.getResult();
decisionReasons.merge(decisionVariables.getReasons());
}
OptimizelyJSON optimizelyJSON = new OptimizelyJSON(variableMap);

Expand Down Expand Up @@ -1305,10 +1303,12 @@ private List<OptimizelyDecideOption> getAllOptions(List<OptimizelyDecideOption>
return copiedOptions;
}

private Map<String, Object> getDecisionVariableMap(@Nonnull FeatureFlag flag,
@Nonnull Variation variation,
@Nonnull Boolean featureEnabled,
@Nonnull DecisionReasons decisionReasons) {
@Nonnull
private DecisionResponse<Map<String, Object>> getDecisionVariableMap(@Nonnull FeatureFlag flag,
@Nonnull Variation variation,
@Nonnull Boolean featureEnabled) {
DecisionReasons reasons = new DecisionReasons();

Map<String, Object> valuesMap = new HashMap<String, Object>();
for (FeatureVariable variable : flag.getVariables()) {
String value = variable.getDefaultValue();
Expand All @@ -1321,15 +1321,15 @@ private Map<String, Object> getDecisionVariableMap(@Nonnull FeatureFlag flag,

Object convertedValue = convertStringToType(value, variable.getType());
if (convertedValue == null) {
decisionReasons.addError(DecisionMessage.VARIABLE_VALUE_INVALID.reason(variable.getKey()));
reasons.addError(DecisionMessage.VARIABLE_VALUE_INVALID.reason(variable.getKey()));
} else if (convertedValue instanceof OptimizelyJSON) {
convertedValue = ((OptimizelyJSON) convertedValue).toMap();
}

valuesMap.put(variable.getKey(), convertedValue);
}

return valuesMap;
return new DecisionResponse(valuesMap, reasons);
}

/**
Expand Down
50 changes: 23 additions & 27 deletions core-api/src/main/java/com/optimizely/ab/bucketing/Bucketer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
*
* Copyright 2016-2017, 2019, Optimizely and contributors
* Copyright 2016-2017, 2019-2021 Optimizely and contributors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -20,12 +20,12 @@
import com.optimizely.ab.bucketing.internal.MurmurHash3;
import com.optimizely.ab.config.*;
import com.optimizely.ab.optimizelydecision.DecisionReasons;
import com.optimizely.ab.optimizelydecision.DecisionResponse;
import com.optimizely.ab.optimizelydecision.DefaultDecisionReasons;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import javax.annotation.concurrent.Immutable;
import java.util.List;

Expand Down Expand Up @@ -69,8 +69,7 @@ private String bucketToEntity(int bucketValue, List<TrafficAllocation> trafficAl

private Experiment bucketToExperiment(@Nonnull Group group,
@Nonnull String bucketingId,
@Nonnull ProjectConfig projectConfig,
@Nonnull DecisionReasons reasons) {
@Nonnull ProjectConfig projectConfig) {
// "salt" the bucket id using the group id
String bucketKey = bucketingId + group.getId();

Expand All @@ -89,9 +88,11 @@ private Experiment bucketToExperiment(@Nonnull Group group,
return null;
}

private Variation bucketToVariation(@Nonnull Experiment experiment,
@Nonnull String bucketingId,
@Nonnull DecisionReasons reasons) {
@Nonnull
private DecisionResponse<Variation> bucketToVariation(@Nonnull Experiment experiment,
@Nonnull String bucketingId) {
DecisionReasons reasons = DefaultDecisionReasons.newInstance();

// "salt" the bucket id using the experiment id
String experimentId = experiment.getId();
String experimentKey = experiment.getKey();
Expand All @@ -111,13 +112,13 @@ private Variation bucketToVariation(@Nonnull Experiment experiment,
experimentKey);
logger.info(message);

return bucketedVariation;
return new DecisionResponse(bucketedVariation, reasons);
}

// user was not bucketed to a variation
String message = reasons.addInfo("User with bucketingId \"%s\" is not in any variation of experiment \"%s\".", bucketingId, experimentKey);
logger.info(message);
return null;
return new DecisionResponse(null, reasons);
}

/**
Expand All @@ -126,26 +127,26 @@ private Variation bucketToVariation(@Nonnull Experiment experiment,
* @param experiment The Experiment in which the user is to be bucketed.
* @param bucketingId string A customer-assigned value used to create the key for the murmur hash.
* @param projectConfig The current projectConfig
* @param reasons Decision log messages
* @return Variation the user is bucketed into or null.
* @return A {@link DecisionResponse} including the {@link Variation} that user is bucketed into (or null) and the decision reasons
*/
@Nullable
public Variation bucket(@Nonnull Experiment experiment,
@Nonnull String bucketingId,
@Nonnull ProjectConfig projectConfig,
@Nonnull DecisionReasons reasons) {
@Nonnull
public DecisionResponse<Variation> bucket(@Nonnull Experiment experiment,
@Nonnull String bucketingId,
@Nonnull ProjectConfig projectConfig) {
DecisionReasons reasons = DefaultDecisionReasons.newInstance();

// ---------- Bucket User ----------
String groupId = experiment.getGroupId();
// check whether the experiment belongs to a group
if (!groupId.isEmpty()) {
Group experimentGroup = projectConfig.getGroupIdMapping().get(groupId);
// bucket to an experiment only if group entities are to be mutually exclusive
if (experimentGroup.getPolicy().equals(Group.RANDOM_POLICY)) {
Experiment bucketedExperiment = bucketToExperiment(experimentGroup, bucketingId, projectConfig, reasons);
Experiment bucketedExperiment = bucketToExperiment(experimentGroup, bucketingId, projectConfig);
if (bucketedExperiment == null) {
String message = reasons.addInfo("User with bucketingId \"%s\" is not in any experiment of group %s.", bucketingId, experimentGroup.getId());
logger.info(message);
return null;
return new DecisionResponse(null, reasons);
} else {

}
Expand All @@ -155,7 +156,7 @@ public Variation bucket(@Nonnull Experiment experiment,
String message = reasons.addInfo("User with bucketingId \"%s\" is not in experiment \"%s\" of group %s.", bucketingId, experiment.getKey(),
experimentGroup.getId());
logger.info(message);
return null;
return new DecisionResponse(null, reasons);
}

String message = reasons.addInfo("User with bucketingId \"%s\" is in experiment \"%s\" of group %s.", bucketingId, experiment.getKey(),
Expand All @@ -164,14 +165,9 @@ public Variation bucket(@Nonnull Experiment experiment,
}
}

return bucketToVariation(experiment, bucketingId, reasons);
}

@Nullable
public Variation bucket(@Nonnull Experiment experiment,
@Nonnull String bucketingId,
@Nonnull ProjectConfig projectConfig) {
return bucket(experiment, bucketingId, projectConfig, DefaultDecisionReasons.newInstance());
DecisionResponse<Variation> decisionResponse = bucketToVariation(experiment, bucketingId);
reasons.merge(decisionResponse.getReasons());
return new DecisionResponse<>(decisionResponse.getResult(), reasons);
}

//======== Helper methods ========//
Expand Down
Loading