2004 reduce logging bigints #2406
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I'm considering updating the DeFramer unit test to mock out the logger and check that it is called as expected in cases of valid and invalid HELLOs |
added 3 commits
June 10, 2021 13:27
Signed-off-by: Justin Florentine <[email protected]>
…e body of the corrupt HELLO Signed-off-by: Justin Florentine <[email protected]>
Signed-off-by: Justin Florentine <[email protected]>
jflo
force-pushed
the
2004-Reduce-logging-bigints
branch
from
aecd26d to
7dff2ad
Compare
Signed-off-by: Justin Florentine <[email protected]>
jflo
force-pushed
the
2004-Reduce-logging-bigints
branch
from
a4f9dae to
4da65d8
Compare
matkt
reviewed
Jun 11, 2021
| @@ -109,12 +109,12 @@ protected void decode(final ChannelHandlerContext ctx, final ByteBuf in, final L | |||
| try { | |||
| peerInfo = HelloMessage.readFrom(message).getPeerInfo(); | |||
| } catch (final RLPException e) { | |||
| LOG.debug("Received invalid HELLO message", e); | |||
| LOG.warn("Received invalid HELLO message, set log level to TRACE for message body", e); | |||
There was a problem hiding this comment.
Are we sure that passing this log in warning does not risk spamming the logs of a node running on the mainnet or on ropsten etc ? Do we often receive invalid messages of this type ?
There was a problem hiding this comment.
Great point. I looked at log aggregation for all of our canaries and only see 1 occurrence of this exception. This should not result in logspam at warn instead of debug.
1 task
eum602
pushed a commit
to lacchain/besu
that referenced
this pull request
Nov 3, 2023
* seems like a reasonable use for TRACE level instead of DEBUG * hint to user they should adjust logging if they really want to see the body of the corrupt HELLO * include actual nodeId length in error message Signed-off-by: Justin Florentine <[email protected]> Co-authored-by: Justin Florentine <[email protected]> Co-authored-by: matkt <[email protected]> Co-authored-by: garyschulte <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR description
Avoids logging (potentially large) corrupt HELLO messages when connecting with peers.
Fixed Issue(s)
fixes #2004
Changelog
Failures to parse HELLO now include byte length differences on nodeId in the exception message, as well as guidance to adjust the debug level to trace if user is really interested in seeing the entire peerInfo.