Allow initializing logger with additional tracing Layer #140969
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
S-waiting-on-review
|
I don't understand why the build fails, locally it works. The CI logs are 400MB, but here are the last few lines: |
|
Looks like something in this PR is causing a lot more debug messages to be emitted. Some of those debug messages use the "def path formatting" machinery, which is expensive and so there are safeguards that this machinery is only called when there are actual compilation errors to emit -- which isn't the case here, so we ultimately get an ICE. But the ICE isn't the actual problem, the actual problem is that debug tracing now seems to be enabled by default. |
RalfJung
reviewed
May 20, 2025
RalfJung
reviewed
May 20, 2025
compiler/rustc_log/src/lib.rs
Outdated
| @@ -124,7 +135,8 @@ pub fn init_logger(cfg: LoggerConfig) -> Result<(), Error> { | |||
| Err(_) => {} // no wraptree | |||
| } | |||
|
|
|||
| let subscriber = tracing_subscriber::Registry::default().with(filter).with(layer); | |||
| let subscriber = | |||
| Registry::default().with(additional_tracing_layer).with(layer.with_filter(filter)); | |||
There was a problem hiding this comment.
This seems to change the way the filter is applied, which might explain the explosion of debug messages?
|
@rustbot author |
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
This comment has been minimized.
This comment has been minimized.
The previous method, where a layer would be passed directly, required to pass a "no-op" layer when no custom layer was needed. This should have in theory worked, however having a no-op layer seems to change the way the tracing lib applies filters internally, leading to some debug!() being printed despite them being out of the minimum level for the filters. Note however that this behavior was very inconsistent, and e.g. some debug!() would get printed and some others wouldn't, for no apparent reason.
|
@rustbot ready Sorry for the late response. I went down a rabbit hole trying to figure out why I still haven't got to the culprit, but I have a feeling that it might be due to the default implementation of Anyway, for now I changed the code in this PR to use Example of
|
rustbot
added
S-waiting-on-review
|
Actually, after further testing in Miri, I noticed that adding any custom layer (e.g. by calling |
RalfJung
reviewed
Jun 11, 2025
| @@ -124,7 +150,7 @@ pub fn init_logger(cfg: LoggerConfig) -> Result<(), Error> { | |||
| Err(_) => {} // no wraptree | |||
| } | |||
|
|
|||
| let subscriber = tracing_subscriber::Registry::default().with(filter).with(layer); | |||
| let subscriber = build_subscriber().with(layer.with_filter(filter)); | |||
There was a problem hiding this comment.
So making this .with(filter).with(layer) as before does not work?
This is the one part of the PR I am not sure about, since I don't know much about tracing.
Cc @oli-obk
There was a problem hiding this comment.
No, because it will apply the filter to every layer, including any custom layer we might add
There was a problem hiding this comment.
So despite the fact that the filter is added after the custom layers, it takes effect before them, preventing events from reaching them...?
There was a problem hiding this comment.
Yes, see the last paragraph here: https://docs.rs/tracing-subscriber/0.3.16/tracing_subscriber/layer/index.html#global-filtering
There was a problem hiding this comment.
Wow, that's not what I expected.^^
If it doesn't happen from this PR alone, I'm fine with landing it. However it'd still be worth finding a minimal example for a bugreport. |
|
@rustbot ready Thank you for the review, I removed the |
|
This LGTM from my side. @oli-obk do you want to take a look or should I just r+? |
|
@bors r=RalfJung,oli-obk |
bors
added
S-waiting-on-bors
|
@bors rollup |
bors
added a commit
that referenced
this pull request
Jun 14, 2025
Rollup of 16 pull requests Successful merges: - #140969 (Allow initializing logger with additional tracing Layer) - #141352 (builtin dyn impl no guide inference) - #142046 (add Vec::peek_mut) - #142273 (tests: Minicore `extern "gpu-kernel"` feature test) - #142302 (Rework how the disallowed qualifier in function type diagnostics are generated) - #142405 (Don't hardcode the intrinsic return types twice in the compiler) - #142434 ( Pre-install JS dependencies in tidy Dockerfile) - #142439 (doc: mention that intrinsics should not be called in user code) - #142441 (Delay replacing escaping bound vars in `FindParamInClause`) - #142449 (Require generic params for const generic params) - #142452 (Remove "intermittent" wording from `ReadDir`) - #142459 (Remove output helper bootstrap) - #142460 (cleanup search graph impl) - #142461 (compiletest: Clarify that `--no-capture` is needed with `--verbose`) - #142475 (Add platform support docs & maintainers for *-windows-msvc) - #142480 (tests: Convert two handwritten minicores to add-core-stubs) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
that referenced
this pull request
Jun 14, 2025
Rollup merge of #140969 - Stypox:logger-layer, r=RalfJung,oli-obk Allow initializing logger with additional tracing Layer This PR adds functions to the `rustc_log` and `rustc_driver_impl` crates to allow initializing the logger with an additional `tracing_subscriber::Layer`. This will be used in Miri to save trace events to file using e.g. [`tracing-chrome`](https://github.com/thoren-d/tracing-chrome)'s or [`tracing-tracy`](https://github.com/nagisa/rust_tracy_client)'s `Layer`s. Additional context on the choice of signature can be found in [this Zulip thread](https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/Adding.20a.20dependency.20to.20Miri.20that.20depends.20on.20a.20rustc.20dep/near/515707776). I re-exported `tracing_subscriber::{Layer, Registry};` from `rustc_log` so that `rustc_driver_impl` can use them in the function signature without depending on `tracing_subscriber` directly. I did this to avoid copy-pasting the dependency line with all of the enabled features from the `rustc_log` to the `rustc_driver_impl`'s Cargo.toml, which would have possibly led to redundancies and inconsistencies. r? `@RalfJung`
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds functions to the
rustc_logandrustc_driver_implcrates to allow initializing the logger with an additionaltracing_subscriber::Layer. This will be used in Miri to save trace events to file using e.g.tracing-chrome's ortracing-tracy'sLayers.Additional context on the choice of signature can be found in this Zulip thread.
I re-exported
tracing_subscriber::{Layer, Registry};fromrustc_logso thatrustc_driver_implcan use them in the function signature without depending ontracing_subscriberdirectly. I did this to avoid copy-pasting the dependency line with all of the enabled features from therustc_logto therustc_driver_impl's Cargo.toml, which would have possibly led to redundancies and inconsistencies.r? @RalfJung