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

tracing/log doesn't work with tokio::spawn / .with_current_subscriber(). #2913

Open
Lorak-mmk opened this issue Mar 18, 2024 · 5 comments
Open

Comments

@Lorak-mmk
Copy link

Lorak-mmk commented Mar 18, 2024

Bug Report

Version

└── tracing v0.1.40
├── tracing-attributes v0.1.27 (proc-macro)
└── tracing-core v0.1.32

Platform

Fedora 39

uname -a:
Linux scyllaptop 6.7.9-200.fc39.x86_64 #1 SMP PREEMPT_DYNAMIC Wed Mar 6 19:35:04 UTC 2024 x86_64 GNU/Linux

Description

Let me start by saying that I'm not 100% it's a bug - it may be be lack of knowledge in regards to usage of tracing.
I'm one of maintainters of a library ( https://github.com/scylladb/scylla-rust-driver ) that uses tokio and tracing.
We call .with_current_subscriber() on each future passed to tokio::spawn which according to docs ( https://docs.rs/tracing/latest/tracing/instrument/trait.WithSubscriber.html#method.with_current_subscriber )
seems like what we should do as a library.

This works as intended when using tracing subscriber. Example:
Consider a crate with the following Cargo.toml (it will be used for another example later, thus unused dependencies):

[package]
name = "reproducer"
version = "1.0.0"
edition = "2021"

[dependencies]
env_logger = "0.10"
log = "0.4"
tracing = { version = "0.1.36", features = [ "log" ] }
tracing-subscriber = "0.3"
tokio = { version = "1.27", features = [ "full" ] }

and the following main.rs:

use tracing::instrument::WithSubscriber;

#[tokio::main]
async fn main() {
    let subscriber = tracing_subscriber::FmtSubscriber::new();
    tracing::subscriber::set_global_default(subscriber).unwrap();

    log::info!("info log");
    tracing::info!("info tracing");

    tokio::spawn(
        async move {
            log::info!("spawned info log");
            tracing::info!("spawned info tracing")
        }
        .with_current_subscriber(),
    )
    .await
    .unwrap();

    log::info!("another info log");
    tracing::info!("another info tracing");
}

After cargo run traces are printed and logs are not, as expected:

cargo run
   Compiling reproducer v1.0.0 (/tmp/tracing_issue)
    Finished dev [unoptimized + debuginfo] target(s) in 0.56s
     Running `target/debug/reproducer`
2024-03-18T15:48:35.651650Z  INFO reproducer: info tracing
2024-03-18T15:48:35.651700Z  INFO reproducer: spawned info tracing
2024-03-18T15:48:35.651727Z  INFO reproducer: another info tracing

Now let's switch tracing subscriber for env_logger. As far as I understand from tracing's docs env_logger should print my traces because I enabled log feature on tracing crate.
New main.rs:

use tracing::instrument::WithSubscriber;

#[tokio::main]
async fn main() {
    env_logger::init_from_env(env_logger::Env::new().default_filter_or("info"));

    log::info!("info log");
    tracing::info!("info tracing");

    tokio::spawn(
        async move {
            log::info!("spawned info log");
            tracing::info!("spawned info tracing")
        }
        .with_current_subscriber(),
    )
    .await
    .unwrap();

    log::info!("another info log");
    tracing::info!("another info tracing");
}

Output:

cargo run
   Compiling reproducer v1.0.0 (/tmp/tracing_issue)
    Finished dev [unoptimized + debuginfo] target(s) in 0.78s
     Running `target/debug/reproducer`
[2024-03-18T15:53:43Z INFO  reproducer] info log
[2024-03-18T15:53:43Z INFO  reproducer] info tracing
[2024-03-18T15:53:43Z INFO  reproducer] spawned info log
[2024-03-18T15:53:43Z INFO  reproducer] another info log

As you can see, no traces are printed after the tokio::spawn call - interestingly it affects even the tracing call in the same function.
It's not the problem with log calls - issue persists even if I remove them.

This results in our library not being usable with log loggers like env_logger even if we enable log feature :(

@Lorak-mmk
Copy link
Author

Note: log-always works correctly and gives following output with env_logger:

cargo run
   Compiling tracing v0.1.40
   Compiling reproducer v1.0.0 (/tmp/tracing_issue)
    Finished dev [unoptimized + debuginfo] target(s) in 0.87s
     Running `target/debug/reproducer`
[2024-03-18T15:59:20Z INFO  reproducer] info log
[2024-03-18T15:59:20Z INFO  reproducer] info tracing
[2024-03-18T15:59:20Z INFO  reproducer] spawned info log
[2024-03-18T15:59:20Z INFO  reproducer] spawned info tracing
[2024-03-18T15:59:20Z INFO  reproducer] another info log
[2024-03-18T15:59:20Z INFO  reproducer] another info tracing

But this is not something I want to enable in the library - IIUC it adds overhead as it generates 2 event for each tracing call.

@kaffarell
Copy link
Contributor

Hmm, this is interesting... I think the .with_current_subscriber call creates a NoSubscriber in case a default subscriber doesn't exist. Then afterwards, every event is pushed to the NoSubscriber (which discards them) instead of being forwarded as a log event.

But I can't seem to find the code that decides whether to log or not @hawkw ?

@hawkw
Copy link
Member

hawkw commented Mar 19, 2024

The log feature only emits log records if no tracing Subscriber has ever been set. Your calls to with_current_subscriber are setting a subscriber, disabling the log crate output. This is why it works with log-always, but not with log.

A library should not generally enable the log or log-always feature flags, and generally should never be responsible for setting the default subscriber. Instead, user code that depends on the library should enable tracing's log crate support if it wants to, and set the default subscriber if it wishes to use tracing. I don't think the calls to with_current_subscriber are necessary in a library in general.

@hawkw
Copy link
Member

hawkw commented Mar 19, 2024

With that said, we probably could fix this by having with_current_subscriber check if the current subscriber is NoSubscriber, and if it is, having its future do nothing, rather than setting the subscriber as default.

@Lorak-mmk
Copy link
Author

I don't think the calls to with_current_subscriber are necessary in a library in general.

What should the library do instead? It seemed reasonable that when spawning a task we want it to use current subscriber, not some default one.

Lorak-mmk added a commit to Lorak-mmk/scylla-rust-driver that referenced this issue May 2, 2024
Those calls were introduced in e557095,
with message:
```
Now, when the driver spawns a task to run a new future on it, that
future will use the same subscriber as the code that spawned the task in
the first place.
```

There is unfortunately no explanation about when it is necessary.
I don't see any problems after removing those - logs are still collected
correctly using a tracing subscriber.
Those calls however have a harmful side-effect: they prevent usage of
`log` loggers to listen to driver logs using `log` feature in `tracing`
crate. After reporting the problem to `tracing` crate:
tokio-rs/tracing#2913
one of maintainers said that using `.with_current_subscriber()` in
a library is not necessary in general.

As I don't see any issue caused by removing these calls, but their
existence cause an issue, they are removed in this commit.
Lorak-mmk added a commit to Lorak-mmk/scylla-rust-driver that referenced this issue May 14, 2024
Those calls were introduced in e557095,
with message:
```
Now, when the driver spawns a task to run a new future on it, that
future will use the same subscriber as the code that spawned the task in
the first place.
```

There is unfortunately no explanation about when it is necessary.
I don't see any problems after removing those - logs are still collected
correctly using a tracing subscriber.
Those calls however have a harmful side-effect: they prevent usage of
`log` loggers to listen to driver logs using `log` feature in `tracing`
crate. After reporting the problem to `tracing` crate:
tokio-rs/tracing#2913
one of maintainers said that using `.with_current_subscriber()` in
a library is not necessary in general.

As I don't see any issue caused by removing these calls, but their
existence cause an issue, they are removed in this commit.
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

No branches or pull requests

3 participants