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

Consider enabling debug logging by default for contributors #76608

Open
jyn514 opened this issue Sep 11, 2020 · 8 comments
Open

Consider enabling debug logging by default for contributors #76608

jyn514 opened this issue Sep 11, 2020 · 8 comments
Labels
A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself C-feature-request Category: A feature request, i.e: not implemented / a PR. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@jyn514
Copy link
Member

jyn514 commented Sep 11, 2020

There was a lot of discussion in #76588 about enabling logging and it was almost a 10% perf hit on some benchmarks. So as it stands, the tradeoff is probably not worth it, especially since most of the time you aren't looking at debug! logging.

However, having debug! completely compiled out is extremely confusing the first time you go to add a debug!. While rustc-dev-guide can work on making this more discoverable, ideally this would always be on so you don't have to worry about it. I would love to get the perf impact low enough that this can be on by default (or even always on).

One possible way to work towards this is by moving the more verbose logging to trace! and compiling out trace instead. This allows keeping the (truly stupendous) amount of logging currently, but still making debug! work by default. Since trace! is rarely used, I expect this to be much less confusing for new contributors.

Originally posted by @jyn514 in #76588 (comment)

@jyn514 jyn514 added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) C-feature-request Category: A feature request, i.e: not implemented / a PR. labels Sep 11, 2020
@Mark-Simulacrum
Copy link
Member

I'm not sure how feasible it is to implement, but I think a message printed at startup along the lines of "you enabled debug logging in a filter, but it's been statically disabled, consider toggling debug-logging" would be a great partial fix here.

@RalfJung
Copy link
Member

here was a lot of discussion in #76588 about enabling logging and it was almost a 10% perf hit on some benchmarks.

So that was 10% just from logging being enabled, without also enabling debug assertions or actually printing anything? That seems a lot.

I recall @eddyb hypothesizing that the perf hit is mostly caused by debug assertions.

@Mark-Simulacrum
Copy link
Member

@hawkw, do you know if tracing upstream supports the "warn on statically disabled logging?" I mention here: #76608 (comment)?

I see https://docs.rs/tracing/0.1.19/tracing/subscriber/trait.Subscriber.html#method.max_level_hint, which looks promising, but I don't see a good way of detecting whether the environment variable filter parses to something that is more verbose than the current static (compile-time) levels.

@jyn514
Copy link
Member Author

jyn514 commented Sep 13, 2020

do you know if tracing upstream supports the "warn on statically disabled logging?" I mention here: #76608 (comment)?

This wouldn't need changes from tracing I don't think - we could add a cfg(feature = "max_level_info") to rustc_driver and print out a message on initialization if RUSTC_LOG was set to some value that includes debugging.

@Mark-Simulacrum
Copy link
Member

Hm, by just seeing if it includes debug or trace levels? I guess we could, but it seems like maybe tracing upstream would be interested in printing a warning when this situation is detected. I would love to have this more universally true, rather than specific to rustc.

@jyn514 jyn514 added the A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself label Sep 13, 2020
@hawkw
Copy link
Contributor

hawkw commented Sep 14, 2020

Hm, by just seeing if it includes debug or trace levels? I guess we could, but it seems like maybe tracing upstream would be interested in printing a warning when this situation is detected. I would love to have this more universally true, rather than specific to rustc.

👋 This is something that we'd love to be able to do, but we don't currently support. It would be great to get a tracing feature request describing the particular use-case so we could see about implementing something upstream.

With that said, I imagine it would definitely be possible to write a simple implementation in rustc as a stop-gap until we land something upstream.

@jyn514
Copy link
Member Author

jyn514 commented Sep 14, 2020

Opened tokio-rs/tracing#975 for this.

@jyn514
Copy link
Member Author

jyn514 commented May 25, 2023

There was a lot of discussion in #76588 about enabling logging and it was almost a 10% perf hit on some benchmarks.

So that was 10% just from logging being enabled, without also enabling debug assertions or actually printing anything? That seems a lot.

It's now around 4.5%: #111924 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself C-feature-request Category: A feature request, i.e: not implemented / a PR. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants