-
Notifications
You must be signed in to change notification settings - Fork 36
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
Introduce Bot Filtering #121
Changes from 5 commits
46c964e
6b48a7b
ca14331
c458c08
0a7e74f
9e05758
b938de3
d94231a
cd7b448
c4359d1
e3295cd
aabb4a5
fe607b4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ | |
|
||
from . import version | ||
from .helpers import event_tag_utils | ||
from .helpers.enums import ControlAttributes | ||
|
||
|
||
class Event(object): | ||
|
@@ -85,6 +86,15 @@ def _get_anonymize_ip(self): | |
|
||
return self.config.get_anonymize_ip_value() | ||
|
||
def _get_bot_filtering(self): | ||
""" Get bot filtering bool | ||
|
||
Returns: | ||
'botFiltering' value in the datafile. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit. I would similarly also change the comment for IP anonymization in the method above to Boolean representing whether IP anonymization is enabled or not. |
||
""" | ||
|
||
return self.config.get_bot_filtering_value() | ||
|
||
@abstractmethod | ||
def _get_time(self): | ||
""" Get time in milliseconds to be added. | ||
|
@@ -172,19 +182,28 @@ def _get_attributes(self, attributes): | |
if not attributes: | ||
return [] | ||
|
||
for attribute_key in attributes.keys(): | ||
for attribute_key in sorted(attributes.keys()): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we need to sort? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. attributes is a dict. Order of keys is not guaranteed to be same for all python versions. Hence for eventpayload unit tests to pass for all versions, we need to ensure that they get appended in the same order. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does ordering matter? Let us not do this. It seems unnecessary. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @aliabbasrizvi attributes is a dict. Order of keys is not guaranteed to be same for all python versions. Hence for eventpayload unit tests to pass for all versions, we need to ensure that they get appended in the same order. |
||
attribute_value = attributes.get(attribute_key) | ||
# Omit falsy attribute values | ||
if attribute_value: | ||
attribute = self.config.get_attribute(attribute_key) | ||
if attribute: | ||
attribute_id = self.config.get_attribute_id(attribute_key) | ||
if attribute_id: | ||
params.append({ | ||
self.EventParams.EVENT_ID: attribute.id, | ||
'entity_id': attribute_id, | ||
'key': attribute_key, | ||
'type': self.EventParams.CUSTOM, | ||
'value': attribute_value, | ||
}) | ||
|
||
# Append Bot Filtering Attribute | ||
if isinstance(self._get_bot_filtering(), bool): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice. |
||
params.append({ | ||
'entity_id': ControlAttributes.BOT_FILTERING, | ||
'key': ControlAttributes.BOT_FILTERING, | ||
'type': self.EventParams.CUSTOM, | ||
'value': self._get_bot_filtering(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lets call |
||
}) | ||
|
||
return params | ||
|
||
def _get_required_params_for_impression(self, experiment, variation_id): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,8 @@ | |
SUPPORTED_VERSIONS = [V2_CONFIG_VERSION] | ||
UNSUPPORTED_VERSIONS = [V1_CONFIG_VERSION] | ||
|
||
RESERVED_ATTRIBUTE_PREFIX = '$opt_' | ||
|
||
|
||
class ProjectConfig(object): | ||
""" Representation of the Optimizely project config. """ | ||
|
@@ -56,6 +58,7 @@ def __init__(self, datafile, logger, error_handler): | |
self.feature_flags = config.get('featureFlags', []) | ||
self.rollouts = config.get('rollouts', []) | ||
self.anonymize_ip = config.get('anonymizeIP', False) | ||
self.bot_filtering = config.get('botFiltering', None) | ||
|
||
# Utility maps for quick lookup | ||
self.group_id_map = self._generate_key_map(self.groups, 'id', entities.Group) | ||
|
@@ -363,20 +366,28 @@ def get_event(self, event_key): | |
self.error_handler.handle_error(exceptions.InvalidEventException(enums.Errors.INVALID_EVENT_KEY_ERROR)) | ||
return None | ||
|
||
def get_attribute(self, attribute_key): | ||
""" Get attribute for the provided attribute key. | ||
def get_attribute_id(self, attribute_key): | ||
""" Get attribute ID for the provided attribute key. | ||
|
||
Args: | ||
attribute_key: Attribute key for which attribute is to be fetched. | ||
|
||
Returns: | ||
Attribute corresponding to the provided attribute key. | ||
Attribute ID corresponding to the provided attribute key. | ||
""" | ||
|
||
attribute = self.attribute_key_map.get(attribute_key) | ||
has_reserved_prefix = attribute_key.startswith(RESERVED_ATTRIBUTE_PREFIX) | ||
|
||
if attribute: | ||
return attribute | ||
if has_reserved_prefix: | ||
self.logger.warning(('Attribute %s unexpectedly has reserved prefix %s; using attribute ID ' | ||
'instead of reserved attribute name.' % (attribute_key, RESERVED_ATTRIBUTE_PREFIX))) | ||
|
||
return attribute.id | ||
|
||
if has_reserved_prefix and attribute_key != enums.ControlAttributes.BOT_FILTERING: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the not condition? I do not thing it is needed. Lets remove the second condition. |
||
return attribute_key | ||
|
||
self.logger.error('Attribute "%s" is not in datafile.' % attribute_key) | ||
self.error_handler.handle_error(exceptions.InvalidAttributeException(enums.Errors.INVALID_ATTRIBUTE_ERROR)) | ||
|
@@ -595,3 +606,12 @@ def get_anonymize_ip_value(self): | |
""" | ||
|
||
return self.anonymize_ip | ||
|
||
def get_bot_filtering_value(self): | ||
""" Gets the bot filtering value. | ||
|
||
Returns: | ||
A boolean value that indicates if bot filtering should be enabled. | ||
""" | ||
|
||
return self.bot_filtering |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -172,6 +172,7 @@ def test_init__with_v4_datafile(self): | |
'revision': '42', | ||
'version': '4', | ||
'anonymizeIP': False, | ||
'botFiltering': True, | ||
'events': [{ | ||
'key': 'test_event', | ||
'experimentIds': ['111127'], | ||
|
@@ -387,6 +388,7 @@ def test_init__with_v4_datafile(self): | |
self.assertEqual(config_dict['revision'], project_config.revision) | ||
self.assertEqual(config_dict['experiments'], project_config.experiments) | ||
self.assertEqual(config_dict['events'], project_config.events) | ||
self.assertEqual(config_dict['botFiltering'], project_config.bot_filtering) | ||
|
||
expected_group_id_map = { | ||
'19228': entities.Group( | ||
|
@@ -679,6 +681,20 @@ def test_get_project_id(self): | |
|
||
self.assertEqual(self.config_dict['projectId'], self.project_config.get_project_id()) | ||
|
||
def test_get_bot_filtering(self): | ||
""" Test that bot filtering is retrieved correctly when using get_bot_filtering_value. """ | ||
|
||
# Assert bot filtering is None when not provided in data file | ||
self.assertTrue('botFiltering' not in self.config_dict) | ||
self.assertIsNone(self.project_config.get_bot_filtering_value()) | ||
|
||
# Assert bot filtering is retrieved as provided in the data file | ||
opt_obj = optimizely.Optimizely(json.dumps(self.config_dict_with_features)) | ||
project_config = opt_obj.config | ||
self.assertEqual(self.config_dict_with_features['botFiltering'], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit. This styling seems wrong
|
||
project_config.get_bot_filtering_value() | ||
) | ||
|
||
def test_get_experiment_from_key__valid_key(self): | ||
""" Test that experiment is retrieved correctly for valid experiment key. """ | ||
|
||
|
@@ -787,16 +803,32 @@ def test_get_event__invalid_key(self): | |
|
||
self.assertIsNone(self.project_config.get_event('invalid_key')) | ||
|
||
def test_get_attribute__valid_key(self): | ||
""" Test that attribute is retrieved correctly for valid attribute key. """ | ||
def test_get_attribute_id__valid_key(self): | ||
""" Test that attribute ID is retrieved correctly for valid attribute key. """ | ||
|
||
self.assertEqual(entities.Attribute('111094', 'test_attribute'), | ||
self.project_config.get_attribute('test_attribute')) | ||
self.assertEqual('111094', | ||
self.project_config.get_attribute_id('test_attribute')) | ||
|
||
def test_get_attribute__invalid_key(self): | ||
def test_get_attribute_id__invalid_key(self): | ||
""" Test that None is returned when provided attribute key is invalid. """ | ||
|
||
self.assertIsNone(self.project_config.get_attribute('invalid_key')) | ||
self.assertIsNone(self.project_config.get_attribute_id('invalid_key')) | ||
|
||
def test_get_attribute_id__reserved_key(self): | ||
""" Test that Attribute Key is returned as ID when provided attribute key is reserved key. """ | ||
self.assertEqual('$opt_user_agent', | ||
self.project_config.get_attribute_id('$opt_user_agent')) | ||
|
||
def test_get_attribute_id__unknown_key_with_opt_prefix(self): | ||
""" Test that Attribute Key is returned as ID when provided attribute key is not | ||
present in the datafile but has $opt prefix. """ | ||
self.assertEqual('$opt_interesting', | ||
self.project_config.get_attribute_id('$opt_interesting')) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit. Indentation seems off. s of self should align with the ' |
||
|
||
def test_get_attribute_id__key_is_bot_filtering_enum(self): | ||
""" Test that None is returned when provided attribute key is | ||
equal to '$opt_bot_filtering'. """ | ||
self.assertIsNone(self.project_config.get_attribute_id('$opt_bot_filtering')) | ||
|
||
def test_get_group__valid_id(self): | ||
""" Test that group is retrieved correctly for valid group ID. """ | ||
|
@@ -1074,6 +1106,7 @@ def test_set_forced_variation_when_called_to_remove_forced_variation(self): | |
|
||
|
||
class ConfigLoggingTest(base.BaseTest): | ||
|
||
def setUp(self): | ||
base.BaseTest.setUp(self) | ||
self.optimizely = optimizely.Optimizely(json.dumps(self.config_dict), | ||
|
@@ -1136,14 +1169,25 @@ def test_get_event__invalid_key(self): | |
|
||
mock_config_logging.error.assert_called_once_with('Event "invalid_key" is not in datafile.') | ||
|
||
def test_get_attribute__invalid_key(self): | ||
def test_get_attribute_id__invalid_key(self): | ||
""" Test that message is logged when provided attribute key is invalid. """ | ||
|
||
with mock.patch.object(self.project_config, 'logger') as mock_config_logging: | ||
self.project_config.get_attribute('invalid_key') | ||
self.project_config.get_attribute_id('invalid_key') | ||
|
||
mock_config_logging.error.assert_called_once_with('Attribute "invalid_key" is not in datafile.') | ||
|
||
def test_get_attribute_id__key_with_opt_prefix_but_not_a_control_attribute(self): | ||
""" Test that message is logged when provided attribute key has $opt_ in prefix and | ||
key is not one of the control attributes. """ | ||
self.project_config.attribute_key_map['$opt_abc'] = entities.Attribute('007', '$opt_abc') | ||
|
||
with mock.patch.object(self.project_config, 'logger') as mock_config_logging: | ||
self.project_config.get_attribute_id('$opt_abc') | ||
|
||
mock_config_logging.warning.assert_called_once_with(("Attribute $opt_abc unexpectedly has reserved prefix $opt_; " | ||
"using attribute ID instead of reserved attribute name.")) | ||
|
||
def test_get_group__invalid_id(self): | ||
""" Test that message is logged when provided group ID is invalid. """ | ||
|
||
|
@@ -1210,12 +1254,12 @@ def test_get_event__invalid_key(self): | |
enums.Errors.INVALID_EVENT_KEY_ERROR, | ||
self.project_config.get_event, 'invalid_key') | ||
|
||
def test_get_attribute__invalid_key(self): | ||
def test_get_attribute_id__invalid_key(self): | ||
""" Test that exception is raised when provided attribute key is invalid. """ | ||
|
||
self.assertRaisesRegexp(exceptions.InvalidAttributeException, | ||
enums.Errors.INVALID_ATTRIBUTE_ERROR, | ||
self.project_config.get_attribute, 'invalid_key') | ||
self.project_config.get_attribute_id, 'invalid_key') | ||
|
||
def test_get_group__invalid_id(self): | ||
""" Test that exception is raised when provided group ID is invalid. """ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit. I would usually just import enums and refer to
enums.ControlAttributes