fix(configureRequestLog): use default when configured with undefined #1029
Conversation
|
Size Change: 0 B Total Size: 687 kB ℹ️ View Unchanged
|
|
|
||
| logClientRequest(request, reply); | ||
| } catch (error) { | ||
| console.error(error); |
There was a problem hiding this comment.
shouldn't we use the logger here?
There was a problem hiding this comment.
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
| endTimer(request, $RequestFullDuration); | ||
|
|
||
| logClientRequest(request, reply); | ||
| try { |
There was a problem hiding this comment.
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.
not for onResponse hooks, this currently fails silently
| @@ -137,7 +138,7 @@ const logClientRequest = (request, reply) => { | |||
| logger.info(configuredLog); | |||
| }; | |||
|
|
|||
| export const setConfigureRequestLog = (newConfigureRequestLog) => { | |||
| export const setConfigureRequestLog = (newConfigureRequestLog = passThrough) => { | |||
There was a problem hiding this comment.
why would somebody call setConfigureRequestLog without passing an argument?
There was a problem hiding this comment.
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.
because it shouldn't be called without an argument
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 the write from fs will throw an error if data is not provided. If we want a set to be "safe" I would rename it to something like safeSetConfigureRequestLog, but, still, ideally, the user should always pass data
Description
if configureRequestLog is not provided by root module, request logs are not being logged.
Motivation and Context
re enable request logs
How Has This Been Tested?
Test suite and locally
Types of Changes
Checklist:
What is the Impact to Developers Using One App?