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

Added loging level through mqtt #110

Merged
merged 14 commits into from
Jun 14, 2018
Merged

Added loging level through mqtt #110

merged 14 commits into from
Jun 14, 2018

Conversation

ciotlosm
Copy link
Contributor

@ciotlosm ciotlosm commented Jun 11, 2018

Added loging level through mqtt

Just logging entity (not libraries) and no documentation added in this PR. Couldn't figure out how to print nicely log level as didn't want to have it as "critical" the information that you're switching log level.

Implements #81 parially

  • Allow setting log level through mqtt
  • Allow setting log level through configuration
  • Allow setting log level for dependencies (where possible, like Configure logging #81 (comment))
  • Force debug on start-up for logger when received DEBUG env variable
  • Documentation on both mqtt and configuration for log_level

} else if (option === 'log_level') {
const level = message.toString().toLowerCase();
if (allowedLogLevels.includes(level)) {
logger.warn(`Switching log level to '${level}'`);
Copy link
Owner

Choose a reason for hiding this comment

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

This is not a warning, I think logger.info fits better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you set this to info, but you set log level to warn you miss the message that log level has changed.
If it is acceptable happy to change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

logger.transports.console.level = level;
logger.transports.file.level = level;
} else {
logger.error(`Could not set log level to '${level}'`);
Copy link
Owner

@Koenkk Koenkk Jun 11, 2018

Choose a reason for hiding this comment

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

Would be nice to log the possible values. e.g.

logger.error(`Could not set log level to '${level}', allowed levels are ${allowedLogLevels}`);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

@ciotlosm
Copy link
Contributor Author

It seems that even if https://github.com/visionmedia/debug is capable of dynamic enable/disable, it's not possible to do it across all because dependencies have debug pinned to old version without support :(

I will try a few more workarounds but I doubt I will be in luck

@Koenkk
Copy link
Owner

Koenkk commented Jun 12, 2018

Is this needed for Allow setting log level for dependencies (where possible, like #81 (comment))? I think this is a 'nice to have'.

@ciotlosm
Copy link
Contributor Author

I'll move debug messages in zigbee2mqtt to logger and let the rest use visionmedia/debug. Any reason for using debug here?

@Koenkk
Copy link
Owner

Koenkk commented Jun 12, 2018

@ciotlosm all debug within zigbee2mqtt can be replaced by logger.debug

@ciotlosm
Copy link
Contributor Author

Will look to move debug messages for dependencies into a separate PR if I find a workaround.

@ciotlosm
Copy link
Contributor Author

ciotlosm commented Jun 14, 2018

@Koenkk Updated also documentation. Can you please have a look?
The only use case I haven't covered yet it's to set DEBUG to * when receiving 'debug' from configuration.yaml. Not sure if it's possible, but will give it a try.

Want me to squash messages into a single commit?

Koenkk added a commit that referenced this pull request Jun 14, 2018
@ciotlosm
Copy link
Contributor Author

Did a rebase on top of latest master

@Koenkk
Copy link
Owner

Koenkk commented Jun 14, 2018

@Koenkk Koenkk merged commit 3591f4f into Koenkk:master Jun 14, 2018
@Koenkk
Copy link
Owner

Koenkk commented Jun 14, 2018

Thanks!

@ciotlosm
Copy link
Contributor Author

Perfect. Should I close the logging issue?

wilmardo pushed a commit to wilmardo/zigbee2mqtt that referenced this pull request Sep 26, 2019
wilmardo pushed a commit to wilmardo/zigbee2mqtt that referenced this pull request Sep 26, 2019
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.

None yet

2 participants