-
Notifications
You must be signed in to change notification settings - Fork 87
fix(configureRequestLog): use default when configured with undefined #1029
Changes from all commits
b15048b
656f01a
914f81a
98b73f6
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 |
---|---|---|
|
@@ -35,8 +35,9 @@ export const $RequestOverhead = Symbol('$RequestOverhead'); | |
export const $RouteHandler = Symbol('$RouteHandler'); | ||
export const $ResponseBuilder = Symbol('$ResponseBuilder'); | ||
|
||
const passThrough = ({ log }) => log; | ||
const UTILS = { | ||
configureRequestLog: ({ log }) => log, | ||
configureRequestLog: passThrough, | ||
}; | ||
|
||
const getLocale = (req) => { | ||
|
@@ -137,7 +138,7 @@ const logClientRequest = (request, reply) => { | |
logger.info(configuredLog); | ||
}; | ||
|
||
export const setConfigureRequestLog = (newConfigureRequestLog) => { | ||
export const setConfigureRequestLog = (newConfigureRequestLog = passThrough) => { | ||
UTILS.configureRequestLog = newConfigureRequestLog; | ||
}; | ||
|
||
|
@@ -200,12 +201,17 @@ const fastifyPlugin = (fastify, _opts, done) => { | |
}); | ||
|
||
fastify.addHook('onResponse', async (request, reply) => { | ||
endTimer(request, $ResponseBuilder); | ||
// same as 'reply.getResponseTime()' but our approach | ||
// helps us to make the code cleaner | ||
endTimer(request, $RequestFullDuration); | ||
|
||
logClientRequest(request, reply); | ||
try { | ||
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. why is the try/catch needed here? it catches the error, logs it, and throws it, meaning that we only need to log it right? Fastify handles uncaught errors for us (https://www.fastify.io/docs/latest/Reference/Errors/) so no need for a try/catch 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. not for |
||
endTimer(request, $ResponseBuilder); | ||
// same as 'reply.getResponseTime()' but our approach | ||
// helps us to make the code cleaner | ||
endTimer(request, $RequestFullDuration); | ||
|
||
logClientRequest(request, reply); | ||
} catch (error) { | ||
console.error(error); | ||
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. shouldn't we use the logger here? 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. this does use it. console is monkey patched to use the logger https://github.com/americanexpress/one-app/blob/main/src/server/utils/logging/setup.js#L28 |
||
throw error; | ||
} | ||
}); | ||
|
||
done(); | ||
|
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.
why would somebody call
setConfigureRequestLog
without passing an argument?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.
why should this be allowed to fail when called without an argument ? It's called every time during onModule load. This is also how its handled in 5.x.x and should not have been removed.
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.
because it shouldn't be called without an argument
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.
It shouldn't but it is. I don't see the issue having a default to improve resiliency
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.
https://github.com/americanexpress/one-app/blob/5.x.x/src/server/utils/logging/serverMiddleware.js#L98-L100
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 won't block it but it's a bad practice. The function by starting with the word
set
lets the developer know that it "sets" something, and the data will always be expected (you cannot set something with no data, otherwise would be like a reset). That's the reason why all functions, like thewrite
fromfs
will throw an error if data is not provided. If we want a set to be "safe" I would rename it to something likesafeSetConfigureRequestLog
, but, still, ideally, the user should always pass data