Common format for transaction logging #423

lsebrie · 2018-01-17T18:56:21Z

Transactions now use a basic format tx=transaction-hash

colltoaction · 2018-01-17T22:04:25Z

rskj-core/src/main/java/co/rsk/mine/MinerUtils.java


                    txsToRemove.add(tx);
                    continue;
                }

                if (!expectedNonce.equals(txNonce)) {
-                    logger.warn("Invalid nonce, expected {}, found {}, tx {}", expectedNonce.toString(), txNonce.toString(), Hex.toHexString(tx.getHash()));
+                    logger.warn("Invalid nonce, expected {}, found {}, tx={}", expectedNonce, txNonce, hexHash);


here we log the BigInteger nonce, vs the Hex.toHexString(nonce) before. we should make it consistent.

colltoaction · 2018-01-17T22:04:47Z

rskj-core/src/main/java/co/rsk/mine/MinerUtils.java

-                String hash = null == tx.getHash() ? "" : Hex.toHexString(tx.getHash());
-                logger.warn("Error when processing transaction: " + hash, e);
+                String hash = null == tx.getHash() ? "" : Hex.toHexString(tx.getHash());;
+                logger.warn(String.format("Error when processing tx=%s" + hash), e);


there's a bug here. you're concatenating instead of formatting.

colltoaction · 2018-01-17T22:05:11Z

rskj-core/src/main/java/co/rsk/mine/MinerUtils.java

            } catch (Exception e) {
                // Txs that can't be selected by any reason should be removed from pending state
-                String hash = null == tx.getHash() ? "" : Hex.toHexString(tx.getHash());
-                logger.warn("Error when processing transaction: " + hash, e);
+                String hash = null == tx.getHash() ? "" : Hex.toHexString(tx.getHash());;


remove extra semicolon

aeidelman · 2018-01-18T19:22:27Z

rskj-core/src/main/java/co/rsk/validators/TxsMinGasPriceRule.java

-                    panicProcessor.panic("invalidmingasprice", "Tx gas price is under the Min gas Price of the block");
+                    String logMessage = String.format("Tx gas price is under the Min gas Price of the block tx=%s", Hex.toHexString(tx.getHash()));
+                    logger.warn(logMessage);
+                    panicProcessor.panic("invalidmingasprice", logMessage);


This panic can be removed

I'll remove it in the other PR 😅

ajlopez · 2018-01-19T11:13:07Z

rskj-core/src/main/java/co/rsk/mine/MinerUtils.java

            } catch (Exception e) {
                // Txs that can't be selected by any reason should be removed from pending state
                String hash = null == tx.getHash() ? "" : Hex.toHexString(tx.getHash());
-                logger.warn("Error when processing transaction: " + hash, e);
+                logger.warn(String.format("Error when processing tx=%s", hash), e);


Any reason to use String.format instead of {} in log message?

It's because if we use {} we lose the ability to hold the stack trace of the exception. The method that allows a Throwable only allows sending a plain String

ajlopez · 2018-01-19T11:14:55Z

rskj-core/src/main/java/co/rsk/validators/TxsMinGasPriceRule.java

@@ -49,8 +50,9 @@ public boolean isValid(Block block) {
        if(CollectionUtils.isNotEmpty(block.getTransactionsList())) {
            for (Transaction tx : txs) {
                if (!(tx instanceof RemascTransaction) && tx.getGasPriceAsInteger().compareTo(blockMgp) < 0) {
-                    logger.warn("Tx gas price is under the Min gas Price of the block");
-                    panicProcessor.panic("invalidmingasprice", "Tx gas price is under the Min gas Price of the block");
+                    String logMessage = String.format("Tx gas price is under the Min gas Price of the block tx=%s", Hex.toHexString(tx.getHash()));


Same comment, why use String.format in one place, and log message with {} in other places?

Here the idea was to avoid doing string formatting 2 times for warning and panic. Maybe in the other PR this panic disappears and makes less sense doing things like this.

ajlopez · 2018-01-19T11:16:53Z

rskj-core/src/main/java/org/ethereum/core/ImmutableTransaction.java

    }

    @Override
    public void setGasLimit(byte[] gasLimit) {
-        throw new ImmutableTransactionException("trying to set gas limit");
+        throw new ImmutableTransactionException(String.format("trying to set gas limit tx=%s", Hex.toHexString(this.getHash())));


The String.format FORCES the execution and concatenation of the references expressions. Log message with {} only invokes .toString when the message level is on.

Here I'm adding some info on the Exception, I was tryng to avoid simple concatenation

rskops · 2018-02-02T18:08:05Z

SonarQube analysis reported 27 issues

17 major
4 minor
6 info

Top 10 issues

MinerUtils.java#L71: Define and throw a dedicated exception instead of using a generic one.
MinerUtils.java#L109: Define and throw a dedicated exception instead of using a generic one.
MinerUtils.java#L120: Define and throw a dedicated exception instead of using a generic one.
MinerUtils.java#L196: Method co.rsk.mine.MinerUtils.filterTransactions(List, List, Map, Repository, BigInteger) does not presize the allocation of a collection
TxsMinGasPriceRule.java#L39: Remove this unused "panicProcessor" private field.
HandshakeHandler.java#L101: Method org.ethereum.net.rlpx.HandshakeHandler.decode(ChannelHandlerContext, ByteBuf, List) passes a concatenated string to SLF4J's format string
HandshakeHandler.java#L142: The Cyclomatic Complexity of this method "decodeHandshake" is 15 which is greater than 10 authorized.
HandshakeHandler.java#L142: Define and throw a dedicated exception instead of using a generic one.
HandshakeHandler.java#L160: Either log or rethrow this exception.
HandshakeHandler.java#L180: Method org.ethereum.net.rlpx.HandshakeHandler.decodeHandshake(ChannelHandlerContext, ByteBuf) passes a concatenated string to SLF4J's format string

aeidelman

Approved

colltoaction reviewed Jan 17, 2018

View reviewed changes

aeidelman reviewed Jan 18, 2018

View reviewed changes

ajlopez suggested changes Jan 19, 2018

View reviewed changes

lsebrie force-pushed the tx_logging_format branch 2 times, most recently from be4d638 to 02300c1 Compare January 22, 2018 15:41

aeidelman added this to the Bamboo v0.4.1 milestone Jan 22, 2018

ajlopezrsk previously approved these changes Feb 1, 2018

View reviewed changes

lsebrie added 2 commits February 2, 2018 14:55

Common format for transaction logging

42c097b

Minor fixes on MinerUtils

a639f95

lsebrie dismissed ajlopezrsk’s stale review via a639f95 February 2, 2018 17:55

lsebrie force-pushed the tx_logging_format branch from 02300c1 to a639f95 Compare February 2, 2018 17:55

aeidelman approved these changes Feb 2, 2018

View reviewed changes

aeidelman merged commit 1ee92ae into master Feb 2, 2018

aeidelman deleted the tx_logging_format branch February 2, 2018 18:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Common format for transaction logging #423

Common format for transaction logging #423

lsebrie commented Jan 17, 2018

colltoaction Jan 17, 2018

lsebrie Jan 18, 2018

colltoaction Jan 17, 2018

lsebrie Jan 18, 2018

colltoaction Jan 17, 2018

lsebrie Jan 18, 2018

aeidelman Jan 18, 2018

lsebrie Jan 18, 2018

aeidelman Jan 18, 2018

ajlopez Jan 19, 2018

lsebrie Jan 19, 2018

ajlopez Jan 19, 2018

lsebrie Jan 19, 2018

ajlopez Jan 19, 2018

lsebrie Jan 19, 2018

rskops commented Feb 2, 2018

aeidelman left a comment

Common format for transaction logging #423

Common format for transaction logging #423

Conversation

lsebrie commented Jan 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rskops commented Feb 2, 2018

Top 10 issues

aeidelman left a comment

Choose a reason for hiding this comment