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
Changes from 2 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
86 changes: 79 additions & 7 deletions src/main/java/com/jcabi/log/VerboseProcess.java
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,16 @@ public final class VerboseProcess implements Closeable {
*/
private transient boolean closed;

/**
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 static fields should always go before non-static ones

* Maximum number of log lines for a stack trace
*/
public static final int DEFUALT_MAX_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 public static final should not be used at all: http://www.yegor256.com/2015/07/06/public-static-literals.html


/**
* Maximum number of log lines for 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 according to http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html, first sentence should alsways end with period

*/
public static volatile int maxStackLength = DEFUALT_MAX_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 somehow avoid static mutable data that is visible for everyone?


/**
* Public ctor.
* @param prc The process to work with
Expand Down Expand Up @@ -281,7 +291,7 @@ private String stdout(final boolean check) {
this.process, result.code(), result.stdout().length(),
System.currentTimeMillis() - start
);
if (check && result.code() != 0) {
if (check && (result.code() != 0)) {
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 this change was needed?

Copy link
Author

Choose a reason for hiding this comment

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

auto-formatting in my IDE. i'll back it out since it's just cosmetic

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, please just address this comment

throw new IllegalArgumentException(
Logger.format(
"Non-zero exit code %d: %[text]s",
Expand Down Expand Up @@ -389,6 +399,20 @@ private static void close(final Closeable res) {
* Stream monitor.
*/
private static final class Monitor implements Callable<Void> {

/**
* Prefix "at "
*/
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"
*/
private static final String PREFIX_CB = "Caused by";
/**
* Prefix "... "
*/
private static final String PREFIX_DOTS = "... ";

/**
* Stream to read.
*/
Expand Down Expand Up @@ -420,6 +444,34 @@ private static final class Monitor implements Callable<Void> {
this.output = out;
this.level = lvl;
}

/**
* Checks if line is part of a stack trace and should be appended.
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 does it mean that stactraces can have just these 3 prefixes and otherwise they are not stacktraces?

Copy link
Author

@dean-e-clark dean-e-clark Sep 10, 2016

Choose a reason for hiding this comment

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

@mkordas It's not what they start with, but it's what comes after the initial 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 OK, thanks for explaining

* @param string String to check
* @return boolean result
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 result should be changed to Result - there is no particular reason behind that, just the common style followed in this repo

*/
private static boolean shouldAppend(final String 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 can you come up with other name for the variable? String string sounds a bit strange ;)

final String leftStrip = stripStart(string);
return leftStrip.startsWith(PREFIX_AT) || leftStrip.startsWith(PREFIX_CB) || leftStrip.startsWith(PREFIX_DOTS);
Copy link

Choose a reason for hiding this comment

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

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

}

/**
* Strips whitespace at beginning of 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 I think string here could be lowercase :)

* @param string String to strip
* @return string Stripped String
*/
private static String stripStart(final String string) {
if ((string == null) || string.isEmpty()) {
return 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 null must be eliminated from the code completely, see http://www.yegor256.com/2014/05/13/why-null-is-bad.html

}
final int stringLength = string.length();
Copy link

Choose a reason for hiding this comment

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

int start = 0;
while ((start != stringLength) && Character.isWhitespace(string.charAt(start))) {
start++;
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 here it doesn't matter, but generally I'd prefer ++start

}
return string.substring(start);
}

@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(
Expand All @@ -433,6 +485,9 @@ public Void call() throws Exception {
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?

StringBuilder sb = new StringBuilder();
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 local variables could be just one word like builder

String previousLine = 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 here as well, it should be just previous

int lineCount = 0;
Copy link

Choose a reason for hiding this comment

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

while (true) {
if (Thread.interrupted()) {
Logger.debug(
Expand All @@ -441,16 +496,33 @@ public Void call() throws Exception {
);
break;
}
if (previousLine != null) {
sb.append(previousLine).append(System.getProperty("line.separator"));
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 System.getProperty("line.separator") is duplicated, can we extract it to a variable?

previousLine = 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 again, this PR cannot have null at all, it's billion-dollar mistake 😉

}

Copy link

Choose a reason for hiding this comment

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

final String line = reader.readLine();
if (line == null) {
if (sb.length() > 0) {
final String logText = sb.toString();
Logger.log(this.level, VerboseProcess.class, ">> %s", logText);
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 >> %s is duplicated, it should be extracted to common variable

writer.write(logText);
}
break;
}
Logger.log(
this.level, VerboseProcess.class,
">> %s", line
);
writer.write(line);
writer.newLine();

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 methods are not allowed to have any empty lines

if (shouldAppend(line) && (++lineCount < maxStackLength)) {
sb.append(line).append(System.getProperty("line.separator"));
} else {
if (sb.length() > 0) {
final String logText = sb.toString();
Logger.log(this.level, VerboseProcess.class, ">> %s", logText);
writer.write(logText);
sb = new StringBuilder();
}
lineCount = 1;
previousLine = line;
}
}
} catch (final ClosedByInterruptException ex) {
Thread.interrupted();
Expand Down