Commons Logging
  1. Commons Logging
  2. LOGGING-47

[logging][PATCH] Improvements to wrapper classes

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.1.0
    • Labels:
      None
    • Environment:

      Operating System: All
      Platform: All

      Description

      There are several issues with the wrapper classes.

      1) Logging null objects or not
      If the message object is null, the logger logs the String "NULL" to the
      underlying log system. Currently some wrappers just return and some log the
      String "NULL".
      -> The patch unifies that. All wrappers will log the String "NULL".

      2) Exception safe toString
      Currently all wrappers would fail if an toString method throws a
      RuntimeException. See
      http://marc.theaimsgroup.com/?l=jakarta-commons-dev&m=113664590418487&w=2
      for details.
      -> The patch logs an error message instead and an additional log record with
      the exception in a central, private doLog method.

      3) isLoggable check
      Currently some wrappers check the log level in each log method and some wrappers
      check the level in one central method, which applies the log message and
      throwable to the underlying log system.
      -> The patch unifies that. All wrappers will use a doLog method and check the
      level there. One exception: AvalonLogger which has no Level/Priority object.

      3) JavaDoc
      Every wrapper has its own javadoc for the log methods, sometime with reference
      to the Log interface, sometimes with the underlying log level, ...
      -> The patch unifies that. All wrapper log methods are commented in the same way.

      4) Additional class
      There is one additional class for the error message, to have a single point of
      change.

      Please check the patch for correct format.

        Activity

        Hide
        Dennis Lundberg added a comment -

        The Javadoc improvements where commited by rdonkin prior to the 1.1.0 release.

        Show
        Dennis Lundberg added a comment - The Javadoc improvements where commited by rdonkin prior to the 1.1.0 release.
        Hide
        rdonkin@apache.org added a comment -

        Thanks for the revised patches.

        I'll go through them now.

        Robert

        Show
        rdonkin@apache.org added a comment - Thanks for the revised patches. I'll go through them now. Robert
        Hide
        Boris Unckel added a comment -

        Created an attachment (id=17457)
        Patch for SimpleLog

        • JavaDoc
        • single doLog method for all levels
        • Null behaviour (null object message will be logged)
        • Null safe
        • Formatting of source code may show more diff as needed (sorry)!
          • Nothing else **
        Show
        Boris Unckel added a comment - Created an attachment (id=17457) Patch for SimpleLog JavaDoc single doLog method for all levels Null behaviour (null object message will be logged) Null safe Formatting of source code may show more diff as needed (sorry)! Nothing else **
        Hide
        Boris Unckel added a comment -

        Created an attachment (id=17456)
        Patch for LogKitLogger

        • JavaDoc
        • single doLog method for all levels
        • Null behaviour (null object message will be logged)
        • Null safe
        • Formatting of source code may show more diff as needed (sorry)!
          • Nothing else **
        Show
        Boris Unckel added a comment - Created an attachment (id=17456) Patch for LogKitLogger JavaDoc single doLog method for all levels Null behaviour (null object message will be logged) Null safe Formatting of source code may show more diff as needed (sorry)! Nothing else **
        Hide
        Boris Unckel added a comment -

        Created an attachment (id=17455)
        Patch for Log4J13Logger

        • JavaDoc
        • single doLog method for all levels
        • Null behaviour (null object message will be logged)
        • Null safe
        • Formatting of source code may show more diff as needed (sorry)!
          • Nothing else **
        Show
        Boris Unckel added a comment - Created an attachment (id=17455) Patch for Log4J13Logger JavaDoc single doLog method for all levels Null behaviour (null object message will be logged) Null safe Formatting of source code may show more diff as needed (sorry)! Nothing else **
        Hide
        Boris Unckel added a comment -

        Created an attachment (id=17454)
        Patch for Log4J12Logger

        • JavaDoc
        • single doLog method for all levels
        • Null behaviour (null object message will be logged)
        • Null safe
        • Formatting of source code may show more diff as needed (sorry)!
          • Nothing else **
        Show
        Boris Unckel added a comment - Created an attachment (id=17454) Patch for Log4J12Logger JavaDoc single doLog method for all levels Null behaviour (null object message will be logged) Null safe Formatting of source code may show more diff as needed (sorry)! Nothing else **
        Hide
        Boris Unckel added a comment -

        Created an attachment (id=17453)
        Patch for Jdk14Logger

        • JavaDoc
        • single doLog method for all levels
        • Null behaviour (null object message will be logged)
        • Null safe
        • Formatting of source code may show more diff as needed (sorry)!
          • Nothing else **
        Show
        Boris Unckel added a comment - Created an attachment (id=17453) Patch for Jdk14Logger JavaDoc single doLog method for all levels Null behaviour (null object message will be logged) Null safe Formatting of source code may show more diff as needed (sorry)! Nothing else **
        Hide
        Boris Unckel added a comment -

        Created an attachment (id=17452)
        Patch for Jdk13LumberjackLogger

        • JavaDoc
        • single doLog method for all levels
        • Null behaviour (null object message will be logged)
        • Null safe
        • Formatting of source code may show more diff as needed (sorry)!
          • Nothing else **
        Show
        Boris Unckel added a comment - Created an attachment (id=17452) Patch for Jdk13LumberjackLogger JavaDoc single doLog method for all levels Null behaviour (null object message will be logged) Null safe Formatting of source code may show more diff as needed (sorry)! Nothing else **
        Hide
        Boris Unckel added a comment -

        Created an attachment (id=17451)
        Patch for AvalonLogger

        • JavaDoc
        • per level log method (doLogTrace, doLogDebug...)
        • Null behaviour (null object message will be logged)
        • Null safe
        • Formatting of source code may show more diff as needed (sorry)!
          • Nothing else **
        Show
        Boris Unckel added a comment - Created an attachment (id=17451) Patch for AvalonLogger JavaDoc per level log method (doLogTrace, doLogDebug...) Null behaviour (null object message will be logged) Null safe Formatting of source code may show more diff as needed (sorry)! Nothing else **
        Hide
        Boris Unckel added a comment -

        (From update of attachment 17360)
        The patch was rejected by the project due to complexity and unwanted feature of
        exception handling.

        Show
        Boris Unckel added a comment - (From update of attachment 17360) The patch was rejected by the project due to complexity and unwanted feature of exception handling.
        Hide
        Simon Kitching added a comment -

        As William noted, the use of String.valueOf(msg) on the input object to avoid
        exceptions has significant implications. Suppose a user passes a custom
        MessageRecord object as the parameter, and then has a custom Log4J Renderer
        which takes this object and builds a string out of it in some clever way. By
        forcing the message parameter to a string at the JCL level, that breaks the code
        completely.

        Boris makes the point that if someone is doing this then they really should be
        using log4j directly, not JCL, because that won't work if JCL is bound to
        anything other than log4j. Nevertheless, I'm inclined to avoid introding such
        breakage in this release, and make a note of that point for JCL2, where we could
        potentially state that all "rendering" of the message object is done by JCL for
        consistency. Currently, 1.1 is very close to being backwards compatible and I
        would prefer to keep it that way.

        Show
        Simon Kitching added a comment - As William noted, the use of String.valueOf(msg) on the input object to avoid exceptions has significant implications. Suppose a user passes a custom MessageRecord object as the parameter, and then has a custom Log4J Renderer which takes this object and builds a string out of it in some clever way. By forcing the message parameter to a string at the JCL level, that breaks the code completely. Boris makes the point that if someone is doing this then they really should be using log4j directly, not JCL, because that won't work if JCL is bound to anything other than log4j. Nevertheless, I'm inclined to avoid introding such breakage in this release, and make a note of that point for JCL2, where we could potentially state that all "rendering" of the message object is done by JCL for consistency. Currently, 1.1 is very close to being backwards compatible and I would prefer to keep it that way.
        Hide
        rdonkin@apache.org added a comment -

        Yeh: a combined patch with so many changes makes things very difficult. I agree
        with Simon's analysis of the patch (too big to apply and would prefer a less
        centralised implementation).

        I might see if I can extract the javadoc improvements.

        If they were separate, decentralised patch's I'd probably be willing to take the
        risk of applying both the consistent NULL patch and the toString exception handling.

        Show
        rdonkin@apache.org added a comment - Yeh: a combined patch with so many changes makes things very difficult. I agree with Simon's analysis of the patch (too big to apply and would prefer a less centralised implementation). I might see if I can extract the javadoc improvements. If they were separate, decentralised patch's I'd probably be willing to take the risk of applying both the consistent NULL patch and the toString exception handling.
        Hide
        Simon Kitching added a comment -

        Hi Boris,

        Thanks very much for your patch. Unfortunately I'm inclined not to apply this
        patch to the next release, for the reasons below. However I agree these points
        should definitely be kept in mind when designing JCL 2.0.

        The implementation adds overhead to the critical path for logging messages,
        including an extra virtual method call and a call to the underlying logger to
        determine whether the loglevel is enabled. There's some additional overhead if
        the message is actually logged, but that's not so important as the act of really
        logging a message is likely to be much larger than the overhead introduced.

        The patch is also reasonably large, touching a significant portion of all the
        logging classes. That introduces the risk of new bugs; JCL's unit test suite
        isn't too bad but it's definitely not 100% coverage so we can't just trust the
        tests to pick up any issues.

        The issue of behaviour when NULL is passed as the object to log is valid; it
        would be nice for JCL to impose consistent behaviour rather than leaving it up
        to the underlying logging library. However it's not a major issue.

        The issue of the message-object to be logged throwing an exception in its
        toString is really pretty unusual. Almost all the time, a String or StringBuffer
        will be passed here, and they never have problems with toString. In the rare
        case where it does occur, it's the caller's fault anyway. Yes, this would be
        nice to handle but I don't think the risk of introducing a bug is worth it at
        the moment.

        Consistency of code - yes that would be nice, but it's not critical.

        JavaDoc - again, this would be nice but it's not critical. I would consider a
        javadoc-only patch, but can't be bothered to extract the javadoc bits out of the
        attached all-in-one patchfile.

        Centralising code for future maintainability - well, yes good idea. But this
        code doesn't have a lot of lifetime left; hopefully the release after next will
        be JCL 2.0, with significant work done on all parts of the library.

        Anyone else got an opinion on this?

        Show
        Simon Kitching added a comment - Hi Boris, Thanks very much for your patch. Unfortunately I'm inclined not to apply this patch to the next release, for the reasons below. However I agree these points should definitely be kept in mind when designing JCL 2.0. The implementation adds overhead to the critical path for logging messages, including an extra virtual method call and a call to the underlying logger to determine whether the loglevel is enabled. There's some additional overhead if the message is actually logged, but that's not so important as the act of really logging a message is likely to be much larger than the overhead introduced. The patch is also reasonably large, touching a significant portion of all the logging classes. That introduces the risk of new bugs; JCL's unit test suite isn't too bad but it's definitely not 100% coverage so we can't just trust the tests to pick up any issues. The issue of behaviour when NULL is passed as the object to log is valid; it would be nice for JCL to impose consistent behaviour rather than leaving it up to the underlying logging library. However it's not a major issue. The issue of the message-object to be logged throwing an exception in its toString is really pretty unusual. Almost all the time, a String or StringBuffer will be passed here, and they never have problems with toString. In the rare case where it does occur, it's the caller's fault anyway. Yes, this would be nice to handle but I don't think the risk of introducing a bug is worth it at the moment. Consistency of code - yes that would be nice, but it's not critical. JavaDoc - again, this would be nice but it's not critical. I would consider a javadoc-only patch, but can't be bothered to extract the javadoc bits out of the attached all-in-one patchfile. Centralising code for future maintainability - well, yes good idea. But this code doesn't have a lot of lifetime left; hopefully the release after next will be JCL 2.0, with significant work done on all parts of the library. Anyone else got an opinion on this?
        Hide
        Boris Unckel added a comment -

        (In reply to comment #4)
        > At least for log4j, this is wrong, since the conversion of Objects to Strings
        > is controlled by the log4j configuration. With this patch, you've preempted
        > that functionallity.

        You are right. log4j suffers currently from the same problem (no exception safe
        message rendering). It is a design decision to have one behaviour for all
        classes, or one behaviour for underlying logging supporting object as message
        and another for underlying APIs which require String.

        I would suggest to apply the patch now (since log4j has no exception safe
        rendering) and change it, if it has the capability to deal with exceptions in
        toString.

        Or in other words:
        What causes the user more pain: Missing individual formatting or missing a good
        exception handling strategy?
        The org.apache.log4j.or.DefaultRenderer does nothing but o.toString()

        The patch has one advantage: You would have to change at one single place per
        Log4j1xLogger (x 2,3)...

        Show
        Boris Unckel added a comment - (In reply to comment #4) > At least for log4j, this is wrong, since the conversion of Objects to Strings > is controlled by the log4j configuration. With this patch, you've preempted > that functionallity. You are right. log4j suffers currently from the same problem (no exception safe message rendering). It is a design decision to have one behaviour for all classes, or one behaviour for underlying logging supporting object as message and another for underlying APIs which require String. I would suggest to apply the patch now (since log4j has no exception safe rendering) and change it, if it has the capability to deal with exceptions in toString. Or in other words: What causes the user more pain: Missing individual formatting or missing a good exception handling strategy? The org.apache.log4j.or.DefaultRenderer does nothing but o.toString() The patch has one advantage: You would have to change at one single place per Log4j1xLogger (x 2,3)...
        Hide
        william.barker added a comment -

        At least for log4j, this is wrong, since the conversion of Objects to Strings
        is controlled by the log4j configuration. With this patch, you've preempted
        that functionallity.

        Show
        william.barker added a comment - At least for log4j, this is wrong, since the conversion of Objects to Strings is controlled by the log4j configuration. With this patch, you've preempted that functionallity.
        Hide
        Boris Unckel added a comment -

        Created an attachment (id=17360)
        Improvements for null message behavior, exception in toString behavior,
        javadoc.

        This time correct version.

        Show
        Boris Unckel added a comment - Created an attachment (id=17360) Improvements for null message behavior, exception in toString behavior, javadoc. This time correct version.
        Hide
        Boris Unckel added a comment -

        (From update of attachment 17359)
        Patch contains error.

        Show
        Boris Unckel added a comment - (From update of attachment 17359) Patch contains error.
        Hide
        Boris Unckel added a comment -

        Created an attachment (id=17359)
        Improvements for null message behavior, exception in toString behavior,
        javadoc.

        Show
        Boris Unckel added a comment - Created an attachment (id=17359) Improvements for null message behavior, exception in toString behavior, javadoc.

          People

          • Assignee:
            Unassigned
            Reporter:
            Boris Unckel
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development