-
Notifications
You must be signed in to change notification settings - Fork 83
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.
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
[FSSDK-11003] disposable service implementation #981
Changes from 4 commits
060cbc3
11c850d
17d27a3
e516e39
08d0df4
bad40b4
f2c3b5a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,6 +30,9 @@ import { areEventContextsEqual } from "./event_builder/user_event"; | |
import { EVENT_PROCESSOR_STOPPED, FAILED_TO_DISPATCH_EVENTS, FAILED_TO_DISPATCH_EVENTS_WITH_ARG } from "../exception_messages"; | ||
import { sprintf } from "../utils/fns"; | ||
|
||
export const DEFAULT_MIN_BACKOFF = 1000; | ||
export const DEFAULT_MAX_BACKOFF = 32000; | ||
|
||
export type EventWithId = { | ||
id: string; | ||
event: ProcessableEvent; | ||
|
@@ -209,7 +212,8 @@ export class BatchEventProcessor extends BaseService implements EventProcessor { | |
if (!batch) { | ||
return; | ||
} | ||
|
||
|
||
this.dispatchRepeater.reset(); | ||
this.dispatchBatch(batch, closing); | ||
} | ||
|
||
|
@@ -218,10 +222,6 @@ export class BatchEventProcessor extends BaseService implements EventProcessor { | |
return Promise.reject('Event processor is not running'); | ||
} | ||
|
||
if (this.eventQueue.length == this.batchSize) { | ||
this.flush(); | ||
} | ||
|
||
const eventWithId = { | ||
id: this.idGenerator.getId(), | ||
event: event, | ||
|
@@ -232,13 +232,30 @@ export class BatchEventProcessor extends BaseService implements EventProcessor { | |
if (this.eventQueue.length > 0 && !areEventContextsEqual(this.eventQueue[0].event, event)) { | ||
this.flush(); | ||
} | ||
this.eventQueue.push(eventWithId); | ||
|
||
this.eventQueue.push(eventWithId); | ||
|
||
if (this.eventQueue.length == this.batchSize) { | ||
this.flush(); | ||
} else if (!this.dispatchRepeater.isRunning()) { | ||
this.dispatchRepeater.start(); | ||
} | ||
|
||
} | ||
|
||
start(): void { | ||
if (!this.isNew()) { | ||
return; | ||
} | ||
if(this.disposable) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we override the makeDisposable method and do this inside that |
||
this.batchSize = 1; | ||
this.retryConfig = { | ||
maxRetries: Math.min(this.retryConfig?.maxRetries ?? 5, 5), | ||
backoffProvider: | ||
this.retryConfig?.backoffProvider || | ||
(() => new ExponentialBackoff(DEFAULT_MIN_BACKOFF, DEFAULT_MAX_BACKOFF, 500)), | ||
}; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. on old line 245 below, we should check disposable before starting failedEventRepeater. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So we will not start the "failedEventRepeater" if event processor is disposable? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes |
||
super.start(); | ||
this.state = ServiceState.Running; | ||
this.dispatchRepeater.start(); | ||
|
@@ -254,7 +271,6 @@ export class BatchEventProcessor extends BaseService implements EventProcessor { | |
} | ||
|
||
if (this.isNew()) { | ||
// TOOD: replace message with imported constants | ||
this.startPromise.reject(new Error(EVENT_PROCESSOR_STOPPED)); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -207,6 +207,39 @@ describe('DefaultOdpEventManager', () => { | |
} | ||
}); | ||
|
||
it('should flush the queue immediately if disposable, regardless of the batchSize', async () => { | ||
const apiManager = getMockApiManager(); | ||
const repeater = getMockRepeater() | ||
apiManager.sendEvents.mockResolvedValue({ statusCode: 200 }); | ||
// spy on the flush method | ||
const odpEventManager = new DefaultOdpEventManager({ | ||
repeater, | ||
apiManager: apiManager, | ||
batchSize: 10, | ||
retryConfig: { | ||
maxRetries: 3, | ||
backoffProvider: vi.fn(), | ||
}, | ||
}); | ||
|
||
odpEventManager.updateConfig({ | ||
integrated: true, | ||
odpConfig: config, | ||
}); | ||
odpEventManager.makeDisposable(); | ||
odpEventManager.start(); | ||
|
||
await expect(odpEventManager.onRunning()).resolves.not.toThrow(); | ||
|
||
const event = makeEvent(0); | ||
odpEventManager.sendEvent(event); | ||
await exhaustMicrotasks(); | ||
|
||
expect(apiManager.sendEvents).toHaveBeenCalledTimes(1); | ||
expect(apiManager.sendEvents).toHaveBeenNthCalledWith(1, config, [event]); | ||
expect(repeater.reset).toHaveBeenCalledTimes(1); | ||
}) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we also add tests for repeater stop when disposable = true? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the actual implementation There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh I see we have assertions for repeater.reset here. We can ignore this comment There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we can add a test to assert the repeater is not started, similar to eventProcessor |
||
it('drops events and logs if the state is not running', async () => { | ||
const apiManager = getMockApiManager(); | ||
apiManager.sendEvents.mockResolvedValue({ statusCode: 200 }); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -105,6 +105,11 @@ export class DefaultOdpEventManager extends BaseService implements OdpEventManag | |
if (!this.isNew) { | ||
return; | ||
} | ||
// Override for disposable event manager | ||
if(this.disposable) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we ovverride makeDisposable() and do this inside that? |
||
this.retryConfig.maxRetries = Math.min(this.retryConfig.maxRetries, 5); | ||
this.batchSize = 1 | ||
} | ||
|
||
super.start(); | ||
if (this.odpIntegrationConfig) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,6 +51,7 @@ const getMockOdpEventManager = () => { | |
getState: vi.fn(), | ||
updateConfig: vi.fn(), | ||
sendEvent: vi.fn(), | ||
makeDisposable: vi.fn(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we add test that it makes the eventManager disposable when its makeDisposable() is called? |
||
}; | ||
}; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -90,6 +90,10 @@ export class DefaultOdpManager extends BaseService implements OdpManager { | |
if (!this.isNew()) { | ||
return; | ||
} | ||
|
||
if(this.disposable) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we ovverride the makeDisposable() method and do this inside that? |
||
this.eventManager.makeDisposable(); | ||
} | ||
|
||
this.state = ServiceState.Starting; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,13 +16,10 @@ | |
import { describe, it, expect, vi } from 'vitest'; | ||
import Optimizely from '.'; | ||
import { getMockProjectConfigManager } from '../tests/mock/mock_project_config_manager'; | ||
import * as logger from '../plugins/logger'; | ||
import * as jsonSchemaValidator from '../utils/json_schema_validator'; | ||
import { LOG_LEVEL } from '../common_exports'; | ||
import { createNotificationCenter } from '../notification_center'; | ||
import testData from '../tests/test_data'; | ||
import { getForwardingEventProcessor } from '../event_processor/forwarding_event_processor'; | ||
import { LoggerFacade } from '../modules/logging'; | ||
import { createProjectConfig } from '../project_config/project_config'; | ||
import { getMockLogger } from '../tests/mock/mock_logger'; | ||
|
||
|
@@ -39,12 +36,12 @@ describe('Optimizely', () => { | |
|
||
const notificationCenter = createNotificationCenter({ logger, errorHandler }); | ||
|
||
it('should pass ssr to the project config manager', () => { | ||
it('should pass disposable option to the project config manager', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it should pass the disposable option to eventProcessor and odpManager as well, can we modify this test to assert those as well? |
||
const projectConfigManager = getMockProjectConfigManager({ | ||
initConfig: createProjectConfig(testData.getTestProjectConfig()), | ||
}); | ||
|
||
vi.spyOn(projectConfigManager, 'setSsr'); | ||
vi.spyOn(projectConfigManager, 'makeDisposable'); | ||
|
||
const instance = new Optimizely({ | ||
clientEngine: 'node-sdk', | ||
|
@@ -54,16 +51,16 @@ describe('Optimizely', () => { | |
logger, | ||
notificationCenter, | ||
eventProcessor, | ||
isSsr: true, | ||
disposable: true, | ||
isValidInstance: true, | ||
}); | ||
|
||
expect(projectConfigManager.setSsr).toHaveBeenCalledWith(true); | ||
expect(projectConfigManager.makeDisposable).toHaveBeenCalled(); | ||
// eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
// @ts-ignore | ||
expect(instance.getProjectConfig()).toBe(projectConfigManager.config); | ||
// eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
// @ts-ignore | ||
expect(projectConfigManager.isSsr).toBe(true); | ||
expect(projectConfigManager.disposable).toBe(true); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -470,6 +470,25 @@ describe('PollingDatafileManager', () => { | |
expect(repeater.stop).toHaveBeenCalled(); | ||
}); | ||
|
||
it('stops repeater after successful initialization if disposable is true', async () => { | ||
const repeater = getMockRepeater(); | ||
const requestHandler = getMockRequestHandler(); | ||
const mockResponse = getMockAbortableRequest(Promise.resolve({ statusCode: 200, body: '{"foo": "bar"}', headers: {} })); | ||
requestHandler.makeRequest.mockReturnValueOnce(mockResponse); | ||
|
||
const manager = new PollingDatafileManager({ | ||
repeater, | ||
requestHandler, | ||
sdkKey: 'keyThatExists', | ||
}); | ||
manager.makeDisposable(); | ||
manager.start(); | ||
repeater.execute(0); | ||
|
||
await expect(manager.onRunning()).resolves.not.toThrow(); | ||
expect(repeater.stop).toHaveBeenCalled(); | ||
}); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we also add tests that stops retrying to initialize after 5 failed attempts if disposable is true? |
||
it('saves the datafile in cache', async () => { | ||
const repeater = getMockRepeater(); | ||
const requestHandler = getMockRequestHandler(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add tests for the following as well when disposable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding your number 2 - Current logic is -
dispatchRepeater
starts, dispatch immediately and then stops. Isn't this expected ? Or am I missing something here ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
according to the refactored logic in this PR, if batchSize == 1, dispatch repeater should never start. It will immediately dispatch the event, the repeater start is in the else branch