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

When using tracing-log, log::log_enabled! is always true #1249

Closed
pchickey opened this issue Feb 18, 2021 · 10 comments
Closed

When using tracing-log, log::log_enabled! is always true #1249

pchickey opened this issue Feb 18, 2021 · 10 comments
Assignees
Labels
crate/log Related to the `tracing-log` crate crate/subscriber Related to the `tracing-subscriber` crate kind/bug Something isn't working

Comments

@pchickey
Copy link

pchickey commented Feb 18, 2021

Bug Report

Version

├── tracing-subscriber v0.2.15
│ ├── tracing v0.1.22 ()
│ ├── tracing-log v0.1.1
│ │ └── tracing-core v0.1.17 (
)

Platform

Linux finknottle 5.8.0-43-generic #49~20.04.1-Ubuntu SMP Fri Feb 5 09:57:56 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux

Crates

tracing-log

Description

We use tracing-log (via tracing-subscriber) in several executables in the Wasmtime project (https://github.com/bytecodealliance/wasmtime/), and log macros in many crates and dependencies of the project.

We recently discovered that tracing-log was causing every log_enabled!(LogLevel::Debug) in the project to be true, which incurs some costly formatting of datastructures into strings, even when the log level is disabled (e.g. RUST_LOG is undefined).

In our particular case this ended up being a 2x slowdown of our JIT compiler.

@davidbarsky
Copy link
Member

I believe this issue will be resolved in #1248.

@hawkw
Copy link
Member

hawkw commented Feb 18, 2021

@davidbarsky yeah, I'm going to keep this open until I get it backported and @pchickey can confirm that it resolves the problem.

hawkw added a commit that referenced this issue Feb 18, 2021
Depends on #1247.

Since `tracing-subscriber`'s `init` and `try_init` functions set the
global default collector, we can use the collector's max-level hint as
the max level for the log crate, as well. This should significantly
improve performance for skipping `log` records that fall below the
collector's max level, as they will not have to call the
`LogTracer::enabled` method.

This will prevent issues like bytecodealliance/wasmtime#2662
from occurring in the future. See also #1249.

In order to implement this, I also changed the `FmtCollector`'s
`try_init` to just use `util::SubscriberInitExt`'s `try_init` function,
so that the same code isn't duplicated in multiple places. I also
added `AsLog` and `AsTrace` conversions for `LevelFilter`s in
the `tracing-log` crate.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this issue Feb 18, 2021
Depends on #1247.

Since `tracing-subscriber`'s `init` and `try_init` functions set the
global default subscriber, we can use the subscriber's max-level hint as
the max level for the log crate, as well. This should significantly
improve performance for skipping `log` records that fall below the
collector's max level, as they will not have to call the
`LogTracer::enabled` method.

This will prevent issues like bytecodealliance/wasmtime#2662
from occurring in the future. See also #1249.

In order to implement this, I also changed the `FmtSubscriber`'s
`try_init` to just use `util::SubscriberInitExt`'s `try_init` function,
so that the same code isn't duplicated in multiple places. I also
added `AsLog` and `AsTrace` conversions for `LevelFilter`s in
the `tracing-log` crate.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
Copy link
Member

hawkw commented Feb 18, 2021

@pchickey okay, you should be able to test this out with a git dependency on this branch: #1250

I believe you will have to patch the tracing-subscriber and tracing-log crates, since the tracing-subscriber fix depends on changes in tracing-log. The easiest way might be pointing .cargo/config at a local git checkout, rather than messing around with [patch].

@hawkw hawkw self-assigned this Feb 18, 2021
@hawkw hawkw added crate/log Related to the `tracing-log` crate crate/subscriber Related to the `tracing-subscriber` crate kind/bug Something isn't working labels Feb 18, 2021
@pchickey
Copy link
Author

Unfortunately the backport branch (#1250) isn't quite working yet. Alex put together this https://gist.github.com/alexcrichton/bfaa02d0833e1a95709e3432a1d18c46to reproduce it - "log level enabled" prints in both v0.1.x and eliza/backport-log-stuff

@hawkw
Copy link
Member

hawkw commented Feb 19, 2021

Unfortunately the backport branch (#1250) isn't quite working yet. Alex put together this https://gist.github.com/alexcrichton/bfaa02d0833e1a95709e3432a1d18c46to reproduce it - "log level enabled" prints in both v0.1.x and eliza/backport-log-stuff

Ah, looks like there's actually just two bugs here. I added some dbg!s:

[tracing-core/src/callsite.rs:56] dispatch.max_level_hint() = None
[tracing-core/src/callsite.rs:57] level_hint = LevelFilter::TRACE
[tracing-log/src/log_tracer.rs:260] self.filter = Trace
[examples/examples/log-enabled.rs:8] tracing_core::LevelFilter::current() = LevelFilter::TRACE
log level enabled

It looks like the FmtSubscriber type in particular does not forward the max level hint correctly. If I change the example to set up the subscriber like this:

    use tracing_subscriber::prelude::*;
    tracing_subscriber::registry()
        .with(tracing_subscriber::fmt::layer())
        .with(tracing_subscriber::EnvFilter::from_default_env())
        .init();

everything works correctly, and "log level enabled" is not printed.

This should be pretty easy to fix: we just want to ensure that method is forwarded in the Subscriber implementation for the FmtSubscriber wrapper type.

@hawkw
Copy link
Member

hawkw commented Feb 19, 2021

#1251 fixes the FmtSubscriber bug (technically the FmtCollector bug, since it's against 0.2). I'll backport that to 0.1 once it merges.

I'm going to go ahead and merge the backport branch, since it does in fact fix the issue, except when using the FmtSubscriber type (as in the repro).

hawkw added a commit that referenced this issue Feb 19, 2021
Depends on #1247.

Since `tracing-subscriber`'s `init` and `try_init` functions set the
global default subscriber, we can use the subscriber's max-level hint as
the max level for the log crate, as well. This should significantly
improve performance for skipping `log` records that fall below the
collector's max level, as they will not have to call the
`LogTracer::enabled` method.

This will prevent issues like bytecodealliance/wasmtime#2662
from occurring in the future. See also #1249.

In order to implement this, I also changed the `FmtSubscriber`'s
`try_init` to just use `util::SubscriberInitExt`'s `try_init` function,
so that the same code isn't duplicated in multiple places. I also
added `AsLog` and `AsTrace` conversions for `LevelFilter`s in
the `tracing-log` crate.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
Copy link
Member

hawkw commented Feb 19, 2021

@pchickey just triple-checking: landing both #1250 and backporting #1251 to v0.1.x fixes the repro case for you, correct?

@pchickey
Copy link
Author

Yes, I checked that the merge of those branches fixes both the small repro case and the bigger issue over in wasmtime.

@hawkw
Copy link
Member

hawkw commented Feb 19, 2021

One last PR: #1254 should ensure we handle log_enabled! correctly in even more cases.

@hawkw
Copy link
Member

hawkw commented Feb 21, 2021

closed by...the sum total of all the various PRs i mentioned here.

@hawkw hawkw closed this as completed Feb 21, 2021
kaffarell pushed a commit to kaffarell/tracing that referenced this issue May 22, 2024
Depends on tokio-rs#1247.

Since `tracing-subscriber`'s `init` and `try_init` functions set the
global default subscriber, we can use the subscriber's max-level hint as
the max level for the log crate, as well. This should significantly
improve performance for skipping `log` records that fall below the
collector's max level, as they will not have to call the
`LogTracer::enabled` method.

This will prevent issues like bytecodealliance/wasmtime#2662
from occurring in the future. See also tokio-rs#1249.

In order to implement this, I also changed the `FmtSubscriber`'s
`try_init` to just use `util::SubscriberInitExt`'s `try_init` function,
so that the same code isn't duplicated in multiple places. I also
added `AsLog` and `AsTrace` conversions for `LevelFilter`s in
the `tracing-log` crate.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate/log Related to the `tracing-log` crate crate/subscriber Related to the `tracing-subscriber` crate kind/bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants