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

        Joe Merten created issue -
        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.
        Matt Sicker made changes -
        Field Original Value New Value
        Attachment Add_PrintStream_view_LOG4J2-481.patch [ 12621459 ]
        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.
        Matt Sicker made changes -
        Attachment Add_PrintStream_view_LOG4J2-481.patch [ 12621512 ]
        Matt Sicker made changes -
        Attachment Add_PrintStream_view_LOG4J2-481.patch [ 12621459 ]
        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.
        Matt Sicker made changes -
        Attachment EventLoggerTest.patch [ 12621513 ]
        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.
        Matt Sicker made changes -
        Attachment 0001-Add-LoggerStream-and-tests.patch [ 12621535 ]
        Matt Sicker made changes -
        Attachment Add_PrintStream_view_LOG4J2-481.patch [ 12621512 ]
        Matt Sicker made changes -
        Attachment EventLoggerTest.patch [ 12621513 ]
        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.
        Ralph Goers made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Fix Version/s 2.0-rc1 [ 12325011 ]
        Resolution Fixed [ 1 ]
        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); } }
        Remko Popma made changes -
        Resolution Fixed [ 1 ]
        Status Resolved [ 5 ] Reopened [ 4 ]
        Hide
        Ralph Goers added a comment -

        Resolved in revision 1557619.

        Show
        Ralph Goers added a comment - Resolved in revision 1557619.
        Ralph Goers made changes -
        Status Reopened [ 4 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        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.
        Matt Sicker made changes -
        Attachment 0001-Remove-dead-code.patch [ 12622555 ]
        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.
        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Resolved Resolved
        14d 7h 34m 1 Ralph Goers 12/Jan/14 02:51
        Resolved Resolved Reopened Reopened
        13h 1m 1 Remko Popma 12/Jan/14 15:53
        Reopened Reopened Resolved Resolved
        6h 39m 1 Ralph Goers 12/Jan/14 22:32

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development