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

Duplicate experiment Key issue with multi feature flag #347

Merged
merged 8 commits into from
Jul 9, 2021
2 changes: 1 addition & 1 deletion optimizely/bucketer.py
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ def bucket(self, project_config, experiment, user_id, bucketing_id):
variation_id = self.find_bucket(project_config, bucketing_id,
experiment.id, experiment.trafficAllocation)
if variation_id:
variation = project_config.get_variation_from_id(experiment.key, variation_id)
variation = project_config.get_variation_from_id_by_experiment_id(experiment.id, variation_id)
return variation, decide_reasons

else:
Expand Down
4 changes: 2 additions & 2 deletions optimizely/decision_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,7 @@ def get_variation_for_rollout(self, project_config, rollout, user_id, attributes
if rollout and len(rollout.experiments) > 0:
for idx in range(len(rollout.experiments) - 1):
logging_key = str(idx + 1)
rollout_rule = project_config.get_experiment_from_key(rollout.experiments[idx].get('key'))
rollout_rule = project_config.get_experiment_from_id(rollout.experiments[idx].get('id'))

# Check if user meets audience conditions for targeting rule
audience_conditions = rollout_rule.get_audience_conditions_or_ids()
Expand Down Expand Up @@ -387,7 +387,7 @@ def get_variation_for_rollout(self, project_config, rollout, user_id, attributes
break

# Evaluate last rule i.e. "Everyone Else" rule
everyone_else_rule = project_config.get_experiment_from_key(rollout.experiments[-1].get('key'))
everyone_else_rule = project_config.get_experiment_from_id(rollout.experiments[-1].get('id'))
audience_conditions = everyone_else_rule.get_audience_conditions_or_ids()
audience_eval, audience_reasons = audience_helper.does_user_meet_audience_conditions(
project_config,
Expand Down
57 changes: 49 additions & 8 deletions optimizely/project_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ def __init__(self, datafile, logger, error_handler):

# Utility maps for quick lookup
self.group_id_map = self._generate_key_map(self.groups, 'id', entities.Group)
self.experiment_key_map = self._generate_key_map(self.experiments, 'key', entities.Experiment)
self.experiment_id_map = self._generate_key_map(self.experiments, 'id', entities.Experiment)
self.event_key_map = self._generate_key_map(self.events, 'key', entities.Event)
self.attribute_key_map = self._generate_key_map(self.attributes, 'key', entities.Attribute)

Expand All @@ -84,27 +84,36 @@ def __init__(self, datafile, logger, error_handler):
self.rollout_id_map = self._generate_key_map(self.rollouts, 'id', entities.Layer)
for layer in self.rollout_id_map.values():
for experiment in layer.experiments:
self.experiment_key_map[experiment['key']] = entities.Experiment(**experiment)
self.experiment_id_map[experiment['id']] = entities.Experiment(**experiment)

self.audience_id_map = self._deserialize_audience(self.audience_id_map)
for group in self.group_id_map.values():
experiments_in_group_key_map = self._generate_key_map(group.experiments, 'key', entities.Experiment)
for experiment in experiments_in_group_key_map.values():
experiments_in_group_id_map = self._generate_key_map(group.experiments, 'id', entities.Experiment)
for experiment in experiments_in_group_id_map.values():
experiment.__dict__.update({'groupId': group.id, 'groupPolicy': group.policy})
self.experiment_key_map.update(experiments_in_group_key_map)
self.experiment_id_map.update(experiments_in_group_id_map)

self.experiment_id_map = {}
self.experiment_key_map = {}
self.variation_key_map = {}
self.variation_id_map = {}
self.variation_variable_usage_map = {}
for experiment in self.experiment_key_map.values():
self.experiment_id_map[experiment.id] = experiment
self.variation_id_map_by_experiment_id = {}
self.variation_key_map_by_experiment_id = {}

for experiment in self.experiment_id_map.values():
self.experiment_key_map[experiment.key] = experiment
self.variation_key_map[experiment.key] = self._generate_key_map(
experiment.variations, 'key', entities.Variation
)

self.variation_id_map[experiment.key] = {}
self.variation_id_map_by_experiment_id[experiment.id] = {}
self.variation_key_map_by_experiment_id[experiment.id] = {}

for variation in self.variation_key_map.get(experiment.key).values():
self.variation_id_map[experiment.key][variation.id] = variation
self.variation_id_map_by_experiment_id[experiment.id][variation.id] = variation
self.variation_key_map_by_experiment_id[experiment.id][variation.key] = variation
self.variation_variable_usage_map[variation.id] = self._generate_key_map(
variation.variables, 'id', entities.Variation.VariableUsage
)
Expand Down Expand Up @@ -557,3 +566,35 @@ def is_feature_experiment(self, experiment_id):
"""

return experiment_id in self.experiment_feature_map

def get_variation_from_id_by_experiment_id(self, experiment_id, variation_id):
""" Gets experiment id and variation id
Copy link
Contributor

Choose a reason for hiding this comment

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

Gets only variation by variation id under specific experiment id.

Copy link
Contributor

Choose a reason for hiding this comment

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

please rephrase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

comments changed to better describe what is happening.


Returns:
The variation for the experiment id and variation id
or empty dict if not found
"""
if (experiment_id in self.variation_id_map_by_experiment_id and
variation_id in self.variation_id_map_by_experiment_id[experiment_id]):
return self.variation_id_map_by_experiment_id[experiment_id][variation_id]

self.logger.error('Variation with id "%s" not defined in the datafile for experiment "%s".',
variation_id, experiment_id)

return {}

def get_variation_from_key_by_experiment_id(self, experiment_id, variation_key):
""" Gets experiment id and variation key
Copy link
Contributor

Choose a reason for hiding this comment

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

please rephrase and have more clear explanation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

comments changed to better describe what is happening.


Returns:
The variation for the experiment id and variation key
or empty dict if not found
"""
if (experiment_id in self.variation_key_map_by_experiment_id and
variation_key in self.variation_key_map_by_experiment_id[experiment_id]):
return self.variation_key_map_by_experiment_id[experiment_id][variation_key]

self.logger.error('Variation with key "%s" not defined in the datafile for experiment "%s".',
variation_key, experiment_id)

return {}
25 changes: 25 additions & 0 deletions tests/test_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ def test_init(self):
self.config_dict['groups'][0]['trafficAllocation'],
)
}

Copy link

Choose a reason for hiding this comment

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

?

Copy link

Choose a reason for hiding this comment

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

Only curious because it was by itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we need a new line to satisfy Flake8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

formatter...

expected_experiment_key_map = {
'test_experiment': entities.Experiment(
'111127',
Expand Down Expand Up @@ -1213,3 +1214,27 @@ def test_is_feature_experiment(self):

self.assertStrictFalse(project_config.is_feature_experiment(experiment.id))
self.assertStrictTrue(project_config.is_feature_experiment(feature_experiment.id))

def test_get_variation_from_id_by_experiment_id(self):

opt_obj = optimizely.Optimizely(json.dumps(self.config_dict))
project_config = opt_obj.config_manager.get_config()

experiment_id = '111127'
variation_id = '111128'

variation = project_config.get_variation_from_id_by_experiment_id(experiment_id, variation_id)

self.assertIsInstance(variation, entities.Variation)

def test_get_variation_from_key_by_experiment_id(self):

opt_obj = optimizely.Optimizely(json.dumps(self.config_dict))
project_config = opt_obj.config_manager.get_config()

experiment_id = '111127'
variation_key = 'control'

variation = project_config.get_variation_from_key_by_experiment_id(experiment_id, variation_key)

self.assertIsInstance(variation, entities.Variation)