Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Replace neqo_common::log with log crate #1963

Closed
mxinden opened this issue Jul 4, 2024 · 5 comments · Fixed by #2291
Closed

Replace neqo_common::log with log crate #1963

mxinden opened this issue Jul 4, 2024 · 5 comments · Fixed by #2291

Comments

@mxinden
Copy link
Collaborator

mxinden commented Jul 4, 2024

neqo* currently uses neqo_common::log for all things logging. It is a wrapper around the log crate.

What is the benefit of using neqo_common::log over using the log crate directly? From what I can tell:

  • neqo_common::log does not require explicit instantiation of the logger (e.g. via env_logger::init) but instead tries to initialize on each log invocation.
    #[macro_export]
    macro_rules! log_invoke {
    ($lvl:expr, $ctx:expr, $($arg:tt)*) => ( {
    ::neqo_common::log::init(None);
    ::neqo_common::do_log!($lvl, "[{}] {}", $ctx, format!($($arg)*));
    } )
    }
  • Uses elapsed time instead of absolute time
    "{}s{:3}ms {} {}",
    elapsed.as_secs(),
    elapsed.as_millis() % 1000,

Are the benefits worth a custom way of logging? Should a library instantiate a logger instead of the conventional instantiation by the calling binary?

Related, other Mozilla code using log:
https://searchfox.org/mozilla-central/search?q=%5Elog+%3D&path=*Cargo.toml&case=true&regexp=true

@larseggert
Copy link
Collaborator

I'd be OK with removing this.

@larseggert
Copy link
Collaborator

There are some special things neqo_common::log allows, e.g.,

qdebug!(
[format!("{fd:p}")],
"Got resumption token {}",
hex_snip_middle(&v)
);

Can we do this with log?

@larseggert
Copy link
Collaborator

So I worked on this a bit in #2291. I do think we need a wrapper of some sort like we have in neqo-common::log, because otherwise we'd need to initialize env_logger in every single test, which is a pain.

@mxinden
Copy link
Collaborator Author

mxinden commented Dec 18, 2024

Agreed. I didn't think of env_logger instantiation in unit tests. Sorry about that.

@larseggert
Copy link
Collaborator

OK. I will leave neqo-common::log in place w/some changes.

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

Successfully merging a pull request may close this issue.

2 participants