-
Notifications
You must be signed in to change notification settings - Fork 136
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Format some logs better #157
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Is it worth standardizing some logging conventions in the contributing guidelines?
trin-core/src/portalnet/events.rs
Outdated
@@ -82,7 +97,12 @@ impl PortalnetEvents { | |||
self.process_utp_byte_stream().await; | |||
} | |||
_ => { | |||
warn!("Non supported protocol : {}", protocol); | |||
warn!( | |||
"Received TalkRequest on uknown protocol from={} protocol={} body={}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "unknown" is misspelled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks way better! Just a thought, but if it's not too cumbersome to also include which overlay network the incoming/outgoing messages are on (state/history/etc.) that would be useful imo
@@ -24,7 +22,10 @@ impl StateEvents { | |||
.process_one_request(&talk_request) | |||
.await | |||
{ | |||
Ok(r) => Message::Response(r).to_bytes(), | |||
Ok(r) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like it's worth replicating these changes over in trin-history
?
For sure! I'm just not sure exactly how / what style we want to go with. But if anybody has thoughts, or an initial structure in mind, go ahead and open a pr and we can continue the discussion there. |
@njgheorghita Just pushed ee29540 which tries to add which network each message came in on, the output now looks like this:
This isn't a trivial change so giving you one last look before merging! |
Looks great! lgtm! |
A stab at #150:
Before:
After: