Commons Logging
  1. Commons Logging
  2. LOGGING-9

[logging] Log.trace() doesn't use log4j 1.3 trace methods

    Details

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

      Operating System: other
      Platform: Other

      Description

      Jakarta Commons Logging 1.0.4 detects log4j 1.3 and alters its behaviour
      accordingly but continues to map its trace methods to log4j debug methods
      while log4j 1.3 now has its own trace methods. The following patch fixes this
      bug:

      Index: commons-
      logging/src/java/org/apache/commons/logging/impl/Log4JLogger.java
      ===================================================================
      — commons-logging/src/java/org/apache/commons/logging/impl/Log4JLogger.java
      (revision 161137)
      +++ commons-logging/src/java/org/apache/commons/logging/impl/Log4JLogger.java
      (working copy)
      @@ -84,7 +84,7 @@
      if(is12)

      { getLogger().log(FQCN, (Priority) Level.DEBUG, message, null ); }

      else

      { - getLogger().log(FQCN, Level.DEBUG, message, null ); + getLogger().log(FQCN, Level.TRACE, message, null ); }

      }

      @@ -97,7 +97,7 @@
      if(is12)

      { getLogger().log(FQCN, (Priority) Level.DEBUG, message, t ); }

      else

      { - getLogger().log(FQCN, Level.DEBUG, message, t ); + getLogger().log(FQCN, Level.TRACE, message, t ); }

      }

      @@ -277,7 +277,11 @@

      • For Log4J, this returns the value of <code>isDebugEnabled()</code>
        */
        public boolean isTraceEnabled() {
      • return getLogger().isDebugEnabled();
        + if(is12) { + return getLogger().isDebugEnabled(); + }

        else

        { + return getLogger().isTraceEnabled(); + }

        }

      /**

        Issue Links

          Activity

          Hide
          Simon Kitching added a comment -

          Hi Peter,

          I'm a bit concerned about the fact that this implementation requires testing a
          boolean flag for each call to Log.isTraceEnabled or Log.trace. Testing a single
          boolean isn't too bad, but logging is supposed to be highly tuned. Yes, the
          current code already does this boolean testing - but I'm not sure I agree with it.

          An alternative would be to instead create a Log4J13Logger class which subclasses
          Log4JLogger and overrides the trace and isTraceEnabled methods. This should
          produce the same effect without needing a boolean flag - though it means
          modifying LogFactoryImpl to do the testing for log4j version instead (so it can
          select the correct Log wrapper class constructor to call).

          However if no-one else is concerned about the performance implications of an
          extra boolean test in isTraceEnabled, then your patch could certainly be applied
          as-is.

          If we stay with the current implementation, I think it would be a good idea to
          rename "is12" to "isPre13" or similar; I was confused at first by this variable
          name (not your fault, Peter!).

          Show
          Simon Kitching added a comment - Hi Peter, I'm a bit concerned about the fact that this implementation requires testing a boolean flag for each call to Log.isTraceEnabled or Log.trace. Testing a single boolean isn't too bad, but logging is supposed to be highly tuned. Yes, the current code already does this boolean testing - but I'm not sure I agree with it. An alternative would be to instead create a Log4J13Logger class which subclasses Log4JLogger and overrides the trace and isTraceEnabled methods. This should produce the same effect without needing a boolean flag - though it means modifying LogFactoryImpl to do the testing for log4j version instead (so it can select the correct Log wrapper class constructor to call). However if no-one else is concerned about the performance implications of an extra boolean test in isTraceEnabled, then your patch could certainly be applied as-is. If we stay with the current implementation, I think it would be a good idea to rename "is12" to "isPre13" or similar; I was confused at first by this variable name (not your fault, Peter!).
          Hide
          Peter DeGregorio added a comment -

          Hi Simon,
          I was wondering about continual testing of the is12 flag in isTraceEnabled.
          It's obviously going to cost some time, though I don't really have the
          expertise to know how much, so I'm curious what others think. If you're looking
          for a volunteer, I'd enjoy taking a look at submitting a Log4J13Logger and the
          required LogFactoryImpl modification if that would help, or do the suggested
          rename of is12 to isPre13, though I may not be able to turn it around very
          quickly and I'll have questions (like should I actually be working on 1.0.5 and
          how to do unit testing).
          Regards,
          Peter

          Show
          Peter DeGregorio added a comment - Hi Simon, I was wondering about continual testing of the is12 flag in isTraceEnabled. It's obviously going to cost some time, though I don't really have the expertise to know how much, so I'm curious what others think. If you're looking for a volunteer, I'd enjoy taking a look at submitting a Log4J13Logger and the required LogFactoryImpl modification if that would help, or do the suggested rename of is12 to isPre13, though I may not be able to turn it around very quickly and I'll have questions (like should I actually be working on 1.0.5 and how to do unit testing). Regards, Peter
          Hide
          rdonkin@apache.org added a comment -

          Real life costs are very hard to guesstimate. Might be nice to get some numbers
          (but not essential).

          I'd suggest that the bigger cost would likely be the class cast performed. This
          is going to make logging to early versions of Log4J expensive. Refactoring in a
          fashion suggested by Simon may therefore makes quite a lot of sense.

          However, I would say that it may be better to leave Log4JLogger with less
          performant support for all versions of Log4J retaining the internal boolean.
          This could then be used from the command line when required.

          If do go down this route, we'll end up with one general purpose logger not used
          by LogFactory and two special purpose loggers used by LogFactoryImpl. The
          classloader which defines LogFactoryImpl would be the one that determines which
          version of LogJ to use. Not sure whether that's better or worse that the current
          situation.

          Show
          rdonkin@apache.org added a comment - Real life costs are very hard to guesstimate. Might be nice to get some numbers (but not essential). I'd suggest that the bigger cost would likely be the class cast performed. This is going to make logging to early versions of Log4J expensive. Refactoring in a fashion suggested by Simon may therefore makes quite a lot of sense. However, I would say that it may be better to leave Log4JLogger with less performant support for all versions of Log4J retaining the internal boolean. This could then be used from the command line when required. If do go down this route, we'll end up with one general purpose logger not used by LogFactory and two special purpose loggers used by LogFactoryImpl. The classloader which defines LogFactoryImpl would be the one that determines which version of LogJ to use. Not sure whether that's better or worse that the current situation.
          Hide
          Simon Kitching added a comment -

          Just a note: log4j 1.3 still hasn't been released yet, and there are debates on
          the log4j list over which future version will provide the TRACE level. So this
          needs to be put on hold until a log4j version with trace actually exists.

          Show
          Simon Kitching added a comment - Just a note: log4j 1.3 still hasn't been released yet, and there are debates on the log4j list over which future version will provide the TRACE level. So this needs to be put on hold until a log4j version with trace actually exists.
          Hide
          Christian Gruber added a comment -

          log4j 1.2.12 is out, which has implemented the trace level.
          So it now might be the right time to remove the hold?

          Show
          Christian Gruber added a comment - log4j 1.2.12 is out, which has implemented the trace level. So it now might be the right time to remove the hold?
          Hide
          Simon Kitching added a comment -

          Thanks for the note. It's definitely the right time to look at this again.

          Unfortunately there isn't much developer time being spent on commons-logging at
          the moment; all the developers (including myself) appear to be occupied with
          other things at the moment. A new commons-logging release isn't looking likely
          in the near future.

          Show
          Simon Kitching added a comment - Thanks for the note. It's definitely the right time to look at this again. Unfortunately there isn't much developer time being spent on commons-logging at the moment; all the developers (including myself) appear to be occupied with other things at the moment. A new commons-logging release isn't looking likely in the near future.
          Hide
          Jörg Hohwiller added a comment -

          two points from me:

          1. As you can read on http://www.qos.ch/logging/preparingFor13.jsp on the long
          run they want to kick out the legacy stuff such as Category and Priority.
          So eighter we should kick this out completely (which is my suggestion) or we
          have a very strong point for creating a separate Log4J13Logger which is already
          in SVN
          and is using Logger and Level.

          2. The current SVN codebase has renamed Log4JLogger to Log4J12Logger. I am not
          sure if this is a good idea for compatibility. If someone just upgrades the jar
          and is using the Log4JLogger set up somewhere in his config, his code may not
          work anymore (as expected).

          Show
          Jörg Hohwiller added a comment - two points from me: 1. As you can read on http://www.qos.ch/logging/preparingFor13.jsp on the long run they want to kick out the legacy stuff such as Category and Priority. So eighter we should kick this out completely (which is my suggestion) or we have a very strong point for creating a separate Log4J13Logger which is already in SVN and is using Logger and Level. 2. The current SVN codebase has renamed Log4JLogger to Log4J12Logger. I am not sure if this is a good idea for compatibility. If someone just upgrades the jar and is using the Log4JLogger set up somewhere in his config, his code may not work anymore (as expected).
          Hide
          Marc Attiyeh added a comment -

          Is this bug going to stay open forever? What should commons-logging users do
          if they want to make use of the log4j trace method?

          Show
          Marc Attiyeh added a comment - Is this bug going to stay open forever? What should commons-logging users do if they want to make use of the log4j trace method?
          Hide
          Dennis Lundberg added a comment -

          The code that is in SVN now handles trace level logging for log4j 1.2.12+ and
          1.3alpha-7.

          Show
          Dennis Lundberg added a comment - The code that is in SVN now handles trace level logging for log4j 1.2.12+ and 1.3alpha-7.

            People

            • Assignee:
              Unassigned
              Reporter:
              Peter DeGregorio
            • Votes:
              2 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development