Details

    • Type: New Feature New Feature
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0-beta9
    • Fix Version/s: 2.0-rc1
    • Component/s: API
    • Labels:
      None
    • Environment:

      Linux, Java 1.7

      Description

      I would like to have method (or class) to use a logger as a PrintStream, like e.g.:

      PrintStream Logger.getStream(Level level);

      or maybe like this http://www.java2s.com/Open-Source/Java/Testing/jacareto/jacareto/toolkit/log4j/LogOutputStream.java.htm

      (as I recently discovered, org.apache.commons.exec.LogOutputStream is doing different things).

        Activity

        Hide
        Remko Popma added a comment -

        Hi Joe,
        Would you like to contribute a patch?
        That would give the best chance of getting this feature in.
        Best regards,
        Remko

        Show
        Remko Popma added a comment - Hi Joe, Would you like to contribute a patch? That would give the best chance of getting this feature in. Best regards, Remko
        Hide
        Matt Sicker added a comment -

        I like this idea, and I'll take a whack at making a patch for my first contribution.

        Show
        Matt Sicker added a comment - I like this idea, and I'll take a whack at making a patch for my first contribution.
        Hide
        Matt Sicker added a comment -

        Here's a simple implementation of the desired behavior. Includes a unit test and default implementation in spi.

        Show
        Matt Sicker added a comment - Here's a simple implementation of the desired behavior. Includes a unit test and default implementation in spi.
        Hide
        Ralph Goers added a comment -

        This patch won't work correctly as anything logged through the LoggerStream will have incorrect caller data.

        Show
        Ralph Goers added a comment - This patch won't work correctly as anything logged through the LoggerStream will have incorrect caller data.
        Hide
        Matt Sicker added a comment -

        Good catch. I'll work on this a bit more.

        Show
        Matt Sicker added a comment - Good catch. I'll work on this a bit more.
        Hide
        Matt Sicker added a comment -

        Here's an updated version. I also updated some unit tests in order to test markers in LoggerTest.

        Show
        Matt Sicker added a comment - Here's an updated version. I also updated some unit tests in order to test markers in LoggerTest.
        Hide
        Matt Sicker added a comment -

        Forgot to update another test that's affected by the TestLogger change. Here you go.

        Show
        Matt Sicker added a comment - Forgot to update another test that's affected by the TestLogger change. Here you go.
        Hide
        Ralph Goers added a comment -

        This looks better but I still have a couple of questions regarding why/how this is useful:
        1. if you do:

        LoggerStream stream = logger.getStream(Level.INFO);
        stream.print("Hello");
        stream.print(", ");
        stream.print("World");
        stream.flush();
        

        then the line number of the location info will be for the call to flush. If they call println or simply include a newline then it will be for the print. That seems like it could lead to confusion.
        2. If the flush is forgotten the data will accumulate until something else calls flush or a newline is written. This could lead to "odd" behavior.
        3. The user could just as easily do:

        StringBuilder sb = new StringBuilder();
        sb.append("Hello");
        sb.append(", ");
        sb.append("World);
        logger.info(sb.toString());
        

        Why isn't that preferable to using a stream?

        Show
        Ralph Goers added a comment - This looks better but I still have a couple of questions regarding why/how this is useful: 1. if you do: LoggerStream stream = logger.getStream(Level.INFO); stream.print( "Hello" ); stream.print( ", " ); stream.print( "World" ); stream.flush(); then the line number of the location info will be for the call to flush. If they call println or simply include a newline then it will be for the print. That seems like it could lead to confusion. 2. If the flush is forgotten the data will accumulate until something else calls flush or a newline is written. This could lead to "odd" behavior. 3. The user could just as easily do: StringBuilder sb = new StringBuilder(); sb.append( "Hello" ); sb.append( ", " ); sb.append("World); logger.info(sb.toString()); Why isn't that preferable to using a stream?
        Hide
        Matt Sicker added a comment -

        The stream doesn't have to be flushed anymore now that it's looking for newlines in the write() methods. The underlying PrintStream is using auto-flush, too, so that'll flush for any new lines or multi-byte write()'s (at least that's how JDK7 has it implemented).

        As to why it's useful? It helps provide a bridge to legacy logging systems or legacy code that doesn't use a logger. Plus, there's the handy hack for dumb APIs as well.

        PrintStream out = new PrintStream(logger.getStream(Level.INFO));
        PrintStream err = new PrintStream(logger.getStream(Level.ERROR));
        System.setOut(out);
        System.setErr(err);
        
        Show
        Matt Sicker added a comment - The stream doesn't have to be flushed anymore now that it's looking for newlines in the write() methods. The underlying PrintStream is using auto-flush, too, so that'll flush for any new lines or multi-byte write()'s (at least that's how JDK7 has it implemented). As to why it's useful? It helps provide a bridge to legacy logging systems or legacy code that doesn't use a logger. Plus, there's the handy hack for dumb APIs as well. PrintStream out = new PrintStream(logger.getStream(Level.INFO)); PrintStream err = new PrintStream(logger.getStream(Level.ERROR)); System .setOut(out); System .setErr(err);
        Hide
        Matt Sicker added a comment -

        Here's an updated patch with better tests.

        Show
        Matt Sicker added a comment - Here's an updated patch with better tests.
        Hide
        Ralph Goers added a comment -

        Patch applied in revision 1557489. Thanks! Please verify and close.

        Show
        Ralph Goers added a comment - Patch applied in revision 1557489. Thanks! Please verify and close.
        Hide
        Remko Popma added a comment - - edited

        Re-opening, as a failing JUnit test revealed a problem when running on Windows.

        The test that fails is LoggerTest.getStream() "Message should be blank-ish". The expected value is " DEBUG ", but the actual value of results.get(2) is " DEBUG \r" on Windows.

        I think this is happening because the line separator on Windows is "\r\n", and the LoggerStream class only works correctly if the line separator is "\n". Probably both of these methods in LoggerStream$HelperStream are incorrect as it looks like they only trim off the trailing "\n" character, which leaves a trailing "\r" character on Windows:

        @Override
        public synchronized void write(int b) {
            super.write(b);
            if (b == '\n') {
                log(count - 1);
            }
        }
        
        @Override
        public synchronized void write(byte[] b, int off, int len) {
            super.write(b, off, len);
            int newLine = lastIndexOf('\n');
            if (newLine != -1) {
                log(newLine);
            }
        }
        
        Show
        Remko Popma added a comment - - edited Re-opening, as a failing JUnit test revealed a problem when running on Windows. The test that fails is LoggerTest.getStream() "Message should be blank-ish". The expected value is " DEBUG " , but the actual value of results.get(2) is " DEBUG \r" on Windows. I think this is happening because the line separator on Windows is "\r\n" , and the LoggerStream class only works correctly if the line separator is "\n" . Probably both of these methods in LoggerStream$HelperStream are incorrect as it looks like they only trim off the trailing "\n" character, which leaves a trailing "\r" character on Windows: @Override public synchronized void write( int b) { super .write(b); if (b == '\n') { log(count - 1); } } @Override public synchronized void write( byte [] b, int off, int len) { super .write(b, off, len); int newLine = lastIndexOf('\n'); if (newLine != -1) { log(newLine); } }
        Hide
        Ralph Goers added a comment -

        Resolved in revision 1557619.

        Show
        Ralph Goers added a comment - Resolved in revision 1557619.
        Hide
        Matt Sicker added a comment -

        Missed a couple things from the latest fix. Here's a patch.

        Show
        Matt Sicker added a comment - Missed a couple things from the latest fix. Here's a patch.
        Hide
        Remko Popma added a comment -

        Matt,
        I applied your patch with some minor changes.
        Biggest change was that I un-@ignored LoggerTest.getStream() - this test passes on my Windows PC.

        Fixed in revision 1557643.
        Please verify and close.

        Show
        Remko Popma added a comment - Matt, I applied your patch with some minor changes. Biggest change was that I un-@ignored LoggerTest.getStream() - this test passes on my Windows PC. Fixed in revision 1557643. Please verify and close.
        Hide
        Remko Popma added a comment -

        Matt, Ralph, one more thing:
        I am not sure that the current code in LoggerStream$HelperStream correctly handles '\r' (that are not followed by '\n') characters. Is that not a use case we need to defend against?

        @Override
        public synchronized void write(int b) {
            if (b == '\r') {
                return;
            }
            super.write(b);
            if (b == '\n') {
                log(count - 1);
            }
        }
        
        @Override
        public synchronized void write(byte[] b, int off, int len) {
            for (int i = 0; i < len; ++i) {
                write(b[off + i]);
            }
        }
        
        Show
        Remko Popma added a comment - Matt, Ralph, one more thing: I am not sure that the current code in LoggerStream$HelperStream correctly handles '\r' (that are not followed by '\n' ) characters. Is that not a use case we need to defend against? @Override public synchronized void write( int b) { if (b == '\r') { return ; } super .write(b); if (b == '\n') { log(count - 1); } } @Override public synchronized void write( byte [] b, int off, int len) { for ( int i = 0; i < len; ++i) { write(b[off + i]); } }
        Hide
        Matt Sicker added a comment -

        I was going to change the code to just find the first non-whitespace character from the end of the buffer, but the code got patched before I had a chance to do it.

        I know Mac doesn't use it anymore, but '\r' used to be a legit end of line character, too. I think finding the last non-whitespace character would be a good end of line. It covers all edge cases.

        Show
        Matt Sicker added a comment - I was going to change the code to just find the first non-whitespace character from the end of the buffer, but the code got patched before I had a chance to do it. I know Mac doesn't use it anymore, but '\r' used to be a legit end of line character, too. I think finding the last non-whitespace character would be a good end of line. It covers all edge cases.
        Hide
        Remko Popma added a comment -

        Ok, what if someone does this:

        stream.print("something\r");
        

        ... and then never uses the stream anymore? (So no following characters?)

        Show
        Remko Popma added a comment - Ok, what if someone does this: stream.print( "something\r" ); ... and then never uses the stream anymore? (So no following characters?)
        Hide
        Matt Sicker added a comment -

        Are there any existing tests that show off caller data? I think this would be an important feature to keep unit tested for this interface as well.

        Show
        Matt Sicker added a comment - Are there any existing tests that show off caller data? I think this would be an important feature to keep unit tested for this interface as well.

          People

          • Assignee:
            Unassigned
            Reporter:
            Joe Merten
          • Votes:
            1 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development