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

chore(quinn-udp): tweak log levels #2048

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

thomaseizinger
Copy link
Contributor

This PR is a proposal to remove the two ERROR logs that we currently have in quinn-udp. Rationale is explained separately in each commit message.

It is common to set up automated error reporting based on `ERROR` and
potentially also `WARN` logs. Whilst GSO being unsupported is certainly
something worthwhile logging, the `ERROR` log level seems a bit
excessive and leads to unactionable errors reports.

The system can still operate with `max_gso_segments == 1`. As such, this
codepath "merely" indicates a state change in the system but not a fatal
error. As such, logging this on INFO level seems more appropriate.
Logging AND returning errors is considered an anti-pattern because it
leads to duplicate logs.
@djc
Copy link
Member

djc commented Nov 18, 2024

This makes sense to me, thanks!

@thomaseizinger
Copy link
Contributor Author

The CI failure seems like a fluke to me? Could you rerun those perhaps?

@djc
Copy link
Member

djc commented Nov 18, 2024

This doesn't look like a fluke:

error: unused import: `error`
    --> quinn-udp\src\windows.rs:17:18
     |
  17 |     log::{debug, error},
     |                  ^^^^^
     |
     = note: `-D unused-imports` implied by `-D warnings`
     = help: to override `-D warnings` add `#[allow(unused_imports)]`

Copy link
Contributor

@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.

Works for me. Thanks.

@thomaseizinger
Copy link
Contributor Author

This doesn't look like a fluke:

error: unused import: `error`
    --> quinn-udp\src\windows.rs:17:18
     |
  17 |     log::{debug, error},
     |                  ^^^^^
     |
     = note: `-D unused-imports` implied by `-D warnings`
     = help: to override `-D warnings` add `#[allow(unused_imports)]`

Ah didn't see that in the output, makes sense!

Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants