Skip to content

Commit 4db0eae

Browse files
mikeproeng37mikeproeng37
authored
Refactor order of bucketing operations (#47)
1 parent 6ba07f3 commit 4db0eae

File tree

8 files changed

+123
-126
lines changed

8 files changed

+123
-126
lines changed

‎optimizely/bucketer.py

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -103,16 +103,6 @@ def bucket(self, experiment, user_id):
103103
if not experiment:
104104
return None
105105

106-
# Check if user is white-listed for a variation
107-
forced_variations = experiment.forcedVariations
108-
if forced_variations and user_id in forced_variations:
109-
variation_key = forced_variations.get(user_id)
110-
variation = self.config.get_variation_from_key(experiment.key, variation_key)
111-
if variation:
112-
self.config.logger.log(enums.LogLevels.INFO,
113-
'User "%s" is forced in variation "%s".' % (user_id, variation_key))
114-
return variation
115-
116106
# Determine if experiment is in a mutually exclusive group
117107
if experiment.groupPolicy in GROUP_POLICIES:
118108
group = self.config.get_group(experiment.groupId)
@@ -143,3 +133,25 @@ def bucket(self, experiment, user_id):
143133

144134
self.config.logger.log(enums.LogLevels.INFO, 'User "%s" is in no variation.' % user_id)
145135
return None
136+
137+
def get_forced_variation(self, experiment, user_id):
138+
""" Determine if a user is forced into a variation for the given experiment and return that variation.
139+
140+
Args:
141+
experiment: Object representing the experiment for which user is to be bucketed.
142+
user_id: ID for the user.
143+
144+
Returns:
145+
Variation in which the user with ID user_id is forced into. None if no variation.
146+
"""
147+
148+
forced_variations = experiment.forcedVariations
149+
if forced_variations and user_id in forced_variations:
150+
variation_key = forced_variations.get(user_id)
151+
variation = self.config.get_variation_from_key(experiment.key, variation_key)
152+
if variation:
153+
self.config.logger.log(enums.LogLevels.INFO,
154+
'User "%s" is forced in variation "%s".' % (user_id, variation_key))
155+
return variation
156+
157+
return None

‎optimizely/event_builder.py

Lines changed: 15 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ def _add_common_params(self, user_id, attributes):
9797

9898

9999
class EventBuilder(BaseEventBuilder):
100-
""" Class which encapsulates methods to build events for tracking
100+
""" Class which encapsulates methods to build events for tracking
101101
impressions and conversions using the new endpoints. """
102102

103103
IMPRESSION_ENDPOINT = 'https://logx.optimizely.com/log/decision'
@@ -190,7 +190,7 @@ def _add_required_params_for_conversion(self, event_key, user_id, event_tags, va
190190
event_key: Key representing the event which needs to be recorded.
191191
user_id: ID for user.
192192
event_tags: Dict representing metadata associated with the event.
193-
valid_experiments: List of tuples representing valid experiments for the event.
193+
valid_experiments: List of tuples representing valid experiments IDs and variation IDs.
194194
"""
195195

196196
self.params[self.EventParams.IS_GLOBAL_HOLDBACK] = False
@@ -219,19 +219,18 @@ def _add_required_params_for_conversion(self, event_key, user_id, event_tags, va
219219
self.params[self.EventParams.EVENT_FEATURES].append(event_feature)
220220

221221
self.params[self.EventParams.LAYER_STATES] = []
222-
for experiment in valid_experiments:
223-
variation = self.bucketer.bucket(experiment, user_id)
224-
if variation:
225-
self.params[self.EventParams.LAYER_STATES].append({
226-
self.EventParams.LAYER_ID: experiment.layerId,
227-
self.EventParams.REVISION: self.config.get_revision(),
228-
self.EventParams.ACTION_TRIGGERED: True,
229-
self.EventParams.DECISION: {
230-
self.EventParams.EXPERIMENT_ID: experiment.id,
231-
self.EventParams.VARIATION_ID: variation.id,
232-
self.EventParams.IS_LAYER_HOLDBACK: False
233-
}
234-
})
222+
for experiment_id, variation_id in valid_experiments:
223+
experiment = self.config.get_experiment_from_id(experiment_id)
224+
self.params[self.EventParams.LAYER_STATES].append({
225+
self.EventParams.LAYER_ID: experiment.layerId,
226+
self.EventParams.REVISION: self.config.get_revision(),
227+
self.EventParams.ACTION_TRIGGERED: True,
228+
self.EventParams.DECISION: {
229+
self.EventParams.EXPERIMENT_ID: experiment.id,
230+
self.EventParams.VARIATION_ID: variation_id,
231+
self.EventParams.IS_LAYER_HOLDBACK: False
232+
}
233+
})
235234

236235
self.params[self.EventParams.EVENT_ID] = self.config.get_event(event_key).id
237236
self.params[self.EventParams.EVENT_NAME] = event_key
@@ -265,7 +264,7 @@ def create_conversion_event(self, event_key, user_id, attributes, event_tags, va
265264
user_id: ID for user.
266265
attributes: Dict representing user attributes and values.
267266
event_tags: Dict representing metadata associated with the event.
268-
valid_experiments: List of tuples representing valid experiments for the event.
267+
valid_experiments: List of tuples representing experiments IDs and variation IDs.
269268
270269
Returns:
271270
Event object encapsulating the conversion event.

‎optimizely/helpers/experiment.py

Lines changed: 0 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -25,20 +25,3 @@ def is_experiment_running(experiment):
2525
"""
2626

2727
return experiment.status in ALLOWED_EXPERIMENT_STATUS
28-
29-
30-
def is_user_in_forced_variation(forced_variations, user_id):
31-
""" Determine if the user is in a forced variation.
32-
33-
Args:
34-
forced_variations: Dict representing forced variations for the experiment.
35-
user_id: User to check for in whitelist.
36-
37-
Returns:
38-
Boolean depending on whether user is in forced variation or not.
39-
"""
40-
41-
if forced_variations and user_id in forced_variations:
42-
return True
43-
44-
return False

‎optimizely/optimizely.py

Lines changed: 73 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ def __init__(self, datafile, event_dispatcher=None, logger=None, error_handler=N
5050
self.error_handler = error_handler or noop_error_handler
5151

5252
try:
53-
self._validate_inputs(datafile, skip_json_validation)
53+
self._validate_instantiation_options(datafile, skip_json_validation)
5454
except exceptions.InvalidInputException as error:
5555
self.is_valid = False
5656
self.logger = SimpleLogger()
@@ -75,15 +75,15 @@ def __init__(self, datafile, event_dispatcher=None, logger=None, error_handler=N
7575
self.bucketer = bucketer.Bucketer(self.config)
7676
self.event_builder = event_builder.EventBuilder(self.config, self.bucketer)
7777

78-
def _validate_inputs(self, datafile, skip_json_validation):
79-
""" Helper method to validate all input parameters.
78+
def _validate_instantiation_options(self, datafile, skip_json_validation):
79+
""" Helper method to validate all instantiation parameters.
8080
8181
Args:
8282
datafile: JSON string representing the project.
8383
skip_json_validation: Boolean representing whether JSON schema validation needs to be skipped or not.
8484
8585
Raises:
86-
Exception if provided input is invalid.
86+
Exception if provided instantiation options are valid.
8787
"""
8888

8989
if not skip_json_validation and not validator.is_datafile_valid(datafile):
@@ -98,39 +98,74 @@ def _validate_inputs(self, datafile, skip_json_validation):
9898
if not validator.is_error_handler_valid(self.error_handler):
9999
raise exceptions.InvalidInputException(enums.Errors.INVALID_INPUT_ERROR.format('error_handler'))
100100

101-
def _validate_preconditions(self, experiment, user_id, attributes):
101+
def _validate_preconditions(self, experiment, attributes=None, event_tags=None):
102102
""" Helper method to validate all pre-conditions before we go ahead to bucket user.
103103
104104
Args:
105105
experiment: Object representing the experiment.
106-
user_id: ID for user.
107106
attributes: Dict representing user attributes.
108107
109108
Returns:
110109
Boolean depending upon whether all conditions are met or not.
111110
"""
112-
113-
if attributes and not validator.are_attributes_valid(attributes):
114-
self.logger.log(enums.LogLevels.ERROR, 'Provided attributes are in an invalid format.')
115-
self.error_handler.handle_error(exceptions.InvalidAttributeException(enums.Errors.INVALID_ATTRIBUTE_FORMAT))
111+
if not self._validate_user_inputs(attributes, event_tags):
116112
return False
117113

118114
if not experiment_helper.is_experiment_running(experiment):
119115
self.logger.log(enums.LogLevels.INFO, 'Experiment "%s" is not running.' % experiment.key)
120116
return False
121117

122-
if experiment_helper.is_user_in_forced_variation(experiment.forcedVariations, user_id):
123-
return True
118+
return True
124119

125-
if not audience_helper.is_user_in_experiment(self.config, experiment, attributes):
126-
self.logger.log(
127-
enums.LogLevels.INFO,
128-
'User "%s" does not meet conditions to be in experiment "%s".' % (user_id, experiment.key)
129-
)
120+
def _validate_user_inputs(self, attributes=None, event_tags=None):
121+
""" Helper method to validate user inputs.
122+
123+
Args:
124+
attributes: Dict representing user attributes.
125+
event_tags: Dict representing metadata associated with an event.
126+
127+
Returns:
128+
Boolean True if inputs are valid. False otherwise.
129+
130+
"""
131+
132+
if attributes and not validator.are_attributes_valid(attributes):
133+
self.logger.log(enums.LogLevels.ERROR, 'Provided attributes are in an invalid format.')
134+
self.error_handler.handle_error(exceptions.InvalidAttributeException(enums.Errors.INVALID_ATTRIBUTE_FORMAT))
135+
return False
136+
137+
if event_tags and not validator.are_event_tags_valid(event_tags):
138+
self.logger.log(enums.LogLevels.ERROR, 'Provided event tags are in an invalid format.')
139+
self.error_handler.handle_error(exceptions.InvalidEventTagException(enums.Errors.INVALID_EVENT_TAG_FORMAT))
130140
return False
131141

132142
return True
133143

144+
def _get_valid_experiments_for_event(self, event, user_id, attributes):
145+
""" Helper method to determine which experiments we should track for the given event.
146+
147+
Args:
148+
event: The event which needs to be recorded.
149+
user_id: ID for user.
150+
attributes: Dict representing user attributes.
151+
152+
Returns:
153+
List of tuples representing valid experiment IDs and variation IDs into which the user is bucketed.
154+
"""
155+
valid_experiments = []
156+
for experiment_id in event.experimentIds:
157+
experiment = self.config.get_experiment_from_id(experiment_id)
158+
variation_key = self.get_variation(experiment.key, user_id, attributes)
159+
160+
if not variation_key:
161+
self.logger.log(enums.LogLevels.INFO, 'Not tracking user "%s" for experiment "%s".' % (user_id, experiment.key))
162+
continue
163+
164+
variation = self.config.get_variation_from_key(experiment.key, variation_key)
165+
valid_experiments.append((experiment_id, variation.id))
166+
167+
return valid_experiments
168+
134169
def activate(self, experiment_key, user_id, attributes=None):
135170
""" Buckets visitor and sends impression event to Optimizely.
136171
@@ -148,22 +183,15 @@ def activate(self, experiment_key, user_id, attributes=None):
148183
self.logger.log(enums.LogLevels.ERROR, enums.Errors.INVALID_DATAFILE.format('activate'))
149184
return None
150185

151-
experiment = self.config.get_experiment_from_key(experiment_key)
152-
if not experiment:
153-
self.logger.log(enums.LogLevels.INFO, 'Not activating user "%s".' % user_id)
154-
return None
155-
156-
if not self._validate_preconditions(experiment, user_id, attributes):
157-
self.logger.log(enums.LogLevels.INFO, 'Not activating user "%s".' % user_id)
158-
return None
159-
160-
variation = self.bucketer.bucket(experiment, user_id)
186+
variation_key = self.get_variation(experiment_key, user_id, attributes)
161187

162-
if not variation:
188+
if not variation_key:
163189
self.logger.log(enums.LogLevels.INFO, 'Not activating user "%s".' % user_id)
164190
return None
165191

166192
# Create and dispatch impression event
193+
experiment = self.config.get_experiment_from_key(experiment_key)
194+
variation = self.config.get_variation_from_key(experiment_key, variation_key)
167195
impression_event = self.event_builder.create_impression_event(experiment, variation.id, user_id, attributes)
168196
self.logger.log(enums.LogLevels.INFO, 'Activating user "%s" in experiment "%s".' % (user_id, experiment.key))
169197
self.logger.log(enums.LogLevels.DEBUG,
@@ -191,11 +219,6 @@ def track(self, event_key, user_id, attributes=None, event_tags=None):
191219
self.logger.log(enums.LogLevels.ERROR, enums.Errors.INVALID_DATAFILE.format('track'))
192220
return
193221

194-
if attributes and not validator.are_attributes_valid(attributes):
195-
self.logger.log(enums.LogLevels.ERROR, 'Provided attributes are in an invalid format.')
196-
self.error_handler.handle_error(exceptions.InvalidAttributeException(enums.Errors.INVALID_ATTRIBUTE_FORMAT))
197-
return
198-
199222
if event_tags:
200223
if isinstance(event_tags, numbers.Number):
201224
event_tags = {
@@ -204,24 +227,16 @@ def track(self, event_key, user_id, attributes=None, event_tags=None):
204227
self.logger.log(enums.LogLevels.WARNING,
205228
'Event value is deprecated in track call. Use event tags to pass in revenue value instead.')
206229

207-
if not validator.are_event_tags_valid(event_tags):
208-
self.logger.log(enums.LogLevels.ERROR, 'Provided event tags are in an invalid format.')
209-
self.error_handler.handle_error(exceptions.InvalidEventTagException(enums.Errors.INVALID_EVENT_TAG_FORMAT))
210-
return
230+
if not self._validate_user_inputs(attributes, event_tags):
231+
return
211232

212233
event = self.config.get_event(event_key)
213234
if not event:
214235
self.logger.log(enums.LogLevels.INFO, 'Not tracking user "%s" for event "%s".' % (user_id, event_key))
215236
return
216237

217238
# Filter out experiments that are not running or that do not include the user in audience conditions
218-
valid_experiments = []
219-
for experiment_id in event.experimentIds:
220-
experiment = self.config.get_experiment_from_id(experiment_id)
221-
if not self._validate_preconditions(experiment, user_id, attributes):
222-
self.logger.log(enums.LogLevels.INFO, 'Not tracking user "%s" for experiment "%s".' % (user_id, experiment.key))
223-
continue
224-
valid_experiments.append(experiment)
239+
valid_experiments = self._get_valid_experiments_for_event(event, user_id, attributes)
225240

226241
# Create and dispatch conversion event if there are valid experiments
227242
if valid_experiments:
@@ -259,10 +274,23 @@ def get_variation(self, experiment_key, user_id, attributes=None):
259274

260275
experiment = self.config.get_experiment_from_key(experiment_key)
261276
if not experiment:
277+
self.logger.log(enums.LogLevels.INFO, 'Experiment key "%s" is invalid. Not activating user "%s".' % (experiment_key, user_id))
262278
return None
263279

264-
if not self._validate_preconditions(experiment, user_id, attributes):
280+
if not self._validate_preconditions(experiment, attributes):
265281
return None
282+
283+
forcedVariation = self.bucketer.get_forced_variation(experiment, user_id)
284+
if forcedVariation:
285+
return forcedVariation.key
286+
287+
if not audience_helper.is_user_in_experiment(self.config, experiment, attributes):
288+
self.logger.log(
289+
enums.LogLevels.INFO,
290+
'User "%s" does not meet conditions to be in experiment "%s".' % (user_id, experiment.key)
291+
)
292+
return None
293+
266294
variation = self.bucketer.bucket(experiment, user_id)
267295

268296
if variation:

‎tests/helpers_tests/test_experiment.py

Lines changed: 0 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -33,19 +33,3 @@ def test_is_experiment_running__status_not_running(self):
3333
'42', 'test_experiment', 'Some Status', [], [], {}, [], '43')) as mock_get_experiment:
3434
self.assertFalse(experiment.is_experiment_running(self.project_config.get_experiment_from_key('test_experiment')))
3535
mock_get_experiment.assert_called_once_with('test_experiment')
36-
37-
def test_is_user_in_forced_variation__no_forced_variation_dict(self):
38-
""" Test that is_user_in_forced_variation returns False when experiment has no forced variations. """
39-
40-
self.assertFalse(experiment.is_user_in_forced_variation(None, 'test_user'))
41-
self.assertFalse(experiment.is_user_in_forced_variation({}, 'test_user'))
42-
43-
def test_is_user_in_forced_variation__user_not_in_forced_variation(self):
44-
""" Test that is_user_in_forced_variation returns False when user is not in experiment's forced variations. """
45-
46-
self.assertFalse(experiment.is_user_in_forced_variation({'user_1': 'control'}, 'test_user'))
47-
48-
def test_is_user_in_forced_variation__user_in_forced_variation(self):
49-
""" Test that is_user_in_forced_variation returns True when user is in experiment's forced variations. """
50-
51-
self.assertTrue(experiment.is_user_in_forced_variation({'user_1': 'control'}, 'user_1'))

0 commit comments

Comments
 (0)