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.

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

adjust log levels and remove tracing::trace!() #879

Merged
merged 3 commits into from
Oct 9, 2024
Merged

Conversation

ppca
Copy link
Contributor

@ppca ppca commented Oct 8, 2024

gcp has no log level for trace!, so I changed trace!() to debug() or info().
I've also gone thru all the log messages and adjusted some of their levels so debug would be the lowest priority, info being somewhat important, and warn being alerting

kmaus-near
kmaus-near previously approved these changes Oct 8, 2024
@@ -146,7 +146,7 @@ impl Indexer {
block_height: BlockHeight,
gcp: &GcpService,
) -> Result<(), DatastoreStorageError> {
tracing::trace!(block_height, "update_block_height");
tracing::debug!(block_height, "update_block_height");
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like this and many other logs do not have a span. Do we want to add it everywhere and in the indexer in particular?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's going to have a target in the log to filter on. We don't use span at all.

Comment on lines 171 to 175
if uncompacted > 0 {
tracing::debug!(
tracing::info!(
uncompacted,
compacted,
"{from:?} sent messages in {:?};",
Copy link
Member

Choose a reason for hiding this comment

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

how come this is now info? Do you think it's important to surface this info? This is going to spam a lot

Copy link
Contributor Author

@ppca ppca Oct 9, 2024

Choose a reason for hiding this comment

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

Yeah this helps to know if messages were successfully sent, what spams is "sending encrypted", this one is much better

loop {
let msg_result = self.receiver.try_recv();
match msg_result {
Ok(msg) => {
tracing::trace!("received a new message");
tracing::debug!("received a new message");
Copy link
Member

Choose a reason for hiding this comment

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

I vote to get rid of this log actually. Does not add much value because we're supposed to always be receiving messages. And this is going to show per protocol message which can be in the millions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nah. Actually when things get stuck, we start "no new message received". And it helps to know if it's the node not receiving anything or it not doing anything

queue.push(msg);
}
Err(TryRecvError::Empty) => {
tracing::trace!("no new messages received");
tracing::debug!("no new messages received");
Copy link
Member

Choose a reason for hiding this comment

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

Same with this one. If anything we should have a message that shows how many messages we received in this loop

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same with above

Copy link
Member

Choose a reason for hiding this comment

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

so this is for every triple/presignature/signature message, so it's better just to keep tally of how many messages we received instead of just printing the same message on every message. We'd still be able to understand that the node is receiving messages

chain-signatures/node/src/protocol/signature.rs Outdated Show resolved Hide resolved
chain-signatures/node/src/protocol/triple.rs Outdated Show resolved Hide resolved
@ppca ppca force-pushed the xiangyi/logging branch from 837e162 to b5bb71d Compare October 9, 2024 15:09
@ppca
Copy link
Contributor Author

ppca commented Oct 9, 2024

oh folks! We could ignore some spammy logs when debugging by clicking hide entries
Screenshot 2024-10-09 at 8 16 33 AM

@ppca ppca merged commit 02001ef into develop Oct 9, 2024
2 of 3 checks passed
@ppca ppca deleted the xiangyi/logging branch October 9, 2024 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants