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

fix(kad): reduce noise of "remote supports our protocol" log #4278

Merged
merged 10 commits into from
Aug 7, 2023
Merged

fix(kad): reduce noise of "remote supports our protocol" log #4278

merged 10 commits into from
Aug 7, 2023

Conversation

shamil-gadelshin
Copy link
Contributor

@shamil-gadelshin shamil-gadelshin commented Aug 1, 2023 •
edited by thomaseizinger
Loading

Description

This PR changes the logging of the Kademlia connection handler related to the remote Kademlia mode changes:

  • Downgrade log level for the remote kademlia protocol report from info to debug.
  • Introduce connection_id for the handler to improve logging.

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

@shamil-gadelshin
kad: Change connection handler logging.
7ee79d0 
- downgrade log level for the remote kademlia protocol report
- introduce connection_id for the handler to improve logging
@shamil-gadelshin shamil-gadelshin changed the title kad: Change connection handler logging. fix: Change connection handler logging. Aug 1, 2023
thomaseizinger
thomaseizinger approved these changes Aug 1, 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.

I think a changelog entry hinting at the reduced log-spam wouldn't hurt :)

protocols/kad/src/handler.rs Outdated Show resolved Hide resolved
protocols/kad/src/handler.rs Outdated Show resolved Hide resolved
@thomaseizinger thomaseizinger changed the title fix: Change connection handler logging. fix(kad): reduce noise of "remote supports our protocol" log Aug 1, 2023
thomaseizinger
thomaseizinger approved these changes Aug 2, 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 @shamil-gadelshin !

thomaseizinger
thomaseizinger reviewed Aug 3, 2023
View reviewed changes
swarm/CHANGELOG.md Outdated Show resolved Hide resolved
thomaseizinger
thomaseizinger previously approved these changes Aug 3, 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!

@thomaseizinger
Copy link
Contributor

Can you look into the CI failures (ignoring clippy)?

@mxinden
Copy link
Member

mxinden commented Aug 4, 2023

@shamil-gadelshin seems to be missing an update to Cargo.lock. Did you forget to check-in changes to Cargo.lock?

@shamil-gadelshin
Copy link
Contributor Author

@shamil-gadelshin seems to be missing an update to Cargo.lock. Did you forget to check-in changes to Cargo.lock?

Fixed that.

@mergify mergify bot dismissed thomaseizinger’s stale review August 7, 2023 05:37

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

@shamil-gadelshin
fix: update Cargo.toml
1ea7b86 
- update swarm version to 0.43.3
@shamil-gadelshin
Copy link
Contributor Author

Can you look into the CI failures (ignoring clippy)?

There are a clippy (compiler bug) and flood_sub(operation was canceled) after fixing Cargo.lock and Cargo.toml files.

@thomaseizinger
Copy link
Contributor

Can you look into the CI failures (ignoring clippy)?

There are a clippy (compiler bug) and flood_sub(operation was canceled) after fixing Cargo.lock and Cargo.toml files.

The clippy bug is hopefully resolved soon. A PR for that landed just recently: rust-lang/rust-clippy#11190. We don't require clippy to be green for merging so that is okay.

I restarted the floodsub job, seems to have been some network issues.

Thank you for the PR!

thomaseizinger
thomaseizinger previously approved these changes Aug 7, 2023
View reviewed changes
@shamil-gadelshin
Copy link
Contributor Author

I restarted the floodsub job, seems to have been some network issues.

Yes, github was unresponsive at that time.

Thank you for the PR!

You're welcome!

@mergify mergify bot dismissed thomaseizinger’s stale review August 7, 2023 08:47

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

@thomaseizinger
Copy link
Contributor

There seem to be some issues around mergify not being able to update the PR. Let's see if it works now.

@mergify mergify bot merged commit e651552 into libp2p:master Aug 7, 2023
65 of 66 checks passed
thomaseizinger pushed a commit that referenced this pull request Aug 20, 2023
@shamil-gadelshin @thomaseizinger
fix(kad): reduce noise of "remote supports our protocol" log
11bd29d 
This PR changes the logging of the Kademlia connection handler related to the remote Kademlia mode changes:
- Downgrade log level for the remote kademlia protocol report from `info` to `debug`.
- Introduce connection_id for the handler to improve logging.

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

@thomaseizinger thomaseizinger thomaseizinger approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@shamil-gadelshin @thomaseizinger @mxinden