-
Notifications
You must be signed in to change notification settings - Fork 961
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: introduce tracing #4282
feat: introduce tracing #4282
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.
Thank you for starting this!
I've left some notes :)
thank you for the detailed feedback! I'll work to make the changes you suggested and will ping here when its ready for another review |
i've pushed up some changes. To summarize what I've done so far:
will leave some comments on a few threads where I'd like to clarify some things ALSO: |
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.
Thanks, good progress so far :)
The span is working, see the following screenshot from a local run of the ping-example on my machine:
As you can see, some of the information is duplicated. Now that we capture the PeerId
within the connection
span, it is printed redundantly in some of the messages. I think we should remove it from those messages.
Also, as I noted inline the peer_address
prints the entire ConnectedPoint
which is very noisy.
I'd encourage you to run the example yourself (I used RUST_LOG=libp2p_ping=trace,debug cargo run
) and tweak the logs until they are concise and informative.
Once the ping protocol is done, we can move on to other protocols and tidy up the logs there!
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.
Excited for this to land. Thank you for working on it.
Feel free to ignore comments below at this point.
This looks great! @mxinden do you think we should transition more crates or do that in follow-up PRs? |
Either is fine by me. |
if it's ok with you all, I think ill just continue implementing tracing for the other protocols on this branch |
Yes, thank you! |
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.
Apart from the outstanding comments, this is good to merge from my end. Thank you for the work! Looking forward to the insights we will gain from this.
This comment was marked as resolved.
This comment was marked as resolved.
i believe i resolved the outstanding comments, hopefully I didnt miss anything. the metrics example is really neat! |
This comment was marked as resolved.
This comment was marked as resolved.
# Conflicts: # libp2p/src/tutorials/ping.rs
@mxinden I believe all your comments have been addressed, please have another look! |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
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.
Great work everyone!
@eserilev Thank you especially for the continuous work here! Migrations like this are very tedious but I am excited for the insights this will bring to our users. I will mention this prominently in our release notes. |
We replace `log` with `tracing` across the codebase. Where possible, we make use of structured logging now instead of templating strings. `tracing` offers the ability to also record "span"s. A span lasts until its dropped and describes the entire duration that it is active for. All logs (in `tracing` term "events") are hierarchically embedded in all parent-spans). We introduce several spans: - On debug level: One for `new_outgoing_connection`, `new_incoming_connection` and `new_established_connection` - On debug level: `Connection::poll`, `Swarm::poll` and `Pool::poll` - On trace level: `NetworkBehaviour::poll` for each implementation of `NetworkBehaviour` - On trace level: `ConnectionHandler::poll` for each implementation of (protocol) `ConnectionHandler`s The idea here is that logging on debug level gives you a decent overview of what the system is doing. You get spans for the duration of connections and how often each connection gets polled. Dropping down to trace level gives you an extremely detailed view of how long each individual `ConnectionHandler` was executed as part of `poll` which could be used for detailed analysis on how busy certain handlers are. Most importantly, simply logging on `info` does not give you any spans. We consider `info` to be a good default that should be reasonably quiet. Resolves libp2p#1533. Pull-Request: libp2p#4282.
Description
We replace
log
withtracing
across the codebase. Where possible, we make use of structured logging now instead of templating strings.tracing
offers the ability to also record "span"s. A span lasts until its dropped and describes the entire duration that it is active for. All logs (intracing
term "events") are hierarchically embedded in all parent-spans).We introduce several spans:
new_outgoing_connection
,new_incoming_connection
andnew_established_connection
Connection::poll
,Swarm::poll
andPool::poll
NetworkBehaviour::poll
for each implementation ofNetworkBehaviour
ConnectionHandler::poll
for each implementation of (protocol)ConnectionHandler
sThe idea here is that logging on debug level gives you a decent overview of what the system is doing. You get spans for the duration of connections and how often each connection gets polled.
Dropping down to trace level gives you an extremely detailed view of how long each individual
ConnectionHandler
was executed as part ofpoll
which could be used for detailed analysis on how busy certain handlers are.Most importantly, simply logging on
info
does not give you any spans. We considerinfo
to be a good default that should be reasonably quiet.Resolves #1533.
Notes & open questions
Change checklist