refactor: use tracing's structured logging #4174
Conversation
aatifsyed
requested review from
hanabi1224 and
elmattic
and removed request for
a team
| @@ -271,7 +271,7 @@ where | |||
| match network.hello_request(peer_id, request).await { | |||
| Ok(response) => response, | |||
| Err(e) => { | |||
| debug!("Hello request failed: {}", e); | |||
| debug!(error = %e, "hello request failed"); | |||
There was a problem hiding this comment.
Should we use ? here to log the inner error since e is of anyhow::Error type?
Err(error) => {
debug!(?error, "hello request failed");
return;
}
| "header_len: {header_len}, msg.bls_msg_includes.len(): {}, msg.secp_msg_includes.len(): {}", | ||
| msg.bls_msg_includes.len(), | ||
| msg.secp_msg_includes.len() | ||
| %header_len, msg.bls_msg_includes.len = %msg.bls_msg_includes.len(), msg.secp_msg_includes.len = %msg.secp_msg_includes.len() |
There was a problem hiding this comment.
nit: kinda nice, but confusing. I would never be able to deduce what msg.bls_msg_includes.len = %msg.bls_msg_includes.len() does without the diff.
There was a problem hiding this comment.
I think this just comes down to knowing tracing::event's API, like knowing format! or printf 🤷
There was a problem hiding this comment.
Perhaps it would be easier to understand if you did something like bls_msg_includes_len = .... It's not blocking, but this is not reading well nor should reading the code require one to know tracing API specifics. format! and printf are intuitive, while this just looks like you are assigning a value to a property of an object. The confusion here comes from the fact that msg variable is actually defined in this scope and it does have the bls_msg_includes property.
There was a problem hiding this comment.
While I agree that the tracing API is not particularly obvious, it seems a standard in the Rust ecosystem https://blessed.rs/crates#section-common-subsection-logging. As such, we should learn how to utilize it and get familiar with it. I recommend tinkering a bit to get a hold of it, what results are produced etc. We can't stay in the printf world forever. 🦖
aatifsyed
changed the title
$ git diff main | cat | rg '@@' | wc -l
182So many call sites |
|
I'll look into |
| } else { | ||
| info!("Weight tie left unbroken, default to {}", other.key()); | ||
| info!(default = %other.key() , "weight tie defaulted"); |
There was a problem hiding this comment.
Let's not use the default, I believe it's a keyword to be used in the future.
| @@ -386,7 +381,7 @@ where | |||
| { | |||
| Ok(tipset) => tipset, | |||
| Err(why) => { | |||
| debug!("Querying full tipset failed: {}", why); | |||
| debug!(reason = %why, "querying full tipset failed"); | |||
There was a problem hiding this comment.
#4143
Semi-automated with https://github.com/aatifsyed/synsert/blob/5f7d39c1ff1ae47d2b028b045378197036a71fa1/examples/tracing.rs
Change checklist