Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-1180

Initialization for log

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: log
    • Labels:
      None
    • Epic Link:
    • Sprint:
      DB_S14_9
    • Team:
      Data Access and Database

      Description

      Working on DM-1005 I found that initialization of log package causes me a lot of trouble. I hope we can improve that.

      Currently log can be initialized in one of the following ways:

      • calling LOG_CONFIG(file_name) where file name has to be passed somehow to the initialization code, configures loggers according to given config file
      • calling LOG_CONFIG() defines some basic configuration which kind of works but isn't usually what I want
      • not calling any of the above but setting environment variable LOG4CXX_CONFIGURATION to a config file name, which is equivalent to first option (this may not be intended use case but it works now)
      • if I don't do any of the above then log4cxx prints error messages and disables logging completely

      Our use cases for log may be non-conventional but I have to implement them anyways. There are basically two cases where I have troubles:

      • plugins for xrootd (implemented as libraries), these do not have a simple way to pass configuration file name except through environment variable, I could use LOG4CXX_CONFIGURATION for that (without calling LOG_CONFIG() but I also want to produce reasonable output when I forget to set LOG4CXX_CONFIGURATION. Another complication is that there may be more than one plugin loaded at the same time, deciding which one has to initialize logging is complicated.
      • a (potentially large) number of unit tests. We could add LOG_CONFIG() to each one of them but I'd like to avoid that for a number of reasons.

      So I think what I'd like to have is an option to not call LOG_CONFIG() which would still produce reasonable output even without LOG4CXX_CONFIGURATION (in other words make call to LOG_CONFIG() implicit). But I would also like to be able to change logger configuration via LOG4CXX_CONFIGURATION when LOG_CONFIG() is called (explicitly or implicitly).

      I propose to modify log to do following:

      • any call to logger methods implicitly initializes it (via call to LOG_CONFIG()) if it has not been initialized.
      • if LOG_CONFIG() is called without file name and LOG4CXX_CONFIGURATION is set then it should be equivalent to LOG_CONFIG(getenv("LOG4CXX_CONFIGURATION"))

      Objections, suggestions?

        Attachments

          Issue Links

            Activity

            Hide
            ktl Kian-Tat Lim added a comment -

            If you're just responding to comments, it's usually OK to go forward without another review.

            In this case, you have a number of trailing whitespace problems in Log.h. config_env does not meet variable naming rules. LOG_CTX foo(""); does nothing; I'm not sure it should be allowed. Similarly, LOG_PUSHCTX("") should probably be forbidden. Log::configure() should call init() to avoid repeating code, shouldn't it?

            Show
            ktl Kian-Tat Lim added a comment - If you're just responding to comments, it's usually OK to go forward without another review. In this case, you have a number of trailing whitespace problems in Log.h . config_env does not meet variable naming rules. LOG_CTX foo(""); does nothing; I'm not sure it should be allowed. Similarly, LOG_PUSHCTX("") should probably be forbidden. Log::configure() should call init() to avoid repeating code, shouldn't it?
            Hide
            salnikov Andy Salnikov added a comment -

            OK, I got rid of whitespaces and disallowed empty context names (exception will be raised). Log::configure() is doing slightly different thing from what init() does so for now I want to keep them separate.

            OK to merge into master?

            Show
            salnikov Andy Salnikov added a comment - OK, I got rid of whitespaces and disallowed empty context names (exception will be raised). Log::configure() is doing slightly different thing from what init() does so for now I want to keep them separate. OK to merge into master?
            Hide
            salnikov Andy Salnikov added a comment -

            There are 6 commits, I'm not sure if they need to be squashed before merge, I'd prefer to keep them as they are.

            Show
            salnikov Andy Salnikov added a comment - There are 6 commits, I'm not sure if they need to be squashed before merge, I'd prefer to keep them as they are.
            Hide
            jbecla Jacek Becla added a comment -

            I am totally fine with 6 commits, as long as you do --no-ff when merging to master.

            Show
            jbecla Jacek Becla added a comment - I am totally fine with 6 commits, as long as you do --no-ff when merging to master.
            Hide
            salnikov Andy Salnikov added a comment -

            OK, pushed and closed.

            Show
            salnikov Andy Salnikov added a comment - OK, pushed and closed.

              People

              • Assignee:
                salnikov Andy Salnikov
                Reporter:
                salnikov Andy Salnikov
                Reviewers:
                Kian-Tat Lim
                Watchers:
                Andy Salnikov, Jacek Becla, Kian-Tat Lim, Robert Lupton
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel