Sync active request byrange ids logs #6914
Conversation
jimmygchen
added
ready-for-review
| // not losing information | ||
| impl_display!(BlocksByRangeRequestId, "{}/{}", id, parent_request_id); | ||
| impl_display!(BlobsByRangeRequestId, "{}/{}", id, parent_request_id); | ||
| impl_display!(DataColumnsByRangeRequestId, "{}/{}", id, parent_request_id); |
There was a problem hiding this comment.
This is definitely better than before! although it may still be not as straight forward to search in an indexed log viewer, compared to other logs where we have key/values.
e.g.
123/122/RangeSync/0/5492900659401505034
This is quite hard to read without memorizing the order or looking at the code.
Also if we do want to go ahead with this format, would it make more sense to have the order the other way around? i.e. parent request id first? (actually i realise it doesn't matter coz we have requester at the end)
| }, | ||
| }, | ||
| }; | ||
| assert_eq!(format!("{id}"), "123/122/RangeSync/0/54"); |
There was a problem hiding this comment.
I found having consecutive numbers make it harder to guess what they are.
Would something like this be easier to understand?
123/Parent/122/RangeSync/Batch/0/Chain/54
There was a problem hiding this comment.
These are debug logs, only use will use this. Have you seen @AgeManning 's batch state codes? It's even harder to follow 🤣 . Remember that before we just had a number with no information at all. This IDs get logged quite a lot when sync is running I believe succinctness is good
There was a problem hiding this comment.
I disagree with this, because for anyone else to read this (even anyone from our team) we'd have to look it up in the code to know how to use this log, and I don't think adding Parent and Batch to it makes it a lot more verbose than it is now.
jimmygchen
added
waiting-on-author
There was a problem hiding this comment.
Please see comment above.
#6914 (comment)
If it helps with debugging happy to merge now and revisit later
michaelsproul
added
v7.0.0-beta.0
Squashed commit of the following: commit cbc378a Author: dapplion <35266934+dapplion@users.noreply.github.com> Date: Wed Feb 5 12:55:13 2025 -0300 De-duplicate code commit 4583116 Author: dapplion <35266934+dapplion@users.noreply.github.com> Date: Sun Jan 26 14:16:55 2025 +0400 Add log when sending batch to processor commit cacfa06 Author: dapplion <35266934+dapplion@users.noreply.github.com> Date: Sun Jan 26 13:01:02 2025 +0400 Concise request Ids for sync commit 4627f38 Author: dapplion <35266934+dapplion@users.noreply.github.com> Date: Sun Jan 26 12:00:03 2025 +0400 Consistent logging in sync rpc requests
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