feat: implement conditional request logging#5732
Conversation
Eomm
left a comment
There was a problem hiding this comment.
Why not?
const Fastify = require('./')
const fastify = Fastify({ logger: true })
fastify.get('/', {
handler: () => { return 'foo' },
logLevel: 'silent',
})
fastify.inject('/')|
@Eomm That is an option in some cases, but it has very limited granularity, and sometimes you may not even have necessary level of control, e. g. if route is registered by a third-party plugin that you are using. Also you may want to log some domain business logic, but not the "incoming request". |
metcoder95
left a comment
There was a problem hiding this comment.
I believe it can be useful to apply custom logic to decide wether or not trigger logs accordingly to specific filters; but I'm wondering if this does not overlaps with route-level (e.g. the ones that @Eomm pointed out).
Also think that maybe this should be allowed to be set on a request basis, in case implementers want to fully disable logging for a registered route or apply custom logic to only that route.
…onditional-request-logging
|
@metcoder95 @climba03003 I've adjusted the code. Are we good with the approach now, can I proceed with TS types and documentation? |
…onditional-request-logging # Conflicts: # lib/config-validator.js # lib/route.js # test/logger/logging.test.js
…' into feat/conditional-request-logging # Conflicts: # lib/route.js # test/logger/logging.test.js
|
Oh, this is a very old PR I forgot My stands here can be resumed like: We need a better API because I think this the 4th PR related to "customize the custom logger" Here, we don't want a This makes everybody happy:
|
|
@Eomm I see this PR as complimentary to #6417. Ideally this one lands first to extend the surface that such customizable layer would allow customizing (currently it is not aiming to do so at all), and I agree that ultimately it needs to live there, but it's unclear when and if we will migrate to that mechanism fully. Amount of ifs here is very small, I don't think there is any meaningful perf penalty. And long-term it would be removed together with the rest of reqLogging configuration. |
metcoder95
left a comment
There was a problem hiding this comment.
I do not have strong opinion on either go with a progressive or all in approach (flag vs fully fledged at once).
The downsides on both ways roots mostly on the amount of work to consolidate it eventually if we seek the fully fledged one to be the one we want to be the final goal.
The PR lgtm (just issues on linting for MD).
|
@metcoder95 Thank you! Should I fix doc lint issues? They did not originate in this PR, but fairly easy to fix. |
|
Just for the linter check to go green |
|
@metcoder95 done! |
|
@Eomm agreed. Should fastify in v6 ship without pino? |
Use-case for this is following: some endpoints generate excessive amount of irrelevant noise (e. g. healthcheck) and are not useful in logs, so it would be great to be able to be able to resolve variable for logging their requests or not conditionally. And the big problem here is that these endpoints (healthchecks, metrics etc) frequently come from 3rd party plugins which register their routes themselves, therefore preventing extra configuration on a route level.
Note: Documentation linting failing does not seem to be related to changes in this PR.
Checklist
npm run testandnpm run benchmarkand the Code of conduct