feat(ep-factory): Implemented Event Factory and User Event Factory #194

msohailhussain · 2019-07-24T18:27:14Z

Summary

UserEventFactory decouples ProjectConfig & provide the object to represent impression and conversion event.
EventFactory represents EventBatch after grouping imp/conv objects.

Test plan

Converted EventBuilder unit tests to EventFactory to make sure, all scenarios are covered to send payload to Server.

coveralls · 2019-07-24T18:37:28Z

Coverage increased (+1.8%) to 98.264% when pulling f97d430 on sohail/pr-193 into 239b687 on master.

# Conflicts: # optimizely/event/entity/conversion_event.py # optimizely/event/entity/decision.py # optimizely/event/entity/event_context.py # optimizely/event/entity/impression_event.py

… sohail/pr-189

zashraf1985 · 2019-08-29T05:50:11Z

@mikeng13... This is ready from our side, It would be great if we can get a review on this.

mjc1283

I left some comments about how invalid data might be handled. The handling of None seems a little haphazard to me.

If the intent is for the event factory to be robust against missing data and return potentially invalid LogEvents, as opposed to crashing or returning Nones, I'd like that to be more obvious. We could have comments explaining what's going on, or validation/cleaning steps performed explicitly in their own areas, instead of mixed throughout the other logic.

But in the first place, why would we create a LogEvent with some missing field, instead of logging error messages and returning None? It seems simpler to only create valid LogEvents.

mjc1283 · 2019-08-30T16:13:09Z

optimizely/event/event_factory.py

+      LogEvent instance.
+    """
+
+    def _dict_clean(obj):


I think _dict_clean should be a static method, not declared inside create_log_event.

optimizely/event/event_factory.py

mjc1283 · 2019-08-30T16:17:51Z

optimizely/event/event_factory.py

+
+  @classmethod
+  def _create_visitor(cls, user_event, logger):
+    if not user_event:


Why is this check necssary? In what circumstances is user_event falsy?

mjc1283 · 2019-08-30T16:23:06Z

optimizely/event/event_factory.py

+    visitors = []
+
+    for user_event in user_events:
+      visitors.append(cls._create_visitor(user_event, logger))


cls._create_visitor could return None. We have to filter out None values from the return value before appending it to visitors.

mjc1283 · 2019-08-30T16:23:53Z

optimizely/event/event_factory.py

+      return visitor
+
+    else:
+      # include log message for invalid event type


We have to log an error message here.

mjc1283 · 2019-08-30T16:35:53Z

optimizely/event/event_factory.py

+
+    if isinstance(user_event, ImpressionEvent):
+      decision = Decision(
+        user_event.experiment.layerId if user_event.experiment else None,


If user_event.experiment is None or user_event.variation is None, that seems like an error or invalid data, right? Shouldn't all ImpressionEvents have experiment and variation?

optimizely/event/event_factory.py

mjc1283 · 2019-08-30T16:45:10Z

optimizely/event/event_factory.py

+    """ Create Vistor Attribute List.
+
+    Args:
+      attributes: Dict representing user attributes and values which need to be recorded.


If attributes might not be a dict, let's mention what else it could be.

mariamjamal94 · 2019-09-02T11:46:10Z

I left some comments about how invalid data might be handled. The handling of None seems a little haphazard to me.

If the intent is for the event factory to be robust against missing data and return potentially invalid LogEvents, as opposed to crashing or returning Nones, I'd like that to be more obvious. We could have comments explaining what's going on, or validation/cleaning steps performed explicitly in their own areas, instead of mixed throughout the other logic.

But in the first place, why would we create a LogEvent with some missing field, instead of logging error messages and returning None? It seems simpler to only create valid LogEvents.

LogEvent is not missing a field here. EventBatch, which is sent as params property of LogEvent can have some None values. Server does not expect any keys with null values in the payload. Hence we simply need to remove those keys which don't have values. I initially did this cleanup here in the EventFactory but now I have moved it inside get_event_params method inside EventBatch class.

optimizely/event/event_factory.py

mjc1283 · 2019-09-03T16:13:08Z

optimizely/event/user_event_factory.py

+      Event object encapsulating the impression event.
+    """
+
+    experiment_key = activated_experiment.key if activated_experiment else None


The API requires experiment id to be sent in a decision. So I don't think we can create a valid impression event if activated_experiment is None.

Wouldn't it be better to return None from create_impression_event in this case? Otherwise we're creating an ImpressionEvent that won't be counted by the API.

mjc1283

I left one more comment about the return value of build_attribute_list. After that, this LGTM. Thanks for addressing all my concerns.

mjc1283 · 2019-09-04T15:25:25Z

optimizely/event/event_factory.py

+    """
+
+    if project_config is None:
+      return None


Let's return an empty list in this case, instead of None.

aliabbasrizvi · 2019-08-28T05:44:15Z

optimizely/event/log_event.py

+
+
+class LogEvent(object):
+  """ Representation of an event which can be sent to the Optimizely logging endpoint. """


nit. Optimizely events API.

# Conflicts: # optimizely/event/payload.py

aliabbasrizvi

This looks good, but I missed reviewing the import order. Please address and then it will be good to merge.

aliabbasrizvi · 2019-09-17T20:49:44Z

optimizely/event/event_factory.py

+
+from .user_event import ConversionEvent, ImpressionEvent
+from .payload import Decision, EventBatch, Snapshot, SnapshotEvent, Visitor, VisitorAttribute
+from .log_event import LogEvent


Should have mentioned earlier, but import order here is incorrect

It should be external packages (alphabetic), internal packages (alphabetic), files from same package (alphabetic)

aliabbasrizvi · 2019-09-17T20:50:24Z

optimizely/event/log_event.py

+  def __init__(self, url, params, http_verb=None, headers=None):
+    self.url = url
+    self.params = params
+    self.http_verb = http_verb or 'GET'


nit. Let's start defaulting to POST. It used to be GET long ago.

aliabbasrizvi · 2019-09-17T20:50:54Z

optimizely/event/user_event_factory.py

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+from .user_event import EventContext, ConversionEvent, ImpressionEvent


Why not import user_event rather than import classes like this? Also, follow alphabetic order.

tests/test_event_factory.py

aliabbasrizvi · 2019-09-17T20:51:39Z

tests/test_event_factory.py

+import unittest
+import uuid
+
+from . import base


Issue here as well as base belongs to same package and should be imported last.

aliabbasrizvi · 2019-09-20T17:21:06Z

optimizely/event/event_factory.py

+from optimizely.helpers import validator
+from . import user_event
+from . import payload
+from . import log_event


nit. Last 3 imports are not alphabetic.

mjamal@folio3.com and others added 4 commits July 19, 2019 12:16

feat: Add event_batch datamodels.

232f0a0

fix: unit test fixes.

2320939

Merge branch 'master' into epmodels

fbb5a68

feat: add event_factory and user_event_factory for event processing.

c162465

Removed third party dependency

633b095

msohailhussain requested review from mikeproeng37 and aliabbasrizvi July 26, 2019 06:22

msohailhussain closed this Jul 26, 2019

msohailhussain reopened this Jul 26, 2019

mnoman09 changed the base branch from master to sohail/pr-189 August 1, 2019 07:41

mnoman09 and others added 6 commits August 1, 2019 00:41

Merge branch 'master' into sohail/pr-189

8a5f531

Merge remote-tracking branch 'origin/sohail/pr-189' into sohail/pr-193

35fcd82

Removed HasAttr

d7201cb

Merge branch 'master' into sohail/pr-189

c829fca

Addressed feedback. Restructured classes.

c3c9d46

Merge branch 'sohail/pr-189' into sohail/pr-193

a2eeb85

# Conflicts: # optimizely/event/entity/conversion_event.py # optimizely/event/entity/decision.py # optimizely/event/entity/event_context.py # optimizely/event/entity/impression_event.py

msohailhussain requested a review from a team as a code owner August 20, 2019 09:49

jamesopti added the 🤖 24 hrs label Aug 20, 2019

mjamal@folio3.com and others added 11 commits August 20, 2019 15:15

update: structural changes.

a045291

fix: addressed more feedback

23ab6ce

update: removed print statements.

6474e98

merged PR 189

1184568

fix: use VisitorAttribute class to create visitor attributes list.

592a306

fix: fix log_event payload.

c4a412a

removed commented code

2a3374c

fix linter error.

ce44c11

Merge branch 'master' into sohail/pr-189

d4f2c66

fix: addressed minor feedback.

80b0963

fix: addressed minor feedback.

071460a

mjamal@folio3.com added 2 commits August 28, 2019 11:38

Merge branch 'sohail/pr-189' of github.com:optimizely/python-sdk into…

900d96d

… sohail/pr-189

Merge branch 'sohail/pr-189' into sohail/pr-193

8cd8547

mikeproeng37 requested a review from mjc1283 August 29, 2019 22:50

mjc1283 suggested changes Aug 30, 2019

View reviewed changes

jamesopti removed the 🤖 24 hrs label Aug 30, 2019

jamesopti assigned msohailhussain Aug 30, 2019

fix: addressed review comments.

e628e0f

mjc1283 suggested changes Sep 3, 2019

View reviewed changes

fix: addresses review comments.

f851754

mjc1283 approved these changes Sep 4, 2019

View reviewed changes

fix: addresses review comment.

65a752b

aliabbasrizvi reviewed Sep 5, 2019

View reviewed changes

mariamjamal94 added 2 commits September 6, 2019 10:15

fix: updated docstring for LogEvent.

8998580

Merge branch 'master' into sohail/pr-193

c3d5b07

# Conflicts: # optimizely/event/payload.py

oakbani changed the base branch from sohail/pr-189 to master September 12, 2019 13:07

oakbani requested a review from aliabbasrizvi September 12, 2019 13:11

Merge branch 'master' into sohail/pr-193

b5d461b

aliabbasrizvi approved these changes Sep 17, 2019

View reviewed changes

fix: address import order issues.

5ef24de

oakbani requested a review from aliabbasrizvi September 18, 2019 09:04

Merge branch 'master' into sohail/pr-193

1d5aeca

aliabbasrizvi approved these changes Sep 20, 2019

View reviewed changes

nit addressed

f97d430

aliabbasrizvi merged commit 6794260 into master Sep 20, 2019

oakbani deleted the sohail/pr-193 branch September 24, 2019 19:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ep-factory): Implemented Event Factory and User Event Factory #194

feat(ep-factory): Implemented Event Factory and User Event Factory #194

msohailhussain commented Jul 24, 2019

coveralls commented Jul 24, 2019 •

edited

zashraf1985 commented Aug 29, 2019

mjc1283 left a comment

mjc1283 Aug 30, 2019

mjc1283 Aug 30, 2019

mjc1283 Aug 30, 2019

mjc1283 Aug 30, 2019

mjc1283 Aug 30, 2019

mjc1283 Aug 30, 2019

mariamjamal94 commented Sep 2, 2019

mjc1283 Sep 3, 2019

mjc1283 left a comment

mjc1283 Sep 4, 2019

aliabbasrizvi Aug 28, 2019

aliabbasrizvi left a comment

aliabbasrizvi Sep 17, 2019

aliabbasrizvi Sep 17, 2019

aliabbasrizvi Sep 17, 2019

aliabbasrizvi Sep 17, 2019

aliabbasrizvi Sep 20, 2019



		class LogEvent(object):
		""" Representation of an event which can be sent to the Optimizely logging endpoint. """

feat(ep-factory): Implemented Event Factory and User Event Factory #194

feat(ep-factory): Implemented Event Factory and User Event Factory #194

Conversation

msohailhussain commented Jul 24, 2019

Summary

Test plan

coveralls commented Jul 24, 2019 • edited

zashraf1985 commented Aug 29, 2019

mjc1283 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariamjamal94 commented Sep 2, 2019

Choose a reason for hiding this comment

mjc1283 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aliabbasrizvi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 24, 2019 •

edited