feat(eventProcessor): Add EventProcessor and BatchEventProcessor #191

rashidsp · 2019-08-01T12:06:58Z

Summary

Introduces an EventProcessor interface.
Introduces a BatchEventProcessor
Excluded batch_event_processor.rb from rubocop's Metrics/ParameterLists rule.

Buffering events within a queue before dispatching is an optimization that should prevent SDK implementations from exhausting resources while increasing throughput. This implementation relies on a EventQueue to buffer events received from one-to-many producers. A single consumer thread continuously polls from this queue to build a batch before emitting the batched LogEvent.

Test Plan

Added unit tests

coveralls · 2019-08-01T12:12:31Z

Coverage decreased (-0.09%) to 99.385% when pulling 184fd06 on rashid/batch-event-processor into 7accf32 on master.

mnoman09

Looks good, Just added few comments.

mnoman09 · 2019-08-01T12:27:48Z

lib/optimizely/event/batch_event_processor.rb

+
+    DEFAULT_BATCH_SIZE = 10
+    DEFAULT_FLUSH_INTERVAL = 30_000
+    DEFAULT_TIMEOUT_INTERVAL = (5000 * 60)


Remove this variable as it is of no use.

mnoman09 · 2019-08-01T12:28:47Z

lib/optimizely/event/batch_event_processor.rb

+      event_dispatcher:,
+      batch_size:,
+      flush_interval:,
+      timeout_interval:,


Remove this as you are just passing it and it is not getting used anywhere.

mnoman09 · 2019-08-01T13:18:35Z

lib/optimizely/event/batch_event_processor.rb

+          @received.wait(@mutex, 0.05)
+        end
+
+        item = @event_queue.pop


Shouldn't this be in mutex.synchronize block?

mnoman09 · 2019-08-01T13:32:28Z

lib/optimizely/event/batch_event_processor.rb

+    def flush_queue!
+      return if @current_batch.empty?
+
+      log_event = Optimizely::EventFactory.create_log_event(@current_batch, @logger)


I think,

We should put current_batch in temp_queue.

Clear current_batch

Pass temp_queue in create_log_event
All this should happen inside synchornized block.
See Csharp for reference.

@msohailhussain what do you suggest?

mnoman09 · 2019-08-01T13:37:05Z

lib/optimizely/event/batch_event_processor.rb

+      # Reset the deadline if starting a new batch.
+      @flushing_interval_deadline = Helpers::DateTimeUtils.create_timestamp + @flush_interval if @current_batch.empty?
+
+      @logger.log(Logger::DEBUG, "Adding user event: #{user_event.event['key']} to btach.")


Typo:

Suggested change

@logger.log(Logger::DEBUG, "Adding user event: #{user_event.event['key']} to btach.")

@logger.log(Logger::DEBUG, "Adding user event: #{user_event.event['key']} to batch.")

mnoman09 · 2019-08-01T13:39:59Z

lib/optimizely/event/batch_event_processor.rb

+      @flushing_interval_deadline = Helpers::DateTimeUtils.create_timestamp + @flush_interval if @current_batch.empty?
+
+      @logger.log(Logger::DEBUG, "Adding user event: #{user_event.event['key']} to btach.")
+      @current_batch << user_event


I think we should also add mutex lock here

We add mutex lock, when we add event into EventQueue!

mnoman09 · 2019-08-01T13:48:31Z

lib/optimizely/event/event_processor.rb

+  class EventProcessor
+    # EventProcessor interface is used to provide an intermediary processing stage within
+    # event production. It's assumed that the EventProcessor dispatches events via a provided
+    # EventHandler.


nit: change it to EventDispatcher.

mnoman09 · 2019-08-01T14:08:05Z

spec/event/batch_event_processor_spec.rb

+    allow(@event_dispatcher).to receive(:dispatch_event).with(instance_of(Optimizely::Event))
+  end
+
+  it 'return empty event queue event is processed' do


Can we also make sure if the dispatchEvent is getting called and logevent is correct?

msohailhussain

please address comments

msohailhussain · 2019-08-02T17:16:02Z

.rubocop_todo.yml

    - 'lib/optimizely/event/entity/snapshot_event.rb'
    - 'lib/optimizely/event/entity/conversion_event.rb'
    - 'lib/optimizely/event/entity/event_context.rb'
+    - 'lib/optimizely/event/batch_event_processor.rb'


alphabetize order

msohailhussain · 2019-08-02T17:17:19Z

lib/optimizely/event/batch_event_processor.rb

+      event_dispatcher:,
+      batch_size:,
+      flush_interval:,
+      start_by_default: false,


i don't think so it's really needed.

msohailhussain · 2019-08-02T17:19:08Z

lib/optimizely/event/batch_event_processor.rb

+      @event_dispatcher = event_dispatcher
+      @batch_size = batch_size || DEFAULT_BATCH_SIZE
+      @flush_interval = flush_interval || DEFAULT_BATCH_INTERVAL
+      @logger = logger


shouldn't it assign NoOpLogger if its nil

msohailhussain · 2019-08-02T17:20:40Z

lib/optimizely/event/batch_event_processor.rb

+      @mutex = Mutex.new
+      @received = ConditionVariable.new
+      @current_batch = []
+      @disposed = false


disposed is it also implemented in http_project_config_manager?

msohailhussain · 2019-08-02T17:22:18Z

lib/optimizely/event/batch_event_processor.rb

+        return
+      end
+
+      if @event_queue.include? user_event


what this condition mean?

Updated for DEFAULT_QUEUE_CAPACITY

msohailhussain · 2019-08-02T17:32:16Z

lib/optimizely/event/batch_event_processor.rb

+        @current_batch = []
+      end
+
+      # Reset the deadline if starting a new batch.


Why this condition can't b in the if

Inline condition; Rubocop's auto-fix

mikeproeng37 · 2019-08-07T22:42:36Z

lib/optimizely/event/batch_event_processor.rb

+    attr_reader :event_queue
+
+    DEFAULT_BATCH_SIZE = 10
+    DEFAULT_BATCH_INTERVAL = 30_000


What's with the _?

RuboCop's change, following ruby's style guide.

Please add a comment about what the unit of this number is.

mikeproeng37 · 2019-08-07T22:47:23Z

spec/event/batch_event_processor_spec.rb

+    allow(Optimizely::EventFactory).to receive(:create_log_event).with(any_args)
+    expected_batch = []
+    counter = 0
+    until counter >= 10


Can we process 11 and make sure the batch only has 10?

mikeproeng37

Mostly looks good, I just have a few comments

mikeproeng37 · 2019-08-14T18:24:56Z

lib/optimizely/event/batch_event_processor.rb

+        end
+
+        if item.is_a? Optimizely::UserEvent
+          @logger.log(Logger::DEBUG, "Received add to batch signal. with event: #{item.event['key']}.")


Let's not log here as it would be extremely noisy

I think this is not noisy. because when any impression conversion event occurs, then it will be logged, which is OK. I may remove, if you still think it's noisy.

I still think it is noisy. Let's leave it out unless someone actually requests for this level of verbosity.

mikeproeng37 · 2019-08-14T18:25:49Z

lib/optimizely/event/batch_event_processor.rb

+    def run
+      loop do
+        if Helpers::DateTimeUtils.create_timestamp > @flushing_interval_deadline
+          @logger.log(


I think @msohailhussain mentioned that during testing this was very verbose as it was logged many times. If he is removing from C# SDK it should be removed here as well.

That's not the place, it's under this condition, am going to remove.

if item.nil? @logger.log(Logger::DEBUG, 'Empty item, sleeping for 50ms.') sleep(0.05) next end

mnoman09

looks good. Just add conditions that if batch_size is None or batch_size <= 0 than use default batch size. same goes for flush_time_interval

mikeproeng37

lgtm

aliabbasrizvi

Mostly small changes

aliabbasrizvi · 2019-08-19T18:41:46Z

lib/optimizely/event/batch_event_processor.rb

@@ -0,0 +1,200 @@
+# frozen_string_literal: true
+


Rubocop's rule Layout/EmptyLineAfterMagicComment

aliabbasrizvi · 2019-08-19T18:42:26Z

lib/optimizely/event/batch_event_processor.rb

+  class BatchEventProcessor < EventProcessor
+    # BatchEventProcessor is a batched implementation of the Interface EventProcessor.
+    # Events passed to the BatchEventProcessor are immediately added to a EventQueue.
+    # The BatchEventProcessor maintains a single consumer thread that pulls events off of


This sentence seems incomplete.

aliabbasrizvi · 2019-08-19T18:42:53Z

lib/optimizely/event/batch_event_processor.rb

+    attr_reader :event_queue
+
+    DEFAULT_BATCH_SIZE = 10
+    DEFAULT_BATCH_INTERVAL = 30_000


Please add a comment about what the unit of this number is.

aliabbasrizvi · 2019-08-19T18:43:41Z

lib/optimizely/event/batch_event_processor.rb

+      @batch_size = if (batch_size.is_a? Integer) && positive_number?(batch_size)
+                      batch_size
+                    else
+                      DEFAULT_BATCH_SIZE


A message should be logger here that batch_size is being set to default value.

aliabbasrizvi · 2019-08-19T18:49:39Z

lib/optimizely/event/batch_event_processor.rb

+      end
+
+      # Reset the deadline if starting a new batch.
+      @flushing_interval_deadline = (Helpers::DateTimeUtils.create_timestamp + @flush_interval) if @current_batch.empty?


If I understand correctly, create_timestamp creates time in local timezone: https://github.com/optimizely/ruby-sdk/blob/master/lib/optimizely/helpers/date_time_utils.rb#L23-L26

Shouldn't that time be in UTC?

aliabbasrizvi · 2019-08-19T18:50:20Z

lib/optimizely/event/batch_event_processor.rb

+        @logger.log(Logger::DEBUG, 'Revisions mismatched: Flushing current batch.')
+        return true
+      end
+      # Projects should match


nit. New line above this.

aliabbasrizvi

Conditionally approving. Please address feedback before merging.

msohailhussain

lgtm

feat(eventProcessor): Adds EventProcessor and BatchEventProcessor

28ea204

rashidsp added the WIP Work in progress label Aug 1, 2019

rashidsp changed the title ~~feat(eventProcessor): Adds EventProcessor and BatchEventProcessor- Do Not Review~~ feat(eventProcessor): BatchEventProcessor- Do Not Review Aug 1, 2019

rashidsp requested a review from mnoman09 August 1, 2019 12:07

mnoman09 suggested changes Aug 1, 2019

View reviewed changes

rashidsp force-pushed the rashid/batch-event-processor branch from c7c2552 to 28ea204 Compare August 2, 2019 07:53

Addressing feedback

f5db35b

rashidsp requested a review from mnoman09 August 2, 2019 16:28

msohailhussain suggested changes Aug 2, 2019

View reviewed changes

rashidsp added 3 commits August 5, 2019 20:33

Addresses review

cfcf5a8

resolves test issue

717adb0

fixes logger issue

6eec6bd

rashidsp changed the title ~~feat(eventProcessor): BatchEventProcessor- Do Not Review~~ feat(eventProcessor): Adds EventProcessor and BatchEventProcessor Aug 5, 2019

rashidsp removed the WIP Work in progress label Aug 5, 2019

tests: adds SizedQueue

a31a9da

mikeproeng37 suggested changes Aug 7, 2019

View reviewed changes

rashidsp added 2 commits August 8, 2019 09:58

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

309ee3a

Addresses feedback

096ae95

rashidsp requested a review from mikeproeng37 August 8, 2019 08:37

mikeproeng37 suggested changes Aug 14, 2019

View reviewed changes

Removed verbose log.

b409edf

rashidsp requested a review from a team as a code owner August 15, 2019 10:38

jamesopti added the 🤖 24 hrs label Aug 15, 2019

rashidsp added 3 commits August 16, 2019 10:12

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

9c019c7

Resolves unit test

2fd378f

removes log

efab9f4

mnoman09 approved these changes Aug 16, 2019

View reviewed changes

adds positive number conditions

3c4a980

rashidsp requested a review from mikeproeng37 August 17, 2019 12:20

mikeproeng37 approved these changes Aug 17, 2019

View reviewed changes

aliabbasrizvi suggested changes Aug 19, 2019

View reviewed changes

jamesopti removed the 🤖 24 hrs label Aug 19, 2019

jamesopti assigned rashidsp Aug 19, 2019

aliabbasrizvi approved these changes Aug 19, 2019

View reviewed changes

fixes visitor_attributes bug

6021aef

msohailhussain changed the title ~~feat(eventProcessor): Adds EventProcessor and BatchEventProcessor~~ feat(eventProcessor): AddsEventProcessor and BatchEventProcessor Aug 20, 2019

msohailhussain changed the title ~~feat(eventProcessor): AddsEventProcessor and BatchEventProcessor~~ feat(eventProcessor): Add EventProcessor and BatchEventProcessor Aug 20, 2019

Addressed review

184fd06

rashidsp requested review from aliabbasrizvi and removed request for aliabbasrizvi August 20, 2019 07:08

msohailhussain approved these changes Aug 22, 2019

View reviewed changes

mikeproeng37 merged commit c6a2bfe into master Aug 22, 2019

rashidsp deleted the rashid/batch-event-processor branch August 29, 2019 19:11

	@logger.log(Logger::DEBUG, "Adding user event: #{user_event.event['key']} to btach.")
	@logger.log(Logger::DEBUG, "Adding user event: #{user_event.event['key']} to batch.")

feat(eventProcessor): Add EventProcessor and BatchEventProcessor #191

feat(eventProcessor): Add EventProcessor and BatchEventProcessor #191

Uh oh!

Conversation

rashidsp commented Aug 1, 2019

Summary

Test Plan

Uh oh!

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

Uh oh!

Uh oh!

mnoman09 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!

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!

mikeproeng37 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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mnoman09 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Aug 1, 2019 •

edited

Loading

msohailhussain Aug 15, 2019 •

edited

Loading

mnoman09 left a comment •

edited

Loading