Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refact: Refactored Forced decision #365

Merged
Merged
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
196 changes: 75 additions & 121 deletions optimizely/decision_service.py
Original file line number Diff line number Diff line change
@@ -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,70 @@ 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 Decision(experiment=rule, variation=forced_decision_variation,
source=enums.DecisionSources.ROLLOUT), 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 +451,31 @@ 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:
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)
84 changes: 26 additions & 58 deletions optimizely/project_config.py
Original file line number Diff line number Diff line change
@@ -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 = {}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

holding this PR for now...


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():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we running this loop again see line 127 can we reuse it?

# 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.
4 changes: 2 additions & 2 deletions tests/test_decision_service.py
Original file line number Diff line number Diff line change
@@ -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,