[FSSDK-11177] Decision Service CMAB + Optimizely Client + Impression event adjustment #394

junaed-optimizely · 2025-10-23T15:45:28Z

Summary

Decision service change: propagate CMAB decisions through DecisionService, bucketer, decision metadata, and related core flows.
Impression event change: include CMAB variation IDs and UUIDs in impression event payloads.
Exposed CMAB configuration: add CmabConfig, factory setters, and initialization plumbing to pass cache size, TTL, and custom caches into CMAB service creation.

Test Plan

Extend CMAB-related coverage in service, factory, impression event, integration, and user-context test suites.

Issues

FSSDK-11177
FSSDK-11186

raju-opti · 2025-10-23T17:48:22Z

OptimizelySDK/Bucketing/Bucketer.cs

+        /// <param name="userId">User identifier</param>
+        /// <param name="trafficAllocations">Traffic allocations to use for bucketing</param>
+        /// <returns>Entity ID (string) if user is bucketed, null otherwise</returns>
+        public virtual Result<string> BucketToEntityId(ProjectConfig config, Experiment experiment,


We can accept ExperimentCore parameter, and reuse the part upto BucketToEntityId for the Bucket metheod below. Logic for bucket would be:

get entityId by calling BucketToEntityId()

get the variation from the config and return using the entityId from step 1

raju-opti · 2025-10-23T18:06:37Z

OptimizelySDK/Cmab/DefaultCmabService.cs

+
+            if (config.CustomCache != null)
+            {
+                _cmabCache = config.CustomCache as LruCache<CmabCacheEntry>;


we should not assume that customCache must be LruCache. It could be any implementation

the class field _cmabCache should also be of type ICache<>, not LruCache

raju-opti · 2025-10-23T18:09:50Z

OptimizelySDK/Cmab/DefaultCmabService.cs

+        /// <param name="cmabConfig">Configuration for CMAB cache. If null, default values are used.</param>
+        /// <param name="cmabClient">Client for fetching decisions from the CMAB prediction service. If null, a default client is created.</param>
+        /// <param name="logger">Optional logger for recording service operations.</param>
+        public DefaultCmabService(CmabConfig cmabConfig = null,


instead of taking in the config here, we can take in a ICache<> here and push the resposibility of supplying the cache to the caller, similar to cmabClient

I see all of them can be null. We can just make them required and put the construction responsibility to the caller

raju-opti · 2025-10-30T13:52:37Z

OptimizelySDK/Odp/ICache.cs

        void Save(string key, T value);
        T Lookup(string key);
        void Reset();
+        void Remove(string key);


This is a breaking change. We cannot just add the remove method to the old Odp ICache interface. We need a new interface that has the same methods with the remove method added. We can extend this ICache like so
interface ICacheWithRemove extends ICache.

Also, it would be better to put the new extended interface in a separate common package instead of the odp package

raju-opti · 2025-10-30T13:54:07Z

OptimizelySDK/OptimizelyDecisions/DecisionReasons.cs

+        /// <summary>
+        /// CMAB UUID associated with the decision for contextual multi-armed bandit experiments.
+        /// </summary>
+        public string CmabUuid { get; set; }


THe CmabUuid should not be in the DecisionReasons class

raju-opti · 2025-10-30T15:28:26Z

OptimizelySDK/Bucketing/DecisionService.cs

+            var userId = user.GetUserId();
+
+            // Check if CMAB is properly configured
+            if (experiment.Cmab == null)


can we get rid of this check, the caller can ensure this this is called only with cmab (in our case, it is done already)

raju-opti · 2025-10-30T15:33:20Z

OptimizelySDK/Bucketing/DecisionService.cs

+                new TrafficAllocation
+                {
+                    EntityId = "$",
+                    EndOfRange = experiment.Cmab.TrafficAllocation ?? 0,


can we just use experiment.Cmab.TrafficAllocation (without the ??)

raju-opti · 2025-10-30T15:47:00Z

OptimizelySDK/Bucketing/DecisionService.cs

+            }
+
+            // User is in CMAB traffic allocation, fetch decision from CMAB service
+            if (CmabService == null)


when the sdk is released with cmab, the cmabService must be non null cause we are instantiating it ourself, so no reason for it to be null. We can remove this check

raju-opti · 2025-10-30T17:49:14Z

OptimizelySDK/Optimizely.cs

            NotificationCenter = notificationCenter ?? new NotificationCenter(Logger);
+
+#if USE_CMAB
+            if (cmabService == null)


We will always need a cmabService. We can remove this check and just construct a cmab service

raju-opti · 2025-10-30T17:49:50Z

OptimizelySDK/Optimizely.cs

+                    cache = new LruCache<CmabCacheEntry>(cacheSize, cacheTtl, Logger);
+                }
+
+                var cmabRetryConfig = new CmabRetryConfig(1, TimeSpan.FromMilliseconds(100));


can we define these constants (1, 100) instead of hardcoding here

raju-opti · 2025-10-30T17:52:30Z

OptimizelySDK/Optimizely.cs

-                reasonsToReport);
+                reasonsToReport
+#if USE_CMAB
+                , flagDecision.CmabUuid


the publicly returned value from decide() should not have the cmabUuid, it's just an internal detail. THe OptimizelyDecision type should not change

raju-opti · 2025-10-30T17:54:30Z

OptimizelySDK/OptimizelyDecisions/OptimizelyDecision.cs

+        /// <summary>
+        /// CMAB UUID associated with the decision for contextual multi-armed bandit experiments.
+        /// </summary>
+        public string CmabUuid { get; private set; }


I see that this is the public return value for decide(). We should not add the cmabUuid here, it's only for internal purposes

raju-opti · 2025-10-30T17:55:43Z

OptimizelySDK/OptimizelyFactory.cs

+        /// </summary>
+        /// <param name="cacheSize">Maximum number of entries in the CMAB cache.</param>
+        /// <param name="cacheTtl">Time-to-live for CMAB cache entries.</param>
+        public static void SetCmabCacheConfig(int cacheSize, TimeSpan cacheTtl)


can we change to SetCmabConfig instead?

instead of passing each cmab config option separately as the method parameter, we can have a single method that takes a single CmabConfig parameter. This publicly exposed CmabConfig should be in this same package/namespace.

junaed-optimizely added 11 commits October 13, 2025 21:58

[FSSDK-11177] initial changes

887fbdf

[FSSDK-11177] improvements

35a475c

[FSSDK-11177] bucketer cmab test

8d4a7ff

[FSSDK-11177] decision service cmab test

a327881

[FSSDK-11177] test addition

d579956

[FSSDK-11177] test fix

3733e9d

[FSSDK-11981] fixes

9dc900f

[FSSDK-11981] plan removal

8ad779a

[FSSDK-11177] test adjustment

2904f1a

[FSSDK-11177] format change

ae7b3dd

[FSSDK-11177] public api exposed for cmab cache

40843ac

optimizely-codereview-bot assigned junaed-optimizely Oct 23, 2025

junaed-optimizely added 4 commits October 23, 2025 22:19

[FSSDK-11177] format fix

a568a45

[FSSDK-11177] format fix

c0ba24d

[FSSDK-11177] format fix 3

90f228c

[FSSDK-11177] format fix 4

13791c0

junaed-optimizely requested a review from raju-opti October 23, 2025 18:16

raju-opti reviewed Oct 27, 2025

View reviewed changes

junaed-optimizely added 4 commits October 27, 2025 21:42

[FSSDK-11177] bucketer common logic extraction

a217982

[FSSDK-11177] icache instead of lru

2c9fad8

[FSSDK-11177] cmab service constructor adjustment

ab8eefc

[FSSDK-11177] format fix

f1a33cc

raju-opti requested changes Oct 30, 2025

View reviewed changes

[FSSDK-11177] Decision Service CMAB + Optimizely Client + Impression event adjustment #394

Are you sure you want to change the base?

[FSSDK-11177] Decision Service CMAB + Optimizely Client + Impression event adjustment #394

Uh oh!

Conversation

junaed-optimizely commented Oct 23, 2025

Summary

Test Plan

Issues

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants