feat: add childLoggerFactory config by brettwillis · Pull Request #4760 · fastify/fastify

brettwillis · 2023-05-17T04:26:48Z

Allow customisation of child logger bindings (or the instance itself) via child logger factory function.

Checklist

run npm run test and npm run benchmark
tests and/or benchmarks are included
documentation is changed or added
commit message and code follows the Developer's Certification of Origin
and the Code of conduct

brettwillis · 2023-05-17T04:29:04Z

Referring to #4751 (comment), not sure if we should also do a fastify.setChildLoggerFactory instance method to allow it to be set/encapsulated by a plugin?

If so, not sure if there's anything special I have to do to allow encapsulation etc...

metcoder95 · 2023-05-17T08:10:34Z

On my side, I think it can be an interesting addition, especially for plugins to have capabilities to also maybe decorate the logger with extra properties (e.g. traceId, custom headers, etc.)

Though, I would like to hear from @fastify/core their thoughts 🙂

lib/logger.js

metcoder95 · 2023-05-17T08:21:16Z

fastify.js

  options.disableRequestLogging = disableRequestLogging
  options.ajv = ajvOptions
  options.clientErrorHandler = options.clientErrorHandler || defaultClientErrorHandler
+  options.childLoggerFactory = options.childLoggerFactory || defaultChildLoggerFactory


For being correctly encapsulated down the road to child contexts, we will need to tighten it to the Context constructor. This will help to be inherited or altered if e.g. a user wants the child logger factory to be customized on A but not on the root instance.

metcoder95 · 2023-05-17T08:21:36Z

test/childLoggerFactory.test.js

Can we test it on nested contexts?

mcollina

Could you add a test for the new types?

mcollina · 2023-05-17T09:01:31Z

We use tsd for testing the types

brettwillis · 2023-05-18T21:32:23Z

Added the type check in the last commit

mcollina

lgtm

brettwillis · 2023-05-19T08:10:50Z

If we want to add fastify.setChildLoggerFactory() for encapsulation, then I need some guidance. Really not sure where to begin so need some hints. Do I just add the method to the root fastify object returned from the constructor?

lib/fourOhFour.js

metcoder95 · 2023-05-19T08:34:31Z

If we want to add fastify.setChildLoggerFactory() for encapsulation, then I need some guidance. Really not sure where to begin so need some hints. Do I just add the method to the root fastify object returned from the constructor?

To achieve this we need to do the following:

We will need to define a default childLoggerFactory fn when instantiating fastify. We need hide the child logger fn behind a symbol.
We need to define it within the Context object. It should use the one attached to fastify at the previous step using the symbol.
The fastify.setChildLoggerFactory should modify the default child logger fn hidden with the symbol, so the context object can reference it within nested encapsulations. e.g. setSerializerCompiler

Let me know if I had something wrong or if any doubt 😄

Update

Sorry, missed the last part 😅
4. On the createChildLogger fn, you'll need to access the context for the child logger fn to create the new logger. Of course, using the symbol from step 1

brettwillis · 2023-05-25T23:20:33Z

Ok I added encapsulation per-plugin and per-route, like errorHandler etc. And it looks like I've done it right because I added tests similar to errorHandler and it seems to be working. Let me know your thoughts.

My only note is: configuration options like errorHandler which have a setErrorHandler() funciton on the fastify instance don't currently also have a constructor option. However for the child logger factory we have both the childLoggerFactory constructor option, and the setChildLoggerFactory option. Do we remove the constructor option now or keep both?

metcoder95

Do we remove the constructor option now or keep both?

You're right. I think that only having the one through the method is enough, but happy to hear other thoughts 🙂

lib/route.js

test/encapsulated-child-logger-factory.test.js

types/instance.d.ts

lib/fourOhFour.js

types/instance.d.ts

test/encapsulated-child-logger-factory.test.js

lib/fourOhFour.js

fastify.js

lib/route.js

test/encapsulated-child-logger-factory.test.js

brettwillis · 2023-06-19T21:07:38Z

So sorry guys, I posted a whole bunch of comments several weeks ago and didn't realise I had to submit the review for them to appear 🤦🏼

Here is what is pending review

@metcoder95 a need your clarification here (feat: add childLoggerFactory config #4760 (comment))
@metcoder95 I implemented your suggestion (feat: add childLoggerFactory config #4760 (comment)) but it had a couple of implications (feat: add childLoggerFactory config #4760 (comment), feat: add childLoggerFactory config #4760 (comment)) which seem benign but need you to confirm.
Need consensus on whether, now that we have added instance method setChildLoggerFactory, we should remove the childLoggerFactory constructor option or not.

metcoder95 · 2023-06-21T08:18:40Z

So sorry guys, I posted a whole bunch of comments several weeks ago and didn't realise I had to submit the review for them to appear 🤦🏼

Here is what is pending review

@metcoder95 a need your clarification here (Add childLoggerFactory config #4760 (comment))

Done ✅

@metcoder95 I implemented your suggestion (Add childLoggerFactory config #4760 (comment)) but it had a couple of implications (Add childLoggerFactory config #4760 (comment), Add childLoggerFactory config #4760 (comment)) which seem benign but need you to confirm.

✅ from my side

Need consensus on whether, now that we have added instance method setChildLoggerFactory, we should remove the childLoggerFactory constructor option or not.

I'm +1 on removing the one from the constructor and stick to the method from the instance. In that manner works as we have for setNotFoundHandler and others

brettwillis · 2023-06-27T22:59:53Z

I'm +1 on removing the one from the constructor and stick to the method from the instance. In that manner works as we have for setNotFoundHandler and others

Ok I removed the constructor option.

brettwillis · 2023-06-27T23:04:46Z

test/inject.test.js

Ok so it seems like the changes I made to onBadUrlContext (making it a proper context) has made some (not all?) of the recent #4826 obsolete.

It seems part of that PR was to fix parts of the context being undefined because it wasn't a proper context, now it is a proper context.

I don't think it's an issue. Just pointing it out. I needed to remove that test that expects routeConfig to be undefined as it is no longer undefined.

metcoder95

Overall, LGTM 👍
Just about some left over and a conflict to be solved, and ready to go

test/inject.test.js

docs/Reference/Routes.md

Co-authored-by: James Sumners <321201+jsumners@users.noreply.github.com>

metcoder95 · 2023-07-05T09:19:11Z

Linter seems failing 🙁

brettwillis · 2023-07-05T10:23:37Z

Linter seems failing 🙁

Seems to be on files I didn't touch, but another PR #4879 . Do you want me to fix it here or you do somewhere else?

metcoder95 · 2023-07-06T08:04:44Z

Seems to be on files I didn't touch, but another PR #4879 . Do you want me to fix it here or you do somewhere else?

Ha, interesting. Didn't know how it passed the checks, let's open a new PR for it and rebase this branch. Opened #4882.

metcoder95 · 2023-07-11T08:02:27Z

Are we good to merge it?

cc: @mcollina

brettwillis · 2023-07-11T08:33:16Z

Thanks team and especially @metcoder95 for your support on this one 🙂

github-actions · 2024-07-12T00:23:35Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

Add childLoggerFactory and tests, docs

b12ba69

metcoder95 reviewed May 17, 2023

View reviewed changes

mcollina requested changes May 17, 2023

View reviewed changes

Bind childLoggerFactory to Fastify instance

558d1a8

mcollina approved these changes May 19, 2023

View reviewed changes

metcoder95 reviewed May 19, 2023

View reviewed changes

lib/fourOhFour.js Outdated Show resolved Hide resolved

lib/fourOhFour.js Outdated Show resolved Hide resolved

Eomm added the semver-minor Issue or PR that should land as semver minor label May 25, 2023

Logger factory encapsulated for plugins and routes

4d5f50e

metcoder95 reviewed May 26, 2023

View reviewed changes

lib/route.js Outdated Show resolved Hide resolved

test/encapsulated-child-logger-factory.test.js Show resolved Hide resolved

types/instance.d.ts Show resolved Hide resolved

brettwillis and others added 4 commits May 27, 2023 13:07

Separate FastifyChildLoggerFactory type

42194b5

Rationalise createChildLogger params

ad413d6

Merge branch 'main' into child-logger-hook

0810425

Merge branch 'main' into child-logger-hook

2e569e4

brettwillis commented Jun 19, 2023

View reviewed changes

Merge branch 'main' into child-logger-hook

6fd67a4

Remove childLoggerFactory constructor option

ca1004c

brettwillis commented Jun 27, 2023

View reviewed changes

Fdawgs changed the title ~~Add childLoggerFactory config~~ feat: add childLoggerFactory config Jul 2, 2023

Merge branch 'main' into child-logger-hook

e291dab

metcoder95 reviewed Jul 3, 2023

View reviewed changes

test/inject.test.js Outdated Show resolved Hide resolved

jsumners reviewed Jul 3, 2023

View reviewed changes

docs/Reference/Routes.md Outdated Show resolved Hide resolved

brettwillis and others added 3 commits July 4, 2023 21:48

Update docs/Reference/Routes.md

356af49

Co-authored-by: James Sumners <321201+jsumners@users.noreply.github.com>

Merge branch 'main' into child-logger-hook

81e2251

Remove obsolete test

3d23360

Merge branch 'main' into child-logger-hook

3b065e3

Merge branch 'main' into child-logger-hook

583ce1f

metcoder95 approved these changes Jul 6, 2023

View reviewed changes

Merge branch 'main' into child-logger-hook

1c3578d

mcollina merged commit 66bad3e into fastify:main Jul 11, 2023

brettwillis deleted the child-logger-hook branch July 11, 2023 08:32

pavelkornev mentioned this pull request Aug 8, 2023

TypeScript issue when passing Pino instance to fastify constructor #4960

Open

2 tasks

github-actions bot locked as resolved and limited conversation to collaborators Jul 12, 2024

Uh oh!

Conversation

brettwillis commented May 17, 2023

Checklist

Uh oh!

brettwillis commented May 17, 2023

Uh oh!

metcoder95 commented May 17, 2023

Uh oh!

Uh oh!

metcoder95 May 17, 2023

Choose a reason for hiding this comment

Uh oh!

metcoder95 May 17, 2023

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented May 17, 2023

Uh oh!

brettwillis commented May 18, 2023

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

brettwillis commented May 19, 2023

Uh oh!

Uh oh!

Uh oh!

metcoder95 commented May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brettwillis commented May 25, 2023

Uh oh!

metcoder95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brettwillis commented Jun 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

metcoder95 commented Jun 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brettwillis commented Jun 27, 2023

Uh oh!

brettwillis Jun 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brettwillis Jun 27, 2023

Choose a reason for hiding this comment

Uh oh!

metcoder95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

metcoder95 commented Jul 5, 2023

Uh oh!

brettwillis commented Jul 5, 2023

Uh oh!

metcoder95 commented Jul 6, 2023

Uh oh!

metcoder95 commented Jul 11, 2023

Uh oh!

brettwillis commented Jul 11, 2023

Uh oh!

github-actions bot commented Jul 12, 2024

Uh oh!

metcoder95 commented May 19, 2023 •

edited

Loading

brettwillis commented Jun 19, 2023 •

edited

Loading

metcoder95 commented Jun 21, 2023 •

edited

Loading

brettwillis Jun 27, 2023 •

edited

Loading