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

General Logger enhancements #369

Open
zehnm opened this issue Dec 23, 2019 · 5 comments
Open

General Logger enhancements #369

zehnm opened this issue Dec 23, 2019 · 5 comments

Comments

@zehnm
Copy link
Member

@zehnm zehnm commented Dec 23, 2019

I'm opening this issue to discuss and plan general enhancements for the Logger component.

Please add comments, further ideas or objections so we can specify all needed enhancements for the next release.
My suggestions and requirements are the following:

Expected Behavior or Design

  • Configurable filename
    The filename should be configurable, at least with a client settable prefix. Default: TBD.
    Reasons:
    • On the YIO remote the log files will be configured to be stored in /var/log.
      They should have a prefix to easily distinguish them from other log files.
    • A prefix makes sure that the included file purger doesn't delete other files.
  • Rolling file appender
    The hard coded 1 logfile per hour in Logger::writeWile needs to be configurable. Usually a rolling file logger is rolled over at midnight or after a certain file size has been reached. In my opinion one log file per hour is too much and the default of deleting files after 12 hours way too short.
  • We need to discuss on how to handle file deletion.
    On Linux platforms I'd prefer to delete / archive them with default concepts, e.g. "logrotate". For other platforms we can still do it in the app. Therefore this needs to be configurable.
    • purgeHours == 0 disables purging.
    • Log deletion must be continuous (e.g. once a day). At the moment this is only done at Logger creation. This doesn't work for long uptimes and can create hundreds of files!
  • Logger should be a pure utility class without dependencies to other YIO classes.
    The Config dependency is already removed (see additional information below) but there's still a dependency to PluginInterface.
    • Q: Why is this dependency required?
      In my opinion the registration should be handled outside of the logger: either the plugin loader registers the logging categories or we create a generic configuration option. Maybe I don't understand the concept but to me log categories are usually plain and simple in logging frameworks: each category has a configured log level. No need for callbacks into integration plugins. Example:

      logging: {
          categories: {
              Spotify: DEBUG,
              WifiCtrl: WARN,
              WpaCtrl: INFO,
              homeAssistant: CRIT
          }
      }
      

      The logging categories are default Q_LOGGING_CATEGORY and defined by the individual class(es). E.g. in wifi_wpasupplicant.cpp:

      static Q_LOGGING_CATEGORY(CLASS_LC, "WpaCtrl");
      
      qCWarning(CLASS_LC) << "Failed to establish connection to AP"
      

      Or is this not possible with the Logger? I.e. a category must be registered explicitly with the Logger and default Qt logging doesn't work?

Possible Solution

The following a logging configuration section has already been added to config.json to configure the Logger:

    "settings": 
    {
        "language": "en_US",
        "autobrightness": true,
        "proximity": 40,
        "shutdowntime": 7200,
        "softwareupdate": false,
        "wifitime": 240,
        "bluetootharea": false,
        "paired_dock": "YIO-Dock-30AEA4D73C34",
        "logging": {
            "path": "/var/log",
            "level": "INFO",
            "console": true,
            "showSource": true,
            "queueSize": 100,
            "purgeHours": 72
        }
    },

Logger initialization in main.cpp:

    QMap<QString, QVariant> logCfg = config.getSettings().value("logging").toMap();
    Logger logger(logCfg.value("path", appPath + "/log").toString(),
                  logCfg.value("level", "WARN").toString(),
                  logCfg.value("console", true).toBool(),
                  logCfg.value("showSource", true).toBool(),
                  logCfg.value("queueSize", 100).toInt(),
                  logCfg.value("purgeHours", 72).toInt());

Detailed Description and Additional Information

The following changes to the Logger component have already been done and commited to the develop branch:

  • Renamed parameter debug to console which enables / disables console logging.
    This was confusing and has nothing to do with debug in general. The console output is just another log destination. Note: on the remote the console log ends up in journald.

  • Removed Config singleton dependency.
    The Logger class should not depend on the Config class singleton. There's no need for the specific Config class and it makes it harder to test (Yes, unit tests are urgently needed for the whole project and will be introduced soon) and reuse.

    • The single "log" parameter which was read from Config is now the changed logLevel parameter. All other options were already constructor parameters or are available as setters. It's better to handle all configuration parameters the same way.
    • Future option: ONLY in case there will be many more parameters:
      Provide a generic const QVariantMap &config parameter containing all logging related configuration options from Config. (Same concept as in HardwareFactory::build).

@martonborzak @nklerk @ChristianRiedl please review & comment.

@zehnm zehnm added the enhancement label Dec 23, 2019
zehnm added a commit that referenced this issue Dec 23, 2019
- Removed Config dependency
- Added configuration loading from config.json
- Renamed debug property to console
@zehnm zehnm added this to Backlog in YIO Remote software via automation Dec 23, 2019
@zehnm zehnm added this to the Release 0.3 milestone Dec 23, 2019
@zehnm zehnm moved this from Backlog to Accepted in YIO Remote software Dec 23, 2019
@zehnm zehnm changed the title [DRAFT] General Logger enhancements General Logger enhancements Dec 23, 2019
@zehnm zehnm moved this from Accepted to Design in YIO Remote software Dec 23, 2019
@ChristianRiedl

This comment has been minimized.

Copy link
Member

@ChristianRiedl ChristianRiedl commented Dec 23, 2019

Here my comments :

  • Configurable filename : OK
  • Rolling file behavior :
    Configurable rolling time interval + maximum size, OK
    There is a allready a purgefile function intended for cyclical purging, which can be called cyclically. Who will call it ? Maybe there is somewhere a general daily cleanup function, or I can create an internal timer.
  • Plugin interface dependence
    Logging level of all registered categories are controlled per category by Logger class (and YioApi).
    Finally the logging will be shown in the web_configurator and it is very useful to adjust the loglevels per category at runtime.
    This is the reason why Logger holds the references of the QLoggingCategory of all registered log "clients" ( defineLogCategory (const QString& category, .... Setting the loglevel for a category is not straight forward because the QMsgType is not ordered as expected, and I wanted to have this logic only on one place. For the integrations DLLs I did not want to keep such references to objects inside the dll. For this reason the PluginInterface has a method setLogEnabled to set the log level for the plugin.
    When you declare the logging category with Q_LOGGING_CATEGORY (name, "xxx") you get a const reference and cannot call setEnabled and have to use the filter mechanism which is not well suited for our environment I think. When you declare the logging category QLoggingCategory name ("xxx") you can use it with the qCDebug ... in the same way and you setEnabledcan be used. This is the reason why I went this way. And I was not sure if "logging category" registered by Qt inside a DLL can be controlled from outside.
    And my Logger class also gives an overview about all logger clients including per level counters. I did not find a Qt function to get all logger categories. This is the reason why logger clients in my concept register their category in Logger class. Integrations register their category implicitely with the PluginInterface. I do not see a real disadvantage doing it this way.

The changes you have made are OK for me I also dislike singletons.

zehnm added a commit that referenced this issue Dec 23, 2019
Use application_path/log as default if not configured.
Changed log path logic:
- key not set or "." => <application_path>/log
- "" (empty)         => no log file
@zehnm

This comment has been minimized.

Copy link
Member Author

@zehnm zehnm commented Dec 24, 2019

For the rolling file appender we can limit it to one simple option. It would be nice to have everything that e.g. log4j in the Java world provides, but we are not writing a generic logger library.

@martonborzak @nklerk what do you prefer? Size based - rotate after configurable amount, e.g. 10MB - or a fixed rotation at midnight?

In regards to setting the log level within integration plugins I first have to dig into how Qt logging works and understand the described issue in the last comment with QLoggingCategory.

@martonborzak

This comment has been minimized.

Copy link
Member

@martonborzak martonborzak commented Dec 24, 2019

@carp3-noctem

This comment has been minimized.

Copy link
Member

@carp3-noctem carp3-noctem commented Jan 13, 2020

Remove Split of Files at every hour

@carp3-noctem

This comment has been minimized.

Copy link
Member

@carp3-noctem carp3-noctem commented Jan 13, 2020

Add Manual Disconnect of integrations to logger (no logline is created at the moment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.