Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#87 Grouping stack traces into single log statements for sub-processes #90

Open
wants to merge 46 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 33 commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
3bd48e8
https://github.com/jcabi/jcabi-log/issues/87
dean-e-clark Sep 8, 2016
a52948b
Updating formatting standards
dean-e-clark Sep 8, 2016
828ee4f
Adding some styling changes and a new test
dean-e-clark Sep 10, 2016
534bb5c
Adding missing class
dean-e-clark Sep 10, 2016
fcb0007
Styling updates
dean-e-clark Sep 10, 2016
80df3b6
Adding more styling changes
dean-e-clark Sep 10, 2016
2975b06
More styling updates
dean-e-clark Sep 10, 2016
1d48789
Hopefully final styling changes
dean-e-clark Sep 10, 2016
fed0c7d
More styling changes
dean-e-clark Sep 10, 2016
256d4f3
Light refactoring and some more styling changes
dean-e-clark Sep 10, 2016
6e07c94
More styling changes
dean-e-clark Sep 10, 2016
a3bfd27
Styling
dean-e-clark Sep 10, 2016
12c800b
More styling
dean-e-clark Sep 10, 2016
0516292
Even more styling
dean-e-clark Sep 10, 2016
cc2ab93
Last styling?
dean-e-clark Sep 10, 2016
e2a9c1b
Fixing typo to fix test
dean-e-clark Sep 10, 2016
0c00490
Further styling updates
dean-e-clark Sep 12, 2016
ed7a354
Another styling tweak
dean-e-clark Sep 12, 2016
5c92779
Styling and trying to fix test timing issue
dean-e-clark Sep 12, 2016
c2873ae
More styling
dean-e-clark Sep 12, 2016
9f257d7
Styling changes to make Travis CI happy
dean-e-clark Sep 12, 2016
1ea04f2
Changing variable name
dean-e-clark Sep 12, 2016
bbc143b
More styling changes to make Travis CI happy
dean-e-clark Sep 12, 2016
9ad8ac3
More styling updates
dean-e-clark Sep 13, 2016
74be2fa
Even more styling updates
dean-e-clark Sep 13, 2016
8a10cf9
More styling updates
dean-e-clark Sep 13, 2016
2a1b9cc
Removing duplicate string
dean-e-clark Sep 13, 2016
4a4a465
Avoiding ConsecutiveLiteralAppends with String.format()
dean-e-clark Sep 13, 2016
85cfd53
Refactored appending lines and fixed a PMD violation
dean-e-clark Sep 13, 2016
4dc1ca1
Modifying javadoc to try to resolve PMD violation
dean-e-clark Sep 13, 2016
6b8e6f4
Trying to get around false positive PMD issue
dean-e-clark Sep 13, 2016
6deffec
Removing ClosedByInterruptException per Michal's request
dean-e-clark Sep 13, 2016
c8c1efa
Fixing typo, adding back catch statement, and other minor styling
dean-e-clark Sep 13, 2016
df9445a
Making additional styling changes
dean-e-clark Sep 19, 2016
63c19a2
Removing unused constants
dean-e-clark Sep 19, 2016
8e4a50f
PMD styling change
dean-e-clark Sep 19, 2016
d14e4da
Combining assertions in test and replacing boolean with Assert.fail()
dean-e-clark Dec 1, 2016
ac89ad5
Merge remote-tracking branch 'upstream/master'
dean-e-clark Dec 1, 2016
c796d56
Merge remote-tracking branch 'upstream/master'
dean-e-clark Dec 1, 2016
399c3ed
Merge branch 'master' of https://github.com/dean-e-clark/jcabi-log.git
dean-e-clark Dec 1, 2016
c4d4ac1
Styling changes
dean-e-clark Dec 1, 2016
0b0ebe6
More styling
dean-e-clark Dec 1, 2016
308aec4
Removing trailing space
dean-e-clark Dec 1, 2016
61d46ab
Suppressing imports PMD warning
dean-e-clark Dec 1, 2016
699ad4d
Merge branch 'master' into master
dean-e-clark Dec 20, 2017
25f2849
Merge branch 'master' into master
dean-e-clark Dec 20, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
156 changes: 135 additions & 21 deletions src/main/java/com/jcabi/log/VerboseProcess.java
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,38 @@ private static void close(final Closeable res) {
* Stream monitor.
*/
private static final class Monitor implements Callable<Void> {

/**
* Maximum number of log lines for a stack trace. Set to 1000
* to avoid storing too many lines in memory before flushing.
*/
private static final int MAX_STACK_LENGTH = 1000;
/**
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dean-e-clark fields should be separated by empty line

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dean-e-clark can you also separate these constants with empty lines?

* Prefix "at ". Needed to check if line is part of stack trace.
*/
private static final String PREFIX_AT = "at ";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dean-e-clark this looks like http://www.yegor256.com/2015/09/01/redundant-variables-are-evil.html, can you inline all redundant variables?

/**
* Prefix "Caused by". Needed to check if line is part of stack trace.
*/
private static final String PREFIX_CB = "Caused by";
/**
* Prefix "... ". Needed to check if line is part of stack trace.
*/
private static final String PREFIX_DOTS = "... ";
/**
* Format string for log statements.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dean-e-clark can you give more information here why we need it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkordas we need those three String constants to identify if lines are part of a stack trace

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dean-e-clark right, so can you add this info to Javadoc

*/
private static final String LOG_FORMAT = ">> %s";
/**
* Empty String.
*/
private static final String EMPTY_STRING = "";
/**
* Newline string.
*/
private static final String NEW_LINE = System.getProperty(
"line.separator"
);
/**
* Stream to read.
*/
Expand Down Expand Up @@ -420,38 +452,20 @@ private static final class Monitor implements Callable<Void> {
this.output = out;
this.level = lvl;
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dean-e-clark I didn't mean to remove this logic. I just meant that when you had

throws IOException, ClosedByInterruptException

the second exception was unncecessary, as ClosedByInterruptException is already IOException

@Override
public Void call() throws Exception {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dean-e-clark this method has become massive, it needs to be split across many private ones

final BufferedReader reader = new BufferedReader(
Channels.newReader(
Channels.newChannel(this.input),
VerboseProcess.UTF_8
Channels.newChannel(this.input), VerboseProcess.UTF_8
)
);
try {
final BufferedWriter writer = new BufferedWriter(
new OutputStreamWriter(this.output, VerboseProcess.UTF_8)
);
try {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dean-e-clark for so complex logic added in this PR I'd expect at least dozen of unit tests, but I don't see any 😞

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have you read this: http://www.yegor256.com/2017/03/24/tdd-that-works.html?

What about writing unit tests only when bugs will appear?

while (true) {
if (Thread.interrupted()) {
Logger.debug(
VerboseProcess.class,
"explicitly interrupting read from buffer"
);
break;
}
final String line = reader.readLine();
if (line == null) {
break;
}
Logger.log(
this.level, VerboseProcess.class,
">> %s", line
);
writer.write(line);
writer.newLine();
}
this.logFromReader(reader, writer);
} catch (final ClosedByInterruptException ex) {
Thread.interrupted();
Logger.debug(
Expand All @@ -473,6 +487,106 @@ public Void call() throws Exception {
}
return null;
}

/**
* Logs supplied StringBuilder to supplied Logger and Writer.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dean-e-clark this sounds a bit strange, we are not supplying any Logger here, just reader and writer

* @param reader Reader to use
* @param writer Writer to use
* @throws IOException writer could throw this
*/
private void logFromReader(final BufferedReader reader,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dean-e-clark Javadoc is missing here

final BufferedWriter writer) throws IOException {
final StringBuilder builder = new StringBuilder();
String previous = EMPTY_STRING;
int count = 0;
while (true) {
if (Thread.interrupted()) {
Logger.debug(
VerboseProcess.class,
"explicitly interrupting read from buffer"
);
break;
}
if (!previous.isEmpty()) {
appendLine(previous, builder);
previous = EMPTY_STRING;
}
final String line = reader.readLine();
if (line == null) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dean-e-clark as I mentioned before, null is prohibited

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkordas this is from the existing code, and I can't control how readLine() works

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dean-e-clark you are right - sorry, I didn't notice

logAndClear(writer, this.level, builder);
break;
}
if (shouldAppend(line)
&& count < MAX_STACK_LENGTH) {
appendLine(line, builder);
++count;
} else {
if (builder.length() > 0) {
logAndClear(writer, this.level, builder);
}
count = 1;
previous = line;
}
}
}

/**
* Appends a line and a newline character to the builder.
* @param line String to append
* @param builder StringBuilder with log statement
*/
private static void appendLine(final String line,
final StringBuilder builder) {
builder.append(line);
builder.append(NEW_LINE);
}

/**
* Logs StringBuilder to supplied Logger and Writer then clears out
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dean-e-clark again, we do not supply Logger here as parameter

* the builder.
* @param writer Writer to use
* @param level Level to log at
* @param builder StringBuilder with log statement
* @throws IOException writer could throw this
*/
private static void logAndClear(final BufferedWriter writer,
final Level level, final StringBuilder builder)
throws IOException {
if (builder.length() > 0) {
final String text = builder.toString();
Logger.log(level, VerboseProcess.class, LOG_FORMAT, text);
writer.write(text);
}
builder.setLength(0);
}

/**
* Checks if line is part of a stack trace and should be appended.
* @param input String to check
* @return Result, true or false
*/
private static boolean shouldAppend(final String input) {
final String stripped = stripStart(input);
return stripped.startsWith(PREFIX_AT)
|| stripped.startsWith(PREFIX_CB)
|| stripped.startsWith(PREFIX_DOTS);
}

/**
* Strips whitespace at beginning of String.
* @param input String to strip
* @return Stripped string
*/
private static String stripStart(final String input) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dean-e-clark by the way, can we do just .trim() on the string?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dean-e-clark this method should be named like a noun, please read http://www.yegor256.com/images/books/elegant-objects/seven-pages.pdf

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkordas removed this in favor of String.trim()

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dean-e-clark I don't see your changes, can you please push them?

final int length = input.length();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dean-e-clark can we inline this variable?

int start = 0;
while (start != length
&& Character.isWhitespace(input.charAt(start))) {
++start;
}
return input.substring(start);
}

}

/**
Expand Down
13 changes: 13 additions & 0 deletions src/test/java/com/jcabi/log/SupplierLoggerTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import org.apache.log4j.Level;
import org.hamcrest.MatcherAssert;
import org.hamcrest.Matchers;
import org.junit.BeforeClass;
import org.junit.Test;

/**
Expand All @@ -43,6 +44,18 @@
@SuppressWarnings("PMD.MoreThanOneLogger")
public final class SupplierLoggerTest {

/**
* Logs before method to avoid timing issue where loggers were
* created "during the default configuration phase of the underlying
* logging system" and failing tests in Travis CI.
*/
@BeforeClass
public static void init() {
final org.apache.log4j.Logger logger =
org.apache.log4j.Logger.getRootLogger();
logger.info("Logging here to avoid timing issue with Travis CI");
}

/**
* SupplierLogger can tell if debug is disabled and the message is not
* logged enabled.
Expand Down
119 changes: 119 additions & 0 deletions src/test/java/com/jcabi/log/VerboseProcessExample.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
/**
* Copyright (c) 2012-2015, jcabi.com
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
* are met: 1) Redistributions of source code must retain the above
* copyright notice, this list of conditions and the following
* disclaimer. 2) Redistributions in binary form must reproduce the above
* copyright notice, this list of conditions and the following
* disclaimer in the documentation and/or other materials provided
* with the distribution. 3) Neither the name of the jcabi.com nor
* the names of its contributors may be used to endorse or promote
* products derived from this software without specific prior written
* permission.
*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
* "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT
* NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND
* FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
* THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
* INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
* (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
* SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
* HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
* STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
* ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED
* OF THE POSSIBILITY OF SUCH DAMAGE.
*/
package com.jcabi.log;

/**
* Example application to help test {@link VerboseProcess}.
* @author Dean Clark ([email protected])
* @version $Id$
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dean-e-clark we also need @since tag here

*/
@SuppressWarnings("PMD.DefaultPackage")
public final class VerboseProcessExample {

/**
* System output line 1.
*/
static final String SYSOUT_1 = "sysout line 1";

/**
* System output line 2.
*/
static final String SYSOUT_2 = "sysout line 2";

/**
* Error output line 1.
*/
static final String SYSERR_1 = "syserr line 1";

/**
* Error output line 2.
*/
static final String SYSERR_2 = "syserr line 2";

/**
* Exception to be thrown and caught.
*/
static final String CAUGHT_ERR_MSG = "throw/catch me";

/**
* Exception to be thrown.
*/
static final String THROWN_ERR_MSG = "just throw me";

/**
* Private constructor. Intentionally empty.
*/
private VerboseProcessExample() { }

/**
* Instantiates instance of this class and calls primary method.
* @param args Any args passed to main method
*/
public static void main(final String... args) {
final VerboseProcessExample instance = new VerboseProcessExample();
instance.doWork();
}

/**
* Will log to standard output and error and then intentionally fail with a
* stack trace.
*/
@SuppressWarnings("PMD.SystemPrintln")
private void doWork() {
System.out.println(SYSOUT_1);
System.err.println(SYSERR_1);
System.out.println(SYSOUT_2);
System.err.println(SYSERR_2);
catchAndThrow();
}

/**
* Call a method which will throw an exception. Then catch and re-throw it.
*/
private static void catchAndThrow() {
try {
countdownAndThrow(2);
} catch (final IllegalStateException ex) {
throw new IllegalStateException(THROWN_ERR_MSG, ex);
}
}

/**
* Recursively loops and then throws an exception.
* @param loops Times to loop
*/
private static void countdownAndThrow(final int loops) {
if (loops == 0) {
throw new IllegalStateException(CAUGHT_ERR_MSG);
}
countdownAndThrow(loops - 1);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dean-e-clark why not just --loops?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkordas loops is final

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dean-e-clark right, thanks for explaining

}

}
Loading