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

switch tokio-util from log to tracing #4539

Merged
merged 2 commits into from
Feb 26, 2022
Merged

Conversation

guswynn
Copy link
Contributor

@guswynn guswynn commented Feb 24, 2022

Motivation

I am trying to remove any usage of log in our dep tree, so I can turn off the tracing-log feature, and I noticed tokio-util still used log, so I am trying to change it to tracing!

Solution

There were only a few callsites using log in tokio-util, so I figured switching them to tracing, like the rest of the tokio ecosystem would be fine!

@hawkw
Copy link
Member

hawkw commented Feb 24, 2022

...I had forgotten tokio-util even had any logging in it!

tokio-util/Cargo.toml Outdated Show resolved Hide resolved
tokio-util/src/codec/framed_impl.rs Outdated Show resolved Hide resolved
@guswynn
Copy link
Contributor Author

guswynn commented Feb 24, 2022

@hawkw I think I need to it to v0.1.25 to pass ci? that should be semver compatible with 0.1.31 (what we use in our repo) right?

@hawkw
Copy link
Member

hawkw commented Feb 24, 2022

@hawkw I think I need to it to v0.1.25 to pass ci? that should be semver compatible with 0.1.31 (what we use in our repo) right?

yeah should be fine

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

lgtm!

my one remaining concern is "what happens to users who expect log to work?" --- perhaps we should enable the tracing/log feature by default, but i don't love that either...

@@ -278,7 +278,7 @@ where

while !pinned.state.borrow_mut().buffer.is_empty() {
let WriteFrame { buffer } = pinned.state.borrow_mut();
trace!("writing; remaining={}", buffer.len());
trace!(remaining = buffer.len(), "writing;");
Copy link
Member

Choose a reason for hiding this comment

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

<3

@guswynn
Copy link
Contributor Author

guswynn commented Feb 24, 2022

I don't love enabling tracing/log either, especially because disabling it would need to go through tokio in some cases (or, one would have to change all the things that depend on tokio-util in your dep tree

brings up a good question: is what log backend you use protected by semver? I am inclined to say no, as there is no actual clean way to track this, but a reasonable answer might be "trace and debug log lines aren't, as they are below the default INFO level of both log and tracing"

one other option is to default to log, and then add a tracing feature, off by default, and additive-features means end users can at least do the weird thing of "depend on some library in your binary's Cargo.toml just to turn on a feature"
additive features seemed to work well here: fede1024/rust-rdkafka#440

@guswynn
Copy link
Contributor Author

guswynn commented Feb 24, 2022

adding an optional feature was my plan for reqwest and some other crates, but I figured the fact that these are trace and in the tokio umbrella meant a hard cut was okay

@Darksonn Darksonn added the A-tokio-util Area: The tokio-util crate label Feb 25, 2022
@Darksonn
Copy link
Contributor

I'm okay with doing this.

@guswynn
Copy link
Contributor Author

guswynn commented Feb 26, 2022

@Darksonn as ci is passing, can this be merged?

@hawkw
Copy link
Member

hawkw commented Feb 26, 2022

seems good to me!

@Darksonn Darksonn merged commit 413c812 into tokio-rs:master Feb 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio-util Area: The tokio-util crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants