Commons Logging
  1. Commons Logging
  2. LOGGING-86

[logging] SimpleLog log method should defer writing for better reuse!

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.0.3
    • Fix Version/s: 1.0.4
    • Labels:
      None
    • Environment:

      Operating System: other
      Platform: Other

      Description

      SimpleLog 'log' method which performs the actual log write to System.err should
      be refactored to instead call a 'doWrite' method which commits the log message
      to the stream. This would allow subclasses to reuse the bulk of the SimpleLog
      logic but direct the output elsewhere (in my case, for instance, a GUI log
      window). This change is too trivial to mandate a full patch, here is a pseudo
      patch:

      /**

      • <p> Do the actual logging.
      • This method assembles the message
      • and then prints to <code>System.err</code>.</p>
        */
        protected void log(int type, Object message, Throwable t) { ... // print to System.err - System.err.println(buf.toString()); + doWrite(buf); }

      + /** Subclasses can easily override this */
      + protected void doWrite(StringBuffer buf)

      { + System.err.println(buf); + }

      Without this patch, the only current solution is reuse by cutting-and-pasting
      the entire 'log' method and changing the single System.err line

        Activity

        Hide
        Dennis Lundberg added a comment -

        The only downside I can see in doing this is that it requires one more
        method-call. How critical is performance in this implementation?

        Show
        Dennis Lundberg added a comment - The only downside I can see in doing this is that it requires one more method-call. How critical is performance in this implementation?
        Hide
        Aaron Hamid added a comment -

        I also noticed that all the logging methods are made final (as well as the
        accessors for debug level), which obviates subclasses, so I had to copy and
        paste my own copy of this class and remove the 'final' keywords from those methods.

        It may be that it is important to performance to keep these things final and to
        not do an extra call, but I wanted to at least point it out.

        Show
        Aaron Hamid added a comment - I also noticed that all the logging methods are made final (as well as the accessors for debug level), which obviates subclasses, so I had to copy and paste my own copy of this class and remove the 'final' keywords from those methods. It may be that it is important to performance to keep these things final and to not do an extra call, but I wanted to at least point it out.
        Hide
        Dennis Lundberg added a comment -

        Having thought some more about this I like the idea. I'm planning to write a
        Logger that can be used in an applet that sends all log entries to a servlet.
        That could easily be done by extending SimpleLog, with the enhancement proposed
        here.

        I can write a patch for it, but I'm currently 2 patches ahead of what's in cvs.
        The attribute prefix (renamed to shortLogName in my patch for COM-1039) should
        also be made protected.

        Do you really need to override the logging methods (and accessors for debug level)?

        Show
        Dennis Lundberg added a comment - Having thought some more about this I like the idea. I'm planning to write a Logger that can be used in an applet that sends all log entries to a servlet. That could easily be done by extending SimpleLog, with the enhancement proposed here. I can write a patch for it, but I'm currently 2 patches ahead of what's in cvs. The attribute prefix (renamed to shortLogName in my patch for COM-1039 ) should also be made protected. Do you really need to override the logging methods (and accessors for debug level)?
        Hide
        Aaron Hamid added a comment -

        Yes, I need to override the logging methods because I am writing a "facade" log
        for my application which captures all logging to a graphical window. As far as
        I know the easiest, most portable, way of doing this while still retaining all
        the implementation-neutrality of jakarta commons logging is to wrap each Log
        with my own application log which captures these messages. My usage is
        something like:

        class SomeClass

        { private static final Log log = new MyAppLog(LogFactory.getLog("SomeClass")); }

        class MyAppLog extends SimpleLog {
        ...
        public void doWrite(StringBuffer buf)

        { // write to my log window }

        public void info(String message)

        { super.info(message); chainedlog.info(message); }

        ...
        }

        MyAppLog will then capture all output (nicely formatted since it is a SimpleLog
        subclass), but also pass it on to the destination log implementation, be it
        Log4j, or jdk 1.4 logging, etc. (technical note, I could equivalently create my
        own custom facade log factory implementation but I think that is a bit of
        overkill just to cache MyAppLog instances which will most likely no two will be
        instantiated with the same name).

        If you have any better idea that using such a facade I'm all ears. The only
        other alternative I see is to write appender implementations in every possible
        back-end, to write to my application window...I don't think that is feasible or
        desirable.

        Show
        Aaron Hamid added a comment - Yes, I need to override the logging methods because I am writing a "facade" log for my application which captures all logging to a graphical window. As far as I know the easiest, most portable, way of doing this while still retaining all the implementation-neutrality of jakarta commons logging is to wrap each Log with my own application log which captures these messages. My usage is something like: class SomeClass { private static final Log log = new MyAppLog(LogFactory.getLog("SomeClass")); } class MyAppLog extends SimpleLog { ... public void doWrite(StringBuffer buf) { // write to my log window } public void info(String message) { super.info(message); chainedlog.info(message); } ... } MyAppLog will then capture all output (nicely formatted since it is a SimpleLog subclass), but also pass it on to the destination log implementation, be it Log4j, or jdk 1.4 logging, etc. (technical note, I could equivalently create my own custom facade log factory implementation but I think that is a bit of overkill just to cache MyAppLog instances which will most likely no two will be instantiated with the same name). If you have any better idea that using such a facade I'm all ears. The only other alternative I see is to write appender implementations in every possible back-end, to write to my application window...I don't think that is feasible or desirable.
        Hide
        Dennis Lundberg added a comment -

        How about this:

        class MyAppLog extends SimpleLog {
        ...
        public void doWrite(StringBuffer buf)

        { super.doWrite(buf); // write to my log window }

        ...
        }

        Show
        Dennis Lundberg added a comment - How about this: class MyAppLog extends SimpleLog { ... public void doWrite(StringBuffer buf) { super.doWrite(buf); // write to my log window } ... }
        Hide
        rdonkin@apache.org added a comment -

        i'm tempted to mark this as WONT FIX for the following reason:

        SimpleLog should have been marked final. (Too late to fix it now.) IMO the right way to reuse SimpleLog
        would be through delegation. So, I'd suggest implementing your facade by delegating to a SimpleLog
        instance rather than inheriting from SimpleLog.

        But I'm willing to be persuaded otherwise (so I'm leaving this open for now).

        Show
        rdonkin@apache.org added a comment - i'm tempted to mark this as WONT FIX for the following reason: SimpleLog should have been marked final. (Too late to fix it now.) IMO the right way to reuse SimpleLog would be through delegation. So, I'd suggest implementing your facade by delegating to a SimpleLog instance rather than inheriting from SimpleLog. But I'm willing to be persuaded otherwise (so I'm leaving this open for now).
        Hide
        Simon Kitching added a comment -

        I'm in favour of Aaron's proposal to add a doWrite(StringBuffer s) method.

        This extra virtual method call occurs only after it is determined that the
        message will be output. Performance is critical when determining if a
        message will be logged, but given that we have now committed to doing IO (which
        is always slow), I don't see an extra virtual method call being significant at
        that point.

        Robert, I think the Log interface is sufficiently complex to make
        reuse-by-delegation a pain; inheritance seems cleaner to me in this case. And it
        seems a quite common use-case too; using all the standard SimpleLog
        functionality but directing the output to some custom destination.

        I also think the doWrite method should take StringBuffer, not String, as a
        parameter, just in case the doWrite method wants to tweak the output string some
        more. Yes this commits the log() method to using StringBuffers forever more, but
        I can't see why it would ever want to use anything else. Still, this could be
        debated...

        Show
        Simon Kitching added a comment - I'm in favour of Aaron's proposal to add a doWrite(StringBuffer s) method. This extra virtual method call occurs only after it is determined that the message will be output. Performance is critical when determining if a message will be logged, but given that we have now committed to doing IO (which is always slow), I don't see an extra virtual method call being significant at that point. Robert, I think the Log interface is sufficiently complex to make reuse-by-delegation a pain; inheritance seems cleaner to me in this case. And it seems a quite common use-case too; using all the standard SimpleLog functionality but directing the output to some custom destination. I also think the doWrite method should take StringBuffer, not String, as a parameter, just in case the doWrite method wants to tweak the output string some more. Yes this commits the log() method to using StringBuffers forever more, but I can't see why it would ever want to use anything else. Still, this could be debated...
        Hide
        Aaron Hamid added a comment -

        Hi guys. In answer to Robert, my original implementation was through
        delegation, and this only worked because I previously arranged to override the
        System.err stream with a custom stream, and then grab the byte[] chunks emitted
        by SimpleLog in my custom stream implementation, and then write that to my
        window. I thought this was more convoluted and less efficient than just being
        able to snag the output dierctly at its source (and it also had the side-effect
        of consuming System.err output produced by code other than SimpleLog).

        I can always reuse through copy/paste, so make the decision you feel is best for
        the package. I just wanted to note what I ran into.

        Show
        Aaron Hamid added a comment - Hi guys. In answer to Robert, my original implementation was through delegation, and this only worked because I previously arranged to override the System.err stream with a custom stream, and then grab the byte[] chunks emitted by SimpleLog in my custom stream implementation, and then write that to my window. I thought this was more convoluted and less efficient than just being able to snag the output dierctly at its source (and it also had the side-effect of consuming System.err output produced by code other than SimpleLog). I can always reuse through copy/paste, so make the decision you feel is best for the package. I just wanted to note what I ran into.
        Hide
        Craig McClanahan added a comment -

        Fixed in nightly build 20040301 (and upcoming 1.0.4 release).

        Show
        Craig McClanahan added a comment - Fixed in nightly build 20040301 (and upcoming 1.0.4 release).

          People

          • Assignee:
            Unassigned
            Reporter:
            Aaron Hamid
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development