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.

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

[Merged by Bors] - remove crit! logging from ListenerClosed event on Ok() #4821

Closed
wants to merge 3 commits into from

Conversation

jxs
Copy link
Member

@jxs jxs commented Oct 10, 2023

Issue Addressed

Since adding Quic support on #4577, and due to quinns api nature LH now triggers the ListenerClosed event.. @michaelsproul noticed we are logging this event as crit! independently of the reason. This PR matches the reason, logging with debug! and error! (instead of crit!) according to its Result

Additional Info

LH will still log crit! until libp2p/rust-libp2p#4621 has been merged

Copy link
Member

@jmcph4 jmcph4 left a comment

Choose a reason for hiding this comment

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

LGTM

@jxs jxs changed the title remove crit! logging from ListenerClosed event remove crit! logging from ListenerClosed event on Ok() Oct 17, 2023
Copy link
Member

@pawanjay176 pawanjay176 left a comment

Choose a reason for hiding this comment

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

Nice!

@pawanjay176 pawanjay176 added the ready-for-merge This PR is ready to merge. label Oct 18, 2023
@pawanjay176
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Oct 18, 2023
## Issue Addressed

Since adding Quic support on #4577, and due to `quinn`s api nature LH now triggers the [`ListenerClosed`](https://docs.rs/libp2p/0.52.3/libp2p/swarm/struct.ListenerClosed.html) event.. @michaelsproul noticed we are logging this event as `crit!` independently of the reason. This PR matches the reason, logging with `debug!` and `error!` (instead of `crit!`) according to its `Result`  
## Additional Info
LH will still log `crit!` until libp2p/rust-libp2p#4621 has been merged
@bors
Copy link

bors bot commented Oct 18, 2023

Pull request successfully merged into unstable.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot changed the title remove crit! logging from ListenerClosed event on Ok() [Merged by Bors] - remove crit! logging from ListenerClosed event on Ok() Oct 18, 2023
@bors bors bot closed this Oct 18, 2023
Gua00va pushed a commit to Gua00va/lighthouse that referenced this pull request Oct 18, 2023
## Issue Addressed

Since adding Quic support on sigp#4577, and due to `quinn`s api nature LH now triggers the [`ListenerClosed`](https://docs.rs/libp2p/0.52.3/libp2p/swarm/struct.ListenerClosed.html) event.. @michaelsproul noticed we are logging this event as `crit!` independently of the reason. This PR matches the reason, logging with `debug!` and `error!` (instead of `crit!`) according to its `Result`  
## Additional Info
LH will still log `crit!` until libp2p/rust-libp2p#4621 has been merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge This PR is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants