update: Refactor CMAB traffic allocation handling and enhance decision service error logging

FarhanAnjum-opti · FarhanAnjum-opti · commit 35cea8425dcf · 2025-07-23T22:02:36.000+06:00
diff --git a/lib/optimizely/bucketer.rb b/lib/optimizely/bucketer.rb
@@ -106,8 +106,8 @@ def bucket_to_entity_id(project_config, experiment, bucketing_id, user_id)
       if experiment['cmab']
         traffic_allocations = [
           {
-            entityId: '$',
-            endOfRange: experiment['cmab']['trafficAllocation']
+            'entityId' => '$',
+            'endOfRange' => experiment['cmab']['trafficAllocation']
           }
         ]
       end
diff --git a/lib/optimizely/decision_service.rb b/lib/optimizely/decision_service.rb
@@ -134,23 +134,25 @@ def get_variation(project_config, experiment_id, user_context, user_profile_trac
         cmab_decision = cmab_decision_result.result
         variation_id = cmab_decision&.variation_id
         cmab_uuid = cmab_decision&.cmab_uuid
+        variation = project_config.get_variation_from_id_by_experiment_id(experiment_id, variation_id)
       else
         # Bucket normally
         variation, bucket_reasons = @bucketer.bucket(project_config, experiment, bucketing_id, user_id)
         decide_reasons.push(*bucket_reasons)
         variation_id = variation ? variation['id'] : nil
         cmab_uuid = nil
-        message = ''
-        if variation_id
-          variation_key = variation['key']
-          message = "User '#{user_id}' is in variation '#{variation_key}' of experiment '#{experiment_id}'."
-        else
-          message = "User '#{user_id}' is in no variation."
-        end
-        @logger.log(Logger::INFO, message)
-        decide_reasons.push(message)
       end
 
+      variation_key = variation['key'] if variation
+      message = if variation_id
+                  "User '#{user_id}' is in variation '#{variation_key}' of experiment '#{experiment_id}'."
+                else
+                  "User '#{user_id}' is in no variation."
+                end
+
+      @logger.log(Logger::INFO, message)
+      decide_reasons.push(message) if message
+
       # Persist bucketing decision
       user_profile_tracker.update_user_profile(experiment_id, variation_id) unless should_ignore_user_profile_service && user_profile_tracker
       VariationResult.new(cmab_uuid, false, decide_reasons, variation_id)
@@ -236,8 +238,6 @@ def get_variation_for_feature_experiment(project_config, feature_flag, user_cont
         variation_id = variation_result.variation_id
         cmab_uuid = variation_result.cmab_uuid
         decide_reasons.push(*reasons_received)
-        puts 'final reasons'
-        puts decide_reasons
         next unless variation_id
 
         variation = project_config.get_variation_from_id_by_experiment_id(experiment_id, variation_id)
@@ -516,7 +516,7 @@ def get_decision_for_cmab_experiment(project_config, experiment, user_context, b
         message = "User \"#{user_context.user_id}\" not in CMAB experiment \"#{experiment['key']}\" due to traffic allocation."
         @logger.log(Logger::INFO, message)
         decide_reasons.push(message)
-        CmabDecisionResult.new(false, nil, decide_reasons)
+        return CmabDecisionResult.new(false, nil, decide_reasons)
       end
 
       # User is in CMAB allocation, proceed to CMAB decision
diff --git a/spec/decision_service_spec.rb b/spec/decision_service_spec.rb
@@ -714,10 +714,10 @@
         decision_result = decision_service.get_variation_for_feature_rollout(config, feature_flag, user_context)
         expect(decision_result.decision).to eq(nil)
         expect(decision_result.reasons).to eq([
-                                "User 'user_1' does not meet the conditions for targeting rule '1'.",
-                                "User 'user_1' does not meet the conditions for targeting rule '2'.",
-                                "User 'user_1' does not meet the conditions for targeting rule 'Everyone Else'."
-                              ])
+                                                "User 'user_1' does not meet the conditions for targeting rule '1'.",
+                                                "User 'user_1' does not meet the conditions for targeting rule '2'.",
+                                                "User 'user_1' does not meet the conditions for targeting rule 'Everyone Else'."
+                                              ])
 
         # verify we tried to bucket in all targeting rules and the everyone else rule
         expect(Optimizely::Audience).to have_received(:user_meets_audience_conditions?).once
@@ -937,4 +937,234 @@
       expect(reasons).to eq(["Variation 'control' is mapped to experiment '111127' and user 'test_user_2' in the forced variation map"])
     end
   end
+  describe 'CMAB experiments' do
+    describe 'when user is in traffic allocation' do
+      it 'should return correct variation and CMAB UUID from CMAB service' do
+        # Create a CMAB experiment configuration
+        cmab_experiment = {
+          'id' => '111150',
+          'key' => 'cmab_experiment',
+          'status' => 'Running',
+          'layerId' => '111150',
+          'audienceIds' => [],
+          'forcedVariations' => {},
+          'variations' => [
+            {'id' => '111151', 'key' => 'variation_1'},
+            {'id' => '111152', 'key' => 'variation_2'}
+          ],
+          'trafficAllocation' => [
+            {'entityId' => '111151', 'endOfRange' => 5000},
+            {'entityId' => '111152', 'endOfRange' => 10_000}
+          ],
+          'cmab' => {'trafficAllocation' => 5000}
+        }
+        user_context = project_instance.create_user_context('test_user', {})
+
+        # Mock experiment lookup to return our CMAB experiment
+        allow(config).to receive(:get_experiment_from_id).with('111150').and_return(cmab_experiment)
+        allow(config).to receive(:experiment_running?).with(cmab_experiment).and_return(true)
+
+        # Mock audience evaluation to pass
+        allow(Optimizely::Audience).to receive(:user_meets_audience_conditions?).and_return([true, []])
+
+        # Mock bucketer to return a valid entity ID (user is in traffic allocation)
+        allow(decision_service.bucketer).to receive(:bucket_to_entity_id)
+          .with(config, cmab_experiment, 'test_user', 'test_user')
+          .and_return(['$', []])
+
+        # Mock CMAB service to return a decision
+        allow(spy_cmab_service).to receive(:get_decision)
+          .with(config, user_context, '111150', [])
+          .and_return(Optimizely::CmabDecision.new(variation_id: '111151', cmab_uuid: 'test-cmab-uuid-123'))
+
+        # Mock variation lookup
+        allow(config).to receive(:get_variation_from_id_by_experiment_id)
+          .with('111150', '111151')
+          .and_return({'id' => '111151', 'key' => 'variation_1'})
+
+        variation_result = decision_service.get_variation(config, '111150', user_context)
+
+        expect(variation_result.variation_id).to eq('111151')
+        expect(variation_result.cmab_uuid).to eq('test-cmab-uuid-123')
+        expect(variation_result.error).to eq(false)
+        expect(variation_result.reasons).to include(
+          "User 'test_user' is in variation 'variation_1' of experiment '111150'."
+        )
+
+        # Verify CMAB service was called
+        expect(spy_cmab_service).to have_received(:get_decision).once
+      end
+    end
+
+    describe 'when user is not in traffic allocation' do
+      it 'should return nil variation and log traffic allocation message' do
+        cmab_experiment = {
+          'id' => '111150',
+          'key' => 'cmab_experiment',
+          'status' => 'Running',
+          'layerId' => '111150',
+          'audienceIds' => [],
+          'forcedVariations' => {},
+          'variations' => [
+            {'id' => '111151', 'key' => 'variation_1'}
+          ],
+          'trafficAllocation' => [
+            {'entityId' => '111151', 'endOfRange' => 10_000}
+          ],
+          'cmab' => {'trafficAllocation' => 1000}
+        }
+        user_context = project_instance.create_user_context('test_user', {})
+
+        # Mock experiment lookup to return our CMAB experiment
+        allow(config).to receive(:get_experiment_from_id).with('111150').and_return(cmab_experiment)
+        allow(config).to receive(:experiment_running?).with(cmab_experiment).and_return(true)
+
+        # Mock audience evaluation to pass
+        allow(Optimizely::Audience).to receive(:user_meets_audience_conditions?).and_return([true, []])
+
+        variation_result = decision_service.get_variation(config, '111150', user_context)
+
+        expect(variation_result.variation_id).to eq(nil)
+        expect(variation_result.cmab_uuid).to eq(nil)
+        expect(variation_result.error).to eq(false)
+        expect(variation_result.reasons).to include(
+          'User "test_user" not in CMAB experiment "cmab_experiment" due to traffic allocation.'
+        )
+
+        # Verify CMAB service was not called since user is not in traffic allocation
+        expect(spy_cmab_service).not_to have_received(:get_decision)
+      end
+    end
+
+    describe 'when CMAB service returns an error' do
+      it 'should return nil variation and include error in reasons' do
+        cmab_experiment = {
+          'id' => '111150',
+          'key' => 'cmab_experiment',
+          'status' => 'Running',
+          'layerId' => '111150',
+          'audienceIds' => [],
+          'forcedVariations' => {},
+          'variations' => [
+            {'id' => '111151', 'key' => 'variation_1'}
+          ],
+          'trafficAllocation' => [
+            {'entityId' => '111151', 'endOfRange' => 10_000}
+          ],
+          'cmab' => {'trafficAllocation' => 5000}
+        }
+        user_context = project_instance.create_user_context('test_user', {})
+
+        # Mock experiment lookup to return our CMAB experiment
+        allow(config).to receive(:get_experiment_from_id).with('111150').and_return(cmab_experiment)
+        allow(config).to receive(:experiment_running?).with(cmab_experiment).and_return(true)
+
+        # Mock audience evaluation to pass
+        allow(Optimizely::Audience).to receive(:user_meets_audience_conditions?).and_return([true, []])
+
+        # Mock bucketer to return a valid entity ID (user is in traffic allocation)
+        allow(decision_service.bucketer).to receive(:bucket_to_entity_id)
+          .with(config, cmab_experiment, 'test_user', 'test_user')
+          .and_return(['$', []])
+
+        # Mock CMAB service to return an error
+        allow(spy_cmab_service).to receive(:get_decision)
+          .with(config, user_context, '111150', [])
+          .and_raise(StandardError.new('CMAB service error'))
+
+        variation_result = decision_service.get_variation(config, '111150', user_context)
+
+        expect(variation_result.variation_id).to be_nil
+        expect(variation_result.cmab_uuid).to be_nil
+        expect(variation_result.error).to eq(true)
+        expect(variation_result.reasons).to include(
+          "Failed to fetch CMAB decision for experiment 'cmab_experiment'"
+        )
+
+        # Verify CMAB service was called but errored
+        expect(spy_cmab_service).to have_received(:get_decision).once
+      end
+    end
+
+    describe 'when user has forced variation' do
+      it 'should return forced variation and skip CMAB service call' do
+        # Use a real experiment from the datafile and modify it to be a CMAB experiment
+        real_experiment = config.get_experiment_from_key('test_experiment')
+        cmab_experiment = real_experiment.dup
+        cmab_experiment['cmab'] = {'trafficAllocation' => 5000}
+
+        user_context = project_instance.create_user_context('test_user', {})
+
+        # Set up forced variation first (using real experiment that exists in datafile)
+        decision_service.set_forced_variation(config, 'test_experiment', 'test_user', 'variation')
+
+        # Mock the experiment to be a CMAB experiment after setting forced variation
+        allow(config).to receive(:get_experiment_from_id).with('111127').and_return(cmab_experiment)
+        allow(config).to receive(:experiment_running?).with(cmab_experiment).and_return(true)
+
+        # Add spy for bucket_to_entity_id method
+        allow(decision_service.bucketer).to receive(:bucket_to_entity_id).and_call_original
+
+        variation_result = decision_service.get_variation(config, '111127', user_context)
+
+        expect(variation_result.variation_id).to eq('111129')
+        expect(variation_result.cmab_uuid).to be_nil
+        expect(variation_result.error).to eq(false)
+        expect(variation_result.reasons).to include(
+          "Variation 'variation' is mapped to experiment '111127' and user 'test_user' in the forced variation map"
+        )
+
+        # Verify CMAB service was not called since user has forced variation
+        expect(spy_cmab_service).not_to have_received(:get_decision)
+        # Verify bucketer was not called since forced variations short-circuit bucketing
+        expect(decision_service.bucketer).not_to have_received(:bucket_to_entity_id)
+      end
+    end
+
+    describe 'when user has whitelisted variation' do
+      it 'should return whitelisted variation and skip CMAB service call' do
+        # Create a CMAB experiment with whitelisted users
+        cmab_experiment = {
+          'id' => '111150',
+          'key' => 'cmab_experiment',
+          'status' => 'Running',
+          'layerId' => '111150',
+          'audienceIds' => [],
+          'forcedVariations' => {
+            'whitelisted_user' => '111151' # User is whitelisted to variation_1
+          },
+          'variations' => [
+            {'id' => '111151', 'key' => 'variation_1'},
+            {'id' => '111152', 'key' => 'variation_2'}
+          ],
+          'trafficAllocation' => [
+            {'entityId' => '111151', 'endOfRange' => 5000},
+            {'entityId' => '111152', 'endOfRange' => 10_000}
+          ],
+          'cmab' => {'trafficAllocation' => 5000}
+        }
+        user_context = project_instance.create_user_context('whitelisted_user', {})
+
+        # Mock experiment lookup to return our CMAB experiment
+        allow(config).to receive(:get_experiment_from_id).with('111150').and_return(cmab_experiment)
+        allow(config).to receive(:experiment_running?).with(cmab_experiment).and_return(true)
+
+        # Mock the get_whitelisted_variation_id method directly
+        allow(decision_service).to receive(:get_whitelisted_variation_id)
+          .with(config, '111150', 'whitelisted_user')
+          .and_return(['111151', "User 'whitelisted_user' is whitelisted into variation 'variation_1' of experiment '111150'."])
+
+        variation_result = decision_service.get_variation(config, '111150', user_context)
+
+        expect(variation_result.variation_id).to eq('111151')
+        expect(variation_result.cmab_uuid).to be_nil
+        expect(variation_result.error).to eq(false)
+        expect(variation_result.reasons).to include(
+          "User 'whitelisted_user' is whitelisted into variation 'variation_1' of experiment '111150'."
+        )
+        # Verify CMAB service was not called since user is whitelisted
+        expect(spy_cmab_service).not_to have_received(:get_decision)
+      end
+    end
+  end
 end
diff --git a/spec/project_spec.rb b/spec/project_spec.rb
@@ -1992,9 +1992,9 @@ def callback(_args); end
         )
         # Ensure featureEnabled is false for this test
         expect(variation_to_return['featureEnabled']).to be false
-  
+
         allow(project_instance.decision_service).to receive(:get_variation_for_feature).and_return(Optimizely::DecisionService::DecisionResult.new(decision_to_return, false, []))
-        
+
         expect(project_instance.notification_center).to receive(:send_notifications).once.with(
           Optimizely::NotificationCenter::NOTIFICATION_TYPES[:LOG_EVENT], any_args
         ).ordered
@@ -4281,6 +4281,33 @@ def callback(_args); end
                   ])
       end
     end
+    describe 'when decision service fails with CMAB error' do
+      it 'should return error decision when CMAB decision service fails' do
+        # Add the HTTP stub to prevent real requests
+        stub_request(:post, 'https://logx.optimizely.com/v1/events')
+          .to_return(status: 200, body: '', headers: {})
+
+        feature_flag_key = 'boolean_single_variable_feature'
+
+        # Mock the decision service to return an error result
+        error_decision_result = double('DecisionResult')
+        allow(error_decision_result).to receive(:decision).and_return(nil)
+        allow(error_decision_result).to receive(:error).and_return(true)
+        allow(error_decision_result).to receive(:reasons).and_return(['CMAB service failed to fetch decision'])
+
+        # Mock get_variations_for_feature_list instead of get_variation_for_feature
+        allow(project_instance.decision_service).to receive(:get_variations_for_feature_list)
+          .and_return([error_decision_result])
+
+        user_context = project_instance.create_user_context('test_user')
+        decision = user_context.decide(feature_flag_key)
+
+        expect(decision.enabled).to eq(false)
+        expect(decision.variation_key).to be_nil
+        expect(decision.flag_key).to eq(feature_flag_key)
+        expect(decision.reasons).to include('CMAB service failed to fetch decision')
+      end
+    end
   end
 
   describe '#decide_all' do

Original file line number	Diff line number	Diff line change
`@@ -106,8 +106,8 @@ def bucket_to_entity_id(project_config, experiment, bucketing_id, user_id)`
`106`	`106`	`if experiment['cmab']`
`107`	`107`	`traffic_allocations = [`
`108`	`108`	`{`
`109`		`- entityId: '$',`
`110`		`- endOfRange: experiment['cmab']['trafficAllocation']`
	`109`	`+ 'entityId' => '$',`
	`110`	`+ 'endOfRange' => experiment['cmab']['trafficAllocation']`
`111`	`111`	`}`
`112`	`112`	`]`
`113`	`113`	`end`