Sync active request byrange ids logs #6869
Conversation
dapplion
added
ready-for-review
| // Since each request Id is deeply nested with various types, if rendered with Debug on logs they | ||
| // take too much visual space. This custom Display implementations make the overall Id short while | ||
| // not losing information | ||
| impl Display for BlocksByRangeRequestId { |
There was a problem hiding this comment.
we can make it dry'er by defining a simple macro that impl's Display for the structs:
macro_rules! impl_display {
($structname: ty, $format: literal, $($field:ident),*) => {
impl Display for $structname {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
write!(f, $format, $(self.$field,)*)
}
}
};
}and then calling it for the simple cases:
impl_display! {BlocksByRangeRequestId, "{}/{}", id, parent_request_id}
impl_display! {DataColumnsByRangeRequestId, "{}/{}", id, parent_request_id}
impl_display! {SingleLookupReqId, "{}/lookup/{}", req_id, lookup_id}| } | ||
| } | ||
|
|
||
| #[cfg(test)] |
There was a problem hiding this comment.
nit:
I don't think we need tests for this, Display is reflected on the logs, if it's not according to what expected it's clear where one change, was there a special reason you added tests for?
There was a problem hiding this comment.
Mainly are regression tests, to ensure that a future mantainer understands the intended format and doesn't change it unintentionally
| let processor = self.clone(); | ||
| let id = process_id.clone(); |
There was a problem hiding this comment.
nit: we can avoid cloning here, we can make ChainSegmentProcessId derive Copy
| resp: Option<RpcResponseResult<R>>, | ||
| peer_id: PeerId, | ||
| get_count: F, |
There was a problem hiding this comment.
why not just accept the count as an usize?
There was a problem hiding this comment.
Because I want to render the count of items in the Ok response
dapplion
force-pushed
the
sync-active-request-byrange
branch
from
b0774e0 to
8e0758c
Compare
jimmygchen
added
waiting-on-author
|
Tests are failing. |
Issue Addressed
Writing and running tests I noted that the sync RPC requests are very verbose now.
DataColumnsByRootRequestId { id: 123, requester: Custody(CustodyId { requester: CustodyRequester(SingleLookupReqId { req_id: 121, lookup_id: 101 }) }) }Since this Id is logged rather often I believe there's value in
Proposed Changes
Write custom
Displayimplementations to render Ids in a more DX format_ DataColumnsByRootRequestId with a block lookup_
DataColumnsByRangeRequestId
Also made the logs format and text consistent across all methods