(fix): add more debug logging. remove extra current_batch set #228

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.

Sign up for GitHub

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

Jump to bottom

Merged

thomaszurkan-optimizely merged 7 commits into master from loggingBranch

Dec 16, 2019

Contributor

thomaszurkan-optimizely commented Dec 14, 2019 •

edited

Loading

Summary

Added more logging for why flush was initiated and flush size.
Remove reset of current_batch that was not wrapped in a lock and was necessary after a call to flush_batch.

Test plan

Augmented test for flush once to include debug message for flushing with batch size.


          add more debug logging

9350e74

thomaszurkan-optimizely requested a review from a team as a code owner

December 14, 2019 01:45

optimizely-codereview-bot assigned thomaszurkan-optimizely

thomaszurkan-optimizely added 2 commits

December 13, 2019 17:59


          take out in seconds. already in seconds

9c0cca8


          Revert "take out in seconds. already in seconds"

a2fe995

This reverts commit 9c0cca8.

Contributor

aliabbasrizvi commented Dec 15, 2019

Please add a better Summary / Test Plan. You have taken the template values right now.

aliabbasrizvi approved these changes

View reviewed changes

Contributor

aliabbasrizvi left a comment

Conditionally approved on addressing provided feedback.

optimizely/event/event_processor.py Outdated

@@ @@ -222,7 +222,7 @@ def flush(self): @@
                   def _flush_queue(self):
                       """ Flushes event_queue by dispatching events. """
+                      self.logger.debug('Flushing the queue.')

Contributor

aliabbasrizvi Dec 15, 2019

Actually it will be better to look up if events are there and say nothing to flush if no events and then say flushing the queue if there are events. Also, perhaps number of user events flushed will be useful.


          update logging to log when batch is empty on flush or flush of batch …

b776edc

…size

coveralls commented Dec 16, 2019

Coverage increased (+0.006%) to 97.605% when pulling b776edc on loggingBranch into b156e21 on master.

5 similar comments

coveralls commented Dec 16, 2019

Coverage increased (+0.006%) to 97.605% when pulling b776edc on loggingBranch into b156e21 on master.

coveralls commented Dec 16, 2019

Coverage increased (+0.006%) to 97.605% when pulling b776edc on loggingBranch into b156e21 on master.

coveralls commented Dec 16, 2019

Coverage increased (+0.006%) to 97.605% when pulling b776edc on loggingBranch into b156e21 on master.

coveralls commented Dec 16, 2019

Coverage increased (+0.006%) to 97.605% when pulling b776edc on loggingBranch into b156e21 on master.

coveralls commented Dec 16, 2019

Coverage increased (+0.006%) to 97.605% when pulling b776edc on loggingBranch into b156e21 on master.

coveralls commented Dec 16, 2019 •

edited

Loading

Coverage increased (+0.005%) to 97.604% when pulling 0c9adba on loggingBranch into b156e21 on master.


          use mock logger

027f277

aliabbasrizvi reviewed

View reviewed changes

optimizely/event/event_processor.py Outdated

@@ @@ -267,6 +270,7 @@ def _add_to_batch(self, user_event): @@
                     user_event: UserEvent Instance.
                   """
                       if self._should_split(user_event):
+                          self.logger.debug('Flush on split.')

Contributor

aliabbasrizvi Dec 16, 2019

nit. Flushing and not flush.


          rename flush_queue to flush_batch and update debug messages. fix one …

ee08eb8

…bug where current_batch was being reset without a lock.

thomaszurkan-optimizely changed the title ~~(fix): add more debug logging~~ (fix): add more debug logging. remove extra current_batch set


          cleanup incorrect comments

0c9adba

thomaszurkan-optimizely merged commit 62c00f3 into master

thomaszurkan-optimizely deleted the loggingBranch branch

December 16, 2019 17:36

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet