-
Notifications
You must be signed in to change notification settings - Fork 84
refactor: migrating metrics server to Fastify #803
Conversation
BREAKING CHANGE: Upgrade from React 16 to 17
BREAKING CHANGE: minimum supported node version is 16
* chore(babel): update packages * chore(commitlint): update * chore(rollup-plugins): update * chore(acorn): uninstall * chore(babel-preset-amex): update to 4 * chore(body-parser): update * chore(dev-deps): update * chore(holocron): update 1.3.0 * chore(redux): update 4.2.0 * chore(core-js): update 3.23.3 * chore(deps): run npm update * chore(husky): update to 8.x * chore(chalk): downgrade to non esm version * chore(webdriverio): update 7.x * feat(dockerfile): update node version to 16.15.1 * chore(deps): update supertest * fix(node): set min version 16.15.1 * chore(deps): dedupe * test(createRequestHtmlFragment): more reliable error message * chore(jest): upgrade 28.1.2
BREAKING CHANGE: Upgrade from React 16 to 17
BREAKING CHANGE: minimum supported node version is 16
* chore(babel): update packages * chore(commitlint): update * chore(rollup-plugins): update * chore(acorn): uninstall * chore(babel-preset-amex): update to 4 * chore(body-parser): update * chore(dev-deps): update * chore(holocron): update 1.3.0 * chore(redux): update 4.2.0 * chore(core-js): update 3.23.3 * chore(deps): run npm update * chore(husky): update to 8.x * chore(chalk): downgrade to non esm version * chore(webdriverio): update 7.x * feat(dockerfile): update node version to 16.15.1 * chore(deps): update supertest * fix(node): set min version 16.15.1 * chore(deps): dedupe * test(createRequestHtmlFragment): more reliable error message * chore(jest): upgrade 28.1.2
Size Change: -299 B (0%) Total Size: 680 kB
ℹ️ View Unchanged
|
}; | ||
|
||
const getLocale = (req) => { | ||
// TODO: Verify if `store` is available |
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.
this only affects the locale in logger
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.
this is not longer a blocker, the Metrics Server does not inject the store, only the SSR Server which is being migrated in a separate PR
src/server/metricsServer.js
Outdated
@@ -1,5 +1,5 @@ | |||
/* | |||
* Copyright 2019 American Express Travel Related Services Company, Inc. | |||
* Copyright 2022 American Express Travel Related Services Company, Inc. |
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.
years in existing files can stay the same
}); | ||
await fastify.register(fp(logging)); | ||
await fastify.register(helmet); | ||
await fastify.register(healthCheck); |
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.
i'm not sold on having the route abstracted into the plugin. I would have to do into each plugin to understand what routes this serves. can you provide your reasoning ?
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.
that's true. We could pass the url through the options but still hides the route. Best option would be to create a reply decorator so we can implement the route outside the plugin
src/server/ssrServer.js
Outdated
@@ -1,5 +1,5 @@ | |||
/* | |||
* Copyright 2019 American Express Travel Related Services Company, Inc. | |||
* Copyright 2022 American Express Travel Related Services Company, Inc. |
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.
this can be reverted
@@ -0,0 +1,179 @@ | |||
/* | |||
* Copyright 2019 American Express Travel Related Services Company, Inc. |
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.
new file so should be this year.
* Copyright 2019 American Express Travel Related Services Company, Inc. | |
* Copyright 2022 American Express Travel Related Services Company, Inc. |
export const $RouteHandler = Symbol('$RouteHandler'); | ||
export const $ResponseBuilder = Symbol('$ResponseBuilder'); | ||
|
||
const TIMERS = {}; |
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.
Could one instance of this be shared across multiple requests ?
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.
just looking through the code, this is not really needed, i'll remove it
Migrating Metrics Server from ExpressJS to Fastify
Description
This PR includes the migration of Metrics Server routes to Fastify + converting logging middleware into a fastify plugin
Metrics Server
All routes have been moved to Fastify
Logging
The logging middleware has been converted into a Fastify plugin. It required some refactoring in order to leverage fastify hooks.
To calculate
ttfb
(time to first byte), instead of monkey patchingwriteHead
function from the raw/nodejs response, I leveraged theonSend
fastify hook which is called right beforewriteHead
is called, providing the same time (technically isn't the same but we are using Math.round when logging, so "it's the same").Besides logging the full request duration and ttfb, I also added extra timers to calculate things like how long it took to start resolving a route, how long it took to resolved a route handler, etc.
Here's an example of a request that's taking 500ms to resolve things before fastify can start resolving the route handler, and the route handler takes 1000ms to resolved.
I achieved this by adding a sleep function to create the delay
These are real metrics from
/metrics
and/im-up
, where we can see that/im-up
is a bit slower because of the healthcheck fn/metrics
/im-up
As we can see, we do not have an overhead on the request (pre route handler) and on the response builder
Motivation and Context
Allows us to run Metrics Server in Fastify without the need of Express
How Has This Been Tested?
Types of Changes
Checklist:
What is the Impact to Developers Using One App?