-
Notifications
You must be signed in to change notification settings - Fork 726
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.
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
basic-authorship
: debug level is now less spammy
#6768
Conversation
/cmd prdoc --bump major --audience node_dev |
"🎁 Prepared block for proposing at {} ({} ms) hash: {:?}; parent_hash: {}; end: {:?}; extrinsics_count: {};", | ||
block.header().number(), | ||
block_took.as_millis(), | ||
<Block as BlockT>::Hash::from(block.header().hash()), | ||
block.header().parent_hash(), | ||
end_reason, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"🎁 Prepared block for proposing at {} ({} ms) hash: {:?}; parent_hash: {}; end: {:?}; extrinsics_count: {};", | |
block.header().number(), | |
block_took.as_millis(), | |
<Block as BlockT>::Hash::from(block.header().hash()), | |
block.header().parent_hash(), | |
end_reason, | |
"🎁 Prepared block for proposing at {} ({} ms) hash: {:?}; parent_hash: {}; extrinsics_count: {};", | |
block.header().number(), | |
block_took.as_millis(), | |
<Block as BlockT>::Hash::from(block.header().hash()), | |
block.header().parent_hash(), |
No need to log this in info.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need it here, this is why I added it. I need to know why block builder is hitting limits. And we may have it "for free" in this line.
I am now confused, it is OK to merge it with end reason or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need it there? You could just enable trace logging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also we have metrics for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its useful to have, with tracing it will spam all tx again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to enable trace, because it is way too spammy. With 7k txs in block it may affect the block time building. And for sure it does not help with collecting logs in grafana/loki - it sometimes breaks with too many logs.
I can add another "if branch" here for debug level, but I think it will be more confusing when reasoning about what to enable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding this because you want to debug one network doesn't make that much sense IMO. Then print the end reason in debug. For normal operators this is not that useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not one network, instead of having important info in log I always need to restart the node whiat is a cost (time). having this log allows very quick assessment of what is happening in block builder.
and logs are not only for operator. after all why does operator need this hash?
I would not propose it if I could live w/o this. And finally @skunert says he finds it useful too.
The
debug
level insc-basic-authorship
is now less spammy. Previously it was outputing logs per individual transactions. It was quite hard to follow the logs (and also generates unneeded traffic in grafana).Now
debug
level only show some internal details, without spamming output with per-transaction logs. They were moved totrace
level.I also added the
EndProposingReason
to the summary INFO message. This allows us to know what was the block limit (which is very useful for debugging).Example: