From a9f6b125b1f2ef496c6137aac0dcb7b1576e40cc Mon Sep 17 00:00:00 2001 From: ozayr-zaviar Date: Wed, 24 Nov 2021 18:16:00 +0500 Subject: [PATCH 1/7] project config refactor --- optimizely/project_config.py | 92 +++++++++++++----------------------- 1 file changed, 34 insertions(+), 58 deletions(-) diff --git a/optimizely/project_config.py b/optimizely/project_config.py index 2ba5c0f5..bbf641fb 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 @@ -120,46 +121,25 @@ def __init__(self, datafile, logger, error_handler): ) self.feature_key_map = self._generate_key_map(self.feature_flags, 'key', entities.FeatureFlag) - + self.flag_variations_map = self.generate_feature_variation_map(self.feature_flags) # 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 - 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: + for variable in self.feature_key_map[feature].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) - - self.flag_rules_map[flag['key']] = experiments + # Dict containing map of experiment ID to feature ID. + # for checking that experiment is a feature experiment or not. + self.experiment_feature_map = {} + for feature in self.feature_key_map.values(): + feature.variables = self._generate_key_map(feature.variables, 'key', entities.Variable) - # 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 exp_id in feature.experimentIds: + # Add this experiment in experiment-feature map. + self.experiment_feature_map[exp_id] = [feature.id] @staticmethod def _generate_key_map(entity_list, key, entity_class): @@ -215,6 +195,28 @@ def get_rollout_experiments(self, rollout): return rollout_experiments + def get_rules_for_flag(self, feature_flag): + rules = map(lambda exp_id: self.experiment_id_map[exp_id], feature_flag['experimentIds']) + rules = list(rules) + rollout = None if len(feature_flag['rolloutId']) == 0 else self.rollout_id_map[feature_flag['rolloutId']] + + if rollout: + rollout_experiments = rollout.experiments + for exp in rollout_experiments: + rules.append(self.experiment_id_map[exp['id']]) + return rules + + def generate_feature_variation_map(self, feature_flags): + flag_variations_map = {} + for flag in feature_flags: + variations = [] + for rule in self.get_rules_for_flag(flag): + 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) + flag_variations_map[flag['key']] = variations + return flag_variations_map + def get_typecast_value(self, value, type): """ Helper method to determine actual value based on type of feature variable. @@ -639,32 +641,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. From 3955db0f7f958d5256811e7b40321ce0d42a5488 Mon Sep 17 00:00:00 2001 From: ozayr-zaviar Date: Wed, 1 Dec 2021 17:29:06 +0500 Subject: [PATCH 2/7] use existing loop to generate flag_variation_map --- optimizely/project_config.py | 38 ++++++++++++++---------------------- 1 file changed, 15 insertions(+), 23 deletions(-) diff --git a/optimizely/project_config.py b/optimizely/project_config.py index bbf641fb..924117ef 100644 --- a/optimizely/project_config.py +++ b/optimizely/project_config.py @@ -121,7 +121,6 @@ def __init__(self, datafile, logger, error_handler): ) self.feature_key_map = self._generate_key_map(self.feature_flags, 'key', entities.FeatureFlag) - self.flag_variations_map = self.generate_feature_variation_map(self.feature_flags) # 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 @@ -137,9 +136,24 @@ def __init__(self, datafile, logger, error_handler): for feature in self.feature_key_map.values(): feature.variables = self._generate_key_map(feature.variables, 'key', entities.Variable) + 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']]) + + 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): @@ -195,28 +209,6 @@ def get_rollout_experiments(self, rollout): return rollout_experiments - def get_rules_for_flag(self, feature_flag): - rules = map(lambda exp_id: self.experiment_id_map[exp_id], feature_flag['experimentIds']) - rules = list(rules) - rollout = None if len(feature_flag['rolloutId']) == 0 else self.rollout_id_map[feature_flag['rolloutId']] - - if rollout: - rollout_experiments = rollout.experiments - for exp in rollout_experiments: - rules.append(self.experiment_id_map[exp['id']]) - return rules - - def generate_feature_variation_map(self, feature_flags): - flag_variations_map = {} - for flag in feature_flags: - variations = [] - for rule in self.get_rules_for_flag(flag): - 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) - flag_variations_map[flag['key']] = variations - return flag_variations_map - def get_typecast_value(self, value, type): """ Helper method to determine actual value based on type of feature variable. From 6768457787eb6beb088b5194ca39ce6440919378 Mon Sep 17 00:00:00 2001 From: ozayr-zaviar Date: Wed, 1 Dec 2021 19:07:01 +0500 Subject: [PATCH 3/7] get_variation_from_experiment_rule and get_variation_from_delivery_rule removed --- optimizely/decision_service.py | 201 +++++++++++++-------------------- tests/test_decision_service.py | 4 +- 2 files changed, 82 insertions(+), 123 deletions(-) diff --git a/optimizely/decision_service.py b/optimizely/decision_service.py index c16a1be2..28317035 100644 --- a/optimizely/decision_service.py +++ b/optimizely/decision_service.py @@ -349,6 +349,8 @@ 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 @@ -364,138 +366,69 @@ def get_variation_for_rollout(self, project_config, feature, user): 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. - - 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. - - Returns: - If forced decision, it returns namedtuple consisting of forced_decision_variation and skip_to_everyone_else - and decision reason log messages. + if forced_decision_variation: + return (forced_decision_variation, skip_to_everyone_else), decide_reasons - 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 + bucketing_id, bucket_reasons = self._get_bucketing_id(user_id, attributes) + decide_reasons += bucket_reasons - # 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) + everyone_else = (index == len(rollout_rules) - 1) + logging_key = "Everyone Else" if everyone_else else str(index + 1) - decide_reasons += reasons_received + rollout_rule = project_config.get_experiment_from_id(rule.id) + audience_conditions = rollout_rule.get_audience_conditions_or_ids() - if forced_decision_variation: - return (forced_decision_variation, skip_to_everyone_else), decide_reasons + audience_decision_response, reasons_received_audience = audience_helper.does_user_meet_audience_conditions( + project_config, audience_conditions, enums.RolloutRuleAudienceEvaluationLogs, + logging_key, attributes, self.logger) - # regular decision - user_id = user.user_id - attributes = user.get_user_attributes() + decide_reasons += reasons_received_audience - 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 +450,37 @@ 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 + 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 + + 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) return self.get_variation_for_rollout(project_config, feature, user_context) diff --git a/tests/test_decision_service.py b/tests/test_decision_service.py index 1d592d20..41e4a590 100644 --- a/tests/test_decision_service.py +++ b/tests/test_decision_service.py @@ -1292,8 +1292,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, From 981ab704ad4819a02a31563d5823ef45365a23a2 Mon Sep 17 00:00:00 2001 From: ozayr-zaviar Date: Wed, 1 Dec 2021 19:53:32 +0500 Subject: [PATCH 4/7] fsc test fix --- optimizely/decision_service.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/optimizely/decision_service.py b/optimizely/decision_service.py index 28317035..a1b7ef92 100644 --- a/optimizely/decision_service.py +++ b/optimizely/decision_service.py @@ -376,7 +376,8 @@ def get_variation_for_rollout(self, project_config, feature, user): decide_reasons += reasons_received if forced_decision_variation: - return (forced_decision_variation, skip_to_everyone_else), decide_reasons + return Decision(experiment=rule, variation=forced_decision_variation, + source=enums.DecisionSources.ROLLOUT), decide_reasons bucketing_id, bucket_reasons = self._get_bucketing_id(user_id, attributes) decide_reasons += bucket_reasons From dcf0a48bcba628b4155e3995abdc843159aa42a8 Mon Sep 17 00:00:00 2001 From: ozayr-zaviar Date: Wed, 1 Dec 2021 20:17:09 +0500 Subject: [PATCH 5/7] comment addressed --- optimizely/project_config.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/optimizely/project_config.py b/optimizely/project_config.py index 924117ef..ff22ae5b 100644 --- a/optimizely/project_config.py +++ b/optimizely/project_config.py @@ -121,19 +121,19 @@ 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 - for feature in self.feature_key_map: - for variable in self.feature_key_map[feature].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 - # Dict containing map of experiment ID to feature ID. + # Dictionary containing dictionary of experiment ID to feature ID. # for checking that experiment is a feature experiment or not. self.experiment_feature_map = {} 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 + feature.variables = self._generate_key_map(feature.variables, 'key', entities.Variable) rules = [] From ed543fa461928ed94b1ba0638e2c876f475c4629 Mon Sep 17 00:00:00 2001 From: ozayr-zaviar Date: Wed, 1 Dec 2021 20:22:01 +0500 Subject: [PATCH 6/7] commented code removed --- optimizely/decision_service.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/optimizely/decision_service.py b/optimizely/decision_service.py index a1b7ef92..582c5518 100644 --- a/optimizely/decision_service.py +++ b/optimizely/decision_service.py @@ -454,12 +454,6 @@ def get_variation_for_feature(self, project_config, feature, user_context, optio decision_variation = None 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 - optimizely_decision_context = OptimizelyUserContext.OptimizelyDecisionContext(feature.key, experiment.key) From 39ab0b016a8f73f36be9c44c67c938e085143b1c Mon Sep 17 00:00:00 2001 From: ozayr-zaviar Date: Thu, 2 Dec 2021 20:52:48 +0500 Subject: [PATCH 7/7] comments from main forced decision PR resolved --- optimizely/decision_service.py | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/optimizely/decision_service.py b/optimizely/decision_service.py index 582c5518..e3e3079b 100644 --- a/optimizely/decision_service.py +++ b/optimizely/decision_service.py @@ -356,14 +356,21 @@ def get_variation_for_rollout(self, project_config, feature, user): 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): skip_to_everyone_else = False @@ -478,4 +485,7 @@ def get_variation_for_feature(self, project_config, feature, user_context, optio message = 'User "{}" is not bucketed into any of the experiments on the feature "{}".'.format( user_context.user_id, feature.key) self.logger.debug(message) - return self.get_variation_for_rollout(project_config, feature, user_context) + 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