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

feat(common): modify Logger to allow setting a prefix #13913

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jochongs
Copy link

@jochongs jochongs commented Aug 26, 2024

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

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

What is the current behavior?

Issue Number: #13841

What is the new behavior?

Hello,

I became interested in this issue, so I made some methods.

As suggested in the issue (app.setLoggerPrefix('my-app-name')), I created a setLoggerPrefix method.

async function bootstrap() {
  const app = await NestFactory.create(AppModule);

  app.setLoggerPrefix('app-name');

  await app.listen(3000);
}

I expected it to be used in this way.

However, I noticed that logging also occurs when NestFactory.create is called. The prefix set with app.setLoggerPrefix does not apply to these log messages because NestFactory.create is called before app.setLoggerPrefix.

To address this, I added a loggerPrefix field to NestApplicationContextOptions, and now you can use it like this:

async function bootstrap() {
  const app = await NestFactory.create(AppModule, { loggerPrefix: 'app-name' });

  await app.listen(3000);
}

What are your thoughts on this approach? I'd appreciate your feedback.

image

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@jochongs jochongs changed the title Issue/13841 feat(common): add logging prefix Aug 26, 2024
@jochongs jochongs changed the title feat(common): add logging prefix feat(common): modify Logger to allow setting a logging prefix Aug 26, 2024
@jochongs jochongs changed the title feat(common): modify Logger to allow setting a logging prefix feat(common): modify Logger to allow setting a logger prefix Aug 26, 2024
@jochongs jochongs changed the title feat(common): modify Logger to allow setting a logger prefix feat(common): modify Logger to allow setting a prefix Aug 26, 2024
@coveralls
Copy link

coveralls commented Aug 26, 2024

Pull Request Test Coverage Report for Build 91c61ea8-273a-4e06-9317-921e6278b09a

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 92.213%

Totals Coverage Status
Change from base Build 7dd97924-96eb-4863-bad1-427333102233: 0.0%
Covered Lines: 6750
Relevant Lines: 7320

💛 - Coveralls

@micalevisk
Copy link
Member

micalevisk commented Sep 1, 2024

If I'm not wrong, that ConsoleLogger is public (ie., can be used by other devs to change logger formatting etc as per https://docs.nestjs.com/techniques/logger#extend-built-in-logger). And so adding a new mandatory method to it implies in a adding a breaking change. Kamil can correct me.


Also, just a minor opinion about this approach: to me, adding setLoggerPrefix to INestApplication interface is a bit confusing as we can use it alongside app.useLogger because we will change the same thing (the logger) in two steps. And this leaves room for adding new methods that act over the logger again, making things even worse in the future.

We can define that prefix and even change the whole presentation already like this:

class MyLogger extends ConsoleLogger {
  formatPid(pid: number) {
    return `[Foo] ${pid} - `
  }
}

// ...

app.useLogger(new MyLogger())

@jochongs
Copy link
Author

jochongs commented Sep 3, 2024

@micalevisk

Hello, thank you so much for your feedback.

I overlooked the useLogger method. Adding a method to INestApplication that allows modifying the prefix could cause confusion with useLogger.

So, I tried a simpler approach by utilizing environment variables. What do you think about this method?

#13940

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