-
Couldn't load subscription status.
- Fork 32
[FSSDK-11170] update: decision service methods for cmab #583
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
base: master
Are you sure you want to change the base?
Changes from 34 commits
26e6393
ad63201
fbed362
53d754a
9905026
78f45bf
9757d49
ecf9199
5e0808f
36d2b4c
5796cb7
d8b0134
b2f270f
a4c3f1c
e4fe788
e75693d
af210d8
42053e4
9a12d72
416bcbd
64f378f
8539166
3cee65c
47c65b5
fe75a85
b0d5090
6db2e88
6fc6446
a80c0d3
a9ae805
f25f824
7363a2f
1c52366
b2dcf9e
89771bc
73d5673
690379c
9c8dd8f
a17becd
ba575ce
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -20,6 +20,10 @@ | |||||||||||||
| import com.optimizely.ab.bucketing.DecisionService; | ||||||||||||||
| import com.optimizely.ab.bucketing.FeatureDecision; | ||||||||||||||
| import com.optimizely.ab.bucketing.UserProfileService; | ||||||||||||||
| import com.optimizely.ab.cmab.service.CmabCacheValue; | ||||||||||||||
| import com.optimizely.ab.cmab.service.CmabService; | ||||||||||||||
| import com.optimizely.ab.cmab.service.CmabServiceOptions; | ||||||||||||||
| import com.optimizely.ab.cmab.service.DefaultCmabService; | ||||||||||||||
| import com.optimizely.ab.config.AtomicProjectConfigManager; | ||||||||||||||
| import com.optimizely.ab.config.DatafileProjectConfig; | ||||||||||||||
| import com.optimizely.ab.config.EventType; | ||||||||||||||
|
|
@@ -45,6 +49,7 @@ | |||||||||||||
| import com.optimizely.ab.event.internal.UserEvent; | ||||||||||||||
| import com.optimizely.ab.event.internal.UserEventFactory; | ||||||||||||||
| import com.optimizely.ab.event.internal.payload.EventBatch; | ||||||||||||||
| import com.optimizely.ab.internal.DefaultLRUCache; | ||||||||||||||
| import com.optimizely.ab.internal.NotificationRegistry; | ||||||||||||||
| import com.optimizely.ab.notification.ActivateNotification; | ||||||||||||||
| import com.optimizely.ab.notification.DecisionNotification; | ||||||||||||||
|
|
@@ -69,12 +74,14 @@ | |||||||||||||
| 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; | ||||||||||||||
|
|
||||||||||||||
| import javax.annotation.Nonnull; | ||||||||||||||
| import javax.annotation.Nullable; | ||||||||||||||
| import javax.annotation.concurrent.ThreadSafe; | ||||||||||||||
|
|
||||||||||||||
| import java.io.Closeable; | ||||||||||||||
| import java.util.ArrayList; | ||||||||||||||
| import java.util.Arrays; | ||||||||||||||
|
|
@@ -84,6 +91,7 @@ | |||||||||||||
| import java.util.Map; | ||||||||||||||
| import java.util.concurrent.locks.ReentrantLock; | ||||||||||||||
|
|
||||||||||||||
| import com.optimizely.ab.cmab.client.CmabClient; | ||||||||||||||
| import static com.optimizely.ab.internal.SafetyUtils.tryClose; | ||||||||||||||
|
|
||||||||||||||
| /** | ||||||||||||||
|
|
@@ -141,8 +149,11 @@ public class Optimizely implements AutoCloseable { | |||||||||||||
| @Nullable | ||||||||||||||
| private final ODPManager odpManager; | ||||||||||||||
|
|
||||||||||||||
| private final CmabService cmabService; | ||||||||||||||
|
|
||||||||||||||
| private final ReentrantLock lock = new ReentrantLock(); | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| private Optimizely(@Nonnull EventHandler eventHandler, | ||||||||||||||
| @Nonnull EventProcessor eventProcessor, | ||||||||||||||
| @Nonnull ErrorHandler errorHandler, | ||||||||||||||
|
|
@@ -152,8 +163,9 @@ private Optimizely(@Nonnull EventHandler eventHandler, | |||||||||||||
| @Nullable OptimizelyConfigManager optimizelyConfigManager, | ||||||||||||||
| @Nonnull NotificationCenter notificationCenter, | ||||||||||||||
| @Nonnull List<OptimizelyDecideOption> defaultDecideOptions, | ||||||||||||||
| @Nullable ODPManager odpManager | ||||||||||||||
| ) { | ||||||||||||||
| @Nullable ODPManager odpManager, | ||||||||||||||
| @Nonnull CmabService cmabService | ||||||||||||||
| ) { | ||||||||||||||
| this.eventHandler = eventHandler; | ||||||||||||||
| this.eventProcessor = eventProcessor; | ||||||||||||||
| this.errorHandler = errorHandler; | ||||||||||||||
|
|
@@ -164,6 +176,7 @@ private Optimizely(@Nonnull EventHandler eventHandler, | |||||||||||||
| this.notificationCenter = notificationCenter; | ||||||||||||||
| this.defaultDecideOptions = defaultDecideOptions; | ||||||||||||||
| this.odpManager = odpManager; | ||||||||||||||
| this.cmabService = cmabService; | ||||||||||||||
|
|
||||||||||||||
| if (odpManager != null) { | ||||||||||||||
| odpManager.getEventManager().start(); | ||||||||||||||
|
|
@@ -1395,7 +1408,8 @@ Map<String, OptimizelyDecision> decideForKeys(@Nonnull OptimizelyUserContext use | |||||||||||||
| private Map<String, OptimizelyDecision> decideForKeys(@Nonnull OptimizelyUserContext user, | ||||||||||||||
| @Nonnull List<String> keys, | ||||||||||||||
| @Nonnull List<OptimizelyDecideOption> options, | ||||||||||||||
| boolean ignoreDefaultOptions) { | ||||||||||||||
| boolean ignoreDefaultOptions, | ||||||||||||||
| boolean useCmab) { | ||||||||||||||
FarhanAnjum-opti marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||
| Map<String, OptimizelyDecision> decisionMap = new HashMap<>(); | ||||||||||||||
|
|
||||||||||||||
| ProjectConfig projectConfig = getProjectConfig(); | ||||||||||||||
|
|
@@ -1440,11 +1454,25 @@ private Map<String, OptimizelyDecision> decideForKeys(@Nonnull OptimizelyUserCon | |||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| List<DecisionResponse<FeatureDecision>> decisionList = | ||||||||||||||
| decisionService.getVariationsForFeatureList(flagsWithoutForcedDecision, user, projectConfig, allOptions); | ||||||||||||||
| decisionService.getVariationsForFeatureList(flagsWithoutForcedDecision, user, projectConfig, allOptions, useCmab); | ||||||||||||||
|
|
||||||||||||||
| for (int i = 0; i < flagsWithoutForcedDecision.size(); i++) { | ||||||||||||||
| DecisionResponse<FeatureDecision> decision = decisionList.get(i); | ||||||||||||||
| boolean error = decision.isError(); | ||||||||||||||
| String experimentKey = null; | ||||||||||||||
| if (decision.getResult() != null && decision.getResult().experiment != null) { | ||||||||||||||
| experimentKey = decision.getResult().experiment.getKey(); | ||||||||||||||
| } | ||||||||||||||
| String flagKey = flagsWithoutForcedDecision.get(i).getKey(); | ||||||||||||||
|
|
||||||||||||||
| if (error) { | ||||||||||||||
| OptimizelyDecision optimizelyDecision = OptimizelyDecision.newErrorDecision(flagKey, user, DecisionMessage.CMAB_ERROR.reason(experimentKey)); | ||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we always report CMAB error for any decision errors? Is this safe? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As far as I understand, we get error from decision service only when cmab fails. So this error flag is only true for cmab errors. @raju-opti can verify. |
||||||||||||||
| decisionMap.put(flagKey, optimizelyDecision); | ||||||||||||||
| if (validKeys.contains(flagKey)) { | ||||||||||||||
| validKeys.remove(flagKey); | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| flagDecisions.put(flagKey, decision.getResult()); | ||||||||||||||
| decisionReasonsMap.get(flagKey).merge(decision.getReasons()); | ||||||||||||||
| } | ||||||||||||||
|
|
@@ -1465,6 +1493,13 @@ private Map<String, OptimizelyDecision> decideForKeys(@Nonnull OptimizelyUserCon | |||||||||||||
| return decisionMap; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| private Map<String, OptimizelyDecision> decideForKeys(@Nonnull OptimizelyUserContext user, | ||||||||||||||
| @Nonnull List<String> keys, | ||||||||||||||
| @Nonnull List<OptimizelyDecideOption> options, | ||||||||||||||
| boolean ignoreDefaultOptions) { | ||||||||||||||
| return decideForKeys(user, keys, options, ignoreDefaultOptions, true); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| Map<String, OptimizelyDecision> decideAll(@Nonnull OptimizelyUserContext user, | ||||||||||||||
| @Nonnull List<OptimizelyDecideOption> options) { | ||||||||||||||
| Map<String, OptimizelyDecision> decisionMap = new HashMap<>(); | ||||||||||||||
|
|
@@ -1482,6 +1517,78 @@ Map<String, OptimizelyDecision> decideAll(@Nonnull OptimizelyUserContext user, | |||||||||||||
| return decideForKeys(user, allFlagKeys, options); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| /** | ||||||||||||||
| * Returns a decision result ({@link OptimizelyDecision}) for a given flag key and a user context, | ||||||||||||||
| * skipping CMAB logic and using only traditional A/B testing. | ||||||||||||||
FarhanAnjum-opti marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
| * This will be called by mobile apps which will use non-blocking legacy ab-tests only (for backward compatibility with android-sdk) | ||||||||||||||
FarhanAnjum-opti marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||
| * | ||||||||||||||
| * @param user An OptimizelyUserContext associated with this OptimizelyClient. | ||||||||||||||
| * @param key A flag key for which a decision will be made. | ||||||||||||||
| * @param options A list of options for decision-making. | ||||||||||||||
| * @return A decision result using traditional A/B testing logic only. | ||||||||||||||
| */ | ||||||||||||||
| OptimizelyDecision decideSync(@Nonnull OptimizelyUserContext user, | ||||||||||||||
| @Nonnull String key, | ||||||||||||||
| @Nonnull List<OptimizelyDecideOption> options) { | ||||||||||||||
| ProjectConfig projectConfig = getProjectConfig(); | ||||||||||||||
| if (projectConfig == null) { | ||||||||||||||
| return OptimizelyDecision.newErrorDecision(key, user, DecisionMessage.SDK_NOT_READY.reason()); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| List<OptimizelyDecideOption> allOptions = getAllOptions(options); | ||||||||||||||
| allOptions.remove(OptimizelyDecideOption.ENABLED_FLAGS_ONLY); | ||||||||||||||
|
|
||||||||||||||
| return decideForKeysSync(user, Arrays.asList(key), allOptions, true).get(key); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| /** | ||||||||||||||
| * Returns decision results for multiple flag keys, skipping CMAB logic and using only traditional A/B testing. | ||||||||||||||
| * This will be called by mobile apps which will use non-blocking legacy ab-tests only (for backward compatibility with android-sdk) | ||||||||||||||
FarhanAnjum-opti marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||
| * | ||||||||||||||
| * @param user An OptimizelyUserContext associated with this OptimizelyClient. | ||||||||||||||
| * @param keys A list of flag keys for which decisions will be made. | ||||||||||||||
| * @param options A list of options for decision-making. | ||||||||||||||
| * @return All decision results mapped by flag keys, using traditional A/B testing logic only. | ||||||||||||||
| */ | ||||||||||||||
| Map<String, OptimizelyDecision> decideForKeysSync(@Nonnull OptimizelyUserContext user, | ||||||||||||||
| @Nonnull List<String> keys, | ||||||||||||||
| @Nonnull List<OptimizelyDecideOption> options) { | ||||||||||||||
| return decideForKeysSync(user, keys, options, false); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| /** | ||||||||||||||
| * Returns decision results for all active flag keys, skipping CMAB logic and using only traditional A/B testing. | ||||||||||||||
| * This will be called by mobile apps which will make synchronous decisions only (for backward compatibility with android-sdk) | ||||||||||||||
| * | ||||||||||||||
| * @param user An OptimizelyUserContext associated with this OptimizelyClient. | ||||||||||||||
| * @param options A list of options for decision-making. | ||||||||||||||
| * @return All decision results mapped by flag keys, using traditional A/B testing logic only. | ||||||||||||||
| */ | ||||||||||||||
| Map<String, OptimizelyDecision> decideAllSync(@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 decideAllSync 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 decideForKeysSync(user, allFlagKeys, options); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| private Map<String, OptimizelyDecision> decideForKeysSync(@Nonnull OptimizelyUserContext user, | ||||||||||||||
| @Nonnull List<String> keys, | ||||||||||||||
| @Nonnull List<OptimizelyDecideOption> options, | ||||||||||||||
| boolean ignoreDefaultOptions) { | ||||||||||||||
| return decideForKeys(user, keys, options, ignoreDefaultOptions, false); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| private List<OptimizelyDecideOption> getAllOptions(List<OptimizelyDecideOption> options) { | ||||||||||||||
| List<OptimizelyDecideOption> copiedOptions = new ArrayList(defaultDecideOptions); | ||||||||||||||
| if (options != null) { | ||||||||||||||
|
|
@@ -1731,6 +1838,7 @@ public static class Builder { | |||||||||||||
| private NotificationCenter notificationCenter; | ||||||||||||||
| private List<OptimizelyDecideOption> defaultDecideOptions; | ||||||||||||||
| private ODPManager odpManager; | ||||||||||||||
| private CmabService cmabService; | ||||||||||||||
|
|
||||||||||||||
| // For backwards compatibility | ||||||||||||||
| private AtomicProjectConfigManager fallbackConfigManager = new AtomicProjectConfigManager(); | ||||||||||||||
|
|
@@ -1842,6 +1950,16 @@ public Builder withODPManager(ODPManager odpManager) { | |||||||||||||
| return this; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| public Builder withCmabClient(CmabClient cmabClient) { | ||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @FarhanAnjum-opti It looks like we need to build withCmabService(), so others like android-sdk can inject full-featured CmabService with client + cacheSize + cacheTTL.
|
||||||||||||||
| int DEFAULT_MAX_SIZE = 1000; | ||||||||||||||
| int DEFAULT_CMAB_CACHE_TIMEOUT = 30 * 60 * 1000; | ||||||||||||||
| DefaultLRUCache<CmabCacheValue> cmabCache = new DefaultLRUCache<>(DEFAULT_MAX_SIZE, DEFAULT_CMAB_CACHE_TIMEOUT); | ||||||||||||||
|
Comment on lines
1992
to
1994
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand cmabCacheSize and cmabCacheTimeout not configurable for now? |
||||||||||||||
| CmabServiceOptions cmabServiceOptions = new CmabServiceOptions(logger, cmabCache, cmabClient); | ||||||||||||||
| DefaultCmabService defaultCmabService = new DefaultCmabService(cmabServiceOptions); | ||||||||||||||
FarhanAnjum-opti marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||
| this.cmabService = defaultCmabService; | ||||||||||||||
| return this; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // Helper functions for making testing easier | ||||||||||||||
| protected Builder withBucketing(Bucketer bucketer) { | ||||||||||||||
| this.bucketer = bucketer; | ||||||||||||||
|
|
@@ -1872,8 +1990,12 @@ public Optimizely build() { | |||||||||||||
| bucketer = new Bucketer(); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| if (cmabService == null) { | ||||||||||||||
| logger.warn("CMAB service is not initiated. CMAB functionality will not be available."); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| if (decisionService == null) { | ||||||||||||||
| decisionService = new DecisionService(bucketer, errorHandler, userProfileService); | ||||||||||||||
| decisionService = new DecisionService(bucketer, errorHandler, userProfileService, cmabService); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| if (projectConfig == null && datafile != null && !datafile.isEmpty()) { | ||||||||||||||
|
|
@@ -1916,7 +2038,7 @@ public Optimizely build() { | |||||||||||||
| defaultDecideOptions = Collections.emptyList(); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| return new Optimizely(eventHandler, eventProcessor, errorHandler, decisionService, userProfileService, projectConfigManager, optimizelyConfigManager, notificationCenter, defaultDecideOptions, odpManager); | ||||||||||||||
| return new Optimizely(eventHandler, eventProcessor, errorHandler, decisionService, userProfileService, projectConfigManager, optimizelyConfigManager, notificationCenter, defaultDecideOptions, odpManager, cmabService); | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.