Skip to content
This repository has been archived by the owner on May 3, 2024. It is now read-only.

feat(createRequestHtmlFragment): implemented circuit breaker #111

Merged
merged 9 commits into from
May 1, 2020
2 changes: 1 addition & 1 deletion __tests__/integration/one-app.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ describe('Tests that require Docker setup', () => {
expect(rawHeaders).not.toHaveProperty('access-control-max-age');
expect(rawHeaders).not.toHaveProperty('access-control-allow-methods');
expect(rawHeaders).not.toHaveProperty('access-control-allow-headers');
// any respnse headers
// any response headers
expect(rawHeaders).not.toHaveProperty('access-control-allow-origin');
expect(rawHeaders).not.toHaveProperty('access-control-expose-headers');
expect(rawHeaders).not.toHaveProperty('access-control-allow-credentials');
Expand Down
100 changes: 99 additions & 1 deletion __tests__/server/middleware/createRequestHtmlFragment.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,24 +14,35 @@
* permissions and limitations under the License.
*/

/* global BigInt */

import url from 'url';
import { browserHistory } from '@americanexpress/one-app-router';
import { Map as iMap, fromJS } from 'immutable';
import { composeModules } from 'holocron';
import match from '../../../src/universal/utils/matchPromisified';
import breaker from '../../../src/server/utils/circuitBreaker';

import * as reactRendering from '../../../src/server/utils/reactRendering';

jest.mock('../../../src/universal/utils/matchPromisified');

jest.mock('holocron', () => ({
composeModules: jest.fn(() => 'composeModules'),
getModule: () => () => 0,
}));

const sleep = (ms) => new Promise((res) => setTimeout(res, ms));
const renderForStringSpy = jest.spyOn(reactRendering, 'renderForString');
const renderForStaticMarkupSpy = jest.spyOn(reactRendering, 'renderForStaticMarkup');
const msToNs = (n) => n * 1e6;

describe('createRequestHtmlFragment', () => {
jest.spyOn(console, 'error').mockImplementation(() => {});
const realHrtime = process.hrtime;
const mockHrtime = (...args) => realHrtime(...args);
mockHrtime.bigint = jest.fn();
process.hrtime = mockHrtime;

Choose a reason for hiding this comment

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

I think this should not really required here, e.g. the breaker should be automatically disabled in tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just needed for the tests that are asserting on the functionality of the breaker


let req;
let res;
Expand Down Expand Up @@ -59,6 +70,8 @@ describe('createRequestHtmlFragment', () => {
});

beforeEach(() => {
jest.clearAllMocks();
breaker.close();
req = jest.fn();
req.headers = {};

Expand All @@ -78,6 +91,10 @@ describe('createRequestHtmlFragment', () => {
renderForStaticMarkupSpy.mockClear();
});

afterAll(() => {
process.hrtime = realHrtime;
});

it('returns a function', () => {
const createRequestHtmlFragment = require(
'../../../src/server/middleware/createRequestHtmlFragment'
Expand All @@ -93,7 +110,6 @@ describe('createRequestHtmlFragment', () => {
const middleware = createRequestHtmlFragment({ createRoutes });
return middleware(req, res, next)
.then(() => {
const { composeModules } = require('holocron');
expect(composeModules).toHaveBeenCalled();
expect(composeModules.mock.calls[0][0]).toMatchSnapshot();
expect(dispatch).toHaveBeenCalledWith('composeModules');
Expand Down Expand Up @@ -229,4 +245,86 @@ describe('createRequestHtmlFragment', () => {
expect(next).toHaveBeenCalled();
/* eslint-enable no-console */
});

it('should open the circuit when event loop lag is > 30ms', async () => {
expect.assertions(5);
Copy link
Contributor

Choose a reason for hiding this comment

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

is this required with async test ?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes

Copy link
Contributor

Choose a reason for hiding this comment

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

checked locally, expect.assertions(5); can be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

expect.assertions ensures we get passed the promise to all the assertions

const createRequestHtmlFragment = require(
'../../../src/server/middleware/createRequestHtmlFragment'
).default;
const middleware = createRequestHtmlFragment({ createRoutes });
process.hrtime.bigint
.mockReturnValueOnce(BigInt(msToNs(100)))
.mockReturnValueOnce(BigInt(msToNs(131)));
await sleep(110);
await middleware(req, res, next);
expect(next).toHaveBeenCalled();
expect(composeModules).not.toHaveBeenCalled();
expect(renderForStringSpy).not.toHaveBeenCalled();
expect(renderForStaticMarkupSpy).not.toHaveBeenCalled();
expect(req.appHtml).toBe('');
});

it('should not open the circuit when event loop lag is < 30ms', async () => {
expect.assertions(5);
const createRequestHtmlFragment = require(
'../../../src/server/middleware/createRequestHtmlFragment'
).default;
const middleware = createRequestHtmlFragment({ createRoutes });
process.hrtime.bigint
.mockReturnValueOnce(BigInt(msToNs(100)))
.mockReturnValueOnce(BigInt(msToNs(120)));
await sleep(110);
await middleware(req, res, next);
expect(next).toHaveBeenCalled();
expect(composeModules).toHaveBeenCalled();
expect(renderForStringSpy).toHaveBeenCalled();
expect(renderForStaticMarkupSpy).not.toHaveBeenCalled();
expect(req.appHtml).toBe('hi');
});

it('should not open the circuit for partials', async () => {
expect.assertions(5);
const createRequestHtmlFragment = require(
'../../../src/server/middleware/createRequestHtmlFragment'
).default;
const middleware = createRequestHtmlFragment({ createRoutes });
getState.mockImplementationOnce(() => fromJS({
rendering: {
renderPartialOnly: true,
},
}));
process.hrtime.bigint
.mockReturnValueOnce(BigInt(msToNs(100)))
.mockReturnValueOnce(BigInt(msToNs(131)));
await sleep(110);
await middleware(req, res, next);
expect(next).toHaveBeenCalled();
expect(composeModules).toHaveBeenCalled();
expect(renderForStringSpy).not.toHaveBeenCalled();
expect(renderForStaticMarkupSpy).toHaveBeenCalled();
expect(req.appHtml).toBe('hi');
});

it('should not open the when scripts are disabled', async () => {
expect.assertions(5);
const createRequestHtmlFragment = require(
'../../../src/server/middleware/createRequestHtmlFragment'
).default;
const middleware = createRequestHtmlFragment({ createRoutes });
getState.mockImplementationOnce(() => fromJS({
rendering: {
disableScripts: true,
},
}));
process.hrtime.bigint
.mockReturnValueOnce(BigInt(msToNs(100)))
.mockReturnValueOnce(BigInt(msToNs(131)));
await sleep(110);
await middleware(req, res, next);
expect(next).toHaveBeenCalled();
expect(composeModules).toHaveBeenCalled();
expect(renderForStringSpy).not.toHaveBeenCalled();
expect(renderForStaticMarkupSpy).toHaveBeenCalled();
expect(req.appHtml).toBe('hi');
});
});
52 changes: 52 additions & 0 deletions __tests__/server/utils/circuitBreaker.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
/*
* Copyright 2020 American Express Travel Related Services Company, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
* or implied. See the License for the specific language governing
* permissions and limitations under the License.
*/

import CircuitBreaker from 'opossum';
import breaker, {
setEventLoopLagThreshold,
getEventLoopLagThreshold,
} from '../../../src/server/utils/circuitBreaker';

describe('Circuit breaker', () => {
it('should be an opossum circuit breaker', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

does this matter if everything else works ?

Copy link
Member Author

Choose a reason for hiding this comment

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

not necessarily

expect(breaker).toBeInstanceOf(CircuitBreaker);
});

// Tests for circuit breaker functionality can be found in
// __tests__/server/middleware/createRequestHtmlFragment.spec.js

describe('setEventLoopLagThreshold', () => {
it('should set a default value of 30ms', () => {
setEventLoopLagThreshold();
expect(getEventLoopLagThreshold()).toBe(30);
});

it('should set value to 30ms if input is not a number', () => {
setEventLoopLagThreshold('hello, world');
expect(getEventLoopLagThreshold()).toBe(30);
});

it('should set the given value', () => {
setEventLoopLagThreshold(44);
expect(getEventLoopLagThreshold()).toBe(44);
});

it('should handle numbers as strings', () => {
setEventLoopLagThreshold('55');
expect(getEventLoopLagThreshold()).toBe(55);
});
});
});
19 changes: 18 additions & 1 deletion __tests__/server/utils/onModuleLoad.spec.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,12 @@ import { setCorsOrigins } from '../../../src/server/middleware/conditionallyAllo
import { extendRestrictedAttributesAllowList, validateSafeRequestRestrictedAttributes } from '../../../src/server/utils/safeRequest';
import { setConfigureRequestLog } from '../../../src/server/utils/logging/serverMiddleware';
import { setCreateSsrFetch } from '../../../src/server/utils/createSsrFetch';
import { getEventLoopLagThreshold } from '../../../src/server/utils/circuitBreaker';

jest.mock('../../../src/server/utils/stateConfig', () => ({
setStateConfig: jest.fn(),
getClientStateConfig: jest.fn(),
getServerStateConfig: jest.fn(),
getServerStateConfig: jest.fn(() => ({ rootModuleName: 'root-module' })),
}));
jest.mock('@americanexpress/env-config-utils');
jest.mock('../../../src/server/utils/readJsonFile', () => () => ({ buildVersion: '4.43.0-0-38f0178d' }));
Expand Down Expand Up @@ -311,6 +312,22 @@ describe('onModuleLoad', () => {
expect(setCorsOrigins).toHaveBeenCalledWith(corsOrigins);
});

it('sets the event loop lag threshold from the root module', () => {
const eventLoopLagThreshold = 50;
expect(getEventLoopLagThreshold()).not.toBe(eventLoopLagThreshold);
onModuleLoad({
module: {
[CONFIGURATION_KEY]: {
csp,
eventLoopLagThreshold,
},
[META_DATA_KEY]: { version: '1.0.14' },
},
moduleName: 'some-root',
});
expect(getEventLoopLagThreshold()).toBe(eventLoopLagThreshold);
});

it('logs when the root module is loaded', () => {
onModuleLoad({
module: { [CONFIGURATION_KEY]: { csp }, [META_DATA_KEY]: { version: '1.0.15' } },
Expand Down
20 changes: 20 additions & 0 deletions docs/api/modules/App-Configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,26 @@ if (!global.BROWSER) {
}
```

## `eventLoopLagThreshold`
**Module Type**
* ✅ Root Module
* 🚫 Child Module

**Shape**
```js
if (!global.BROWSER) {
Module.appConfig = {
eventLoopLagThreshold: Number,
};
}
```

The `eventLoopLagThreshold` directive accepts a number representing the threshold of the event loop lag (in milliseconds) before opening the circuit in our circuit breaker created with [Opossum](https://nodeshift.dev/opossum/). Once the circuit is open, it will remain open for 10 seconds and close at that time pending the event loop lag. The default value is `30`. If you desire to disable the circuit completely, set this value to `Infinity`.

mtomcal marked this conversation as resolved.
Show resolved Hide resolved
**📘 More Information**
* [Frank Lloyd Root's `appConfig`](../../../prod-sample/sample-modules/frank-lloyd-root/0.0.0/src/config.js)
* Library: [Opossum](https://nodeshift.dev/opossum/)

`createSsrFetch` allows for customizing the fetch client used in `one-app` to perform server-side requests.

For example, you may wish to forward cookies or headers from the initial page load request to all the requisite SSR API requests.
Expand Down
23 changes: 23 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,8 @@
"ip": "^1.1.5",
"lean-intl": "^4.2.2",
"matcher": "^2.1.0",
"opossum": "^5.0.0",
"opossum-prometheus": "^0.1.0",
"pidusage": "^2.0.17",
"prom-client": "^11.5.3",
"prometheus-gc-stats": "^0.6.2",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import csp from './csp';
import createFrankLikeFetch from './createFrankLikeFetch';

export default {
eventLoopLagThreshold: Infinity,
Copy link
Contributor

Choose a reason for hiding this comment

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

do these not work with the default ?

Copy link
Member Author

Choose a reason for hiding this comment

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

This prevents the circuit from opening during the integration tests

csp,
corsOrigins: [/\.example.com$/],
configureRequestLog: ({ req, log = {} }) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ FrankLloydRoot.propTypes = {

if (!global.BROWSER) {
FrankLloydRoot.appConfig = {
eventLoopLagThreshold: Infinity,
// eslint-disable-next-line global-require
csp: require('../csp').default,
provideStateConfig: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import csp from './csp';

export default {
eventLoopLagThreshold: Infinity,
csp,
extendSafeRequestRestrictedAttributes: {
cookies: [
Expand Down
16 changes: 14 additions & 2 deletions src/server/middleware/createRequestHtmlFragment.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import url, { Url } from 'url';
import { RouterContext } from '@americanexpress/one-app-router';
import { composeModules } from 'holocron';
import match from '../../universal/utils/matchPromisified';
import breaker from '../utils/circuitBreaker';

import { renderForString, renderForStaticMarkup } from '../utils/reactRendering';

Expand Down Expand Up @@ -69,11 +70,22 @@ export default function createRequestHtmlFragment({ createRoutes }) {
},
}));

await dispatch(composeModules(routeModules));

const state = store.getState();
const disableScripts = state.getIn(['rendering', 'disableScripts']);
const renderPartialOnly = state.getIn(['rendering', 'renderPartialOnly']);

if (disableScripts || renderPartialOnly) {
await dispatch(composeModules(routeModules));
} else {
const fallback = await breaker.fire({ dispatch, modules: routeModules });
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe rename breaker to show its related to getModuleData

Suggested change
const fallback = await breaker.fire({ dispatch, modules: routeModules });
const fallback = await getModuleDataBreaker.fire({ dispatch, modules: routeModules });


if (fallback) {
req.appHtml = '';
req.helmetInfo = {};
return next();
}
}

const renderMethod = (disableScripts || renderPartialOnly)
? renderForStaticMarkup : renderForString;

Expand Down
Loading