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.

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

Improve docs about using Logger #1441

Merged
merged 10 commits into from
Nov 5, 2020

Conversation

mayorandrew
Copy link
Contributor

@mayorandrew mayorandrew commented Aug 29, 2020

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] Other... Please describe:

What is the current behavior?

I was trying to integrate nestjs-pino (json logger) in my app in the best possible way, and struggled a little bit since nestjs docs don't quite clearly explain how to best use custom logger in your app services.

The example with transient scopes seems strange to me because why would I inject custom logger to my services if I am already setting it as globally default using app.useLogger. If I am switching loggers, I would have to do search and replace across all application. Also, I've noticed that when I use default Logger from @nestjs/common it actually uses the custom logger from app.useLogger. This was not clear to me, so I started digging.

I found this amazing Stack Overflow answer https://stackoverflow.com/a/52907695/4601673 but didn't quite get how Nest can use custom logger from app.useLogger if I am instantiating Logger from @nestjs/common in my class.

So I digged through the source. If we look how app.useLogger is implemented (https://github.com/nestjs/nest/blob/67e15afbf33364ac6dcf0ade58d26aade7c2eb1f/packages/core/nest-application-context.ts#L123), it actually calls static method Logger.overrideLogger of the Logger from @nestjs/common which changes the static property Logger.instance (https://github.com/nestjs/nest/blob/01dfe29458a2851e42efe8d5b4d614caf9935742/packages/common/services/logger.service.ts#L70).

Now, if we look, how Logger itself is implemented, whenever you call instance methods like log/warn, it actually calls the related method from the saved Logger.instance (https://github.com/nestjs/nest/blob/01dfe29458a2851e42efe8d5b4d614caf9935742/packages/common/services/logger.service.ts#L108).

Basically, by calling app.useLogger we set the logger which will be used through the default Logger instances from @nestjs/common. So there is no need to inject custom logger if we only use methods from LoggerService interface.

If we dig more into nestjs source code, the logger is actually used the same way, as provided in the SO anwser linked above, so I think that is how it should be. As I see it, nestjs itself and possible third-party libraries would just use Logger from @nestjs/common and assume its implementation agnosticity. And application developer could substitute any custom logger they want via app.useLogger.

Here are some examples of new Logger usage from nestjs repo: https://github.com/nestjs/nest/blob/fc05b244a8a0db15ef60e612c9e2afe0290f995a/packages/core/errors/exception-handler.ts#L5, https://github.com/nestjs/nest/blob/eedd9df6e42aa035d14c2e861f401269d26d4d4f/sample/27-scheduling/src/tasks/tasks.service.ts#L6

Also I have found that passing logger: false to the NestFactory.create can swallow some important initialization errors which are logged before app.useLogger. This was really not obvious from the docs.

What is the new behavior?

I have created a separate section in the docs, explaining how the logger instantiation works when custom logger is used:

import { Logger, Injectable } from '@nestjs/common';

@Injectable()
class MyService {
  logger = new Logger(MyService.name);
  
  doSomething() {
    // Actually uses MyLogger
    this.logger.log('Doing something...');
  }
}
const app = await NestFactory.create(ApplicationModule);
app.useLogger(app.get(MyLogger));
await app.listen(3000);

I have repurposed existing section about injecting custom logger with transient scope into some kind of an advanced example where user may want to use custom logging methods which are not provided by the LoggerService interface.

I have also added explicit warning about using logger: false, maybe it will save someone hours of debugging.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

I have preserved the original section name so links should not be broken.

Other information

Since I am not a native english speaker, there may be some stylistic errors in my descriptions. Please tell me about them so I can improve.

In short, I want to promote the best practice of using new Logger since it fells more consistent than just injecting your logger into your own services. For example, if all library authors use this pattern, it would be easy for me as an app developer to customize logger behavior consistently across all application.

Copy link
Member

@jmcdo29 jmcdo29 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update to the docs. I've added a few comments. Overall, looks like a pretty good addition 😸

content/techniques/logger.md Outdated Show resolved Hide resolved
content/techniques/logger.md Show resolved Hide resolved
Copy link
Member

@jmcdo29 jmcdo29 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johnbiundo if you could take a look as well, but this LGTM.

@kamilmysliwiec kamilmysliwiec merged commit 7f430cc into nestjs:master Nov 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants