Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
d7f69ec
Added Log event notification
mnoman09 Aug 20, 2019
8853cb3
fix: fix imports in test_event_processor.
mnoman09 Aug 21, 2019
5934764
fix: linter issues
mnoman09 Aug 21, 2019
af497f9
Merge branch 'mnoman/AddBatchEP' into mnoman/log_event_notification
mnoman09 Aug 22, 2019
f21c94b
Merge remote-tracking branch 'remotes/main_origin/mnoman/AddBatchEP' …
Aug 22, 2019
ddef208
fix: remove print statement.
Aug 23, 2019
e964048
fix: remove print statement.
Aug 23, 2019
8953a43
Merge branch 'mnoman/log_event_notification' of github.com:optimizely…
Aug 23, 2019
752e35f
Merge branch 'mnoman/AddBatchEP' into mnoman/log_event_notification
Aug 28, 2019
f9a2cb6
Merge branch 'mnoman/AddBatchEP' into mnoman/log_event_notification
Aug 28, 2019
54446b9
Merge branch 'mnoman/log_event_notification' of github.com:optimizely…
Aug 28, 2019
c92f79d
Merge branch 'mnoman/AddBatchEP' into mnoman/log_event_notification
mariamjamal94 Sep 2, 2019
d42d14b
Merge branch 'mnoman/AddBatchEP' into mnoman/log_event_notification
mariamjamal94 Sep 4, 2019
06ac3f0
Merge branch 'mnoman/AddBatchEP' into mnoman/log_event_notification
mariamjamal94 Sep 5, 2019
9630139
Merge branch 'mnoman/AddBatchEP' into mnoman/log_event_notification
mariamjamal94 Sep 5, 2019
2582fb7
Merge branch 'mnoman/AddBatchEP' into mnoman/log_event_notification
mariamjamal94 Sep 6, 2019
75725bd
Merge branch 'mnoman/AddBatchEP' into mnoman/log_event_notification
mariamjamal94 Sep 6, 2019
cb07348
fix: address review comments.
mariamjamal94 Sep 12, 2019
f50e053
Merge branch 'mnoman/AddBatchEP' into mnoman/log_event_notification
mariamjamal94 Sep 13, 2019
bb3f738
fix: remove close method from test_notification_center.
mariamjamal94 Sep 13, 2019
46cc51d
Merge branch 'mnoman/AddBatchEP' into mnoman/log_event_notification
mariamjamal94 Sep 17, 2019
1ab8b32
fix: linting error.
mariamjamal94 Sep 17, 2019
f0dd1fd
Merge branch 'mnoman/AddBatchEP' into mnoman/log_event_notification
mariamjamal94 Sep 17, 2019
4d4e7d1
update: update documentation for event_processor.
mariamjamal94 Sep 17, 2019
cbd918a
Merge branch 'mnoman/AddBatchEP' into mnoman/log_event_notification
mariamjamal94 Sep 17, 2019
4c8b7f3
Merge branch 'mnoman/AddBatchEP' into mnoman/log_event_notification
mariamjamal94 Sep 18, 2019
3b2cb4a
Merge branch 'mnoman/AddBatchEP' into mnoman/log_event_notification
mariamjamal94 Sep 18, 2019
4753c0b
Merge branch 'mnoman/AddBatchEP' into mnoman/log_event_notification
oakbani Sep 20, 2019
3baf024
Merge branch 'mnoman/AddBatchEP' into mnoman/log_event_notification
mariamjamal94 Sep 24, 2019
7e20be7
Merge branch 'mnoman/log_event_notification' of github.com:optimizely…
mariamjamal94 Sep 24, 2019
614a3d4
Merge branch 'mnoman/AddBatchEP' into mnoman/log_event_notification
mariamjamal94 Sep 25, 2019
2c67253
fix: address review comments.
mariamjamal94 Sep 25, 2019
9ebf1a4
fix notification center impoer
oakbani Oct 3, 2019
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
14 changes: 12 additions & 2 deletions optimizely/event/event_processor.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
from optimizely import logger as _logging
from optimizely.closeable import Closeable
from optimizely.event_dispatcher import EventDispatcher as default_event_dispatcher
from optimizely.helpers import validator
from optimizely.helpers import validator, enums
Copy link
Contributor

Choose a reason for hiding this comment

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

nit. I'd recommend splitting up imports into separate lines. Just a conventional thing.


ABC = abc.ABCMeta('ABC', (object,), {'__slots__': ()})

Expand All @@ -40,6 +40,7 @@ def process(user_event):
class BatchEventProcessor(EventProcessor, Closeable):
"""
BatchEventProcessor is a batched implementation of the EventProcessor.

The BatchEventProcessor maintains a single consumer thread that pulls events off of
the blocking queue and buffers them for either a configured batch size or for a
maximum duration before the resulting LogEvent is sent to the EventDispatcher.
Expand All @@ -60,7 +61,8 @@ def __init__(self,
event_queue=None,
batch_size=None,
flush_interval=None,
timeout_interval=None):
timeout_interval=None,
notification_center=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add some good explanations for all the fields here. Something like: https://github.com/optimizely/python-sdk/blob/master/optimizely/optimizely.py#L42-L59

self.event_dispatcher = event_dispatcher or default_event_dispatcher
self.logger = _logging.adapt_logger(logger or _logging.NoOpLogger())
self.event_queue = event_queue or queue.Queue(maxsize=self._DEFAULT_QUEUE_CAPACITY)
Expand All @@ -72,6 +74,8 @@ def __init__(self,
self.timeout_interval = timedelta(milliseconds=timeout_interval) \
if self._validate_intantiation_props(timeout_interval, 'timeout_interval') \
else self._DEFAULT_TIMEOUT_INTERVAL

self.notification_center = notification_center
self._disposed = False
self._is_started = False
self._current_batch = list()
Expand Down Expand Up @@ -163,6 +167,12 @@ def _flush_queue(self):

log_event = EventFactory.create_log_event(to_process_batch, self.logger)

if self.notification_center is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be validating that notification_center is an instance of notification_center.NotificationCenter or does that happen elsewhere?

self.notification_center.send_notifications(
enums.NotificationTypes.LOG_EVENT,
log_event
)

try:
self.event_dispatcher.dispatch_event(log_event)
except Exception as e:
Expand Down
4 changes: 4 additions & 0 deletions optimizely/helpers/enums.py
Original file line number Diff line number Diff line change
Expand Up @@ -123,8 +123,12 @@ class NotificationTypes(object):

TRACK notification listener has the following parameters:
str event_key, str user_id, dict attributes (can be None), event_tags (can be None), Event event

LOG_EVENT notification listener has the following parameter(s):
LogEvent log_event
"""
ACTIVATE = 'ACTIVATE:experiment, user_id, attributes, variation, event'
DECISION = 'DECISION:type, user_id, attributes, decision_info'
OPTIMIZELY_CONFIG_UPDATE = 'OPTIMIZELY_CONFIG_UPDATE'
TRACK = 'TRACK:event_key, user_id, attributes, event_tags, event'
LOG_EVENT = 'LOG_EVENT:log_event'
36 changes: 33 additions & 3 deletions tests/test_event_processor.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,12 @@
from six.moves import queue

from . import base
from optimizely.logger import SimpleLogger
from optimizely.event.payload import Decision, Visitor
from optimizely.event.user_event_factory import UserEventFactory
from optimizely.event.event_processor import BatchEventProcessor
from optimizely.event.log_event import LogEvent
from optimizely.event.user_event_factory import UserEventFactory
from optimizely.helpers import enums
from optimizely.logger import SimpleLogger


class CanonicalEvent(object):
Expand Down Expand Up @@ -115,6 +117,7 @@ def setUp(self, *args, **kwargs):
self.event_name = 'test_event'
self.event_queue = queue.Queue(maxsize=self.DEFAULT_QUEUE_CAPACITY)
self.optimizely.logger = SimpleLogger()
self.notification_center = self.optimizely.notification_center

def tearDown(self):
self._event_processor.close()
Expand All @@ -130,7 +133,8 @@ def _set_event_processor(self, event_dispatcher, logger):
self.event_queue,
self.MAX_BATCH_SIZE,
self.MAX_DURATION_MS,
self.MAX_TIMEOUT_INTERVAL_MS
self.MAX_TIMEOUT_INTERVAL_MS,
self.optimizely.notification_center
)

def test_drain_on_close(self):
Expand Down Expand Up @@ -376,3 +380,29 @@ def test_init__NaN_timeout_interval(self):
# default timeout interval is 5s.
self.assertEqual(self._event_processor.timeout_interval, timedelta(seconds=5))
mock_config_logging.info.assert_called_with('Using default value for timeout_interval.')

def test_notification_center(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

nit. test_notification_center__on_log_event


mock_event_dispatcher = mock.Mock()
callback_hit = [False]

def on_log_event(log_event):
self.assertStrictTrue(isinstance(log_event, LogEvent))
callback_hit[0] = True

self.optimizely.notification_center.add_notification_listener(
enums.NotificationTypes.LOG_EVENT, on_log_event
)

with mock.patch.object(self.optimizely, 'logger') as mock_config_logging:
self._set_event_processor(mock_event_dispatcher, mock_config_logging)

user_event = self._build_conversion_event(self.event_name, self.project_config)
self._event_processor.process(user_event)

self._event_processor.close()

self.assertEqual(True, callback_hit[0])
self.assertEqual(1, len(self.optimizely.notification_center.notification_listeners[
enums.NotificationTypes.LOG_EVENT
]))
17 changes: 17 additions & 0 deletions tests/test_notification_center.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ def on_track_listener(*args):
pass


def on_log_event_listener(*args):
pass


class NotificationCenterTest(unittest.TestCase):

def test_add_notification_listener__valid_type(self):
Expand All @@ -59,6 +63,11 @@ def test_add_notification_listener__valid_type(self):
4, test_notification_center.add_notification_listener(enums.NotificationTypes.TRACK, on_track_listener)
)

self.assertEqual(
5, test_notification_center.add_notification_listener(enums.NotificationTypes.LOG_EVENT,
on_log_event_listener)
)

def test_add_notification_listener__multiple_listeners(self):
""" Test that multiple listeners of the same type can be successfully added. """

Expand Down Expand Up @@ -138,6 +147,7 @@ def another_on_activate_listener(*args):
self.assertEqual(2, len(test_notification_center.notification_listeners[enums.NotificationTypes.ACTIVATE]))
self.assertEqual(1, len(test_notification_center.notification_listeners[enums.NotificationTypes.DECISION]))
self.assertEqual(0, len(test_notification_center.notification_listeners[enums.NotificationTypes.TRACK]))
self.assertEqual(0, len(test_notification_center.notification_listeners[enums.NotificationTypes.LOG_EVENT]))

# Remove one of the activate listeners and assert.
self.assertTrue(test_notification_center.remove_notification_listener(3))
Expand All @@ -164,6 +174,10 @@ def another_on_activate_listener(*args):
3, test_notification_center.add_notification_listener(enums.NotificationTypes.ACTIVATE,
another_on_activate_listener)
)
self.assertEqual(
4, test_notification_center.add_notification_listener(enums.NotificationTypes.LOG_EVENT,
on_log_event_listener)
)

# Try removing a listener which does not exist.
self.assertFalse(test_notification_center.remove_notification_listener(42))
Expand All @@ -180,6 +194,7 @@ def test_clear_notification_listeners(self):
on_config_update_listener)
test_notification_center.add_notification_listener(enums.NotificationTypes.DECISION, on_decision_listener)
test_notification_center.add_notification_listener(enums.NotificationTypes.TRACK, on_track_listener)
test_notification_center.add_notification_listener(enums.NotificationTypes.LOG_EVENT, on_log_event_listener)

# Assert all listeners are there:
for notification_type in notification_center.NOTIFICATION_TYPES:
Expand Down Expand Up @@ -210,6 +225,7 @@ def test_clear_all_notification_listeners(self):
on_config_update_listener)
test_notification_center.add_notification_listener(enums.NotificationTypes.DECISION, on_decision_listener)
test_notification_center.add_notification_listener(enums.NotificationTypes.TRACK, on_track_listener)
test_notification_center.add_notification_listener(enums.NotificationTypes.LOG_EVENT, on_log_event_listener)

# Assert all listeners are there:
for notification_type in notification_center.NOTIFICATION_TYPES:
Expand All @@ -219,6 +235,7 @@ def test_clear_all_notification_listeners(self):
test_notification_center.clear_all_notification_listeners()

for notification_type in notification_center.NOTIFICATION_TYPES:
print(notification_type)
Copy link

Choose a reason for hiding this comment

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

Remove

self.assertEqual(0, len(test_notification_center.notification_listeners[notification_type]))

def set_listener_called_to_true(self):
Expand Down