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

Can't provide custom logger and configure log level without warnings #1040

Closed
4 of 10 tasks
dominics opened this issue Aug 6, 2021 · 1 comment · Fixed by #1078
Closed
4 of 10 tasks

Can't provide custom logger and configure log level without warnings #1040

dominics opened this issue Aug 6, 2021 · 1 comment · Fixed by #1078
Assignees
Labels
bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented good first issue Good for newcomers
Milestone

Comments

@dominics
Copy link

dominics commented Aug 6, 2021

Description

As someone with an existing logger, I want to be able to be able to use Bolt to log with my existing Logger instance, and also control the log level.

Assuming I have implemented the Logger interface (fully, including setLevel()/getLevel()), and I'm passing my implementation into the App as the logger option, then there is no way I can see to set the initial log level on the custom logger in this situation:

  1. The most obvious thing is to try to pass the logLevel option to the App, as well. This should work, but gives a big warning:
    "As `logger` is passed as well, `logLevel` argument won't be used. Set the log level to the `logger` instance instead.",
  2. Unfortunately, the warning is misleading: if we take the warning's advice and try to set the log level on the logger instance before passing it to the App options, it will have no effect! That's because App will immediately call setLevel on whatever logger you passed it, here, overwriting whatever value it had:
    this.logger.setLevel(this.logLevel);
    • In effect, the value is hard-coded here (and the other branch of that if) unless you specify logLevel!:
      this.logLevel = logLevel ?? LogLevel.INFO;

At the moment, this means passing logLevel and logger provides the correct behavior, but gives several warnings. On the other hand, passing just logger means you'd have to wait until the App is initialized, then re-call setLevel on it yourself, to rewrite what the App just did to it on initialization...

I think it's most likely that the warning given in (1) is incorrect, and can be removed.

What type of issue is this? (place an x in one of the [ ])

  • bug
  • enhancement (feature request)
  • question
  • documentation related
  • example code related
  • testing related
  • discussion

Requirements (place an x in each of the [ ])

  • I've read and understood the Contributing guidelines and have done my best effort to follow them.
  • I've read and agree to the Code of Conduct.
  • I've searched for any related issues and avoided creating a duplicate issue.
@gitwave gitwave bot added the untriaged label Aug 6, 2021
@seratch seratch added bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented and removed untriaged labels Aug 6, 2021
@seratch seratch added this to the 3.6.0 milestone Aug 6, 2021
@seratch seratch added the good first issue Good for newcomers label Aug 6, 2021
@seratch
Copy link
Member

seratch commented Aug 6, 2021

Hi @dominics, thanks for taking the time to write this detailed report!

I think it's most likely that the warning given in (1) is incorrect, and can be removed.

You are right about the logging. We can safely remove the warning-level logging from the constructor.

We'll fix this issue in the next release. If you're happy to use your time a little more, your pull requests would be greatly appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants