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

configuration of log object can throw exception which needs to be catchable

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Won't Fix
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: log
    • Labels:
      None
    • Team:
      Data Facility

      Description

      During the configuration of properties, log can instantiate appender classes which can throw exceptions. In order to propagate this up to Python the following code needs to be added to the logLib.i Swig file:

      %lsst_exceptions()
      %include "lsst/pex/exceptions/exceptionsLib.i"

        Attachments

          Issue Links

            Activity

            Hide
            spietrowicz Steve Pietrowicz added a comment -

            The log4cxx code does things both ways. The log4cxx odbcappender can throw an exception, and the log4cxx sockethubappender prints an error. I went with throwing an exception in the EventAppender code because I thought it would be better to know that a broker host name needed to be configured rather than printing a message. The lsst.log pushContext method throws an exception if an invalid name is attempted to be added, so I figured this was the way to go.

            The exception thrown in odbcappender can happen after the appender is configured. I believe the intent there is to attempt to catch that exception in the user's code, because there's a recovery mechanism in place to reconnect to the database if necessary. This goes against the philosophy of "it should always work" though.

            I could do this either way, and at this point I'm leaning to writing the error to stdout the way log4cxx does for other errors. If that's OK with everyone, I'll just drop this ticket completely and change the EventAppender code appropriately. Follows up on the details of that will be in DM-1342.

            Show
            spietrowicz Steve Pietrowicz added a comment - The log4cxx code does things both ways. The log4cxx odbcappender can throw an exception, and the log4cxx sockethubappender prints an error. I went with throwing an exception in the EventAppender code because I thought it would be better to know that a broker host name needed to be configured rather than printing a message. The lsst.log pushContext method throws an exception if an invalid name is attempted to be added, so I figured this was the way to go. The exception thrown in odbcappender can happen after the appender is configured. I believe the intent there is to attempt to catch that exception in the user's code, because there's a recovery mechanism in place to reconnect to the database if necessary. This goes against the philosophy of "it should always work" though. I could do this either way, and at this point I'm leaning to writing the error to stdout the way log4cxx does for other errors. If that's OK with everyone, I'll just drop this ticket completely and change the EventAppender code appropriately. Follows up on the details of that will be in DM-1342 .
            Hide
            ktl Kian-Tat Lim added a comment -

            For logging-time exceptions, I guess we'll have to catch in our C++ interface if they can be thrown by appenders. After further thought, I think that configure-time exceptions are OK, and that we'd rather know about a bad configuration early than late (after other logs are collected). But then I'm not sure that we need to catch those exceptions in Python; what is the Python going to be able to do about it?

            Show
            ktl Kian-Tat Lim added a comment - For logging-time exceptions, I guess we'll have to catch in our C++ interface if they can be thrown by appenders. After further thought, I think that configure-time exceptions are OK, and that we'd rather know about a bad configuration early than late (after other logs are collected). But then I'm not sure that we need to catch those exceptions in Python; what is the Python going to be able to do about it?
            Hide
            spietrowicz Steve Pietrowicz added a comment -

            Yesterday I went ahead and switched everything over to do what log4cxx does in most cases, which is print a message to standard error.

            The odbcappender, smtpappender can throw exceptions. I believe all other appenders write to stderr if things go badly.

            As for catching the exceptions in Python on configure of the EventAppender, if you Python code caught this, it could revert to another logging appender. You couldn't do this as FileAppender stands now, because the IOException it throws on setting things up is caught and an error message it written to stderr.

            In other words, unless we modified log4cxx::FileAppender to throw an exception on configure there would be two different mechanisms for trying to figure out if something went wrong.

            Show
            spietrowicz Steve Pietrowicz added a comment - Yesterday I went ahead and switched everything over to do what log4cxx does in most cases, which is print a message to standard error. The odbcappender, smtpappender can throw exceptions. I believe all other appenders write to stderr if things go badly. As for catching the exceptions in Python on configure of the EventAppender, if you Python code caught this, it could revert to another logging appender. You couldn't do this as FileAppender stands now, because the IOException it throws on setting things up is caught and an error message it written to stderr. In other words, unless we modified log4cxx::FileAppender to throw an exception on configure there would be two different mechanisms for trying to figure out if something went wrong.
            Hide
            danielw Daniel Wang [X] (Inactive) added a comment -

            When I say that "logging should never throw", I probably mean that the act of logging should do something relatively lightweight, and nearly-zero risk. If I configured logging with an odbcappender or an smtpappender, I'm probably not thinking of zero-risk anymore, because those two things can fail quite easily despite all the checks I could think of, and the behavior is no longer lightweight--both odbc and smtp are complex enough that they could reasonably have their own loggers (and thus make our brains explode).

            I guess what I'm trying to say is that if you've got appenders that are similarly involved, you could justify throwing exceptions in those cases, and the calling code should be stingy about invoking the logger, because the probability of throwing is non-negligible (greater than 0.1% or something). Perhaps this is the original case you were trying to address.

            Sorry about potentially bikeshedding this. I'll shutup now.

            Show
            danielw Daniel Wang [X] (Inactive) added a comment - When I say that "logging should never throw", I probably mean that the act of logging should do something relatively lightweight, and nearly-zero risk. If I configured logging with an odbcappender or an smtpappender, I'm probably not thinking of zero-risk anymore, because those two things can fail quite easily despite all the checks I could think of, and the behavior is no longer lightweight--both odbc and smtp are complex enough that they could reasonably have their own loggers (and thus make our brains explode). I guess what I'm trying to say is that if you've got appenders that are similarly involved, you could justify throwing exceptions in those cases, and the calling code should be stingy about invoking the logger, because the probability of throwing is non-negligible (greater than 0.1% or something). Perhaps this is the original case you were trying to address. Sorry about potentially bikeshedding this. I'll shutup now.
            Hide
            spietrowicz Steve Pietrowicz added a comment -

            No worries. Getting all this thought about now and discussed is better than getting way on down the road without having discussed what was done. Details of the current thinking about this is in DM-1342. I'll close this out.

            Show
            spietrowicz Steve Pietrowicz added a comment - No worries. Getting all this thought about now and discussed is better than getting way on down the road without having discussed what was done. Details of the current thinking about this is in DM-1342 . I'll close this out.

              People

              Assignee:
              spietrowicz Steve Pietrowicz
              Reporter:
              spietrowicz Steve Pietrowicz
              Reviewers:
              Jacek Becla
              Watchers:
              Daniel Wang [X] (Inactive), Jacek Becla, Kian-Tat Lim, Steve Pietrowicz
              Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved: