Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: Request Logging Customization #4955

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
+9 −2
Open

feat: Request Logging Customization #4955

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
e0c7c7b
customized log management system
kdrewes Aug 4, 2023
735cdab
customized log management system and modified documentation
kdrewes Aug 4, 2023
05a46bd
Merge branch 'main' into KylesBranch
kdrewes Aug 5, 2023
defc572
Merge branch 'main' into KylesBranch
metcoder95 Aug 7, 2023
0a304c4
Merge branch 'main' into KylesBranch
Eomm Aug 26, 2023
d46f565
customized log management system
kdrewes Aug 4, 2023
f466e0d
customized log management system and modified documentation
kdrewes Aug 4, 2023
09e17ed
modified createRequestLogMessage so it takes childlogger and FastifyR…
kdrewes Aug 6, 2023
89bf726
Attempting to merge the main repository into KylesBranch
kdrewes Aug 7, 2023
29d621f
Merged main respository to KylesBranch
kdrewes Sep 8, 2023
fb761f2
Merge branch 'main' into KylesBranch
metcoder95 Sep 15, 2023
5c1021d
Merge branch 'main' into KylesBranch
Eomm Nov 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/Reference/Server.md
View file
Open in desktop
Expand Up @@ -25,6 +25,7 @@ describes the properties available in that options object.
- [`onConstructorPoisoning`](#onconstructorpoisoning)
- [`logger`](#logger)
- [`disableRequestLogging`](#disablerequestlogging)
- [`createRequestLogMessage`](#createRequestLogMessage)
- [`serverFactory`](#serverfactory)
- [`jsonShorthand`](#jsonshorthand)
- [`caseSensitive`](#casesensitive)
Expand Down Expand Up @@ -1911,6 +1912,7 @@ The properties that can currently be exposed are:
explicitly passed)
- ignoreTrailingSlash
- disableRequestLogging
- createRequestLogMessage
Copy link
Member

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

Copy link
Author

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.

Copy link
Contributor

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?

- maxParamLength
- onProtoPoisoning
- onConstructorPoisoning
Expand Down
1 change: 1 addition & 0 deletions fastify.d.ts
View file
Open in desktop
Expand Up @@ -111,6 +111,7 @@ declare namespace fastify {
requestIdLogLabel?: string;
jsonShorthand?: boolean;
genReqId?: (req: RawRequestDefaultExpression<RawServer>) => string,
createRequestLogMessage?: (req: RawRequestDefaultExpression<RawServer>) => string,
Copy link
Member

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

Copy link
Author

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.

Copy link
Contributor

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?

Copy link
Author

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.

Copy link
Member

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

trustProxy?: boolean | string | string[] | number | TrustProxyFunction,
querystringParser?: (str: string) => { [key: string]: unknown },
/**
Expand Down
8 changes: 6 additions & 2 deletions fastify.js
View file
Open in desktop
Expand Up @@ -114,6 +114,7 @@ function fastify (options) {
const requestIdLogLabel = options.requestIdLogLabel || 'reqId'
const bodyLimit = options.bodyLimit || defaultInitOptions.bodyLimit
const disableRequestLogging = options.disableRequestLogging || false
const createRequestLogMessage = options.createRequestLogMessage

const ajvOptions = Object.assign({
customOptions: {},
Expand Down Expand Up @@ -729,9 +730,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))
Copy link
Member

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)

Copy link
Author

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.

Copy link
Member

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.

Suggested change
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? 🙂

} else {
childLogger.info({ req: request }, 'incoming request')
}
Comment on lines +733 to +737
Copy link
Member

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

Copy link
Author

@kdrewes kdrewes Aug 5, 2023 •
edited by Uzlopak

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.

Copy link
Member

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? 🤔

Copy link
Member

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)

}

return frameworkErrors(new FST_ERR_BAD_URL(path), request, reply)
}
const body = `{"error":"Bad Request","code":"FST_ERR_BAD_URL","message":"'${path}' is not a valid url component","statusCode":400}`
Expand Down