From dbbc0515388eb0a4ca316d07688a7aa898b4531b Mon Sep 17 00:00:00 2001 From: msohailhussain Date: Thu, 2 Dec 2021 11:11:55 -0800 Subject: [PATCH] Refact: Refactored Forced decision (#365) * project config refactor * use existing loop to generate flag_variation_map * get_variation_from_experiment_rule and get_variation_from_delivery_rule removed * fsc test fix * comment addressed * commented code removed * comments from main forced decision PR resolved Co-authored-by: ozayr-zaviar --- optimizely/decision_service.py | 212 ++++++++++++++------------------- optimizely/project_config.py | 84 ++++--------- tests/test_decision_service.py | 4 +- 3 files changed, 116 insertions(+), 184 deletions(-) diff --git a/optimizely/decision_service.py b/optimizely/decision_service.py index c16a1be2..e3e3079b 100644 --- a/optimizely/decision_service.py +++ b/optimizely/decision_service.py @@ -349,153 +349,94 @@ def get_variation_for_rollout(self, project_config, feature, user): array of log messages representing decision making. """ decide_reasons = [] + user_id = user.user_id + attributes = user.get_user_attributes() if not feature or not feature.rolloutId: return Decision(None, None, enums.DecisionSources.ROLLOUT), decide_reasons rollout = project_config.get_rollout_from_id(feature.rolloutId) - rollout_rules = project_config.get_rollout_experiments(rollout) - if not rollout or not rollout_rules: + if not rollout: message = 'There is no rollout of feature {}.'.format(feature.key) self.logger.debug(message) decide_reasons.append(message) return Decision(None, None, enums.DecisionSources.ROLLOUT), decide_reasons + rollout_rules = project_config.get_rollout_experiments(rollout) + + if not rollout_rules: + message = 'Rollout {} has no experiments.'.format(rollout.id) + self.logger.debug(message) + decide_reasons.append(message) + return Decision(None, None, enums.DecisionSources.ROLLOUT), decide_reasons + index = 0 while index < len(rollout_rules): - decision_response, reasons_received = self.get_variation_from_delivery_rule(project_config, - feature, - rollout_rules, index, user) + skip_to_everyone_else = False + # check forced decision first + rule = rollout_rules[index] + optimizely_decision_context = OptimizelyUserContext.OptimizelyDecisionContext(feature.key, rule.key) + forced_decision_variation, reasons_received = user.find_validated_forced_decision( + optimizely_decision_context) decide_reasons += reasons_received - variation, skip_to_everyone_else = decision_response - - if variation: - rule = rollout_rules[index] - feature_decision = Decision(experiment=rule, variation=variation, - source=enums.DecisionSources.ROLLOUT) - - return feature_decision, decide_reasons - - # the last rule is special for "Everyone Else" - index = len(rollout_rules) - 1 if skip_to_everyone_else else index + 1 - - return Decision(None, None, enums.DecisionSources.ROLLOUT), decide_reasons - - def get_variation_from_experiment_rule(self, config, flag_key, rule, user, options): - """ Checks for experiment rule if decision is forced and returns it. - Otherwise returns a regular decision. - - Args: - config: Instance of ProjectConfig. - flag_key: Key of the flag. - rule: Experiment rule. - user: ID and attributes for user. - options: Decide options. - - Returns: - Decision namedtuple consisting of experiment and variation for the user and - array of log messages representing decision making. - """ - decide_reasons = [] - - # check forced decision first - optimizely_decision_context = OptimizelyUserContext.OptimizelyDecisionContext(flag_key, rule.key) - - forced_decision_variation, reasons_received = user.find_validated_forced_decision( - optimizely_decision_context) - decide_reasons += reasons_received - - if forced_decision_variation: - return forced_decision_variation, decide_reasons - - # regular decision - decision_variation, variation_reasons = self.get_variation(config, rule, user, options) - decide_reasons += variation_reasons - return decision_variation, decide_reasons - - def get_variation_from_delivery_rule(self, config, feature, rules, rule_index, user): - """ Checks for delivery rule if decision is forced and returns it. - Otherwise returns a regular decision. + if forced_decision_variation: + return Decision(experiment=rule, variation=forced_decision_variation, + source=enums.DecisionSources.ROLLOUT), decide_reasons - Args: - config: Instance of ProjectConfig. - flag_key: Key of the flag. - rules: Experiment rule. - rule_index: integer index of the rule in the list. - user: ID and attributes for user. + bucketing_id, bucket_reasons = self._get_bucketing_id(user_id, attributes) + decide_reasons += bucket_reasons - Returns: - If forced decision, it returns namedtuple consisting of forced_decision_variation and skip_to_everyone_else - and decision reason log messages. + everyone_else = (index == len(rollout_rules) - 1) + logging_key = "Everyone Else" if everyone_else else str(index + 1) - If regular decision it returns a tuple of bucketed_variation and skip_to_everyone_else - and decision reason log messages - """ - decide_reasons = [] - skip_to_everyone_else = False - bucketed_variation = None + rollout_rule = project_config.get_experiment_from_id(rule.id) + audience_conditions = rollout_rule.get_audience_conditions_or_ids() - # check forced decision first - rule = rules[rule_index] - optimizely_decision_context = OptimizelyUserContext.OptimizelyDecisionContext(feature.key, rule.key) - forced_decision_variation, reasons_received = user.find_validated_forced_decision(optimizely_decision_context) + audience_decision_response, reasons_received_audience = audience_helper.does_user_meet_audience_conditions( + project_config, audience_conditions, enums.RolloutRuleAudienceEvaluationLogs, + logging_key, attributes, self.logger) - decide_reasons += reasons_received - - if forced_decision_variation: - return (forced_decision_variation, skip_to_everyone_else), decide_reasons + decide_reasons += reasons_received_audience - # regular decision - user_id = user.user_id - attributes = user.get_user_attributes() - - bucketing_id, bucket_reasons = self._get_bucketing_id(user_id, attributes) - decide_reasons += bucket_reasons - - everyone_else = (rule_index == len(rules) - 1) - logging_key = "Everyone Else" if everyone_else else str(rule_index + 1) - - rollout_rule = config.get_experiment_from_id(rule.id) - audience_conditions = rollout_rule.get_audience_conditions_or_ids() - - audience_decision_response, reasons_received_audience = audience_helper.does_user_meet_audience_conditions( - config, audience_conditions, enums.RolloutRuleAudienceEvaluationLogs, logging_key, attributes, self.logger) - - decide_reasons += reasons_received_audience + if audience_decision_response: + message = 'User "{}" meets audience conditions for targeting rule {}.'.format(user_id, logging_key) + self.logger.debug(message) + decide_reasons.append(message) - if audience_decision_response: - message = 'User "{}" meets audience conditions for targeting rule {}.'.format(user_id, logging_key) - self.logger.debug(message) - decide_reasons.append(message) + bucketed_variation, bucket_reasons = self.bucketer.bucket(project_config, rollout_rule, user_id, + bucketing_id) + decide_reasons.extend(bucket_reasons) - bucketed_variation, bucket_reasons = self.bucketer.bucket(config, rollout_rule, user_id, - bucketing_id) - decide_reasons.extend(bucket_reasons) + if bucketed_variation: + message = 'User "{}" bucketed into a targeting rule {}.'.format(user_id, logging_key) + self.logger.debug(message) + decide_reasons.append(message) + return Decision(experiment=rule, variation=bucketed_variation, + source=enums.DecisionSources.ROLLOUT), decide_reasons + + elif not everyone_else: + # skip this logging for EveryoneElse since this has a message not for everyone_else + message = 'User "{}" not bucketed into a targeting rule {}. ' \ + 'Checking "Everyone Else" rule now.'.format(user_id, logging_key) + self.logger.debug(message) + decide_reasons.append(message) - if bucketed_variation: - message = 'User "{}" bucketed into a targeting rule {}.'.format(user_id, logging_key) - self.logger.debug(message) - decide_reasons.append(message) + # skip the rest of rollout rules to the everyone-else rule if audience matches but not bucketed. + skip_to_everyone_else = True - elif not everyone_else: - # skip this logging for EveryoneElse since this has a message not for everyone_else - message = 'User "{}" not bucketed into a targeting rule {}. ' \ - 'Checking "Everyone Else" rule now.'.format(user_id, logging_key) + else: + message = 'User "{}" does not meet audience conditions for targeting rule {}.'.format( + user_id, logging_key) self.logger.debug(message) decide_reasons.append(message) - # skip the rest of rollout rules to the everyone-else rule if audience matches but not bucketed. - skip_to_everyone_else = True - - else: - message = 'User "{}" does not meet audience conditions for targeting rule {}.'.format(user_id, logging_key) - self.logger.debug(message) - decide_reasons.append(message) + # the last rule is special for "Everyone Else" + index = len(rollout_rules) - 1 if skip_to_everyone_else else index + 1 - return (bucketed_variation, skip_to_everyone_else), decide_reasons + return Decision(None, None, enums.DecisionSources.ROLLOUT), decide_reasons def get_variation_for_feature(self, project_config, feature, user_context, options=None): """ Returns the experiment/variation the user is bucketed in for the given feature. @@ -517,11 +458,34 @@ def get_variation_for_feature(self, project_config, feature, user_context, optio # Evaluate each experiment ID and return the first bucketed experiment variation for experiment in feature.experimentIds: experiment = project_config.get_experiment_from_id(experiment) - if experiment: - variation, variation_reasons = self.get_variation_from_experiment_rule( - project_config, feature.key, experiment, user_context, options) - decide_reasons += variation_reasons - if variation: - return Decision(experiment, variation, enums.DecisionSources.FEATURE_TEST), decide_reasons + decision_variation = None - return self.get_variation_for_rollout(project_config, feature, user_context) + if experiment: + optimizely_decision_context = OptimizelyUserContext.OptimizelyDecisionContext(feature.key, + experiment.key) + + forced_decision_variation, reasons_received = user_context.find_validated_forced_decision( + optimizely_decision_context) + decide_reasons += reasons_received + + if forced_decision_variation: + decision_variation = forced_decision_variation + else: + decision_variation, variation_reasons = self.get_variation(project_config, + experiment, user_context, options) + decide_reasons += variation_reasons + + if decision_variation: + message = 'User "{}" bucketed into a experiment "{}" of feature "{}".'.format( + user_context.user_id, experiment.key, feature.key) + self.logger.debug(message) + return Decision(experiment, decision_variation, + enums.DecisionSources.FEATURE_TEST), decide_reasons + + message = 'User "{}" is not bucketed into any of the experiments on the feature "{}".'.format( + user_context.user_id, feature.key) + self.logger.debug(message) + variation, rollout_variation_reasons = self.get_variation_for_rollout(project_config, feature, user_context) + if rollout_variation_reasons: + decide_reasons += rollout_variation_reasons + return variation, decide_reasons diff --git a/optimizely/project_config.py b/optimizely/project_config.py index 2ba5c0f5..ff22ae5b 100644 --- a/optimizely/project_config.py +++ b/optimizely/project_config.py @@ -100,6 +100,7 @@ def __init__(self, datafile, logger, error_handler): self.variation_variable_usage_map = {} self.variation_id_map_by_experiment_id = {} self.variation_key_map_by_experiment_id = {} + self.flag_variations_map = {} for experiment in self.experiment_id_map.values(): self.experiment_key_map[experiment.key] = experiment @@ -121,45 +122,38 @@ def __init__(self, datafile, logger, error_handler): self.feature_key_map = self._generate_key_map(self.feature_flags, 'key', entities.FeatureFlag) - # As we cannot create json variables in datafile directly, here we convert - # the variables of string type and json subType to json type - # This is needed to fully support json variables + # Dictionary containing dictionary of experiment ID to feature ID. + # for checking that experiment is a feature experiment or not. self.experiment_feature_map = {} - self.flag_rules_map = {} - - for feature_key, feature_value in self.feature_key_map.items(): - for variable in self.feature_key_map[feature_key].variables: + for feature in self.feature_key_map.values(): + # As we cannot create json variables in datafile directly, here we convert + # the variables of string type and json subType to json type + # This is needed to fully support json variables + for variable in self.feature_key_map[feature.key].variables: sub_type = variable.get('subType', '') if variable['type'] == entities.Variable.Type.STRING and sub_type == entities.Variable.Type.JSON: variable['type'] = entities.Variable.Type.JSON - # loop over features=flags already happening - # get feature variables for eacg flag/feature - feature_value.variables = self._generate_key_map(feature_value.variables, 'key', entities.Variable) - for exp_id in feature_value.experimentIds: - # Add this experiment in experiment-feature map. - self.experiment_feature_map[exp_id] = [feature_value.id] - - # all rules(experiment rules and delivery rules) for each flag - for flag in self.feature_flags: - experiments = [] - if len(flag['experimentIds']) > 0: - for exp_id in flag['experimentIds']: - experiments.append(self.experiment_id_map[exp_id]) - if not flag['rolloutId'] == '': - rollout = self.rollout_id_map[flag['rolloutId']] - - rollout_experiments = self.get_rollout_experiments(rollout) - - if rollout and rollout.experiments: - experiments.extend(rollout_experiments) + feature.variables = self._generate_key_map(feature.variables, 'key', entities.Variable) - self.flag_rules_map[flag['key']] = experiments + rules = [] + variations = [] + for exp_id in feature.experimentIds: + # Add this experiment in experiment-feature map. + self.experiment_feature_map[exp_id] = [feature.id] + rules.append(self.experiment_id_map[exp_id]) + rollout = None if len(feature.rolloutId) == 0 else self.rollout_id_map[feature.rolloutId] + if rollout: + for exp in rollout.experiments: + rules.append(self.experiment_id_map[exp['id']]) - # All variations for each flag - # Datafile does not contain a separate entity for this. - # We collect variations used in each rule (experiment rules and delivery rules) - self.flag_variations_map = self.get_all_variations_for_each_rule(self.flag_rules_map) + for rule in rules: + # variation_id_map_by_experiment_id gives variation entity object while + # experiment_id_map will give us dictionary + for rule_variation in self.variation_id_map_by_experiment_id.get(rule.id).values(): + if len(list(filter(lambda variation: variation.id == rule_variation.id, variations))) == 0: + variations.append(rule_variation) + self.flag_variations_map[feature.key] = variations @staticmethod def _generate_key_map(entity_list, key, entity_class): @@ -639,32 +633,6 @@ def get_variation_from_key_by_experiment_id(self, experiment_id, variation_key): return {} - def get_all_variations_for_each_rule(self, flag_rules_map): - """ Helper method to get all variation objects from each flag. - collects variations used in each rule (experiment rules and delivery rules). - - Args: - flag_rules_map: A dictionary. A map of all rules per flag. - - Returns: - Map of flag variations. - """ - flag_variations_map = {} - - for flag_key, rules in flag_rules_map.items(): - variations = [] - for rule in rules: - # get variations as objects (rule.variations gives list) - variation_objects = self.variation_id_map_by_experiment_id[rule.id].values() - for variation in variation_objects: - # append variation if it's not already in the list - if variation not in variations: - variations.append(variation) - - flag_variations_map[flag_key] = variations - - return flag_variations_map - def get_flag_variation(self, flag_key, variation_attribute, target_value): """ Gets variation by specified variation attribute. diff --git a/tests/test_decision_service.py b/tests/test_decision_service.py index 635d22e0..dc5bbfe7 100644 --- a/tests/test_decision_service.py +++ b/tests/test_decision_service.py @@ -1325,8 +1325,8 @@ def test_get_variation_for_feature__returns_variation_for_feature_in_rollout(sel ) # Assert no log messages were generated - self.assertEqual(0, mock_decision_service_logging.debug.call_count) - self.assertEqual(0, len(mock_decision_service_logging.method_calls)) + self.assertEqual(1, mock_decision_service_logging.debug.call_count) + self.assertEqual(1, len(mock_decision_service_logging.method_calls)) def test_get_variation_for_feature__returns_variation_if_user_not_in_experiment_but_in_rollout( self,