Skip to content

feat(ats): [FSSDK-8690] fix configurable ODP request timeouts #807

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

Merged
merged 3 commits into from
Mar 24, 2023
Merged

feat(ats): [FSSDK-8690] fix configurable ODP request timeouts #807

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b9ba1c4
fix configurable odp request timeouts
jaeopt Mar 23, 2023
2b8119c
clean up
jaeopt Mar 23, 2023
5a2072c
Merge branch 'master' into jae/8690-config-timeout
opti-jnguyen Mar 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 20 additions & 7 deletions packages/optimizely-sdk/lib/plugins/odp_manager/index.browser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,14 @@
* limitations under the License.
*/

import { BROWSER_CLIENT_VERSION, ERROR_MESSAGES, JAVASCRIPT_CLIENT_ENGINE, ODP_USER_KEY } from '../../utils/enums';
import {
BROWSER_CLIENT_VERSION,
ERROR_MESSAGES,
JAVASCRIPT_CLIENT_ENGINE,
ODP_USER_KEY,
REQUEST_TIMEOUT_ODP_SEGMENTS_MS,
REQUEST_TIMEOUT_ODP_EVENTS_MS
} from '../../utils/enums';
import { getLogger, LoggerFacade, LogHandler, LogLevel } from '../../modules/logging';

import { BrowserRequestHandler } from './../../utils/http_request_handler/browser_request_handler';
Expand Down Expand Up @@ -49,16 +56,22 @@ export class BrowserOdpManager extends OdpManager {

if (odpOptions?.segmentsRequestHandler) {
customSegmentRequestHandler = odpOptions.segmentsRequestHandler;
} else if (odpOptions?.segmentsApiTimeout) {
customSegmentRequestHandler = new BrowserRequestHandler(browserLogger, odpOptions.segmentsApiTimeout);
} else {
customSegmentRequestHandler = new BrowserRequestHandler(
browserLogger,
odpOptions?.segmentsApiTimeout || REQUEST_TIMEOUT_ODP_SEGMENTS_MS
);
}

let customEventRequestHandler;

if (odpOptions?.eventRequestHandler) {
customEventRequestHandler = odpOptions.eventRequestHandler;
} else if (odpOptions?.eventApiTimeout) {
customEventRequestHandler = new BrowserRequestHandler(browserLogger, odpOptions.eventApiTimeout);
} else {
customEventRequestHandler = new BrowserRequestHandler(
browserLogger,
odpOptions?.eventApiTimeout || REQUEST_TIMEOUT_ODP_EVENTS_MS
);
}

super({
Expand All @@ -68,8 +81,8 @@ export class BrowserOdpManager extends OdpManager {
maxSize: odpOptions?.segmentsCacheSize,
timeout: odpOptions?.segmentsCacheTimeout,
}),
segmentRequestHandler: customSegmentRequestHandler || new BrowserRequestHandler(browserLogger),
eventRequestHandler: customEventRequestHandler || new BrowserRequestHandler(browserLogger),
segmentRequestHandler: customSegmentRequestHandler,
eventRequestHandler: customEventRequestHandler,
logger: browserLogger,
clientEngine: browserClientEngine,
clientVersion: browserClientVersion,
Expand Down
2 changes: 2 additions & 0 deletions packages/optimizely-sdk/lib/utils/enums/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,8 @@ export enum NOTIFICATION_TYPES {
* Default milliseconds before request timeout
*/
export const REQUEST_TIMEOUT_MS = 60 * 1000; // 1 minute
export const REQUEST_TIMEOUT_ODP_SEGMENTS_MS = 10 * 1000; // 10 secs
export const REQUEST_TIMEOUT_ODP_EVENTS_MS = 10 * 1000; // 10 secs

/**
* ODP User Key Options
Expand Down
19 changes: 19 additions & 0 deletions packages/optimizely-sdk/tests/odpManager.browser.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,13 @@ describe('OdpManager', () => {
expect(browserOdpManager.segmentManager.odpSegmentApiManager.requestHandler.timeout).toBe(4000);
});

it('Browser default Segments API Request Handler timeout should be used when odpOptions does not include segmentsApiTimeout', () => {
const browserOdpManager = new BrowserOdpManager({});

// @ts-ignore
expect(browserOdpManager.segmentManager.odpSegmentApiManager.requestHandler.timeout).toBe(10000);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use REQUEST_TIMEOUT_ODP_SEGMENTS_MS here? same for the other test.

Copy link
Contributor Author

@jaeopt jaeopt Mar 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that comparing against the variable will improve test reliability when adjusting the timeout. In this case, I intentionally check against the number to protect from unintended constant changes :)

});

it('Custom odpOptions.segmentsRequestHandler overrides default Segment API Request Handler', () => {
const odpOptions: OdpOptions = {
segmentsRequestHandler: new BrowserRequestHandler(fakeLogger, 4000),
Expand Down Expand Up @@ -431,6 +438,18 @@ describe('OdpManager', () => {
expect(browserOdpManager.eventManager.apiManager.requestHandler.timeout).toBe(4000);
});

it('Browser default Events API Request Handler timeout should be used when odpOptions does not include eventsApiTimeout', () => {
const odpOptions: OdpOptions = {
};

const browserOdpManager = new BrowserOdpManager({
odpOptions,
});

// @ts-ignore
expect(browserOdpManager.eventManager.apiManager.requestHandler.timeout).toBe(10000);
});

it('Custom odpOptions.eventFlushInterval overrides default Event Manager flush interval', () => {
const odpOptions: OdpOptions = {
eventFlushInterval: 4000,
Expand Down