DNM: logging ux changes based on new logging policy #5457
Closed
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
philknows
commented
May 3, 2023
philknows
commented
May 3, 2023
| @@ -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); | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
This log has been moved to debug
Performance Report✔️ no performance regression detected Full benchmark results
|
philknows
commented
May 3, 2023
philknows
commented
May 3, 2023
philknows
commented
May 3, 2023
philknows
changed the title
philknows
added
the
status-do-not-merge
|
Can this be closed? |
|
Closing. Stale and it probably makes better sense to discuss specific UX logging issues as its own issues for better tracking. |
1 task
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.
Motivation
This PR is a collection of changes committed by the team, dedicated to discussion and refactoring based on our newly implemented logging policy #5299. Lodestar is implementing a better logging policy to improve our UX for continued adoption by the community.
Meant to track and discuss specific portions of the code that require log UX improvements. Do not merge.
Description
Contributors are asked to submit commits to this PR with potential changes and motivation for why a specific change is being proposed.
Closes #5359
Partially closes #4400