diff --git a/optimizely/decision_service.py b/optimizely/decision_service.py index b12352c2..6ebc089d 100644 --- a/optimizely/decision_service.py +++ b/optimizely/decision_service.py @@ -65,7 +65,7 @@ class VariationResult(TypedDict): class _DecisionResultOptional(TypedDict, total=False): - holdout_decision: Decision + holdout_decisions: List[Decision] class DecisionResult(_DecisionResultOptional): @@ -552,7 +552,8 @@ def get_variation( } def get_variation_for_rollout( - self, project_config: ProjectConfig, feature: entities.FeatureFlag, user_context: OptimizelyUserContext + self, project_config: ProjectConfig, feature: entities.FeatureFlag, user_context: OptimizelyUserContext, + holdout_decisions: Optional[list[Decision]] = None ) -> tuple[Decision, list[str]]: """ Determine which experiment/variation the user is in for a given rollout. Returns the variation of the first experiment the user qualifies for. @@ -563,6 +564,7 @@ def get_variation_for_rollout( rollout: Rollout for which we are getting the variation. user: ID and attributes for user. options: Decide options. + holdout_decisions: If given, local holdout hits are appended to it. Returns: Decision namedtuple consisting of experiment and variation for the user and @@ -606,22 +608,14 @@ def get_variation_for_rollout( return Decision(experiment=rule, variation=forced_decision_variation, source=enums.DecisionSources.ROLLOUT, cmab_uuid=None), decide_reasons - local_holdouts = project_config.get_holdouts_for_rule(rule.id) - for holdout in local_holdouts: - local_holdout_decision = self.get_variation_for_holdout( - holdout, user_context, project_config - ) - decide_reasons.extend(local_holdout_decision['reasons']) - - local_decision = local_holdout_decision['decision'] - if local_decision.variation is not None: - message = ( - f"The user '{user_id}' is bucketed into local holdout '{holdout.key}' " - f"for delivery rule '{rule.key}'." - ) - self.logger.info(message) - decide_reasons.append(message) - return local_decision, decide_reasons + local_holdout_hit = self._find_local_holdout_hit( + project_config, rule, 'delivery', user_context, decide_reasons + ) + if local_holdout_hit is not None: + if holdout_decisions is not None: + holdout_decisions.append(local_holdout_hit) + index += 1 + continue bucketing_id, bucket_reasons = self._get_bucketing_id(user_id, attributes) decide_reasons += bucket_reasons @@ -748,6 +742,7 @@ def get_decision_for_flag( global_holdout_result: DecisionResult | None = None global_holdout_key: str | None = None + holdout_decisions: list[Decision] = [] # Check global holdouts (flag level — before any rules are evaluated) global_holdouts = project_config.get_global_holdouts() @@ -781,6 +776,7 @@ def get_decision_for_flag( reasons.append(message) global_holdout_result = holdout_decision global_holdout_key = holdout.key + holdout_decisions.append(holdout_decision['decision']) break # Check experiments then rollouts @@ -807,30 +803,16 @@ def get_decision_for_flag( 'error': False, 'reasons': reasons } - if global_holdout_result is not None: - result['holdout_decision'] = global_holdout_result['decision'] + if holdout_decisions: + result['holdout_decisions'] = holdout_decisions return result - local_holdouts = project_config.get_holdouts_for_rule(experiment.id) - for holdout in local_holdouts: - local_holdout_decision = self.get_variation_for_holdout( - holdout, user_context, project_config - ) - reasons.extend(local_holdout_decision['reasons']) - - local_decision = local_holdout_decision['decision'] - if local_decision.variation is not None: - message = ( - f"The user '{user_id}' is bucketed into local holdout '{holdout.key}' " - f"for experiment rule '{experiment.key}'." - ) - self.logger.info(message) - reasons.append(message) - return { - 'decision': local_holdout_decision['decision'], - 'error': False, - 'reasons': reasons - } + local_holdout_hit = self._find_local_holdout_hit( + project_config, experiment, 'experiment', user_context, reasons + ) + if local_holdout_hit is not None: + holdout_decisions.append(local_holdout_hit) + continue # Get variation for experiment variation_result = self.get_variation( @@ -856,8 +838,8 @@ def get_decision_for_flag( 'error': False, 'reasons': reasons } - if global_holdout_result is not None: - result['holdout_decision'] = global_holdout_result['decision'] + if holdout_decisions: + result['holdout_decisions'] = holdout_decisions return result if global_holdout_result is not None: @@ -870,7 +852,7 @@ def get_decision_for_flag( # If no experiment decision, check rollouts rollout_decision, rollout_reasons = self.get_variation_for_rollout( - project_config, feature_flag, user_context + project_config, feature_flag, user_context, holdout_decisions ) if rollout_reasons: reasons.extend(rollout_reasons) @@ -893,10 +875,32 @@ def get_decision_for_flag( 'error': False, 'reasons': reasons } - if global_holdout_result is not None: - final_result['holdout_decision'] = global_holdout_result['decision'] + if holdout_decisions: + final_result['holdout_decisions'] = holdout_decisions return final_result + def _find_local_holdout_hit( + self, + project_config: ProjectConfig, + rule: entities.Experiment, + rule_type: str, + user_context: OptimizelyUserContext, + reasons: list[str] + ) -> Optional[Decision]: + """Returns the decision of the first local holdout on the rule that buckets the user, else None.""" + for holdout in project_config.get_holdouts_for_rule(rule.id): + holdout_result = self.get_variation_for_holdout(holdout, user_context, project_config) + reasons.extend(holdout_result['reasons']) + if holdout_result['decision'].variation is not None: + message = ( + f"The user '{user_context.user_id}' is bucketed into local holdout '{holdout.key}' " + f"for {rule_type} rule '{rule.key}'. Skipping to the next rule." + ) + self.logger.info(message) + reasons.append(message) + return holdout_result['decision'] + return None + def get_variation_for_holdout( self, holdout: entities.Holdout, diff --git a/optimizely/optimizely.py b/optimizely/optimizely.py index 0f7c1da5..992a4f39 100644 --- a/optimizely/optimizely.py +++ b/optimizely/optimizely.py @@ -1246,7 +1246,7 @@ def _create_optimizely_decision( decision_reasons: Optional[list[str]], decide_options: list[str], project_config: ProjectConfig, - holdout_decision: Optional[Decision] = None + holdout_decisions: Optional[list[Decision]] = None ) -> OptimizelyDecision: user_id = user_context.user_id feature_enabled = False @@ -1265,24 +1265,22 @@ def _create_optimizely_decision( feature_flag = project_config.feature_key_map.get(flag_key) - # Send holdout impression when user was bucketed into a holdout bypassed due to exclude_targeted_deliveries - if (holdout_decision is not None - and decision_source != DecisionSources.HOLDOUT - and OptimizelyDecideOption.DISABLE_DECISION_EVENT not in decide_options): - holdout_enabled = self._get_feature_enabled(holdout_decision.variation) - holdout_rule_key = holdout_decision.experiment.key if holdout_decision.experiment else '' - self._send_impression_event( - project_config, - holdout_decision.experiment, - holdout_decision.variation, - flag_key, - holdout_rule_key, - str(DecisionSources.HOLDOUT), - holdout_enabled, - user_id, - attributes, - holdout_decision.cmab_uuid - ) + # Holdouts the user was bucketed into without ending evaluation: local holdout hits and + # global holdouts bypassed due to exclude_targeted_deliveries. Sent regardless of send_flag_decisions. + if holdout_decisions and OptimizelyDecideOption.DISABLE_DECISION_EVENT not in decide_options: + for holdout_decision in holdout_decisions: + self._send_impression_event( + project_config, + holdout_decision.experiment, + holdout_decision.variation, + flag_key, + holdout_decision.experiment.key if holdout_decision.experiment else '', + str(DecisionSources.HOLDOUT), + self._get_feature_enabled(holdout_decision.variation), + user_id, + attributes, + holdout_decision.cmab_uuid + ) decision_event_dispatched = True # Send impression event if Decision came from a feature @@ -1469,13 +1467,13 @@ def _decide_for_keys( user_context, merged_decide_options ) - holdout_decisions: dict[str, Optional[Decision]] = {} + holdout_decisions: dict[str, list[Decision]] = {} for i in range(0, len(flags_without_forced_decision)): decision = decision_list[i]['decision'] reasons = decision_list[i]['reasons'] error = decision_list[i]['error'] flag_key = flags_without_forced_decision[i].key - holdout_decisions[flag_key] = decision_list[i].get('holdout_decision') + holdout_decisions[flag_key] = decision_list[i].get('holdout_decisions', []) # store error decision against key and remove key from valid keys if error: optimizely_decision = OptimizelyDecision.new_error_decision(flags_without_forced_decision[i].key, @@ -1496,7 +1494,7 @@ def _decide_for_keys( decision_reasons, merged_decide_options, project_config, - holdout_decision=holdout_decisions.get(key) + holdout_decisions=holdout_decisions.get(key) ) enabled_flags_only_missing = OptimizelyDecideOption.ENABLED_FLAGS_ONLY not in merged_decide_options is_enabled = optimizely_decision.enabled diff --git a/tests/test_decision_service.py b/tests/test_decision_service.py index b38a03b2..9d25a32d 100644 --- a/tests/test_decision_service.py +++ b/tests/test_decision_service.py @@ -1510,7 +1510,7 @@ def test_get_variation_for_feature__returns_variation_for_feature_in_rollout(sel ) mock_get_variation_for_rollout.assert_called_once_with( - self.project_config, feature, user + self.project_config, feature, user, [] ) # Assert no log messages were generated diff --git a/tests/test_decision_service_holdout.py b/tests/test_decision_service_holdout.py index da85ae4e..96894647 100644 --- a/tests/test_decision_service_holdout.py +++ b/tests/test_decision_service_holdout.py @@ -12,6 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +import copy import json from unittest import mock @@ -1467,8 +1468,8 @@ def test_global_holdout_miss_falls_through_to_experiment(self): # Branch 2: Local holdout hit — user bucketed into local holdout for rule X # ------------------------------------------------------------------ - def test_local_holdout_hit_returns_holdout_decision_for_experiment_rule(self): - """User bucketed into local holdout for experiment rule X returns holdout decision. + def test_local_holdout_hit_skips_experiment_rule(self): + """User bucketed into local holdout for experiment rule X is excluded from rule X only. The experiment rule ID is '111127' (test_experiment linked to test_feature_in_experiment). The local holdout targets only this rule with full traffic. @@ -1488,14 +1489,13 @@ def test_local_holdout_hit_returns_holdout_decision_for_experiment_rule(self): user_ctx = opt.create_user_context('user_in_local_holdout', {}) result = ds.get_decision_for_flag(feature_flag, user_ctx, config) - decision = result['decision'] - # User must be caught by local holdout - self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT) - # Regular experiment evaluation (get_variation) must not have run + # Rule X is skipped and the hit is recorded; the decision does not come from the holdout + self.assertNotEqual(result['decision'].source, enums.DecisionSources.HOLDOUT) + self.assertEqual([d.experiment.key for d in result['holdout_decisions']], ['local_for_exp']) mock_get_var.assert_not_called() - def test_local_holdout_hit_returns_holdout_decision_for_delivery_rule(self): - """User bucketed into local holdout for delivery/rollout rule returns holdout decision. + def test_local_holdout_hit_skips_delivery_rule(self): + """User bucketed into local holdout for the last delivery rule ends with no rule serving. Rule '211147' is the everyone-else rollout rule for 'test_feature_in_rollout'. The local holdout targets this delivery rule with full traffic. @@ -1512,9 +1512,8 @@ def test_local_holdout_hit_returns_holdout_decision_for_delivery_rule(self): user_ctx = opt.create_user_context('user_delivery_holdout', {}) result = ds.get_decision_for_flag(feature_flag, user_ctx, config) - self.assertIsNotNone(result) - decision = result['decision'] - self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT) + self.assertIsNone(result['decision'].variation) + self.assertEqual([d.experiment.key for d in result['holdout_decisions']], ['local_for_delivery']) # ------------------------------------------------------------------ # Branch 3: Local holdout miss — user falls through to regular rule evaluation @@ -1607,7 +1606,8 @@ def test_local_holdout_applies_to_experiment_rule(self): user_ctx = opt.create_user_context('user_exp_holdout', {}) result = ds.get_decision_for_flag(feature_flag, user_ctx, config) - self.assertEqual(result['decision'].source, enums.DecisionSources.HOLDOUT) + self.assertNotEqual(result['decision'].source, enums.DecisionSources.HOLDOUT) + self.assertEqual(len(result['holdout_decisions']), 1) def test_local_holdout_applies_to_rollout_delivery_rule(self): """Local holdout check applies to rollout/delivery rules in get_decision_for_flag.""" @@ -1624,9 +1624,8 @@ def test_local_holdout_applies_to_rollout_delivery_rule(self): user_ctx = opt.create_user_context('user_delivery_holdout', {}) result = ds.get_decision_for_flag(feature_flag, user_ctx, config) - self.assertIsNotNone(result) - decision = result['decision'] - self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT) + self.assertNotEqual(result['decision'].source, enums.DecisionSources.HOLDOUT) + self.assertEqual(len(result['holdout_decisions']), 1) # ------------------------------------------------------------------ # Precedence: Global → Forced → Local → Regular rule @@ -1858,7 +1857,7 @@ def test_global_holdout_exclude_td_true_blocks_ab_experiment(self): decision = result['decision'] self.assertNotEqual(decision.source, enums.DecisionSources.HOLDOUT) mock_get_var.assert_not_called() - self.assertIsNotNone(result.get('holdout_decision')) + self.assertEqual(len(result['holdout_decisions']), 1) # ------------------------------------------------------------------ # Test 4: Global holdout with exclude_targeted_deliveries=True, no TD matches @@ -1880,7 +1879,7 @@ def test_global_holdout_exclude_td_true_no_td_returns_non_holdout_decision(self) decision = result['decision'] self.assertNotEqual(decision.source, enums.DecisionSources.HOLDOUT) - self.assertIsNotNone(result.get('holdout_decision')) + self.assertEqual(len(result['holdout_decisions']), 1) expected_reason = ( "Holdout \"global_exclude_td\" has excludeTargetedDeliveries enabled, " @@ -1910,8 +1909,8 @@ def test_local_holdout_delivery_rule_exclude_td_true_still_applies(self): user_ctx = opt.create_user_context('user_delivery_exclude', {}) result = ds.get_decision_for_flag(feature_flag, user_ctx, config) - decision = result['decision'] - self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT) + self.assertNotEqual(result['decision'].source, enums.DecisionSources.HOLDOUT) + self.assertEqual(len(result['holdout_decisions']), 1) # ------------------------------------------------------------------ # Test 6: Local holdout on experiment rule (TD type) with exclude_targeted_deliveries=True @@ -1936,8 +1935,8 @@ def test_local_holdout_td_experiment_exclude_td_true_still_applies(self): user_ctx = opt.create_user_context('testUserId', {}) result = ds.get_decision_for_flag(feature_flag, user_ctx, config) - decision = result['decision'] - self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT) + self.assertNotEqual(result['decision'].source, enums.DecisionSources.HOLDOUT) + self.assertEqual(len(result['holdout_decisions']), 1) # ------------------------------------------------------------------ # Test 7: Local holdout on experiment rule (A/B type) with exclude_targeted_deliveries=True @@ -1964,9 +1963,9 @@ def test_local_holdout_ab_experiment_exclude_td_true_still_applies(self): user_ctx = opt.create_user_context('user_ab_holdout', {}) result = ds.get_decision_for_flag(feature_flag, user_ctx, config) - decision = result['decision'] - # A/B experiment should still be blocked by local holdout - self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT) + # A/B experiment is still skipped by the local holdout + self.assertNotEqual(result['decision'].source, enums.DecisionSources.HOLDOUT) + self.assertEqual(len(result['holdout_decisions']), 1) mock_get_var.assert_not_called() # ------------------------------------------------------------------ @@ -2017,8 +2016,8 @@ def test_global_holdout_exclude_td_true_td_match_has_holdout_decision(self): decision = result['decision'] self.assertNotEqual(decision.source, enums.DecisionSources.HOLDOUT) - self.assertIsNotNone(result.get('holdout_decision')) - self.assertEqual(result['holdout_decision'].source, enums.DecisionSources.HOLDOUT) + self.assertEqual(len(result['holdout_decisions']), 1) + self.assertEqual(result['holdout_decisions'][0].source, enums.DecisionSources.HOLDOUT) # ------------------------------------------------------------------ # Test 10: No TD match returns non-holdout with holdout_decision attached @@ -2040,9 +2039,7 @@ def test_global_holdout_exclude_td_true_no_td_has_holdout_decision(self): decision = result['decision'] self.assertNotEqual(decision.source, enums.DecisionSources.HOLDOUT) - holdout_dec = result.get('holdout_decision') - self.assertIsNotNone(holdout_dec) - self.assertEqual(holdout_dec.source, enums.DecisionSources.HOLDOUT) + self.assertEqual([d.source for d in result['holdout_decisions']], [enums.DecisionSources.HOLDOUT]) expected_reason = ( "Holdout \"global_exclude_td\" has excludeTargetedDeliveries enabled, " @@ -2090,3 +2087,101 @@ def capture_notification(notification_type: str, user_id: str, captured_notifications[0].get('decision_event_dispatched'), 'decision_event_dispatched should be True when holdout impression is sent' ) + + +class LocalHoldoutContinueEvaluationTest(base.BaseTest): + """A local holdout hit excludes the user from that rule only; evaluation continues.""" + + def tearDown(self): + if hasattr(self, 'opt_obj'): + self.opt_obj.close() + + def _make_opt(self, local_holdouts, send_flag_decisions=True, experiment_flag_has_rollout=False): + cfg = copy.deepcopy(self.config_dict_with_features) + cfg['holdouts'] = [] + cfg['localHoldouts'] = local_holdouts + cfg['sendFlagDecisions'] = send_flag_decisions + # Everyone Else rule '211147' takes all traffic so a later rule always serves. + for rollout in cfg['rollouts']: + for rule in rollout['experiments']: + if rule['id'] == '211147': + rule['trafficAllocation'] = [{'entityId': '211149', 'endOfRange': 10000}] + if experiment_flag_has_rollout: + for flag in cfg['featureFlags']: + if flag['key'] == 'test_feature_in_experiment': + flag['rolloutId'] = '211111' + self.opt_obj = optimizely_module.Optimizely(json.dumps(cfg)) + return self.opt_obj + + def _decide(self, opt, flag_key): + with mock.patch.object(opt, '_send_impression_event') as mock_send: + decision = opt.create_user_context('test_user', {}).decide(flag_key) + # (rule_key, rule_type) per impression + return decision, [(c.args[4], c.args[5]) for c in mock_send.call_args_list] + + def test_holdout_hit_then_later_rule_serves(self): + opt = self._make_opt( + [_holdout('lh1', 'local_exp', included_rules=['111127'])], + experiment_flag_has_rollout=True, + ) + decision, impressions = self._decide(opt, 'test_feature_in_experiment') + + self.assertEqual(decision.rule_key, '211147') + self.assertEqual(impressions, [ + ('local_exp', str(enums.DecisionSources.HOLDOUT)), + ('211147', str(enums.DecisionSources.ROLLOUT)), + ]) + + def test_holdout_hits_on_several_rules_send_several_holdout_events(self): + opt = self._make_opt( + [ + _holdout('lh1', 'local_rule_1', included_rules=['211127']), + _holdout('lh2', 'local_rule_2', included_rules=['211137']), + ], + send_flag_decisions=False, + ) + decision, impressions = self._decide(opt, 'test_feature_in_rollout') + + self.assertEqual(decision.rule_key, '211147') + self.assertEqual(impressions, [ + ('local_rule_1', str(enums.DecisionSources.HOLDOUT)), + ('local_rule_2', str(enums.DecisionSources.HOLDOUT)), + ]) + + def test_first_matching_holdout_per_rule_wins(self): + opt = self._make_opt( + [ + _holdout('lh1', 'first', included_rules=['211127']), + _holdout('lh2', 'second', included_rules=['211127']), + ], + send_flag_decisions=False, + ) + _, impressions = self._decide(opt, 'test_feature_in_rollout') + + self.assertEqual(impressions, [('first', str(enums.DecisionSources.HOLDOUT))]) + + def test_holdout_hit_then_no_rule_serves(self): + opt = self._make_opt([_holdout('lh1', 'local_exp', included_rules=['111127'])], send_flag_decisions=False) + decision, impressions = self._decide(opt, 'test_feature_in_experiment') + + self.assertIsNone(decision.variation_key) + self.assertFalse(decision.enabled) + self.assertEqual(impressions, [('local_exp', str(enums.DecisionSources.HOLDOUT))]) + + def test_delivery_holdout_hit_sends_event_with_send_flag_decisions_off(self): + opt = self._make_opt( + [_holdout('lh1', 'local_delivery', included_rules=['211147'])], + send_flag_decisions=False, + ) + _, impressions = self._decide(opt, 'test_feature_in_rollout') + + self.assertEqual(impressions, [('local_delivery', str(enums.DecisionSources.HOLDOUT))]) + + def test_disable_decision_event_suppresses_holdout_events(self): + opt = self._make_opt([_holdout('lh1', 'local_delivery', included_rules=['211147'])]) + with mock.patch.object(opt, '_send_impression_event') as mock_send: + opt.create_user_context('test_user', {}).decide( + 'test_feature_in_rollout', [OptimizelyDecideOption.DISABLE_DECISION_EVENT] + ) + + mock_send.assert_not_called()