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

Enhance lsst.log by having a Log object and Python interface

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: log
    • Labels:
      None

      Description

      Based on branch u/ktlim/getLogger in log and requests from DM-3532, implement a lsst::log Python interface through Log objects, and allow controllability of logger names and levels in Python.

        Attachments

          Issue Links

            Activity

            Hide
            hchiang2 Hsin-Fang Chiang added a comment -

            Andy Salnikov would you mind reviewing this ticket?

            The goal of this ticket is to provide a Python interface the pipeline apps want for using lsst::log, so we can migrate pipeline tasks from pex::logging to lsst::log later. It was discussed in RFC-29 and DM-3532, but there will be another RFC before the actual migration.

            Therefore, I try to maintain backwards compatibility in this ticket. That means there are sometimes multiple ways to do the same thing, one is used in qserv and one will be used in pipeline tasks.

            I ran both lsst_distrib and qserv_distrib Jenkins:
            https://ci.lsst.codes/job/stack-os-matrix/compiler=gcc,label=centos-6,python=py2/13164//console
            https://ci.lsst.codes/job/stack-os-matrix/compiler=gcc,label=centos-6,python=py2/13163//console
            It's my first time touching anything that may affect qserv, are there other tests I should run?

            Show
            hchiang2 Hsin-Fang Chiang added a comment - Andy Salnikov would you mind reviewing this ticket? The goal of this ticket is to provide a Python interface the pipeline apps want for using lsst::log, so we can migrate pipeline tasks from pex::logging to lsst::log later. It was discussed in RFC-29 and DM-3532 , but there will be another RFC before the actual migration. Therefore, I try to maintain backwards compatibility in this ticket. That means there are sometimes multiple ways to do the same thing, one is used in qserv and one will be used in pipeline tasks. I ran both lsst_distrib and qserv_distrib Jenkins: https://ci.lsst.codes/job/stack-os-matrix/compiler=gcc,label=centos-6,python=py2/13164//console https://ci.lsst.codes/job/stack-os-matrix/compiler=gcc,label=centos-6,python=py2/13163//console It's my first time touching anything that may affect qserv, are there other tests I should run?
            Hide
            salnikov Andy Salnikov added a comment -

            Looks OK and I left a small bunch of comments on PR. I think that `Log` class can benefit from interface clean up (and it can probably make your Python wrapper a tad simpler).

            Qserv uses C++ API mostly so it looks like your changes should not affect qserv, I'll also try to test qserv with your branch today to see if anything breaks.

            Show
            salnikov Andy Salnikov added a comment - Looks OK and I left a small bunch of comments on PR. I think that `Log` class can benefit from interface clean up (and it can probably make your Python wrapper a tad simpler). Qserv uses C++ API mostly so it looks like your changes should not affect qserv, I'll also try to test qserv with your branch today to see if anything breaks.
            Hide
            salnikov Andy Salnikov added a comment -

            I ran standard qserv integration tests with this log branch, no problems whatsoever.

            Show
            salnikov Andy Salnikov added a comment - I ran standard qserv integration tests with this log branch, no problems whatsoever.
            Hide
            hchiang2 Hsin-Fang Chiang added a comment -

            Thank you very much Andy Salnikov for your review and testing with qserv.

            I've made corrections based on your comments on PR, and further cleaned up the interface. The static methods setLevel, getLevel, and isEnabledFor in c++ are removed and replaced by the non-static ones. The c++ macros API are not changed. I didn't change logMsg and log as they seem used directly in qserv (e.g. https://github.com/lsst/qserv/blob/4dd49beb5ecd74430631d1a9eff22446ddc1b7f2/core/modules/proxy/czarProxy.cc#L116 )

            About making defaultLogger a static method instead of a static variable, I'm not sure I follow the idea and find a static variable a bit cleaner (In that case this patch only replaces static log4cxx::LoggerPtr by static Log). May you talk more about why a defaultLogger() method is preferred? (The current ticket branch has changed it to a method already, though)

            Show
            hchiang2 Hsin-Fang Chiang added a comment - Thank you very much Andy Salnikov for your review and testing with qserv. I've made corrections based on your comments on PR, and further cleaned up the interface. The static methods setLevel , getLevel , and isEnabledFor in c++ are removed and replaced by the non-static ones. The c++ macros API are not changed. I didn't change logMsg and log as they seem used directly in qserv (e.g. https://github.com/lsst/qserv/blob/4dd49beb5ecd74430631d1a9eff22446ddc1b7f2/core/modules/proxy/czarProxy.cc#L116 ) About making defaultLogger a static method instead of a static variable, I'm not sure I follow the idea and find a static variable a bit cleaner (In that case this patch only replaces static log4cxx::LoggerPtr by static Log ). May you talk more about why a defaultLogger() method is preferred? (The current ticket branch has changed it to a method already, though)
            Hide
            salnikov Andy Salnikov added a comment -

            Regarding static log() and logMsg() - I'd like to see them non-static too if only for consistency with other methods. We can update qserv to use non-static methods but this needs some coordination between releasing log and qserv. We could probably start by adding non-static methods to Log class and keeping static for now and when log is released I can work on updating qserv code and removing static methods once qserv is ready.

            Public static class members are problematic for the same reason as global objects - undefined initialization order of globals in C++. If defaultLogger is a static member and some other global (or static) object tries to use it it may happen that defaultLogger is not initialized yet. Making it a function-local static avoids this issue because that static is guaranteed to be initialized on the first call to that function.

            Show
            salnikov Andy Salnikov added a comment - Regarding static log() and logMsg() - I'd like to see them non-static too if only for consistency with other methods. We can update qserv to use non-static methods but this needs some coordination between releasing log and qserv . We could probably start by adding non-static methods to Log class and keeping static for now and when log is released I can work on updating qserv code and removing static methods once qserv is ready. Public static class members are problematic for the same reason as global objects - undefined initialization order of globals in C++. If defaultLogger is a static member and some other global (or static) object tries to use it it may happen that defaultLogger is not initialized yet. Making it a function-local static avoids this issue because that static is guaranteed to be initialized on the first call to that function.
            Hide
            hchiang2 Hsin-Fang Chiang added a comment -

            I see. Thanks a lot for helping me understand.

            I added non-static log() and logMsg() and now everything uses the non-static ones inside this package and it can build without the static ones. But they will be kept for now so the qserv code can be updated later.

            I also renamed defaultLogger() to getDefaultLogger(). I will run Jenkins of lsst_distrib and qserv_distrib before merging.

            Show
            hchiang2 Hsin-Fang Chiang added a comment - I see. Thanks a lot for helping me understand. I added non-static log() and logMsg() and now everything uses the non-static ones inside this package and it can build without the static ones. But they will be kept for now so the qserv code can be updated later. I also renamed defaultLogger() to getDefaultLogger() . I will run Jenkins of lsst_distrib and qserv_distrib before merging.
            Hide
            salnikov Andy Salnikov added a comment -

            Thanks, I opened DM-6978 for myself to take care of qserv and remove those two static methods from Log class.

            Show
            salnikov Andy Salnikov added a comment - Thanks, I opened DM-6978 for myself to take care of qserv and remove those two static methods from Log class.
            Hide
            hchiang2 Hsin-Fang Chiang added a comment -

            Merged to master.

            Show
            hchiang2 Hsin-Fang Chiang added a comment - Merged to master.

              People

              Assignee:
              hchiang2 Hsin-Fang Chiang
              Reporter:
              hchiang2 Hsin-Fang Chiang
              Reviewers:
              Andy Salnikov
              Watchers:
              Andy Salnikov, Hsin-Fang Chiang, Kian-Tat Lim
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  CI Builds

                  No builds found.