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: move to winston logger #67

Merged
merged 40 commits into from
Dec 16, 2020
Merged

feat: move to winston logger #67

merged 40 commits into from
Dec 16, 2020

Conversation

ahochsteger
Copy link
Collaborator

@ahochsteger ahochsteger commented Dec 11, 2020

This PR unifies the logging across zwavejs2mqtt using winston.

Usage:

image

// Simple logger with module name and default config:
const logger = require('./logger.js').module('Zwave')

// Logger setup with module name and specific config:
const logger = require('./logger.js').module('Zwave').setup(
  { enabled: true, level: 'info', logToFile: true, filename: "zwavejs2mqtt.log" }
)

// Do some logging:
logger.info(`Some log message with a ${value}`)
logger.warn('Some warning')

// Reconfigure an existing logger:
logger.setup({ enabled: true, level: 'info', logToFile: true, filename: "zwavejs2mqtt.log" })

// Reconfigure loggers for all modules (e.g. after config changes in the UI):
const loggers = require('./logger.js')
loggers.setupAll({ enabled: true, level: 'info', logToFile: true, filename: "zwavejs2mqtt.log" })

Conditions & Requirements:

  • The logging solution should be as simple as possible while supporting the other requirements
  • Winston should be used the basis for unified logging
  • All loggers should be defined on a central place (logger.js)
  • The primary logging format should be: <timestamp> <level> <moduleName/label>: <logMessage> (e.g. 2020-12-04 09:06:32.568 INFO APP: Zwave2MQTT Version 0.0.0)
  • All loggers should use the same transports
  • All loggers should use the same format by default
  • Loggers for different modules may use a different logging configuration
  • Different log levels should be used to signal event severity (see https://github.com/winstonjs/winston#logging-levels)
  • Modules/labels should be as short as possible. The following should be used: ZWAVE MQTT GATEWAY STORE HASS HTTP APP
  • Log statements with multiple arguments using commas should be converted to the template syntax
  • The logging must be changeable during runtime from the UI
  • It should be able to change the configuration of all loggers at once (to be used by UI for central logging config changes)
  • Runtime changes to the logging settings must be persisted to settings.js
  • The logging must be controlled by settings.js using the existing options logLevel and logToFile
  • Add new setting logEnabled to settings.js and UI

Required changes to the existing codebase:

The following files are affected and show the existing logging implementation that should be migrated to the new logging module:

  • lib/ZwaveClient.js (uses debug) --> winston ZWAVE module
  • app.js (uses debug & morgan) ---> winston HTTP and APP modules
  • lib/jsonStore.js (uses debug) ---> winston STORE module (need to think about this, we could use console otherwise)
  • lib/Gateway.js (uses debug) ---> winston GATEWAY and (maybe) HASS modules
  • lib/MqttClient.js (uses debug) --> winston MQTT module
  • lib/SocketManager.js (uses debug) --> winston SOCKET module
  • lib/debug.js (uses debug) ---> Obsolete, replaced with logger.js

@coveralls
Copy link

coveralls commented Dec 11, 2020

Pull Request Test Coverage Report for Build 426057167

  • 100 of 232 (43.1%) changed or added relevant lines in 7 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.7%) to 28.523%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/SocketManager.js 3 4 75.0%
lib/logger.js 90 94 95.74%
lib/MqttClient.js 0 14 0.0%
lib/Gateway.js 5 23 21.74%
app.js 0 29 0.0%
lib/ZwaveClient.js 0 66 0.0%
Files with Coverage Reduction New Missed Lines %
lib/Gateway.js 2 19.93%
Totals Coverage Status
Change from base Build 422618431: 0.7%
Covered Lines: 1931
Relevant Lines: 6914

💛 - Coveralls

* Use setup() instead of module()
* Allow config to be passed into setup()
* Add `enable` config setting
* Add `level` config setting
* Add `logTofile` config setting
* Add `filename` config setting
* Adjust formatting to requirements
lib/ZwaveClient.js Outdated Show resolved Hide resolved
@ahochsteger ahochsteger changed the title [WIP] Unified winston-based logging module Unified winston-based logging module Dec 11, 2020
@ahochsteger ahochsteger marked this pull request as ready for review December 11, 2020 22:24
@ahochsteger
Copy link
Collaborator Author

@robertsLando the previous debug-based logging is now fully replaced with the new winston-based logging module.

The only thing that is intentionally left is integration into the UI and control using settings.json which requires some discussion about the best approach to propagate logging config changes to the module loggers.

* Add automated tests for all functionality
* Add setupAll() to change config for loggers of all modules
* Code cleanup
@ahochsteger
Copy link
Collaborator Author

@robertsLando after some more work I was able to resolve all open questions and handle the logging configuration in app.js (see setupLogging()).
Changes in the UI now automatically update the logger of all modules.

But there's a side-effect I observed I was not yet able to solve yet:
After changing the logging settings in the UI and restarting the server there are no more logs available from node-zwave-js. Maybe there's a conflict somewhere? At least I tried to not touch other Winston loggers as much as possible (e.g. by prefixing logger names with "zwavejs2mqtt." and only handle these in setupAll()).

So from my POV the PR is ready for review.

@robertsLando
Copy link
Member

After changing the logging settings in the UI and restarting the server there are no more logs available from node-zwave-js. Maybe there's a conflict somewhere

@AlCalzone Do you know if this could cause some conflict with your loggers?

lib/Gateway.js Outdated Show resolved Hide resolved
@robertsLando
Copy link
Member

@ahochsteger Review done. Feel free to squash and merge if you are done here 😄

src/components/Settings.vue Outdated Show resolved Hide resolved
src/components/Settings.vue Outdated Show resolved Hide resolved
src/components/Settings.vue Outdated Show resolved Hide resolved
src/components/Settings.vue Outdated Show resolved Hide resolved
Copy link
Member

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

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

@ahochsteger I would like a cleaner output format. The color should be applied only to level and the timestamp could be gray. It is much pretty to read

@robertsLando
Copy link
Member

I also think you have missed stderrLevels: ['error'] in console transport. If user is using pm2 that will redirect errors to stderror

@robertsLando
Copy link
Member

robertsLando commented Dec 15, 2020

Output now:

Schermata da 2020-12-15 15-01-43

lib/logger.js Outdated Show resolved Hide resolved
src/components/Settings.vue Outdated Show resolved Hide resolved
Copy link
Member

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

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

LGTM

@robertsLando robertsLando merged commit 023fccf into master Dec 16, 2020
@robertsLando robertsLando deleted the winston-logging branch December 16, 2020 16:35
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.

4 participants