feat(decide): add decide API #407
Conversation
| @@ -227,7 +226,7 @@ private Variation activate(@Nullable ProjectConfig projectConfig, | |||
| return variation; | |||
| } | |||
|
|
|||
| private void sendImpression(@Nonnull ProjectConfig projectConfig, | |||
| public void sendImpression(@Nonnull ProjectConfig projectConfig, | |||
There was a problem hiding this comment.
If we move OptimizelyUserContext into the root com.optimizely.ab package then we don't have to open this up so widely, but instead can be make package private
| @@ -71,7 +69,9 @@ private String bucketToEntity(int bucketValue, List<TrafficAllocation> trafficAl | |||
|
|
|||
| private Experiment bucketToExperiment(@Nonnull Group group, | |||
| @Nonnull String bucketingId, | |||
| @Nonnull ProjectConfig projectConfig) { | |||
| @Nonnull ProjectConfig projectConfig, | |||
| @Nullable List<OptimizelyDecideOption> options, | |||
There was a problem hiding this comment.
👍 List makes sense as opposed to array. See comments in #406
| logger.info("User with bucketingId \"{}\" is not in any experiment of group {}.", bucketingId, experimentGroup.getId()); | ||
| DecisionService.logInfo(logger, reasons, "User with bucketingId \"%s\" is not in any experiment of group %s.", bucketingId, experimentGroup.getId()); |
There was a problem hiding this comment.
The use of these static DecisionService.logXXX functions is adding more confusion than clarity. I think it would be more readable to have a function on DecisionReasons that formats the message, then log in the calling class.
// Add and return formatted message
public String addInfof(String format, Object... args) {
String message = String.format(format, args);
addInfo(message);
return message;
}So this one liner becomes two, but its very transparent what is happening, eg:
String reason = reasons.addInfof("User with bucketingId \"{}\" is not in any experiment of group {}.", bucketingId, experimentGroup.getId());
logger.info(reason);You can even log directly within DecisionReasons but I bet that will break a lot of log tests.
| @@ -175,5 +211,12 @@ int generateBucketValue(int hashCode) { | |||
| return (int) Math.floor(MAX_TRAFFIC_VALUE * ratio); | |||
| } | |||
|
|
|||
| Boolean isMethodOverriden(String methodName, Class<?>... params) { | |||
There was a problem hiding this comment.
I don't think we need to worry about custom overrides such as this. Let me know if there's a specific concern or a hypothetical one.
| public static void logError(Logger logger, DecisionReasons reasons, String format, Object... args) { | ||
| String message = String.format(format, args); | ||
| logger.error(message); | ||
| if (reasons != null) reasons.addInfo(message); | ||
| } | ||
|
|
||
| public static void logWarn(Logger logger, DecisionReasons reasons, String format, Object... args) { | ||
| String message = String.format(format, args); | ||
| logger.warn(message); | ||
| if (reasons != null) reasons.addInfo(message); | ||
| } | ||
|
|
||
| public static void logInfo(Logger logger, DecisionReasons reasons, String format, Object... args) { | ||
| String message = String.format(format, args); | ||
| logger.info(message); | ||
| if (reasons != null) reasons.addInfo(message); | ||
| } | ||
|
|
||
| public static void logDebug(Logger logger, DecisionReasons reasons, String format, Object... args) { | ||
| String message = String.format(format, args); | ||
| logger.debug(message); | ||
| if (reasons != null) reasons.addInfo(message); | ||
| } |
There was a problem hiding this comment.
See previous comment. I think we can reduce boiler plate by adding functionality into DecisionReasons.
| if (reasons != null) reasons.addInfo(message); | ||
| } | ||
|
|
||
| Boolean isMethodOverriden(String methodName, Class<?>... params) { |
There was a problem hiding this comment.
Same comment here. This type of method raises a bunch of red-flags and it's not clear to me why this approach was taken.
| List<String> errors; | ||
| List<String> logs; | ||
|
|
||
| public DecisionReasons() { | ||
| this.errors = new ArrayList<String>(); | ||
| this.logs = new ArrayList<String>(); | ||
| } |
There was a problem hiding this comment.
Nit, but you don't need to explicitly define the constructor:
final List<String> errors = new ArrayList<>();
final List<String> logs = new ArrayList<>();There was a problem hiding this comment.
Also consider making these private
|
|
||
| logger.info("Feature \"{}\" is enabled for user \"{}\"? {}", key, userId, flagEnabled); | ||
|
|
||
| return new OptimizelyDecision( |
There was a problem hiding this comment.
OptimizelyUserContext is more of a convenience wrapper, the core "decide" logic behavior should exist in Optimizely#decide.
| private Map<String, Object> copyAttributes() { | ||
| return new HashMap<>(attributes); | ||
| } |
There was a problem hiding this comment.
Should be static, but also this util is not strictly necessary.
| public static String getFlagKeyInvalidMessage(String flagKey) { | ||
| return String.format(FLAG_KEY_INVALID, flagKey); | ||
| } | ||
|
|
||
| public static String getVariableValueInvalidMessage(String variableKey) { | ||
| return String.format(VARIABLE_VALUE_INVALID, variableKey); | ||
| } |
There was a problem hiding this comment.
One-liner "helper" methods then to add more cognitive overhead than they alleviate. Consider in-lining the String.format directly. Alternatively if you wanted to provide some abstraction, then a customer enum class can be used to define the format functionality:
private enum Messages {
FLAG_KEY_INVALID("Flag key \"%s\" is not in datafile."),
VARIABLE_VALUE_INVALID("Variable value for key \"%s\" is invalid or wrong type.");
private final String format;
Messages(String format) {
this.format = format;
}
public String format(Object...args) {
return String.format(format, args);
}
}Which can then be consumed as:
Messages.FLAG_KEY_INVALID.format("key");Note that this implementation reads more direct about what is happening. We're formatting a predefined message.
Summary
The second part of 2 PRs for Decide API: