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

DNM: logging ux changes based on new logging policy #5457

Closed
wants to merge 5 commits into from
Closed

DNM: logging ux changes based on new logging policy #5457

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
52748e4
"Unclean disconnect" candidate
philknows May 3, 2023
b16000d
Block error logging verbose
philknows May 3, 2023
6f133c3
Discuss Service Unavailable: Node syncing
philknows May 3, 2023
330417b
failed to download attester duties verbose
philknows May 3, 2023
a8596ce
Failed to download SyncDuties verbose
philknows May 3, 2023
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
2 changes: 1 addition & 1 deletion packages/beacon-node/src/chain/blocks/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ export async function processBlocks(
if (!(err instanceof BlockError)) {
this.logger.error("Non BlockError received", {}, err);
} else if (!opts.disableOnBlockError) {
this.logger.error("Block error", {slot: err.signedBlock.message.slot}, err);
this.logger.warn("Block error", {slot: err.signedBlock.message.slot}, err);
Copy link
Member Author

Choose a reason for hiding this comment

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

Reference: #3062
Block errors are sometimes expected, especially for unstable/testnet networks and error doesn't feel like the correct verbosity as the user cannot be expected to do something about it. As mentioned in the issue, warn or info may be more appropriate. Should we also consider removing the stacktrace?

Copy link
Contributor

Choose a reason for hiding this comment

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

This log here does not make much sense to be above debug level IF ONLY the consumers handle the error properly. Invalid blocks can be submitted via:

  • gossip: a peer is free to submit and invalid block. That's an uncontrolled event the user can't do much about, so we may want to not log it at all. However, if there's a consensus bug the node will just freeze without warning
  • sync: same as above
  • API: this should never happen as long as the API is trusted. The API also logs errors so this will probably be a duplicate

For now, let's manually review every caller of this path and check if they already log the error and at what level

Copy link
Member

Choose a reason for hiding this comment

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

This log has been moved to debug


if (err.type.code === BlockErrorCode.INVALID_SIGNATURE) {
const {signedBlock} = err;
Expand Down
2 changes: 1 addition & 1 deletion packages/beacon-node/src/network/peers/peerManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -610,7 +610,7 @@ export class PeerManager {
try {
await this.libp2p.hangUp(peer);
} catch (e) {
this.logger.warn("Unclean disconnect", {peer: prettyPrintPeerId(peer)}, e as Error);
this.logger.debug("Unclean disconnect", {peer: prettyPrintPeerId(peer)}, e as Error);
nflaig marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down