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.

Sign up for GitHub

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

Jump to bottom

Reduce logging messages on info level #418

Merged
merged 2 commits into from
Jan 22, 2018
Merged

Reduce logging messages on info level #418

merged 2 commits into from
Jan 22, 2018

Conversation

lsebrie
Copy link
Contributor

@lsebrie lsebrie commented Jan 15, 2018

Many logs were throw on info level and it makes hard to get what's happening on the node by reading logs.
Most of them were kept and demoted to trace so still can be used for debugging but are not useful for the regular user.
Still there are some logs coming from external libraries that should be turned off or silenced from logback.
There are some minor fixes on string formats or incompatible protocol.

diega
diega reviewed Jan 15, 2018
View reviewed changes
rskj-core/src/main/java/org/ethereum/net/rlpx/MessageCodec.java Outdated
if (loggerNet.isInfoEnabled()) {
loggerNet.info("From: \t{} \tRecv: \t{}", channel, msg.toString());
}
loggerNet.trace("From: \t{} \tRecv: \t{}", channel, msg.toString());
Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove this .toString() b/c this call will be made even if the TRACE level is not enabled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

rskj-core/src/main/java/org/ethereum/net/rlpx/FrameCodecHandler.java Outdated
@@ -85,8 +85,7 @@ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws E
loggerNet.debug("FrameCodec failed: ", cause);
} else {
if (cause instanceof IOException) {
loggerNet.info("FrameCodec failed: " + ctx.channel().remoteAddress() + "(" + cause.getMessage() + ")");
loggerNet.debug("FrameCodec failed: " + ctx.channel().remoteAddress(), cause);
loggerNet.info("FrameCodec failed: {} ({})", ctx.channel().remoteAddress(), cause);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would recheck if passing a Throwable as last parameter for the logger methods doesn't have a different semantic in the terms of how this is resolved as part of the logging message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'll keep the trace with throwable as last parameter. It has another behavior 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

I mentioned this b/c of the second {} in the message marker, I'm not sure if it'll use the throwable as part of the message or will just make it available as %ex

rskj-core/src/main/java/org/ethereum/core/TransactionExecutor.java Outdated
if (logger.isInfoEnabled()) {
logger.info("Paying: txGasCost: [{}], gasPrice: [{}], gasLimit: [{}]", txGasCost, toBI(tx.getGasPrice()), txGasLimit);
}
logger.trace("Paying: txGasCost: [{}], gasPrice: [{}], gasLimit: [{}]", txGasCost, toBI(tx.getGasPrice()), txGasLimit);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should keep a .isTraceEnabled() in place, this way the toBI() will not be invoked when the TRACE level is not enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

toBi() shouldn't be expensive but we can keep the check

diega
diega previously approved these changes Jan 15, 2018
View reviewed changes
@aeidelman
Copy link
Collaborator

@lsebrie please fix conflicts

@lsebrie lsebrie force-pushed the logging_levels branch 2 times, most recently from df776f0 to c5ffca1 Compare January 17, 2018 15:12
diega
diega previously approved these changes Jan 17, 2018
View reviewed changes
aeidelman
aeidelman reviewed Jan 19, 2018
View reviewed changes
rskj-core/src/main/java/org/ethereum/core/Transaction.java Outdated
@@ -248,8 +248,6 @@ public void rlpParse() {
byte[] r = transaction.get(7).getRLPData();
byte[] s = transaction.get(8).getRLPData();
this.signature = ECDSASignature.fromComponents(r, s, getRealV(v));
} else {
logger.debug("RLP encoded tx is not signed!");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Although I agree this line should be removed, it was still not clear what was causing it to be logged so many times. I suggest not removing it at this stage, and I'll add a new issue to go deeper into this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe move it to .trace()?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, @diega is right, we can move it to trace. If we can add a tx id it will be useful too, however this could also be part of investigating the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, we keep it as trace 👍

@rskops
Copy link
Collaborator

rskops commented Jan 22, 2018

SonarQube analysis reported 177 issues

  • CRITICAL 4 critical
  • MAJOR 107 major
  • MINOR 27 minor
  • INFO 39 info

Top 10 issues

  1. CRITICAL MinerServerImpl.java#L342: Make the enclosing method "static" or remove this set. rule
  2. CRITICAL MinerServerImpl.java#L346: Make the enclosing method "static" or remove this set. rule
  3. CRITICAL ChannelManagerImpl.java#L320: This call to "add()" may be a performance hot spot if the collection is large. rule
  4. CRITICAL ChannelManagerImpl.java#L328: This call to "remove()" may be a performance hot spot if the collection is large. rule
  5. MAJOR BlockChainImpl.java#L100: Constructor has 8 parameters, which is greater than 7 authorized. rule
  6. MAJOR BlockChainImpl.java#L117: Constructor new co.rsk.core.bc.BlockChainImpl(RskSystemProperties, Repository, BlockStore, ReceiptStore, PendingState, EthereumListener, AdminInfo, BlockValidator) makes call to non-final method rule
  7. MAJOR BlockChainImpl.java#L201: The Cyclomatic Complexity of this method "internalTryToConnect" is 22 which is greater than 10 authorized. rule
  8. MAJOR BlockChainImpl.java#L203: Method co.rsk.core.bc.BlockChainImpl.internalTryToConnect(Block) appears to call the same method on the same object redundantly rule
  9. MAJOR BlockChainImpl.java#L315: Method co.rsk.core.bc.BlockChainImpl.internalTryToConnect(Block) appears to call the same method on the same object redundantly rule
  10. MAJOR BlockChainImpl.java#L374: Add a nested comment explaining why this method is empty, throw an UnsupportedOperationException or complete the implementation. rule

aeidelman
aeidelman approved these changes Jan 22, 2018
View reviewed changes
Copy link
Collaborator

@aeidelman aeidelman left a comment

Choose a reason for hiding this comment

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

Approved

@aeidelman aeidelman merged commit 4892bfd into master Jan 22, 2018
@aeidelman aeidelman deleted the logging_levels branch January 22, 2018 21:39
@aeidelman aeidelman added this to the Bamboo v0.4.1 milestone Jan 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@diega diega diega left review comments

@aeidelman aeidelman aeidelman approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Bamboo 0.4.1
Development

Successfully merging this pull request may close these issues.
4 participants
@lsebrie @aeidelman @rskops @diega