feat: new logger api by aarontravass · Pull Request #5020 · fastify/fastify

aarontravass · 2023-09-04T22:16:46Z

closes #4624

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

aarontravass · 2023-09-04T22:21:21Z

How do I test for precedence? Specifically

If loggerInstance is provided, it takes precedence over { logger: }.

Uzlopak · 2023-09-04T22:34:28Z

precedence:

You define both options with contrary settings and check that the instantiated logger is setup with the options which should take precedence

lib/errors.js

test/serial/logger.1.test.js

…r log instance error

metcoder95

As this is pointing to next, what are your thoughts about enforcing the logger to not valid a loggerInstance?

I believe that we can backport this to main and trigger the deprecation there, meanwhile next can hold the full removal.

docs/Reference/Errors.md

aarontravass · 2023-09-05T20:44:37Z

As this is pointing to next, what are your thoughts about enforcing the logger to not valid a loggerInstance?

I guess we can throw an error if an instance is passed. For main, we can emit a warning instead.

metcoder95 · 2023-09-06T07:47:45Z

SGTM 👍

fastify.d.ts

…ce is passed to logger options

aarontravass · 2023-09-06T19:15:18Z

SGTM 👍

I made the change for this PR. It throws an exception now.

docs/Reference/Errors.md

test/serial/logger.1.test.js

test/types/logger.test-d.ts

metcoder95

LGTM 👍

Uzlopak

LGTM

metcoder95 · 2023-09-10T11:00:49Z

@jsumners would you like to take a look?

jsumners

Looks good to me.

@fastify/core in the original issue there is discussion about throwing versus precedence. What are your opinions on that question?

kibertoad · 2023-09-10T12:07:44Z

@jsumners I prefer throwing, it is less ambiguous

metcoder95 · 2023-09-10T19:25:02Z

I do agree in having a way to inform that both properties has been passed.

An error seems more preemptive to influence take an immediate action. I’ve preference on this option as well.

aarontravass · 2023-09-11T07:30:50Z

If everyone is fine with throwing an error, should I make the change? From: Carlos ***@***.***> Sent: Sunday, September 10, 2023 3:25 PM To: ***@***.***> Cc: ***@***.***>; ***@***.***> Subject: Re: [fastify/fastify] feat: new logger api (PR #5020) I do agree in having a way to inform that both properties has been passed. An error seems more preemptive to influence take an immediate action. I’ve preference on this option as well. — Reply to this email directly, view it on GitHub<#5020 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AKO6F3PCIWOC324EQWC23ODXZYHZTANCNFSM6AAAAAA4K426PA>. You are receiving this because you authored the thread.Message ID: ***@***.***>

Uzlopak · 2023-09-11T07:39:33Z

Yes, an error avoids surprises. Especially if you have some redacting configured to prevent security relevant data to be exposed. If you silently override/precendence, it could result in exposing the information, without noticing it.

So please change it to throwing.

docs/Reference/Errors.md

Uzlopak · 2023-09-12T08:11:58Z

lib/logger.js

  }

+  // if a logger instance is passed to logger, throw an exception
+  if (validateLogger(options.logger)) {


I dont think that this is right. Here we are not checking if an pino configuration is passed, but if logger instance is passed.

But the validateLogger fn checks if a valid logger object is passed. And if the user passes it to logger instea of loggerInstance it should throw an exception.

this is connected with my remark below that we need a pino validation and not a validateLogger call here.

lib/logger.js

Uzlopak · 2023-09-12T08:14:48Z

lib/logger.js

+    throw FST_ERR_LOG_INVALID_LOGGER_CONFIG()
+  }
+
+  if (options.loggerInstance) {


and here we dont use validateLogger to check if the logger has the right shape. i think, the check should be in line 88. first check if loggerInstance is set, if so check with validateLogger if it is a valid instance and do the instantiation and if not throw the Error.

Line 101 basically says that if a logger fn isn't passed to loggerInstance, but loggerInstance still contains some value, we make an assumption that a pino config or some config was passed to loggerInstance even though it was supposed to be passed to logger. Hence, we throw an exception saying loggerInstance only accepts a logger not a configuration object.

Let me repeat what you basically wrote: Something was passed but an loggerInstance. So the Error can not say the second half sentence "not a configuration object".

Ah gotcha. What do you think of A valid logger function was not passed to loggerInstance?

@aarontravass could you breaking into two funcs? validateLoggerInstance and validateLoggerConfig would be clearer and if needed validateLoggerConfig... also spot checked pino there are some errors nested in a tool for normalizing the args/options... not sure if thats best interface for a dev though (I'm assuming devs know its pino but 🤷)

https://github.com/pinojs/pino/blob/master/lib/tools.js#L286

Uzlopak · 2023-09-12T08:16:18Z

lib/errors.js


+  FST_ERR_LOG_INVALID_LOGGER_INSTANCE: createError(
+    'FST_ERR_LOG_INVALID_LOGGER_INSTANCE',
+    'loggerInstance only accepts a logger not a configuration object.',


@Fdawgs
Can you please check the wording.

Also it seems wrong. We dont know if it is a configuration object. If I pass any other Object, e.g. an Error, I should also get this error, despite an Error is not a configuration object.

Uzlopak · 2023-09-12T08:20:25Z

lib/errors.js

+
+  FST_ERR_LOG_INVALID_LOGGER_CONFIG: createError(
+    'FST_ERR_LOG_INVALID_LOGGER_CONFIG',
+    'logger options only accepts a configuration object.',


Maybe pino needs a validate function for configuration objects. So maybe should be checked on pino if there is such a function and if not add it and then use it. Then we can check the passed object AND then it is not "logger options only accepts a configuration object" but then we could say "logger option is not a valid logger configuration"

lib/errors.js

…into aaron/logger

metcoder95 · 2023-09-14T11:03:03Z

Seems CI is failing: https://github.com/fastify/fastify/actions/runs/6172797165/job/16753800498?pr=5020#step:5:15066 🙁

aarontravass · 2023-09-15T14:14:28Z

Seems CI is failing: https://github.com/fastify/fastify/actions/runs/6172797165/job/16753800498?pr=5020#step:5:15066 🙁

It says SIGTERM but it works locally. not sure what's going wrong

lib/errors.js

zrosenbauer

LGTM except a few questions & recommendations

zrosenbauer · 2023-09-18T22:30:54Z

lib/logger.js

+    throw FST_ERR_LOG_INVALID_LOGGER_CONFIG()
+  }
+
+  if (options.loggerInstance) {


@aarontravass could you breaking into two funcs? validateLoggerInstance and validateLoggerConfig would be clearer and if needed validateLoggerConfig... also spot checked pino there are some errors nested in a tool for normalizing the args/options... not sure if thats best interface for a dev though (I'm assuming devs know its pino but 🤷)

https://github.com/pinojs/pino/blob/master/lib/tools.js#L286

fastify.d.ts

docs/Reference/Errors.md

I have no overview about the state of these changes.

Uzlopak · 2023-09-19T14:22:02Z

If you apply changes then please also resolve the conversations. It is not clear which issues are still relevant.

aarontravass · 2023-09-19T18:09:07Z

If you apply changes then please also resolve the conversations. It is not clear which issues are still relevant.

My apologies

leppaott · 2024-10-16T10:33:16Z

docs/Reference/Errors.md

+#### FST_ERR_LOG_INVALID_LOGGER_INSTANCE
+<a id="FST_ERR_LOG_INVALID_LOGGER_INSTANCE"></a>
+
+The loggerInstance only accepts a logger instance, not a configuration object. 


https://fastify.dev/docs/latest/Reference/Logging/
Could have the new loggerInstance?

First, this is over a year old. Open an issue if you have encountered a problem. Second, you have not provided enough information to understand what you are asking. Please provide details in your issue.

feat: new logger api

1ea0719

aarontravass changed the base branch from main to next September 4, 2023 22:22

fix: merged with upstream next

e25160b

jsumners reviewed Sep 5, 2023

View reviewed changes

lib/errors.js Outdated Show resolved Hide resolved

test/serial/logger.1.test.js Show resolved Hide resolved

test: added test to check for precedence and changed error message fo…

ccfac5d

…r log instance error

aarontravass requested a review from jsumners September 5, 2023 15:54

metcoder95 reviewed Sep 5, 2023

View reviewed changes

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

mcollina reviewed Sep 6, 2023

View reviewed changes

fastify.d.ts Outdated Show resolved Hide resolved

fix: removed the warning and instead, an error is thrown if an instan…

3f8c84e

…ce is passed to logger options

aarontravass requested a review from metcoder95 September 6, 2023 19:26

Eomm added semver-major Issue or PR that should land as semver major v5.x Issue or pr related to Fastify v5 labels Sep 7, 2023

Eomm approved these changes Sep 7, 2023

View reviewed changes

Uzlopak reviewed Sep 7, 2023

View reviewed changes

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

metcoder95 reviewed Sep 7, 2023

View reviewed changes

test/serial/logger.1.test.js Show resolved Hide resolved

test/types/logger.test-d.ts Outdated Show resolved Hide resolved

test: added test for logger and addressed comments

264b412

aarontravass requested a review from metcoder95 September 9, 2023 22:02

metcoder95 approved these changes Sep 10, 2023

View reviewed changes

Uzlopak approved these changes Sep 10, 2023

View reviewed changes

jsumners approved these changes Sep 10, 2023

View reviewed changes

aarontravass requested a review from metcoder95 September 11, 2023 21:39

Uzlopak reviewed Sep 11, 2023

View reviewed changes

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

Uzlopak reviewed Sep 11, 2023

View reviewed changes

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

docs: changed error code

36291df

aarontravass requested a review from Uzlopak September 12, 2023 01:32

Uzlopak previously requested changes Sep 12, 2023

View reviewed changes

Uzlopak mentioned this pull request Sep 12, 2023

validateOptions function pinojs/pino#1806

Open

metcoder95 and others added 3 commits September 12, 2023 21:38

Merge branch 'next' into aaron/logger

df43ae8

docs: reverted error code

9390712

Merge branch 'aaron/logger' of https://github.com/aarontravass/fastify …

9afa367

…into aaron/logger

Fdawgs reviewed Sep 15, 2023

View reviewed changes

lib/errors.js Outdated Show resolved Hide resolved

docs: changed wording

7ed186f

zrosenbauer reviewed Sep 18, 2023

View reviewed changes

metcoder95 approved these changes Sep 19, 2023

View reviewed changes

jsumners approved these changes Sep 19, 2023

View reviewed changes

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

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

docs: fixed typos

0b03a23

zrosenbauer approved these changes Sep 19, 2023

View reviewed changes

jsumners merged commit 9c63209 into fastify:next Sep 19, 2023

Eomm mentioned this pull request Oct 18, 2023

New logger API #4624

Closed

aarontravass deleted the aaron/logger branch October 18, 2023 14:55

leppaott reviewed Oct 16, 2024

View reviewed changes

fastify locked as resolved and limited conversation to collaborators Oct 16, 2024

Uh oh!

Conversation

aarontravass commented Sep 4, 2023

Checklist

Uh oh!

aarontravass commented Sep 4, 2023

Uh oh!

Uzlopak commented Sep 4, 2023

Uh oh!

Uh oh!

Uh oh!

metcoder95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aarontravass commented Sep 5, 2023

Uh oh!

metcoder95 commented Sep 6, 2023

Uh oh!

Uh oh!

aarontravass commented Sep 6, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

metcoder95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uzlopak left a comment

Choose a reason for hiding this comment

Uh oh!

metcoder95 commented Sep 10, 2023

Uh oh!

jsumners left a comment

Choose a reason for hiding this comment

Uh oh!

kibertoad commented Sep 10, 2023

Uh oh!

metcoder95 commented Sep 10, 2023

Uh oh!

aarontravass commented Sep 11, 2023 via email

Uh oh!

Uzlopak commented Sep 11, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

metcoder95 commented Sep 14, 2023

Uh oh!

aarontravass commented Sep 15, 2023

Uh oh!

Uh oh!

zrosenbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

leppaott Oct 16, 2024 •

edited

Loading