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

Log Plugin #836

Merged
merged 4 commits into from
Nov 13, 2020
Merged

Log Plugin #836

merged 4 commits into from
Nov 13, 2020

Conversation

cart
Copy link
Member

@cart cart commented Nov 11, 2020

This builds on the work in #789 by adding a new bevy_log plugin. It removes all log crate access in favor of tracing, and all log initialization code to bevy_log. This is nice because the wasm examples are cleaned up significantly.

bevy_log re-exports tracing types, which are now available in bevy::prelude (so you can just do info!("hello")
bevy_utils re-exports tracing (for use in core crates like bevy_app, which can't consume the bevy_log plugin)

I also moved the "chrome tracing" from the example to an optional trace_chrome feature, which enables that subscriber in bevy_log.

Followups:

  • it would be nice to initialize android_logger here too (instead of in the example), but we might need to build an android tracing subscriber.
  • wasm logs got a bit noisier because the released tracing-wasm doesn't let you configure log level Release request old-storyai/tracing-wasm#14 (bevy defaults to disabling trace logging in release mode, so building in release is a temporary workaround for console spam)

@cart
Copy link
Member Author

cart commented Nov 11, 2020

@superdump

Copy link
Contributor

@superdump superdump left a comment

Choose a reason for hiding this comment

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

Looks good to me. Though in your opening statement you said you wouldn’t point to my branch because it can’t be published to crates.io, yet my branch is still in the Cargo.toml.

EDIT: no, you said that in the review of my PR. Well, still, I’m not sure it was intentional to point to my branch.

crates/bevy_log/Cargo.toml Outdated Show resolved Hide resolved
@karroffel karroffel added the C-Feature A new feature, making something new possible label Nov 11, 2020
@cart
Copy link
Member Author

cart commented Nov 11, 2020

@mrk-its this is relevant to wasm logs, so feel free to weigh in
edit: also @smokku 😄

@smokku
Copy link
Member

smokku commented Nov 11, 2020

I just checked-out this branch and it looks very cool on native. Kudos.

Although the constant spamming of messages to browser console will make development for web impossible. (and eventually grind browser tab to a halt)

@cart
Copy link
Member Author

cart commented Nov 11, 2020

Yeah we might need to either wait for them to publish the next version of wasm_tracing or publish our own version while we wait.

@smokku
Copy link
Member

smokku commented Nov 12, 2020

If it would be blocking Bevy release, we might just temporarily disable configuring logging on wasm and rely on people still using log in their apps.

@superdump
Copy link
Contributor

@cart it looks like tracing-wasm has a max_level thing in its configuration. But also there should be a way to define a filter layer even if it didn't. There are other filter layers than just the EnvFilter: https://docs.rs/tracing-subscriber/0.2.15/tracing_subscriber/filter/index.html

@superdump
Copy link
Contributor

Also, it looks like EnvFilter can be constructed from a string and not necessarily from an environment variable: https://docs.rs/tracing-subscriber/0.2.15/tracing_subscriber/filter/struct.EnvFilter.html#method.new

#[cfg(target_arch = "wasm32")]
fn setup_wasm() {
console_error_panic_hook::set_once();
tracing_wasm::set_as_global_default();
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this and do something more like setup_default except use tracing-wasm instead of tracing-chrome. It would be nice if one could pass in or set some variable to configure the filter directives passed to EnvFilter::try_new() as a string.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah the master branch has a max_level configuration item, but the crates.io version doesn't (which is why the issue I filed in their repo is asking for them to cut a new release).

The env filter does seem like a nice workaround though. Good call!

@mrk-its
Copy link
Member

mrk-its commented Nov 12, 2020

I was playing with it today in browser and after little tweaking it works great! Like @superdump mentioned above filterning on wasm can be enabled the same way as for native:

-    tracing_wasm::set_as_global_default();
+    let filter_layer = EnvFilter::try_from_default_env()
+        .or_else(|_| EnvFilter::try_new("info,wgpu=warn"))
+        .unwrap();
+    bevy_utils::tracing::subscriber::set_global_default(
+        Registry::default()
+            .with(filter_layer)
+            .with(tracing_wasm::WASMLayer::new(tracing_wasm::WASMLayerConfig::default())),
+    )
+    .expect("default global");

With bits of web_sys we can also easily pass configuration directives to EnvFilter via url params / url fragment, like:

http://localhost:4040/sprite.html#RUST_LOG=debug or so

@superdump
Copy link
Contributor

superdump commented Nov 12, 2020

Oh, I like that idea! Changing URI fragments don't cause page reloads if I recall correctly... I wonder if you can edit it on the fly and have the filters change...? :) @mrk-its

@mrk-its
Copy link
Member

mrk-its commented Nov 12, 2020

Oh, I like that idea! Changing URI fragments don't cause page reloads if I recall correctly... I wonder if you can edit it on the fly and have the filters change...? :) @mrk-its

@superdump yes: https://developer.mozilla.org/en-US/docs/Web/API/WindowEventHandlers/onhashchange Unfortunately a lot of js apps use fragment some way and it may interfere with them. The other interesting option is window.localStorage (e.g. window.localStorage.RUST_LOG='info' in js console) - the good thing is it will survive page reloads.

@cart
Copy link
Member Author

cart commented Nov 12, 2020

Just added a simple android tracing backend. I'm going to make filtering configurable via LogSettings, then we should be good to go.

Feel free to follow up with WASM niceties like local storage filtering if the fancy strikes you 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants