Conversation
There was a problem hiding this comment.
In general looks good! Thanks for adding these logs.
A minor general nit would be to have the logs be more "structural" instead of embedding logging variables in the messages (e.g. instead of info!("request completed, status={status}"), use info!(status, "request completed"). It can make the logs easier for machine parsing. We don't need to block for this, but feel free to merge in: #538.
rajatarya
left a comment
There was a problem hiding this comment.
I'm slightly concerned about the logs being too verbose from logging each cache hit/miss and each CAS response, along with each reconstruction request start/end.
But, I think the value of having these for debugging/triaging issues is greater than the size of the logs (and the logs get cleaned anyway) so I think we should merge this!
cas_client/src/download_utils.rs
Outdated
| hash, | ||
| }; | ||
| if let Ok(Some(cached)) = cache.get(&key, &fetch_term.range).await.log_error("cache error") { | ||
| info!("Cache hit for hash {} range {:?}", hash, fetch_term.range); |
There was a problem hiding this comment.
This might be noisy. Cache hit & cache miss info! statements might be too verbose.
cas_client/src/download_utils.rs
Outdated
| { | ||
| info!("Writing to local cache failed, continuing. Error: {}", e); | ||
| } else { | ||
| info!("Cache write successful for hash {} range {:?}", hash, fetch_term.range); |
There was a problem hiding this comment.
This might be too verbose as well.
| let status_code = res.status().as_u16(); | ||
| let request_id = request_id_from_response(res); | ||
| debug!(request_id, status_code, "Received CAS response"); | ||
| info!(request_id, status_code, "Received CAS response"); |
There was a problem hiding this comment.
I'm really torn on this one - it does seem too verbose - but super useful when debugging
This PR adds improved logging for the cas_client crate in light of the changes to log file capturing, upgrading a number of debug! calls to info! and adding begin and completion logging for all the remote operations. There should be no functionality changes.