Skip to content
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

feat(decide): add a new set of decide apis #406

Merged
merged 48 commits into from
Dec 11, 2020
Merged

feat(decide): add a new set of decide apis #406

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
9f6c40c
add OptimizelyUserContext
jaeopt Oct 5, 2020
d476539
add decide api
jaeopt Oct 7, 2020
8bea738
add options and reasons
jaeopt Oct 7, 2020
6800e66
add decide-all api
jaeopt Oct 8, 2020
642f957
fix errors
jaeopt Oct 8, 2020
e8745ef
fix merge conflicts
jaeopt Oct 8, 2020
d4e2b1c
change user-context nonnull
jaeopt Oct 8, 2020
2d426a5
Merge branch 'jae/user-context' into jae/decide-api
jaeopt Oct 8, 2020
b8a19a7
fix existing tests for decision service
jaeopt Oct 8, 2020
e3cedab
add more decide tests
jaeopt Oct 9, 2020
2c83fa2
add more tests
jaeopt Oct 9, 2020
bb9f4dc
change ruleKey to have a copy of experimentKey
jaeopt Oct 12, 2020
3223a64
add all tests (except for reasons)
jaeopt Oct 13, 2020
7792926
add more tests
jaeopt Oct 14, 2020
31cce4f
fix per reviews
jaeopt Oct 14, 2020
77e09d5
fix conflicts
jaeopt Oct 14, 2020
4bfa1f7
fix conflicts
jaeopt Oct 14, 2020
fdbf75a
fix to nonnull options and reasons
jaeopt Oct 15, 2020
c0b6c85
remove support for custom decisionservice
jaeopt Oct 15, 2020
a93fc23
thread-safe setAttribute
jaeopt Oct 15, 2020
f803cd7
add option and reasons to condition evaluation
jaeopt Oct 15, 2020
43f7b01
add more tests for reasons
jaeopt Oct 16, 2020
7ce768d
pass options to DecisonReasons
jaeopt Oct 16, 2020
5b6e798
move decide core to optimizely
jaeopt Oct 16, 2020
29d3de4
change variables in decision nonnull
jaeopt Oct 16, 2020
a5954e4
change withDefaultDecideOptions per reviews
jaeopt Oct 19, 2020
6f40cb0
clean up addInfo
jaeopt Oct 22, 2020
0ecc4b6
remove unused evaluate method
jaeopt Oct 22, 2020
28493df
add toString to OptimizelyUserContext and OptimizelyDecision
jaeopt Oct 23, 2020
fe6cde5
check null userId for createUserContext
jaeopt Oct 29, 2020
9933238
merge flag-decisions
jaeopt Oct 29, 2020
82e9036
clean up decision options
jaeopt Oct 30, 2020
73668a9
clean up decide
jaeopt Oct 30, 2020
e0cb8fb
remove options propagation from tests
jaeopt Oct 30, 2020
4c68f74
add DecisionReasons interface
jaeopt Oct 31, 2020
0fbe851
Update core-api/src/main/java/com/optimizely/ab/Optimizely.java
jaeopt Nov 2, 2020
b3165f6
Update core-api/src/main/java/com/optimizely/ab/OptimizelyUserContext…
jaeopt Nov 2, 2020
a1202c3
Merge branch 'jae/user-context' of github.com:optimizely/java-sdk int…
jaeopt Nov 2, 2020
de508fb
fix convenience methods in user context
jaeopt Nov 5, 2020
b9e9e34
Merge branch 'master' into jae/user-context
jaeopt Nov 13, 2020
dd47439
add featureEnabled to metadata for decide-api
jaeopt Nov 13, 2020
af0f1f9
add metadata validation for decide-api
jaeopt Nov 17, 2020
4a35f33
add more tests for decide-api event validations
jaeopt Nov 17, 2020
21c2a9d
add more tests for decide-api event validations
jaeopt Nov 17, 2020
f0de871
Merge branch 'master' into jae/user-context
jaeopt Nov 18, 2020
ff67de0
fix import
jaeopt Nov 19, 2020
70373c1
change decision logs to infos
jaeopt Nov 24, 2020
819b7db
Merge branch 'master' into jae/user-context
jaeopt Dec 10, 2020
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
223 changes: 219 additions & 4 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
Expand Up @@ -34,6 +34,11 @@
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.optimizelyjson.OptimizelyJSON;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -76,7 +81,6 @@ public class Optimizely implements AutoCloseable {

private static final Logger logger = LoggerFactory.getLogger(Optimizely.class);

@VisibleForTesting
final DecisionService decisionService;
@VisibleForTesting
@Deprecated
Expand All @@ -86,6 +90,8 @@ public class Optimizely implements AutoCloseable {
@VisibleForTesting
final ErrorHandler errorHandler;

public final List<OptimizelyDecideOption> defaultDecideOptions;

private final ProjectConfigManager projectConfigManager;

@Nullable
Expand All @@ -104,7 +110,8 @@ private Optimizely(@Nonnull EventHandler eventHandler,
@Nullable UserProfileService userProfileService,
@Nonnull ProjectConfigManager projectConfigManager,
@Nullable OptimizelyConfigManager optimizelyConfigManager,
@Nonnull NotificationCenter notificationCenter
@Nonnull NotificationCenter notificationCenter,
@Nonnull List<OptimizelyDecideOption> defaultDecideOptions
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add an overloaded method and pass [] without defaultDecideOptions?

Copy link
Contributor

Choose a reason for hiding this comment

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

@msohailhussain can you elaborate? this is a private constructor any defaults should be handled in the builder. I don't think we should add another constructor.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah i missed private, this is fine.

) {
this.eventHandler = eventHandler;
this.eventProcessor = eventProcessor;
Expand All @@ -114,6 +121,7 @@ private Optimizely(@Nonnull EventHandler eventHandler,
this.projectConfigManager = projectConfigManager;
this.optimizelyConfigManager = optimizelyConfigManager;
this.notificationCenter = notificationCenter;
this.defaultDecideOptions = defaultDecideOptions;
Copy link
Contributor

Choose a reason for hiding this comment

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

we should clone it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's cloned already in builder, so no need for it here.

}

/**
Expand Down Expand Up @@ -779,7 +787,6 @@ <T> T getFeatureVariableValueForType(@Nonnull String featureKey,
}

// Helper method which takes type and variable value and convert it to object to use in Listener DecisionInfo object variable value
@VisibleForTesting
Object convertStringToType(String variableValue, String type) {
if (variableValue != null) {
switch (type) {
Expand Down Expand Up @@ -1129,6 +1136,202 @@ public OptimizelyConfig getOptimizelyConfig() {
return new OptimizelyConfigService(projectConfig).getConfig();
}

//============ decide ============//

/**
* Create a context of the user for which decision APIs will be called.
*
* A user context will be created successfully even when the SDK is not fully configured yet.
*
* @param userId The user ID to be used for bucketing.
* @param attributes: A map of attribute names to current user attribute values.
* @return An OptimizelyUserContext associated with this OptimizelyClient.
*/
public OptimizelyUserContext createUserContext(@Nonnull String userId,
@Nonnull Map<String, Object> attributes) {
if (userId == null) {
logger.warn("The userId parameter must be nonnull.");
return null;
}

return new OptimizelyUserContext(this, userId, attributes);
}

public OptimizelyUserContext createUserContext(@Nonnull String userId) {
return new OptimizelyUserContext(this, userId);
}

OptimizelyDecision decide(@Nonnull OptimizelyUserContext user,
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, I think having consistent signatures would be preferable. So in this case keep the userId and userAttributes in the signatures within Optimizely. If you have a UserContext object, then might as well use the methods on it (e.g. UserContext#decide).

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 agree. We better keep "user" and I hope we eventually make them all consistent. But for legacy APIs to be deprecated, we keep them as is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm I see, you have this as package private. My suggestion would not require that and still provide access to this functionality.

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'd like to keep these methods in OptimizelyClient as private for all SDKs to mitigate confusion - they can use the methods via user-context only. Let me know if I miss substantial benefits for keeping them public.

@Nonnull String key,
@Nonnull List<OptimizelyDecideOption> options) {

ProjectConfig projectConfig = getProjectConfig();
if (projectConfig == null) {
return OptimizelyDecision.newErrorDecision(key, user, DecisionMessage.SDK_NOT_READY.reason());
}

FeatureFlag flag = projectConfig.getFeatureKeyMapping().get(key);
if (flag == null) {
return OptimizelyDecision.newErrorDecision(key, user, DecisionMessage.FLAG_KEY_INVALID.reason(key));
}

String userId = user.getUserId();
Copy link
Contributor

Choose a reason for hiding this comment

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

am not sure, but we should add a check userContext shouldn't be null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need since this private is called thru a valid user context

Map<String, Object> attributes = user.getAttributes();
Boolean decisionEventDispatched = false;
List<OptimizelyDecideOption> allOptions = getAllOptions(options);
DecisionReasons decisionReasons = DefaultDecisionReasons.newInstance(allOptions);

Map<String, ?> copiedAttributes = new HashMap<>(attributes);
FeatureDecision flagDecision = decisionService.getVariationForFeature(
flag,
userId,
copiedAttributes,
projectConfig,
allOptions,
decisionReasons);
Copy link
Contributor

Choose a reason for hiding this comment

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

It appears that decisionReasons is expected to be modified from inside. In my experience, this kind of flow gets a little confusing when someone is following the code flow. On suggestion can be to return Pair<Decision, Reasons> fromgetVariationForFeature. This will clearly tell that decision reasons are also coming back from this function and original object will also be intact causing no confusion. what do you think

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 point! We also consider that option as well. It's dropped since it's a bit complicated to process and merge (and/or audience, etc) at each stage. We can find better solutions. I think you can try with other sdks with your suggestion and I can come back to this if we see a good solution.


Boolean flagEnabled = false;
if (flagDecision.variation != null) {
if (flagDecision.variation.getFeatureEnabled()) {
flagEnabled = true;
}
}
logger.info("Feature \"{}\" is enabled for user \"{}\"? {}", key, userId, flagEnabled);

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

FeatureDecision.DecisionSource decisionSource = FeatureDecision.DecisionSource.ROLLOUT;
if (flagDecision.decisionSource != null) {
decisionSource = flagDecision.decisionSource;
}

List<String> reasonsToReport = decisionReasons.toReport();
String variationKey = flagDecision.variation != null ? flagDecision.variation.getKey() : null;
// TODO: add ruleKey values when available later. use a copy of experimentKey until then.
// add to event metadata as well (currently set to experimentKey)
String ruleKey = flagDecision.experiment != null ? flagDecision.experiment.getKey() : null;

if (!allOptions.contains(OptimizelyDecideOption.DISABLE_DECISION_EVENT)) {
sendImpression(
projectConfig,
flagDecision.experiment,
userId,
copiedAttributes,
flagDecision.variation,
key,
decisionSource.toString(),
flagEnabled);
decisionEventDispatched = true;
}

DecisionNotification decisionNotification = DecisionNotification.newFlagDecisionNotificationBuilder()
.withUserId(userId)
.withAttributes(copiedAttributes)
.withFlagKey(key)
.withEnabled(flagEnabled)
.withVariables(variableMap)
.withVariationKey(variationKey)
.withRuleKey(ruleKey)
.withReasons(reasonsToReport)
.withDecisionEventDispatched(decisionEventDispatched)
.build();
notificationCenter.send(decisionNotification);

return new OptimizelyDecision(
variationKey,
flagEnabled,
optimizelyJSON,
ruleKey,
key,
user,
reasonsToReport);
}

Map<String, OptimizelyDecision> decideForKeys(@Nonnull OptimizelyUserContext user,
@Nonnull List<String> keys,
@Nonnull List<OptimizelyDecideOption> options) {
Map<String, OptimizelyDecision> decisionMap = new HashMap<>();

ProjectConfig projectConfig = getProjectConfig();
if (projectConfig == null) {
logger.error("Optimizely instance is not valid, failing isFeatureEnabled call.");
return decisionMap;
}

if (keys.isEmpty()) return decisionMap;

List<OptimizelyDecideOption> allOptions = getAllOptions(options);

for (String key : keys) {
OptimizelyDecision decision = decide(user, key, options);
if (!allOptions.contains(OptimizelyDecideOption.ENABLED_FLAGS_ONLY) || decision.getEnabled()) {
decisionMap.put(key, decision);
Copy link
Contributor

Choose a reason for hiding this comment

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

When ENABLED_FLAGS_ONLY is true, why we are sending notification?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure about this comment. Clarification?

}
}

return decisionMap;
}

Map<String, OptimizelyDecision> decideAll(@Nonnull OptimizelyUserContext user,
@Nonnull List<OptimizelyDecideOption> options) {
Map<String, OptimizelyDecision> decisionMap = new HashMap<>();

ProjectConfig projectConfig = getProjectConfig();
if (projectConfig == null) {
logger.error("Optimizely instance is not valid, failing isFeatureEnabled call.");
return decisionMap;
}

List<FeatureFlag> allFlags = projectConfig.getFeatureFlags();
List<String> allFlagKeys = new ArrayList<>();
for (int i = 0; i < allFlags.size(); i++) allFlagKeys.add(allFlags.get(i).getKey());

return decideForKeys(user, allFlagKeys, options);
}

private List<OptimizelyDecideOption> getAllOptions(List<OptimizelyDecideOption> options) {
List<OptimizelyDecideOption> copiedOptions = new ArrayList(defaultDecideOptions);
if (options != null) {
copiedOptions.addAll(options);
}
return copiedOptions;
}

private Map<String, Object> getDecisionVariableMap(@Nonnull FeatureFlag flag,
@Nonnull Variation variation,
@Nonnull Boolean featureEnabled,
@Nonnull DecisionReasons decisionReasons) {
Map<String, Object> valuesMap = new HashMap<String, Object>();
for (FeatureVariable variable : flag.getVariables()) {
String value = variable.getDefaultValue();
if (featureEnabled) {
FeatureVariableUsageInstance instance = variation.getVariableIdToFeatureVariableUsageInstanceMap().get(variable.getId());
if (instance != null) {
value = instance.getValue();
}
}

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

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

return valuesMap;
}

/**
* Helper method which makes separate copy of attributesMap variable and returns it
*
Expand Down Expand Up @@ -1233,6 +1436,7 @@ public static class Builder {
private OptimizelyConfigManager optimizelyConfigManager;
private UserProfileService userProfileService;
private NotificationCenter notificationCenter;
private List<OptimizelyDecideOption> defaultDecideOptions;

// For backwards compatibility
private AtomicProjectConfigManager fallbackConfigManager = new AtomicProjectConfigManager();
Expand Down Expand Up @@ -1304,6 +1508,11 @@ public Builder withDatafile(String datafile) {
return this;
}

public Builder withDefaultDecideOptions(List<OptimizelyDecideOption> defaultDecideOtions) {
this.defaultDecideOptions = defaultDecideOtions;
return this;
}

// Helper functions for making testing easier
protected Builder withBucketing(Bucketer bucketer) {
this.bucketer = bucketer;
Expand Down Expand Up @@ -1372,7 +1581,13 @@ public Optimizely build() {
eventProcessor = new ForwardingEventProcessor(eventHandler, notificationCenter);
}

return new Optimizely(eventHandler, eventProcessor, errorHandler, decisionService, userProfileService, projectConfigManager, optimizelyConfigManager, notificationCenter);
if (defaultDecideOptions != null) {
defaultDecideOptions = Collections.unmodifiableList(defaultDecideOptions);
} else {
defaultDecideOptions = Collections.emptyList();
}

return new Optimizely(eventHandler, eventProcessor, errorHandler, decisionService, userProfileService, projectConfigManager, optimizelyConfigManager, notificationCenter, defaultDecideOptions);
}
}
}
Loading