-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Request Logging Customization #4955
base: main
Are you sure you want to change the base?
Conversation
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.
when we will agree on the api, we should add some tests
@@ -1865,6 +1866,7 @@ Currently the properties that can be exposed are: | |||
explicitly passed) | |||
- ignoreTrailingSlash | |||
- disableRequestLogging | |||
- createRequestLogMessage |
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.
we should describe its value
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.
Absolutely my apologies for not including that earlier. Is there a particular area within docs/Reference/Server.md where I should include it or does anywhere suffice? Thank you.
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.
search for the docs of disableRequestLogging and put them next to it?
if (createRequestLogMessage) { | ||
childLogger.info(createRequestLogMessage(req)) | ||
} else { | ||
childLogger.info({ req: request }, 'incoming request') | ||
} |
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.
instead of executing an if condition in the hot path execution of the request, we could just set a default createRequestLogMessage
function to execute it always
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.
Ok sounds good. So instead of having:
if (createRequestLogMessage) {
childLogger.info(createRequestLogMessage(req))
} else {
childLogger.info({ req: request }, 'incoming request')
}
Should I just replace that code with:
createRequestLogMessage(childLogger, req)?
Please let me know if I am incorrect about this. Thank you.
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.
If the goal is just to create the message, what about:
createRequestLogMessage(req)
and returns a string
that is used as the log message.
Or we are trying to cover something else there? 🤔
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.
@kdrewes I was thinking on:
childLogger.info.call(childLogger, createRequestLogMessage(req))
where the default createRequestLogMessage
returns: [{ req: request }, 'incoming request']
Note the returned array and the call
to avoid to spread the array.
@metcoder95 that usage misses the multiple pino format parameter (the array)
@@ -721,9 +722,12 @@ function fastify (options) { | |||
const reply = new Reply(res, request, childLogger) | |||
|
|||
if (disableRequestLogging === false) { | |||
childLogger.info({ req: request }, 'incoming request') | |||
if (createRequestLogMessage) { | |||
childLogger.info(createRequestLogMessage(req)) |
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.
the user here can set only the 1st parameter of childLogger.info
.
I would think another way to handle it like:
createRequestLogMessage(childLogger, req)
so it is up to the user choose to log info or warning (i used to change the log level based on the request headers)
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.
Ok perfect! I will make sure to modify the parameters so that createRequestLogMessage will accept childLogger as a parameter.
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.
so it is up to the user choose to log info or warning (i used to change the log level based on the request headers)
Hmm, that's a good one. But then I'd like to know if the initial signature is the best for the use case you describe.
The current issue the PR attempts to solve is to customize the log message only, as per #4463. If that's the goal, I think that it is a matter of just applying the correct order to the log statement. i.e.
childLogger.info(createRequestLogMessage(req)) | |
childLogger.info({ req: request }, createRequestLogMessage(req)) |
If something more complex is required, I believe the current documentation for disableRequestLogging
already covers well what t do in those cases. wdyt? 🙂
Note: Fixed the title and the formatting of the intial comment. |
@kdrewes why did you close the PR? |
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.
Overall, I'm good with the shape is taking, but I believe tests are missing. Should we add some to cover the feature? 🙂
@@ -721,9 +722,12 @@ function fastify (options) { | |||
const reply = new Reply(res, request, childLogger) | |||
|
|||
if (disableRequestLogging === false) { | |||
childLogger.info({ req: request }, 'incoming request') | |||
if (createRequestLogMessage) { | |||
childLogger.info(createRequestLogMessage(req)) |
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.
so it is up to the user choose to log info or warning (i used to change the log level based on the request headers)
Hmm, that's a good one. But then I'd like to know if the initial signature is the best for the use case you describe.
The current issue the PR attempts to solve is to customize the log message only, as per #4463. If that's the goal, I think that it is a matter of just applying the correct order to the log statement. i.e.
childLogger.info(createRequestLogMessage(req)) | |
childLogger.info({ req: request }, createRequestLogMessage(req)) |
If something more complex is required, I believe the current documentation for disableRequestLogging
already covers well what t do in those cases. wdyt? 🙂
if (createRequestLogMessage) { | ||
childLogger.info(createRequestLogMessage(req)) | ||
} else { | ||
childLogger.info({ req: request }, 'incoming request') | ||
} |
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.
If the goal is just to create the message, what about:
createRequestLogMessage(req)
and returns a string
that is used as the log message.
Or we are trying to cover something else there? 🤔
@@ -111,6 +111,7 @@ declare namespace fastify { | |||
requestIdLogLabel?: string; | |||
jsonShorthand?: boolean; | |||
genReqId?: (req: RawRequestDefaultExpression<RawServer>) => string, | |||
createRequestLogMessage?: (req: RawRequestDefaultExpression<RawServer>) => string, |
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 believe it receives the FastifyRequest
type instead of http.IncomingMessage
, which is from where the RawRequestDefaultExpression<RawServer>
is based on
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.
Ok no problem, I will change it to FastifyRequest and have it return a string.
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.
Is it FastifyRequest or the RawRequest?
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.
Here's how I set the prototype:
createRequestLogMessage?: (ChildLoggerOptions: ChildLoggerOptions, FastifyRequest) => string
Please let me know if you guys have any alternative suggestions.
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.
Yeah, it should be FastifyRequest
. We already constructed one at the moment we are printing the incoming request
log
Hello, I've had some difficulties with implementing the suggestions I've been provided with. In the fastify.d.ts file I've added the changes createRequestLogMessage?: (ChildLoggerOptions: ChildLoggerOptions, req: FastifyRequest) => string and in fastify.js I removed
and replaced it with
However, when I attempted to commit the changes I ran into multiple errors. Please let me know if you have any suggestions on how I can modify these changes. Thank you. |
Can you paste the errors you faced? |
Yes absolutely. So I altered the syntax so it would state the following:
which did reduce the number of errors I previously was encountering. However I still have one error that I'm coming across. Here's the information on my console. Please let me know if you have any questions PASS test/bodyLimit.test.js 17 OK 83.82ms SKIP test/build/error-serializer.test.js 1 skip of 2 824.519ms PASS test/async-await.test.js 81 OK 862.828ms SKIP test/close-pipelining.test.js 1 skip of 6 194.57ms PASS test/connectionTimeout.test.js 6 OK 77.726ms PASS test/content-length.test.js 22 OK 105.779ms PASS test/plugin.test.js 221 OK 326.558ms test/router-options.test.js
test: Should supply Fastify request to the logger in frameworkErrors wrapper - FAIL test/router-options.test.js FAIL test/router-options.test.js FAIL test/router-options.test.js FAIL test/router-options.test.js FAIL test/router-options.test.js 5 failed of 41 194.77ms PASS test/schema-examples.test.js 23 OK 325.87ms SKIP test/stream.test.js 1 skip of 85 280.882ms PASS test/unsupported-httpversion.test.js 2 OK 67.034ms 🌈 SUMMARY RESULTS 🌈 SKIP test/build/error-serializer.test.js 1 skip of 2 824.519ms SKIP test/close-pipelining.test.js 1 skip of 6 194.57ms SKIP test/close.test.js 1 skip of 82 7s FAIL test/router-options.test.js 5 failed of 41 194.77ms SKIP test/stream.test.js 1 skip of 85 280.882ms Suites: 1 failed, 131 passed, 132 of 132 completed
|
Maybe you can try to isolate a single test and see what might be wrong. See tap#only |
Hi @metcoder95, thanks for looking this over for me. What would be able example of isolating a single test so that I can determine the problem? In the meantime, I will continue investigating the matter further. Thank you. |
Hey guys, I've been experimenting with the code and it appears that the error doesn't appear until I update the following prototype from: so just to make sure I'm doing this correctly, when plugging in a childLogger object as a parameter, would "childLoggerOption: childLoggerOption" be the proper argument to use? Also, is it even worth using childLogger as an argument to begin with? Thank you |
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.
so just to make sure I'm doing this correctly, when plugging in a childLogger object as a parameter, would "childLoggerOption: childLoggerOption" be the proper argument to use? Also, is it even worth using childLogger as an argument to begin with? Thank you
We should be using the childLogger
already constructed a few lines back. See my comment about other approach we can also take: https://github.com/fastify/fastify/pull/4955/files#r1285197031
@@ -111,6 +111,7 @@ declare namespace fastify { | |||
requestIdLogLabel?: string; | |||
jsonShorthand?: boolean; | |||
genReqId?: (req: RawRequestDefaultExpression<RawServer>) => string, | |||
createRequestLogMessage?: (req: RawRequestDefaultExpression<RawServer>) => string, |
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.
Yeah, it should be FastifyRequest
. We already constructed one at the moment we are printing the incoming request
log
Hey guys, I just wanted to let you know I've been caught up with the program I'm enrolled in but will continue working on this as soon as possible. This will not be forgot. Please let me know if you have any questions in the meantime. Thank you. |
…equest)and also modified the documentation by describing the value of createRequestLogMessage
Checklist
npm run test
andnpm run benchmark
Hello, I've been working on Im currently working on issue #4463 to customize our log management system. Here are the changes made so far:
• Included a condition in the fastify.js file which dictates whether createRequestLogMessage will be implemented. Here is the condition below:
• Defined the createRequestLogMessage function in fastify.js
• Added the necessary prototype of createRequestLogMessage in the fastify.d.ts file
• Modified the documentation within docs/Guides/Server.md so that createRequestLogMessage would be included.
Please let me know if you have any questions or concerns regarding this PR. Thank you
Kyle