Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(core): add logs for OrTransport when trying addresses #4133

Merged
merged 7 commits into from
Jul 5, 2023
Merged

feat(core): add logs for OrTransport when trying addresses #4133

merged 7 commits into from
Jul 5, 2023

Conversation

tcoratger
Copy link
Contributor

@tcoratger tcoratger commented Jun 29, 2023 •
edited by thomaseizinger
Loading

Description

Currently, when trying addresses in listen_on via OrTransport, there are no logs to facilitate debugging. This PR corrects that by providing adequate logs via std::any::type_name method.

Resolves #4072.

Notes & open questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

thomaseizinger
thomaseizinger reviewed Jun 29, 2023
View reviewed changes
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Two suggestions :)

core/src/transport/choice.rs Outdated Show resolved Hide resolved
core/src/transport/choice.rs Outdated Show resolved Hide resolved
thomaseizinger
thomaseizinger reviewed Jun 29, 2023
View reviewed changes
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing the comments! Did you test this with an application or an example?

I am curious what these log lines end up looking like.

mxinden
mxinden reviewed Jun 30, 2023
View reviewed changes
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat. This will help a lot in the future. Thank you.

Also curious what the output looks like. Mind posting it here?

core/src/transport/choice.rs Outdated Show resolved Hide resolved
core/src/transport/choice.rs Outdated Show resolved Hide resolved
@tcoratger
Copy link
Contributor Author

@mxinden @thomaseizinger I've tested with the simplest template in protocols/relay. Here is an example response in the console:

[2023-06-30T19:12:43Z TRACE libp2p_core::transport::choice] Attempting to dial /memory/3374154575862174647 using libp2p_relay::priv_client::transport::Transport
[2023-06-30T19:12:43Z DEBUG libp2p_core::transport::choice] Failed to dial /memory/3374154575862174647 using libp2p_relay::priv_client::transport::Transport
[2023-06-30T19:12:43Z TRACE libp2p_core::transport::choice] Attempting to dial /memory/3374154575862174647 using libp2p_core::transport::memory::MemoryTransport

@thomaseizinger
Copy link
Contributor

Perhaps we should reduce this to just logging the failures to be less noisy?

My guess is that connections succeeding is almost never an issue. It is the rejections that are hard to debug.

Thoughts guys?

@tcoratger
Copy link
Contributor Author

Perhaps we should reduce this to just logging the failures to be less noisy?

My guess is that connections succeeding is almost never an issue. It is the rejections that are hard to debug.

Thoughts guys?

@thomaseizinger As you wish, for me it's not a major problem because the trace! represents a level above in terms of logs and therefore if the user indicates a trace level of log, he knows that he will have additional information. If it wants failures it can set to debug, I thought that was why you favored using trace and debug separately so the user can choose.

But if you prefer, no worries, I can just remove the two traces! :)

@mxinden
Copy link
Member

mxinden commented Jul 2, 2023

Given that the success case is on TRACE level this looks good to me. Though not a strong opinion. Will defer to @thomaseizinger.

Thank you @tcoratger.

thomaseizinger
thomaseizinger previously approved these changes Jul 4, 2023
View reviewed changes
@mergify mergify bot dismissed thomaseizinger’s stale review July 5, 2023 00:30

Approvals have been dismissed because the PR was updated after the send-it label was applied.

@mergify mergify bot merged commit 7308221 into libp2p:master Jul 5, 2023
mxinden added a commit to mxinden/rust-libp2p that referenced this pull request Jul 10, 2023
@mxinden
fix(core/choice): log on dial and fix listen log
ebd4032 
- Fixes typo in `<OrTransport as Transport>::listen_on` using the word "dial" instead of "listen_on".
- Adds logging to `<OrTransport as Transport>::dial`.

Follow-up to libp2p#4133
@mxinden mxinden mentioned this pull request Jul 10, 2023
Merged
4 tasks
mergify bot pushed a commit that referenced this pull request Jul 10, 2023
@mxinden
fix(core/choice): log on dial and fix listen log
2671375 
- Fixes typo in `<OrTransport as Transport>::listen_on` using the word "dial" instead of "listen_on".
- Adds logging to `<OrTransport as Transport>::dial`.

Follow-up to #4133

Pull-Request: #4164.
thomaseizinger pushed a commit that referenced this pull request Aug 20, 2023
@tcoratger @thomaseizinger
feat(core): add logs for OrTransport when trying addresses
37dddd4 
Currently, when trying addresses in `listen_on` via `OrTransport`, there are no logs to facilitate debugging. This PR corrects that by providing adequate logs via `std::any::type_name` method.

Resolves #4072.

Pull-Request: #4133.
thomaseizinger pushed a commit that referenced this pull request Aug 20, 2023
@mxinden @thomaseizinger
fix(core/choice): log on dial and fix listen log
752bf13 
- Fixes typo in `<OrTransport as Transport>::listen_on` using the word "dial" instead of "listen_on".
- Adds logging to `<OrTransport as Transport>::dial`.

Follow-up to #4133

Pull-Request: #4164.
@oblique oblique mentioned this pull request May 17, 2024
Merged
4 tasks
mergify bot pushed a commit that referenced this pull request May 26, 2024
@oblique
chore(webtransport-websys): Change logs to debug level
b83d3ce 
When you enable Kademlia in a large network that uses multiple type of transports, then this log creates a very noisy console and makes meaningful logs hard to see.

We can also remove the log entirely if you prefer. Check: #4015 (comment), #4072, #4133

Pull-Request: #5396.
TimTinkers pushed a commit to unattended-backpack/rust-libp2p that referenced this pull request Sep 14, 2024
@oblique @TimTinkers
chore(webtransport-websys): Change logs to debug level
492f312 
When you enable Kademlia in a large network that uses multiple type of transports, then this log creates a very noisy console and makes meaningful logs hard to see.

We can also remove the log entirely if you prefer. Check: libp2p#4015 (comment), libp2p#4072, libp2p#4133

Pull-Request: libp2p#5396.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mxinden mxinden mxinden approved these changes

@thomaseizinger thomaseizinger thomaseizinger left review comments

Assignees
No one assigned
Labels
send-it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

core: add useful logs to OrTransport when trying addresses
3 participants
@tcoratger @thomaseizinger @mxinden