From cedb0074542def7db38d5a6d4cbd3b11074b4061 Mon Sep 17 00:00:00 2001 From: Matt Carroll Date: Fri, 9 Aug 2019 16:13:45 -0700 Subject: [PATCH 1/5] If batchSize = 0 or flushInterval = 0, log that values are invalid and pick up default values instead --- packages/event-processor/CHANGELOG.MD | 8 +++- .../__tests__/v1EventProcessor.spec.ts | 48 +++++++++++++++++++ .../event-processor/src/eventProcessor.ts | 21 ++++++-- 3 files changed, 72 insertions(+), 5 deletions(-) diff --git a/packages/event-processor/CHANGELOG.MD b/packages/event-processor/CHANGELOG.MD index b5344437e..fd28837cb 100644 --- a/packages/event-processor/CHANGELOG.MD +++ b/packages/event-processor/CHANGELOG.MD @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ## [Unreleased] Changes that have landed but are not yet released. +### New Features +- In `AbstractEventProcessor`, validate maxQueueSize and flushInterval; ignore & use default values when invalid + +### Changed +- Removed transformers, interceptors, and callbacks from `AbstractEventProcessor` + ## [0.2.1] - June 6, 2019 - Wrap the `callback` in `try/catch` when implementing a custom `eventDispatcher`. This ensures invoking the `callback` will always cleanup any pending retry tasks. @@ -18,4 +24,4 @@ events that did not send successfully due to page navigation ## [0.1.0] - March 1, 2019 -Initial release \ No newline at end of file +Initial release diff --git a/packages/event-processor/__tests__/v1EventProcessor.spec.ts b/packages/event-processor/__tests__/v1EventProcessor.spec.ts index 5484e15e4..03e1f4937 100644 --- a/packages/event-processor/__tests__/v1EventProcessor.spec.ts +++ b/packages/event-processor/__tests__/v1EventProcessor.spec.ts @@ -340,5 +340,53 @@ describe('LogTierV1EventProcessor', () => { // flushing should reset queue, at this point only has two events expect(dispatchStub).toHaveBeenCalledTimes(1) }) + + }) + + describe('invalid flushInterval or maxQueueSize', () => { + it('should ignore a flushInterval of 0 and use the default', () => { + const processor = new LogTierV1EventProcessor({ + dispatcher: stubDispatcher, + flushInterval: 0, + maxQueueSize: 10, + }) + processor.start() + + const impressionEvent1 = createImpressionEvent() + processor.process(impressionEvent1) + expect(dispatchStub).toHaveBeenCalledTimes(0) + jest.advanceTimersByTime(30000) + expect(dispatchStub).toHaveBeenCalledTimes(1) + expect(dispatchStub).toHaveBeenCalledWith({ + url: 'https://logx.optimizely.com/v1/events', + httpVerb: 'POST', + params: makeBatchedEventV1([impressionEvent1]), + }) + }) + + it('should ignore a maxQueueSize of 0 and use the default', () => { + const processor = new LogTierV1EventProcessor({ + dispatcher: stubDispatcher, + flushInterval: 30000, + maxQueueSize: 0, + }) + processor.start() + + const impressionEvent1 = createImpressionEvent() + processor.process(impressionEvent1) + expect(dispatchStub).toHaveBeenCalledTimes(0) + const impressionEvents = [impressionEvent1] + for (let i = 0; i < 9; i++) { + const evt = createImpressionEvent() + processor.process(evt) + impressionEvents.push(evt) + } + expect(dispatchStub).toHaveBeenCalledTimes(1) + expect(dispatchStub).toHaveBeenCalledWith({ + url: 'https://logx.optimizely.com/v1/events', + httpVerb: 'POST', + params: makeBatchedEventV1(impressionEvents), + }) + }) }) }) diff --git a/packages/event-processor/src/eventProcessor.ts b/packages/event-processor/src/eventProcessor.ts index 62f8b4597..c852778f2 100644 --- a/packages/event-processor/src/eventProcessor.ts +++ b/packages/event-processor/src/eventProcessor.ts @@ -33,15 +33,17 @@ export interface EventProcessor extends Managed { process(event: ProcessableEvents): void } -const MIN_FLUSH_INTERVAL = 100 +const DEFAULT_FLUSH_INTERVAL = 30000 +const DEFAULT_MAX_QUEUE_SIZE = 10 + export abstract class AbstractEventProcessor implements EventProcessor { protected dispatcher: EventDispatcher protected queue: EventQueue constructor({ dispatcher, - flushInterval = 30000, - maxQueueSize = 3000, + flushInterval = DEFAULT_FLUSH_INTERVAL, + maxQueueSize = DEFAULT_MAX_QUEUE_SIZE, }: { dispatcher: EventDispatcher flushInterval?: number @@ -49,10 +51,21 @@ export abstract class AbstractEventProcessor implements EventProcessor { }) { this.dispatcher = dispatcher + if (flushInterval <= 0) { + logger.warn(`Invalid flushInterval ${flushInterval}, defaulting to ${DEFAULT_FLUSH_INTERVAL}`) + flushInterval = DEFAULT_FLUSH_INTERVAL + } + + maxQueueSize = Math.floor(maxQueueSize) + if (maxQueueSize < 1) { + logger.warn(`Invalid maxQueueSize, defaulting to ${DEFAULT_MAX_QUEUE_SIZE}`) + maxQueueSize = DEFAULT_MAX_QUEUE_SIZE + } + maxQueueSize = Math.max(1, maxQueueSize) if (maxQueueSize > 1) { this.queue = new DefaultEventQueue({ - flushInterval: Math.max(flushInterval, MIN_FLUSH_INTERVAL), + flushInterval, maxQueueSize, sink: buffer => this.drainQueue(buffer), }) From 0d280a49c61a31855fc1ad324649b1e2825dbcbf Mon Sep 17 00:00:00 2001 From: Matt Carroll Date: Mon, 12 Aug 2019 08:34:34 -0700 Subject: [PATCH 2/5] Restore test --- .../__tests__/v1EventProcessor.spec.ts | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/packages/event-processor/__tests__/v1EventProcessor.spec.ts b/packages/event-processor/__tests__/v1EventProcessor.spec.ts index 2f30dc01a..c1373c27b 100644 --- a/packages/event-processor/__tests__/v1EventProcessor.spec.ts +++ b/packages/event-processor/__tests__/v1EventProcessor.spec.ts @@ -369,4 +369,51 @@ describe('LogTierV1EventProcessor', () => { expect(notificationCenter.sendNotifications).toBeCalledWith(NOTIFICATION_TYPES.LOG_EVENT, event) }) }) + + describe('invalid flushInterval or maxQueueSize', () => { + it('should ignore a flushInterval of 0 and use the default', () => { + const processor = new LogTierV1EventProcessor({ + dispatcher: stubDispatcher, + flushInterval: 0, + maxQueueSize: 10, + }) + processor.start() + + const impressionEvent1 = createImpressionEvent() + processor.process(impressionEvent1) + expect(dispatchStub).toHaveBeenCalledTimes(0) + jest.advanceTimersByTime(30000) + expect(dispatchStub).toHaveBeenCalledTimes(1) + expect(dispatchStub).toHaveBeenCalledWith({ + url: 'https://logx.optimizely.com/v1/events', + httpVerb: 'POST', + params: makeBatchedEventV1([impressionEvent1]), + }) + }) + + it('should ignore a maxQueueSize of 0 and use the default', () => { + const processor = new LogTierV1EventProcessor({ + dispatcher: stubDispatcher, + flushInterval: 30000, + maxQueueSize: 0, + }) + processor.start() + + const impressionEvent1 = createImpressionEvent() + processor.process(impressionEvent1) + expect(dispatchStub).toHaveBeenCalledTimes(0) + const impressionEvents = [impressionEvent1] + for (let i = 0; i < 9; i++) { + const evt = createImpressionEvent() + processor.process(evt) + impressionEvents.push(evt) + } + expect(dispatchStub).toHaveBeenCalledTimes(1) + expect(dispatchStub).toHaveBeenCalledWith({ + url: 'https://logx.optimizely.com/v1/events', + httpVerb: 'POST', + params: makeBatchedEventV1(impressionEvents), + }) + }) + }) }) From b53edf914ebdd3b476020c659d9cdacf9a23d137 Mon Sep 17 00:00:00 2001 From: Matt Carroll Date: Mon, 12 Aug 2019 08:36:46 -0700 Subject: [PATCH 3/5] Add missing changelog entry --- packages/event-processor/CHANGELOG.MD | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/event-processor/CHANGELOG.MD b/packages/event-processor/CHANGELOG.MD index fd28837cb..ce7ad0fec 100644 --- a/packages/event-processor/CHANGELOG.MD +++ b/packages/event-processor/CHANGELOG.MD @@ -9,6 +9,7 @@ Changes that have landed but are not yet released. ### New Features - In `AbstractEventProcessor`, validate maxQueueSize and flushInterval; ignore & use default values when invalid +- `AbstractEventProcessor` can be constructed with a `notificationCenter`. When `notificationCenter` is provided, it triggers a log event notification after the event is sent to the event dispatcher ### Changed - Removed transformers, interceptors, and callbacks from `AbstractEventProcessor` From 3de06decfb5480ba19c79fda19d0375fe7a64265 Mon Sep 17 00:00:00 2001 From: Matt Carroll Date: Mon, 12 Aug 2019 08:41:31 -0700 Subject: [PATCH 4/5] Review fixes --- packages/event-processor/src/eventProcessor.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/event-processor/src/eventProcessor.ts b/packages/event-processor/src/eventProcessor.ts index 742d45992..0979b6d90 100644 --- a/packages/event-processor/src/eventProcessor.ts +++ b/packages/event-processor/src/eventProcessor.ts @@ -34,7 +34,7 @@ export interface EventProcessor extends Managed { process(event: ProcessableEvents): void } -const DEFAULT_FLUSH_INTERVAL = 30000 +const DEFAULT_FLUSH_INTERVAL = 30000 // Unit is ms - default flush interval is 30s const DEFAULT_MAX_QUEUE_SIZE = 10 export abstract class AbstractEventProcessor implements EventProcessor { @@ -62,7 +62,7 @@ export abstract class AbstractEventProcessor implements EventProcessor { maxQueueSize = Math.floor(maxQueueSize) if (maxQueueSize < 1) { - logger.warn(`Invalid maxQueueSize, defaulting to ${DEFAULT_MAX_QUEUE_SIZE}`) + logger.warn(`Invalid maxQueueSize ${maxQueueSize}, defaulting to ${DEFAULT_MAX_QUEUE_SIZE}`) maxQueueSize = DEFAULT_MAX_QUEUE_SIZE } From cadc272b4989517f13a18a53e803deae091668bb Mon Sep 17 00:00:00 2001 From: Matt Carroll Date: Mon, 12 Aug 2019 08:44:19 -0700 Subject: [PATCH 5/5] Formatting fixes in eventProcessor --- packages/event-processor/src/eventProcessor.ts | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/packages/event-processor/src/eventProcessor.ts b/packages/event-processor/src/eventProcessor.ts index 0979b6d90..d42c0c1fa 100644 --- a/packages/event-processor/src/eventProcessor.ts +++ b/packages/event-processor/src/eventProcessor.ts @@ -16,10 +16,7 @@ // TODO change this to use Managed from js-sdk-models when available import { Managed } from './managed' import { ConversionEvent, ImpressionEvent } from './events' -import { - EventDispatcher, - EventV1Request, -} from './eventDispatcher' +import { EventDispatcher, EventV1Request } from './eventDispatcher' import { EventQueue, DefaultEventQueue, SingleEventQueue } from './eventQueue' import { getLogger } from '@optimizely/js-sdk-logging' import { NOTIFICATION_TYPES, NotificationCenter } from '@optimizely/js-sdk-utils' @@ -56,13 +53,17 @@ export abstract class AbstractEventProcessor implements EventProcessor { this.dispatcher = dispatcher if (flushInterval <= 0) { - logger.warn(`Invalid flushInterval ${flushInterval}, defaulting to ${DEFAULT_FLUSH_INTERVAL}`) + logger.warn( + `Invalid flushInterval ${flushInterval}, defaulting to ${DEFAULT_FLUSH_INTERVAL}`, + ) flushInterval = DEFAULT_FLUSH_INTERVAL } maxQueueSize = Math.floor(maxQueueSize) if (maxQueueSize < 1) { - logger.warn(`Invalid maxQueueSize ${maxQueueSize}, defaulting to ${DEFAULT_MAX_QUEUE_SIZE}`) + logger.warn( + `Invalid maxQueueSize ${maxQueueSize}, defaulting to ${DEFAULT_MAX_QUEUE_SIZE}`, + ) maxQueueSize = DEFAULT_MAX_QUEUE_SIZE } @@ -87,7 +88,7 @@ export abstract class AbstractEventProcessor implements EventProcessor { const promises = this.groupEvents(buffer).map(eventGroup => { const formattedEvent = this.formatEvents(eventGroup) - return new Promise((resolve) => { + return new Promise(resolve => { this.dispatcher.dispatchEvent(formattedEvent, () => { resolve() }) @@ -95,7 +96,7 @@ export abstract class AbstractEventProcessor implements EventProcessor { if (this.notificationCenter) { this.notificationCenter.sendNotifications( NOTIFICATION_TYPES.LOG_EVENT, - formattedEvent + formattedEvent, ) } })