Skip to content

Commit 46c2720

Browse files
mikechu-optimizelymikechu-optimizely
committed
Code review changes
1 parent f82fbff commit 46c2720

File tree

3 files changed

+61
-74
lines changed

3 files changed

+61
-74
lines changed

‎packages/optimizely-sdk/lib/plugins/odp/odp_event_dispatcher.ts‎

Lines changed: 24 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -158,10 +158,11 @@ export class OdpEventDispatcher implements IOdpEventDispatcher {
158158
}
159159

160160
/**
161-
* Process any events in the main queue in batches
161+
* Process events in the main queue
162+
* @param shouldFlush Flush all events regardless of available queue event count
162163
* @private
163164
*/
164-
private async processQueue(): Promise<void> {
165+
private async processQueue(shouldFlush = false): Promise<void> {
165166
if (this.state !== STATE.RUNNING) {
166167
return;
167168
}
@@ -170,51 +171,35 @@ export class OdpEventDispatcher implements IOdpEventDispatcher {
170171
return;
171172
}
172173

173-
if (!this.queueHasBatches()) {
174-
return;
175-
}
174+
// Flush interval occurred & queue has items
175+
if (shouldFlush && this.queueContainsItems()) {
176+
// clear the queue completely
177+
this.clearCurrentTimeout();
176178

177-
this.clearCurrentTimeout();
179+
this.state = STATE.PROCESSING;
178180

179-
this.state = STATE.PROCESSING;
180-
181-
while (this.queueHasBatches()) {
182-
await this.makeAndSendBatch();
183-
}
181+
while (this.queueContainsItems()) {
182+
await this.makeAndSendBatch();
183+
}
184184

185-
this.state = STATE.RUNNING;
185+
this.state = STATE.RUNNING;
186186

187-
this.setNewTimeout();
188-
}
189-
190-
/**
191-
* Process all events in the main queue in batches until empty
192-
* @private
193-
*/
194-
private async flushQueue(): Promise<void> {
195-
if (this.state !== STATE.RUNNING) {
196-
return;
197-
}
198-
199-
if (!this.isOdpConfigurationReady()) {
200-
return;
187+
this.setNewTimeout();
201188
}
189+
// Check if queue has a full batch available
190+
else if (this.queueHasBatches()) {
191+
this.clearCurrentTimeout();
202192

203-
if (!this.queueContainsItems()) {
204-
return;
205-
}
193+
this.state = STATE.PROCESSING;
206194

207-
this.clearCurrentTimeout();
195+
while (this.queueHasBatches()) {
196+
await this.makeAndSendBatch();
197+
}
208198

209-
this.state = STATE.PROCESSING;
199+
this.state = STATE.RUNNING;
210200

211-
while (this.queueContainsItems()) {
212-
await this.makeAndSendBatch();
201+
this.setNewTimeout();
213202
}
214-
215-
this.state = STATE.RUNNING;
216-
217-
this.setNewTimeout();
218203
}
219204

220205
/**
@@ -234,7 +219,7 @@ export class OdpEventDispatcher implements IOdpEventDispatcher {
234219
if (this.timeoutId !== undefined) {
235220
return;
236221
}
237-
this.timeoutId = setTimeout(() => this.flushQueue(), this.flushInterval);
222+
this.timeoutId = setTimeout(() => this.processQueue(true), this.flushInterval);
238223
}
239224

240225
/**
@@ -306,7 +291,7 @@ export class OdpEventDispatcher implements IOdpEventDispatcher {
306291
public async stop(): Promise<void> {
307292
this.logger.log(LogLevel.DEBUG, 'EventDispatcher stop requested.');
308293

309-
await this.flushQueue();
294+
await this.processQueue(true);
310295

311296
this.state = STATE.STOPPED;
312297
this.logger.log(LogLevel.DEBUG, `EventDispatcher stopped. Queue Count: ${this.queue.length}`);

‎packages/optimizely-sdk/lib/plugins/odp/odp_event_manager.ts‎

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@ import { uuid } from '../../utils/fns';
2020
import { ODP_USER_KEY } from '../../utils/enums';
2121
import { OdpConfig } from './odp_config';
2222
import { OdpEventDispatcher } from './odp_event_dispatcher';
23-
import { OptimizelyOptions } from '../../shared_types';
2423

2524
/**
2625
* Manager for persisting events to the Optimizely Data Platform (ODP)
@@ -45,12 +44,19 @@ export interface IOdpEventManager {
4544
export class OdpEventManager implements IOdpEventManager {
4645
private readonly eventDispatcher: OdpEventDispatcher;
4746
private readonly logger: LogHandler;
48-
private readonly config?: OptimizelyOptions;
49-
50-
public constructor(eventDispatcher: OdpEventDispatcher, logger: LogHandler, config?: OptimizelyOptions) {
47+
private readonly clientEngine?: string;
48+
private readonly clientVersion?: string;
49+
50+
public constructor({ eventDispatcher, logger, clientEngine, clientVersion }: {
51+
eventDispatcher: OdpEventDispatcher,
52+
logger: LogHandler,
53+
clientEngine?: string,
54+
clientVersion?: string,
55+
}) {
5156
this.eventDispatcher = eventDispatcher;
5257
this.logger = logger;
53-
this.config = config;
58+
this.clientEngine = clientEngine;
59+
this.clientVersion = clientVersion;
5460
}
5561

5662
/**
@@ -139,11 +145,11 @@ export class OdpEventManager implements IOdpEventManager {
139145
data.set('idempotence_id', uuid());
140146
data.set('data_source_type', 'sdk');
141147

142-
if (this.config?.clientEngine) {
143-
data.set('data_source', this.config?.clientEngine);
148+
if (this.clientEngine) {
149+
data.set('data_source', this.clientEngine);
144150
}
145-
if (this.config?.clientVersion) {
146-
data.set('data_source_version', this.config?.clientVersion);
151+
if (this.clientVersion) {
152+
data.set('data_source_version', this.clientVersion);
147153
}
148154

149155
sourceData.forEach((value, key) => data.set(key, value));

‎packages/optimizely-sdk/tests/odpEventManager.spec.ts‎

Lines changed: 22 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -22,11 +22,9 @@ import { LogHandler, LogLevel } from '../lib/modules/logging';
2222
import { OdpEvent } from '../lib/plugins/odp/odp_event';
2323
import { RequestHandler } from '../lib/utils/http_request_handler/http';
2424
import { OdpEventDispatcher, STATE } from '../lib/plugins/odp/odp_event_dispatcher';
25-
import { OptimizelyOptions } from '../lib/shared_types';
2625

2726
const API_KEY = 'test-api-key';
2827
const API_HOST = 'https://odp.example.com';
29-
const MOCK_PROCESS_VERSION = 'v16.17.0';
3028
const MOCK_IDEMPOTENCE_ID = 'c1dc758e-f095-4f09-9b49-172d74c53880';
3129
const EVENTS: OdpEvent[] = [
3230
new OdpEvent(
@@ -50,6 +48,9 @@ const EVENTS: OdpEvent[] = [
5048
})),
5149
),
5250
];
51+
// naming for object destructuring
52+
const clientEngine = 'javascript-sdk';
53+
const clientVersion = '4.9.2';
5354
const PROCESSED_EVENTS: OdpEvent[] = [
5455
new OdpEvent(
5556
't1',
@@ -58,8 +59,8 @@ const PROCESSED_EVENTS: OdpEvent[] = [
5859
new Map(Object.entries({
5960
'idempotence_id': MOCK_IDEMPOTENCE_ID,
6061
'data_source_type': 'sdk',
61-
'data_source': 'javascript-sdk',
62-
'data_source_version': MOCK_PROCESS_VERSION,
62+
'data_source': clientEngine,
63+
'data_source_version': clientVersion,
6364
'key-1': 'value1',
6465
'key-2': null,
6566
'key-3': 3.3,
@@ -73,8 +74,8 @@ const PROCESSED_EVENTS: OdpEvent[] = [
7374
new Map(Object.entries({
7475
'idempotence_id': MOCK_IDEMPOTENCE_ID,
7576
'data_source_type': 'sdk',
76-
'data_source': 'javascript-sdk',
77-
'data_source_version': MOCK_PROCESS_VERSION,
77+
'data_source': clientEngine,
78+
'data_source_version': clientVersion,
7879
'key-2': 'value2',
7980
})),
8081
),
@@ -109,28 +110,22 @@ describe('OdpEventManager', () => {
109110
let mockLogger: LogHandler;
110111
let mockApiManager: RestApiManager;
111112
let odpConfig: OdpConfig;
112-
let mockOptimizelyOptions: OptimizelyOptions;
113113

114114
beforeAll(() => {
115115
mockLogger = mock<LogHandler>();
116116
mockApiManager = mock<RestApiManager>();
117117
odpConfig = new OdpConfig(API_KEY, API_HOST, []);
118-
119-
mockOptimizelyOptions = mock<OptimizelyOptions>();
120-
when(mockOptimizelyOptions.clientEngine).thenReturn('javascript-sdk');
121-
when(mockOptimizelyOptions.clientVersion).thenReturn('4.9.2');
122118
});
123119

124120
beforeEach(() => {
125121
resetCalls(mockLogger);
126122
resetCalls(mockApiManager);
127-
resetCalls(mockOptimizelyOptions);
128123
});
129124

130125
it('should log and discard events when event manager not running', () => {
131126
const logger = instance(mockLogger);
132127
const eventDispatcher = new OdpEventDispatcher({ odpConfig, apiManager: instance(mockApiManager), logger });
133-
const eventManager = new OdpEventManager(eventDispatcher, logger);
128+
const eventManager = new OdpEventManager({ eventDispatcher, logger });
134129
// since we've not called start() then...
135130

136131
eventManager.sendEvent(EVENTS[0]);
@@ -149,7 +144,7 @@ describe('OdpEventManager', () => {
149144
logger,
150145
});
151146
eventDispatcher['state'] = STATE.RUNNING; // simulate dispatcher already in running state
152-
const eventManager = new OdpEventManager(eventDispatcher, logger);
147+
const eventManager = new OdpEventManager({ eventDispatcher, logger });
153148

154149
eventManager.sendEvent(EVENTS[0]);
155150

@@ -159,7 +154,7 @@ describe('OdpEventManager', () => {
159154
it('should discard events with invalid data', () => {
160155
const logger = instance(mockLogger);
161156
const eventDispatcher = new OdpEventDispatcher({ odpConfig, apiManager: instance(mockApiManager), logger });
162-
const eventManager = new OdpEventManager(eventDispatcher, logger);
157+
const eventManager = new OdpEventManager({ eventDispatcher, logger });
163158
// make an event with invalid data key-value entry
164159
const badEvent = new OdpEvent(
165160
't3',
@@ -189,7 +184,7 @@ describe('OdpEventManager', () => {
189184
});
190185
eventDispatcher['state'] = STATE.RUNNING; // simulate dispatcher running
191186
eventDispatcher['queue'].push(EVENTS[0]); // simulate event already in queue
192-
const eventManager = new OdpEventManager(eventDispatcher, logger);
187+
const eventManager = new OdpEventManager({ eventDispatcher, logger });
193188

194189
// try adding the second event
195190
eventManager.sendEvent(EVENTS[1]);
@@ -200,15 +195,15 @@ describe('OdpEventManager', () => {
200195
it('should add additional information to each event', () => {
201196
const logger = instance(mockLogger);
202197
const eventDispatcher = new OdpEventDispatcher({ odpConfig, apiManager: instance(mockApiManager), logger });
203-
const eventManager = new OdpEventManager(eventDispatcher, logger, instance(mockOptimizelyOptions));
198+
const eventManager = new OdpEventManager({ eventDispatcher, logger, clientEngine, clientVersion });
204199
const processedEventData = PROCESSED_EVENTS[0].data;
205200

206201
const eventData = eventManager['augmentCommonData'](EVENTS[0].data);
207202

208203
expect((eventData.get('idempotence_id') as string).length).toEqual((processedEventData.get('idempotence_id') as string).length);
209204
expect(eventData.get('data_source_type')).toEqual(processedEventData.get('data_source_type'));
210205
expect(eventData.get('data_source')).toEqual(processedEventData.get('data_source'));
211-
expect(eventData.get('data_source_version')).not.toBeNull();
206+
expect(eventData.get('data_source_version')).toEqual(processedEventData.get('data_source_version'));
212207
expect(eventData.get('key-1')).toEqual(processedEventData.get('key-1'));
213208
expect(eventData.get('key-2')).toEqual(processedEventData.get('key-2'));
214209
expect(eventData.get('key-3')).toEqual(processedEventData.get('key-3'));
@@ -225,7 +220,7 @@ describe('OdpEventManager', () => {
225220
batchSize: 10, // with batch size of 10...
226221
flushInterval: 250,
227222
});
228-
const eventManager = new OdpEventManager(eventDispatcher, logger);
223+
const eventManager = new OdpEventManager({ eventDispatcher, logger });
229224

230225
eventManager.start();
231226
for (let i = 0; i < 25; i += 1) {
@@ -247,7 +242,7 @@ describe('OdpEventManager', () => {
247242
batchSize: 10,
248243
flushInterval: 100,
249244
});
250-
const eventManager = new OdpEventManager(eventDispatcher, logger, instance(mockOptimizelyOptions));
245+
const eventManager = new OdpEventManager({ eventDispatcher, logger, clientEngine, clientVersion });
251246

252247
eventManager.start();
253248
EVENTS.forEach(event => eventManager.sendEvent(event));
@@ -276,7 +271,7 @@ describe('OdpEventManager', () => {
276271
batchSize: 2, // batch size of 2
277272
flushInterval: 100,
278273
});
279-
const eventManager = new OdpEventManager(eventDispatcher, logger);
274+
const eventManager = new OdpEventManager({ eventDispatcher, logger });
280275

281276
eventManager.start();
282277
// send 4 events
@@ -299,7 +294,7 @@ describe('OdpEventManager', () => {
299294
batchSize: 2, // batches of 2 with...
300295
flushInterval: 100,
301296
});
302-
const eventManager = new OdpEventManager(eventDispatcher, logger);
297+
const eventManager = new OdpEventManager({ eventDispatcher, logger });
303298

304299
eventManager.start();
305300
// ...25 events should...
@@ -327,7 +322,7 @@ describe('OdpEventManager', () => {
327322
batchSize: 10,
328323
flushInterval: 100,
329324
});
330-
const eventManager = new OdpEventManager(eventDispatcher, logger, instance(mockOptimizelyOptions));
325+
const eventManager = new OdpEventManager({ eventDispatcher, logger, clientEngine, clientVersion });
331326
const vuid = 'vuid_330e05cad15746d9af8a75b8d10';
332327

333328
eventManager.start();
@@ -361,7 +356,7 @@ describe('OdpEventManager', () => {
361356
logger,
362357
flushInterval: 100,
363358
});
364-
const eventManager = new OdpEventManager(eventDispatcher, logger, instance(mockOptimizelyOptions));
359+
const eventManager = new OdpEventManager({ eventDispatcher, logger, clientEngine, clientVersion });
365360
const vuid = 'vuid_330e05cad15746d9af8a75b8d10';
366361
const fsUserId = 'test-fs-user-id';
367362

@@ -386,12 +381,13 @@ describe('OdpEventManager', () => {
386381
});
387382

388383
it('should apply updated ODP configuration when available', () => {
384+
const logger = instance(mockLogger);
389385
const eventDispatcher = new OdpEventDispatcher({
390386
odpConfig,
391387
apiManager: instance(mockApiManager),
392-
logger: instance(mockLogger),
388+
logger,
393389
});
394-
const eventManager = new OdpEventManager(eventDispatcher, mockLogger);
390+
const eventManager = new OdpEventManager({ eventDispatcher, logger });
395391
const apiKey = 'testing-api-key';
396392
const apiHost = 'https://some.other.example.com';
397393
const segmentsToCheck = ['empty-cart', '1-item-cart'];

0 commit comments

Comments
 (0)