feat(createRequestHtmlFragment): implemented circuit breaker #111

10xLaCroixDrinker · 2020-04-21T19:10:48Z

Description

This implements a circuit breaker using opossum. The circuit opens when event loop delay is > 250ms (by default- is configurable). When the circuit is open there is no SSR. The circuit will go into a half open state after 10s. At that point it will attempt the full request again, the circuit will close.

Motivation and Context

This provides significant performance improvements.

How Has This Been Tested?

In addition to the unit tests, I did some load testing locally. I ran a json server using json-server http://jsonplaceholder.typicode.com/db -d 800 -p 1337, then changed SSR Frank to make its requests to http://localhost:1337/todos. Once that was set up I called autocannon with autocannon localhost:3000/healthy-frank/ssr-frank -c 200 -d 30 --headers "Accept-Language= en-USen;q=0.5" --headers "User-Agent= curl/7.5.0" . Below are the results for master and for this branch.

(Note the circuit breaker is disabled for perf tests by setting eventLoopDelayThreshold to Infinity. To validate locally, that line should be removed.

master

Running 30s test @ http://localhost:3000/healthy-frank/ssr-frank
200 connections

┌─────────┬────────┬─────────┬─────────┬─────────┬────────────┬─────────┬────────────┐
│ Stat    │ 2.5%   │ 50%     │ 97.5%   │ 99%     │ Avg        │ Stdev   │ Max        │
├─────────┼────────┼─────────┼─────────┼─────────┼────────────┼─────────┼────────────┤
│ Latency │ 980 ms │ 1110 ms │ 1426 ms │ 1517 ms │ 1124.38 ms │ 99.5 ms │ 1590.64 ms │
└─────────┴────────┴─────────┴─────────┴─────────┴────────────┴─────────┴────────────┘
┌───────────┬─────┬──────┬─────────┬─────────┬─────────┬─────────┬─────────┐
│ Stat      │ 1%  │ 2.5% │ 50%     │ 97.5%   │ Avg     │ Stdev   │ Min     │
├───────────┼─────┼──────┼─────────┼─────────┼─────────┼─────────┼─────────┤
│ Req/Sec   │ 0   │ 0    │ 184     │ 200     │ 175.1   │ 36.24   │ 144     │
├───────────┼─────┼──────┼─────────┼─────────┼─────────┼─────────┼─────────┤
│ Bytes/Sec │ 0 B │ 0 B  │ 11.1 MB │ 13.4 MB │ 9.75 MB │ 3.82 MB │ 1.73 MB │
└───────────┴─────┴──────┴─────────┴─────────┴─────────┴─────────┴─────────┘

Req/Bytes counts sampled once per second.

5k requests in 30.14s, 292 MB read

This branch

Running 30s test @ http://localhost:3000/healthy-frank/ssr-frank
200 connections

┌─────────┬────────┬────────┬────────┬────────┬──────────┬──────────┬────────────┐
│ Stat    │ 2.5%   │ 50%    │ 97.5%  │ 99%    │ Avg      │ Stdev    │ Max        │
├─────────┼────────┼────────┼────────┼────────┼──────────┼──────────┼────────────┤
│ Latency │ 239 ms │ 291 ms │ 441 ms │ 463 ms │ 312.1 ms │ 99.35 ms │ 1450.22 ms │
└─────────┴────────┴────────┴────────┴────────┴──────────┴──────────┴────────────┘
┌───────────┬─────────┬─────────┬─────────┬─────────┬─────────┬─────────┬─────────┐
│ Stat      │ 1%      │ 2.5%    │ 50%     │ 97.5%   │ Avg     │ Stdev   │ Min     │
├───────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
│ Req/Sec   │ 229     │ 229     │ 661     │ 807     │ 637.5   │ 124.65  │ 229     │
├───────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
│ Bytes/Sec │ 1.91 MB │ 1.91 MB │ 5.51 MB │ 6.72 MB │ 5.31 MB │ 1.03 MB │ 1.91 MB │
└───────────┴─────────┴─────────┴─────────┴─────────┴─────────┴─────────┴─────────┘

Req/Bytes counts sampled once per second.

19k requests in 30.09s, 159 MB read

Types of Changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation (adding or updating documentation)

Checklist:

My change requires a change to the documentation and I have updated the documentation accordingly.
These changes should be applied to a maintenance branch.
This change requires cross browser checks.
Performance tests should be ran against the server prior to merging.
This change impacts caching for client browsers.
This change impacts HTTP headers.
This change adds additional environment variable requirements for One App users.
I have added the Apache 2.0 license header to any new files created.

What is the Impact to Developers Using One App?

No impact to developers other than SSR turning off in the cases that the circuit opens

docs/api/modules/App-Configuration.md

JAdshead · 2020-04-21T22:34:40Z

__tests__/server/middleware/createRequestHtmlFragment.spec.js

@@ -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);


is this required with async test ?

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

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

JAdshead · 2020-04-21T22:46:12Z

__tests__/server/utils/circuitBreaker.spec.js

+} from '../../../src/server/utils/circuitBreaker';
+
+describe('Circuit breaker', () => {
+  it('should be an opossum circuit breaker', () => {


does this matter if everything else works ?

not necessarily

JAdshead · 2020-04-21T22:51:06Z

prod-sample/sample-modules/frank-lloyd-root/0.0.0/src/config.js

@@ -18,6 +18,7 @@ import csp from './csp';
 import createFrankLikeFetch from './createFrankLikeFetch';

 export default {
+  eventLoopLagThreshold: Infinity,


do these not work with the default ?

This prevents the circuit from opening during the integration tests

oneamexbot · 2020-04-22T15:45:16Z

📊 Bundle Size Report

file name	size on disk	gzip
app.js	112.6KB	31.4KB
runtime.js	15KB	5.3KB
vendors.js	128.4KB	38KB
app~vendors.js	405.9KB	105.9KB
legacy/app.js	119.3KB	33KB
legacy/runtime.js	15KB	5.3KB
legacy/vendors.js	163.4KB	44.9KB
legacy/app~vendors.js	412KB	107.6KB

Generated by 🚫 dangerJS against 9047392

mcollina · 2020-04-22T18:45:38Z

__tests__/server/middleware/createRequestHtmlFragment.spec.js

+  const realHrtime = process.hrtime;
+  const mockHrtime = (...args) => realHrtime(...args);
+  mockHrtime.bigint = jest.fn();
+  process.hrtime = mockHrtime;


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

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

mcollina · 2020-04-22T18:51:24Z

src/server/utils/circuitBreaker.js

+const breaker = new CircuitBreaker(getModuleData, options);
+// Just need to connect opossum to prometheus
+// eslint-disable-next-line no-unused-vars
+const metrics = new PrometheusMetrics(breaker, register);


I would probably not expose this at this level. Why was it added here?

Just keeping it close to the code, but yes it makes more since to go in src/server/metrics

mcollina · 2020-04-22T18:53:44Z

src/server/utils/circuitBreaker.js

+  }
+}, 100);
+
+export default breaker;


I think a more reusable and testable implementation would be to make this a factory, so we can avoid pulling in holocron in here, but rather have the getModuleData passed in to the factory instead.

10xLaCroixDrinker · 2020-04-22T20:42:11Z

src/server/utils/circuitBreaker.js

+let eventLoopLagThreshold = 30;
+
+export const setEventLoopLagThreshold = (n) => {
+  eventLoopLagThreshold = Number(n) || 30;


@mcollina what are your thoughts on this default value?

30ms is very aggressive. I would increase quite a bit. The 100-500ms range seems more ok for an SSR app (most React rendering go up to 80-100ms).

mcollina · 2020-04-23T17:22:24Z

src/server/utils/circuitBreaker.js

+breaker.healthCheck(async () => {
+  if (!getModule(rootModuleName)) return;
+  const start = process.hrtime.bigint();
+  await immediate();


You might want to use https://nodejs.org/dist/latest-v12.x/docs/api/perf_hooks.html#perf_hooks_perf_hooks_monitoreventloopdelay_options to measure event loop latency. It adds less overhead.

…express/one-app into feature/circuit-breaker

10xLaCroixDrinker · 2020-04-30T21:02:49Z

@mcollina I did some refactoring here, think you could review again?

mcollina

lgtm

mcollina · 2020-05-01T07:41:37Z

prod-sample/sample-modules/frank-lloyd-root/0.0.0/src/config.js

@@ -18,6 +18,7 @@ import csp from './csp';
 import createFrankLikeFetch from './createFrankLikeFetch';

 export default {
+  eventLoopDelayThreshold: Infinity,


Should this be set to a smaller amount?

This is just to prevent the circuit from opening during the integration tests

JAdshead · 2020-05-01T18:11:24Z

src/server/middleware/createRequestHtmlFragment.jsx

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


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 });

10xLaCroixDrinker requested review from a team as code owners April 21, 2020 19:10

mtomcal reviewed Apr 21, 2020

View reviewed changes

docs/api/modules/App-Configuration.md Show resolved Hide resolved

JAdshead reviewed Apr 21, 2020

View reviewed changes

10xLaCroixDrinker added 2 commits April 22, 2020 08:40

feat(createRequestHtmlFragment): implemented circuit breaker

2f0786e

chore(tests): added missing license header

31da485

10xLaCroixDrinker force-pushed the feature/circuit-breaker branch from e88695d to 31da485 Compare April 22, 2020 15:40

mcollina reviewed Apr 22, 2020

View reviewed changes

10xLaCroixDrinker commented Apr 22, 2020

View reviewed changes

mcollina reviewed Apr 23, 2020

View reviewed changes

JAdshead and others added 4 commits April 23, 2020 12:09

Merge branch 'master' into feature/circuit-breaker

0dc99f2

refactor(circuitBreaker): use built-in perf_hooks

1b3c07f

Merge branch 'master' into feature/circuit-breaker

1060434

Merge branch 'feature/circuit-breaker' of https://github.com/american…

51542c6

…express/one-app into feature/circuit-breaker

10xLaCroixDrinker added 2 commits April 30, 2020 14:23

docs(appconfig): more info on eventLoopDelayThreshold

e53c396

Merge branch 'master' into feature/circuit-breaker

81ffcb4

mcollina previously approved these changes May 1, 2020

View reviewed changes

mtomcal previously approved these changes May 1, 2020

View reviewed changes

JAdshead reviewed May 1, 2020

View reviewed changes

refactor(breaker): more descriptive name

9047392

10xLaCroixDrinker dismissed stale reviews from mtomcal and mcollina via 9047392 May 1, 2020 18:39

JAdshead approved these changes May 1, 2020

View reviewed changes

mtomcal approved these changes May 1, 2020

View reviewed changes

10xLaCroixDrinker merged commit e10f707 into master May 1, 2020

10xLaCroixDrinker deleted the feature/circuit-breaker branch May 1, 2020 18:58

PixnBits mentioned this pull request May 14, 2020

feature: update prom-client #123

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(createRequestHtmlFragment): implemented circuit breaker #111

feat(createRequestHtmlFragment): implemented circuit breaker #111

10xLaCroixDrinker commented Apr 21, 2020 •

edited

Loading

JAdshead Apr 21, 2020

10xLaCroixDrinker Apr 22, 2020

JAdshead Apr 22, 2020

10xLaCroixDrinker Apr 22, 2020

JAdshead Apr 21, 2020

10xLaCroixDrinker Apr 22, 2020

JAdshead Apr 21, 2020

10xLaCroixDrinker Apr 22, 2020

oneamexbot commented Apr 22, 2020 •

edited

Loading

mcollina Apr 22, 2020

10xLaCroixDrinker Apr 22, 2020

mcollina Apr 22, 2020

10xLaCroixDrinker Apr 22, 2020

mcollina Apr 22, 2020

10xLaCroixDrinker Apr 22, 2020

mcollina Apr 23, 2020

mcollina Apr 23, 2020

10xLaCroixDrinker commented Apr 30, 2020

mcollina left a comment

mcollina May 1, 2020

10xLaCroixDrinker May 1, 2020

JAdshead May 1, 2020

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

feat(createRequestHtmlFragment): implemented circuit breaker #111

feat(createRequestHtmlFragment): implemented circuit breaker #111

Conversation

10xLaCroixDrinker commented Apr 21, 2020 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

master

This branch

Types of Changes

Checklist:

What is the Impact to Developers Using One App?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oneamexbot commented Apr 22, 2020 • edited Loading

📊 Bundle Size Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

10xLaCroixDrinker commented Apr 30, 2020

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

10xLaCroixDrinker commented Apr 21, 2020 •

edited

Loading

oneamexbot commented Apr 22, 2020 •

edited

Loading