feat(integrateep): Integrate Event processor #194

msohailhussain · 2019-08-08T19:58:48Z

Summary

Integrated Event Processor with Optimizely class
Added close in Optimizely class to stop ConfigManager and EventProcessor
Default ForwardingEventProcessor is implemented and used while not providing EP

Test plan

Added Unit tests
Need to test with FSC.

coveralls · 2019-08-08T20:06:50Z

Coverage increased (+0.07%) to 99.435% when pulling e802860 on rashid/optly-EP-integration into c6a2bfe on master.

msohailhussain

I don't see any unit tests updated for optimizely_spec. Please check Integration PR in C#, we have to make sure when any Optimizely Activate or Track is called, then the expected impression or conversion event type should be asserted.

msohailhussain · 2019-08-08T20:00:15Z

lib/optimizely.rb

      @decision_service = DecisionService.new(@logger, @user_profile_service)
-      @event_builder = EventBuilder.new(@logger)
+
+      @event_processor = if event_processor.respond_to?(:process)


what does it mean respond_to ?

Checks instance method process exist, similarly we are checking for config_manager; config_manager.respond_to?(:get_config)

msohailhussain · 2019-08-08T20:00:22Z

lib/optimizely.rb

+                         else
+                           ForwardingEventProcessor.new(@event_dispatcher, @logger, @notification_center)
+                         end
+      # @event_builder = EventBuilder.new(@logger)


msohailhussain · 2019-08-08T20:01:12Z

lib/optimizely.rb

-        event_key, user_id, attributes, event_tags, conversion_event
-      )
+      if @notification_center.notification_count(NotificationCenter::NOTIFICATION_TYPES[:TRACK]).positive?
+        conversion_event = EventFactory.create_log_event(user_event, @logger)


Please name it log_event

msohailhussain · 2019-08-08T20:01:57Z

lib/optimizely.rb

+      return if @stopped
+
+      @stopped = true
+      @config_manager.stop! if @config_manager.respond_to?(:stop!)


how you are making sure, that stop property exists for both event_processor and config_manager

I suggest stop should be treated as required for any event_processor or config_manager passed in. We could validate them in the constructor. When stop doesn't exist, we can log a message and ignore it.

msohailhussain · 2019-08-08T20:03:04Z

lib/optimizely.rb

+
+      @logger.log(Logger::INFO, "Activating user '#{user_id}' in experiment '#{experiment_key}'.")
      variation = config.get_variation_from_id(experiment_key, variation_id)
+      impression_event = EventFactory.create_log_event(user_event, @logger)


log_event

msohailhussain · 2019-08-08T20:03:20Z

lib/optimizely.rb

+      impression_event = EventFactory.create_log_event(user_event, @logger)
      @notification_center.send_notifications(
        NotificationCenter::NOTIFICATION_TYPES[:ACTIVATE],
        experiment, user_id, attributes, variation, impression_event


log_event change will be reflected here.

msohailhussain · 2019-08-08T20:04:39Z

lib/optimizely/config_manager/http_project_config_manager.rb

    end

    def start!
      @async_scheduler.start!


Must check if it is stopped, then no need to start again.

msohailhussain · 2019-08-08T20:06:06Z

lib/optimizely/event/forwarding_event_processor.rb

+    # the event batch into a LogEvent to be dispatched.
+    def initialize(event_dispatcher, logger, notification_center = nil)
+      @event_dispatcher = event_dispatcher
+      @logger = logger


what if logger is nil.

…gration

mjc1283 · 2019-08-23T18:18:56Z

lib/optimizely.rb

+      return if @stopped
+
+      @stopped = true
+      @config_manager.stop! if @config_manager.respond_to?(:stop!)


I suggest stop should be treated as required for any event_processor or config_manager passed in. We could validate them in the constructor. When stop doesn't exist, we can log a message and ignore it.

mjc1283 · 2019-08-23T18:39:59Z

lib/optimizely/event/batch_event_processor.rb

      end

-      @is_started = false
+      @started = false


What if stop! is called several times quickly? Would it be more correct this way, with checking @started first?

def stop! return unless @started return unless @thread.alive? # not sure if this is still necessary? @logger.log(Logger::WARN, 'Stopping scheduler.') @started = false @mutex.synchronize do event_queue << SHUTDOWN_SIGNAL @received.signal end @thread.exit end

mjc1283 · 2019-08-23T18:41:39Z

lib/optimizely/event/forwarding_event_processor.rb

+
+    def process(user_event)
+      log_event = Optimizely::EventFactory.create_log_event(user_event, @logger)
+


Is the log event notification implemented yet? I think it should be triggered here.

Implemented in PR: #196

rashidsp added 19 commits August 1, 2019 16:56

feat(eventProcessor): Adds EventProcessor and BatchEventProcessor

28ea204

feat(forwarding-EP): Implements forwarding event processor

bcfca16

feat(notification-center): Adds LogEvent notification

ccbea99

log event in forward-EP

f7a5b97

Addressing feedback

f5db35b

Addressing feedback

32a566e

Addressing feedback

c8ed3f7

fixes log event issue

c0946ea

Addresses review

cfcf5a8

resolves test issue

717adb0

fixes logger issue

6eec6bd

feat(eventprocess): Integrate Event Processor with Optimizely

7ac5896

fixes: failing test

899f3a6

tests: adds SizedQueue

a31a9da

resolves conflicts

93562a1

resolves conflicts

0b7e53d

Merge branch 'rashid/log-event' into rashid/optly-EP-integration

e1f6715

Merge branch 'master' into rashid/batch-event-processor

309ee3a

Addresses feedback

096ae95

msohailhussain commented Aug 8, 2019

View reviewed changes

rashidsp and others added 9 commits August 9, 2019 18:43

Addresses feedback

1341177

reverts log-event changes

ededaa7

resolves conflicts

2512412

config manager update

61e3426

Removed verbose log.

b409edf

Merge branch 'master' into rashid/batch-event-processor

9c019c7

Resolves unit test

2fd378f

removes log

efab9f4

adds positive number conditions

3c4a980

resolves commits

29fc092

msohailhussain requested a review from a team as a code owner August 18, 2019 15:08

jamesopti added the 🤖 24 hrs label Aug 18, 2019

fixes visitor_attributes bug

cbdce5e

msohailhussain changed the title ~~feat(integrateep): Integrate Event processor - WIP~~ feat(integrateep): Integrate Event processor Aug 20, 2019

rashidsp added 3 commits August 20, 2019 10:43

fixes visitor_attributes bug

6021aef

Addressed review

184fd06

Merge branch 'rashid/batch-event-processor' into rashid/optly-EP-inte…

432933c

…gration

rashidsp changed the base branch from rashid/batch-event-processor to master August 23, 2019 08:23

rashidsp added 2 commits August 23, 2019 13:33

resolves conflicts

0e12040

Fixes event bug

d3a7495

mjc1283 suggested changes Aug 23, 2019

View reviewed changes

jamesopti removed the 🤖 24 hrs label Aug 23, 2019

jamesopti assigned msohailhussain Aug 23, 2019

review changes

e802860

mjc1283 approved these changes Aug 27, 2019

View reviewed changes

mikeproeng37 merged commit 2df63ad into master Aug 28, 2019

rashidsp deleted the rashid/optly-EP-integration branch August 29, 2019 19:11


		def process(user_event)
		log_event = Optimizely::EventFactory.create_log_event(user_event, @logger)

feat(integrateep): Integrate Event processor #194

feat(integrateep): Integrate Event processor #194

Uh oh!

Conversation

msohailhussain commented Aug 8, 2019 • edited by rashidsp Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

coveralls commented Aug 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msohailhussain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

msohailhussain commented Aug 8, 2019 •

edited by rashidsp

Loading

coveralls commented Aug 8, 2019 •

edited

Loading