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

Support for user defined log formatting #8374

Closed
wants to merge 5 commits into from
Closed

Support for user defined log formatting #8374

wants to merge 5 commits into from

Conversation

eerii
Copy link
Contributor

@eerii eerii commented Apr 13, 2023

Objective

Adopted #3778.

This pr allows to configure how the bevy log is formatted. The default bevy log can be a bit verbose for some projects, specially the timestamp. The objective is to allow developers to select the format that best suits them.

Example of original bevy log:
2023-04-13T14:19:53.674653Z INFO bevy_render::renderer: [important text!]

Solution

Using tracing_subscriber::fmt::Subscriber, adds the attribute layer to LogPlugin. This allows to pass a tracing_subscriber::fmt::Layer when creating the plugin and modifying the output.

For example this

App::new()
    .add_plugins(DefaultPlugins.set(bevy::log::LogPlugin {
        layer: Box::new(|| Box::new(bevy::log::tracing_subscriber::fmt::layer().pretty().without_time()))
        ..default()
    }))
    .run()

prints

INFO bevy_render::renderer: [important text!]

The main idea comes from @SonicZentropy and their pull request #3778, but it was stale since may. This has all the changes and a fix since now LogSettings are included in the plugin and not in a resource.

Since plugin builders are stateless and not mutable (#8101), the old solution of using an Option<Box<Layer>> no longer works. One solution to fix this is to use a Box<Fn() -> Box<Layer>>. I am unaware of another more idiomatic fix, so if you have something better please revise it. Should plugins builders be mutable in the future, this can be changed back to the original approach.


Changelog

  • Adds field layer to LogPlugin that takes a function returning a tracing_subscriber custom layer.
  • Reexports tracing_subscriber from log so it can be used to configure the plugin.
  • Modifies the logs.rs and system_piping.rs examples to include the new configuration.

Migration Guide

No breaking changes

@github-actions
Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@eerii eerii marked this pull request as draft April 13, 2023 17:34
@eerii eerii marked this pull request as ready for review April 13, 2023 19:42
Copy link
Contributor

@hymm hymm left a comment

Choose a reason for hiding this comment

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

The boxed function is unfortunate, but makes sense when the plugin needs to be immutable. Another option could be to use some interior mutability pattern, but you'd lose the ability to reuse the plugin.

crates/bevy_log/src/lib.rs Outdated Show resolved Hide resolved
examples/app/logs.rs Outdated Show resolved Hide resolved
@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Diagnostics Logging, crash handling, error reporting and performance analysis labels Apr 15, 2023
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Apr 22, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 22, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 22, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 22, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 22, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 23, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 23, 2023
@eerii
Copy link
Contributor Author

eerii commented Apr 23, 2023

Sorry about that, when I made the first change I accidentally moved adding the fmt layer from inside a block that only ran if it was not wasm. There are some more changes since fmt_layer has to be the first one to be added to the subscriber to satisfy trait bounds. If you could review it again, now it should (hopefully) work.

@hymm hymm self-requested a review April 23, 2023 17:30
@hymm
Copy link
Contributor

hymm commented Apr 23, 2023

Am I understanding correctly that custom layers aren't supported on android and wasm32? If so, this should at least be documented.

@alice-i-cecile alice-i-cecile removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Apr 23, 2023
@eerii
Copy link
Contributor Author

eerii commented Apr 24, 2023

I have looked a bit more into this and the issue is that tracing doesn't support wasm/android (tokio-rs/tracing#642), mainly because time panicks, (rust-lang/rust#48564). Bevy is using tracing-wasm and android_log-sys as workarounds for those special cases, passing tracing_wasm::WASMLayerConfig::default and android_tracing::AndroidLayer::default as a default format layer.

We could go about it two ways. Either as it is now, where the user can only specify the layer for all platforms but those two, or we make it so the user has to specify all layers. That could be done with relative ease as we already need to use a function that returns a box due to the plugin mutability requirements, where we could have something like:

LogPlugin {
    layer: Box::new(|| {
        #[cfg(target_arch = "wasm32")]
        return Box::new(tracing_wasm::WASMLayer::new(tracing_wasm::WASMLayerConfig::default()));
        #[cfg(target_os = "android")]
        return Box::new(android_tracing::AndroidLayer::default());

        Box::new(tracing_subscriber::fmt::Layer::default())
    }),
    ..default()
}

This would be great in terms of customizability, but it would be difficult to use as an user. Another option would be to have three parameters, layer, layer_wasm and layer_android and have them be separately configurable, each with their defaults. However, it would mean increasing the number of functions and boxes. A middle ground could be having the layer function return a struct of boxes, one for each of the different platforms which could have defaults.

How do you think we should proceed?

@alice-i-cecile
Copy link
Member

Another option would be to have three parameters, layer, layer_wasm and layer_android and have them be separately configurable, each with their defaults. However, it would mean increasing the number of functions and boxes. A middle ground could be having the layer function return a struct of boxes, one for each of the different platforms which could have defaults.

I dislike these options, as they require the user to reason about platforms they may not care about.

So the option shown by your example gets my vote (with docs).

Estus-Dev pushed a commit to Estus-Dev/bevy that referenced this pull request Jul 10, 2023
# Objective

Adopted bevyengine#3778.

This pr allows to configure how the bevy log is formatted. The default
bevy log can be a bit verbose for some projects, specially the
timestamp. The objective is to allow developers to select the format
that best suits them.

```
Example of original bevy log:
2023-04-13T14:19:53.674653Z INFO bevy_render::renderer: [important text!]
```

## Solution

Using `tracing_subscriber::fmt::Subscriber`, adds the attribute `layer`
to `LogPlugin`. This allows to pass a `tracing_subscriber::fmt::Layer`
when creating the plugin and modifying the output.

For example this

```rust
App::new()
    .add_plugins(DefaultPlugins.set(bevy::log::LogPlugin {
        layer: Box::new(|| Box::new(bevy::log::tracing_subscriber::fmt::layer().pretty().without_time()))
        ..default()
    }))
    .run()
```

prints

```
INFO bevy_render::renderer: [important text!]
```

The main idea comes from @SonicZentropy and their pull request bevyengine#3778,
but it was stale since may. This has all the changes and a fix since now
`LogSettings` are included in the plugin and not in a resource.

Since plugin builders are stateless and not mutable (bevyengine#8101), the old
solution of using an `Option<Box<Layer>>` no longer works. One solution
to fix this is to use a `Box<Fn() -> Box<Layer>>`. I am unaware of
another more idiomatic fix, so if you have something better please
revise it. Should plugins builders be mutable in the future, this can be
changed back to the original approach.

---

## Changelog

- Adds field `layer` to `LogPlugin` that takes a function returning a
`tracing_subscriber` custom layer.
- Reexports `tracing_subscriber` from log so it can be used to configure
the plugin.
- Modifies the `logs.rs` and `system_piping.rs` examples to include the
new configuration.

## Migration Guide

No breaking changes
Estus-Dev pushed a commit to Estus-Dev/bevy that referenced this pull request Jul 10, 2023
# Objective

Adopted bevyengine#3778.

This pr allows to configure how the bevy log is formatted. The default
bevy log can be a bit verbose for some projects, specially the
timestamp. The objective is to allow developers to select the format
that best suits them.

```
Example of original bevy log:
2023-04-13T14:19:53.674653Z INFO bevy_render::renderer: [important text!]
```

## Solution

Using `tracing_subscriber::fmt::Subscriber`, adds the attribute `layer`
to `LogPlugin`. This allows to pass a `tracing_subscriber::fmt::Layer`
when creating the plugin and modifying the output.

For example this

```rust
App::new()
    .add_plugins(DefaultPlugins.set(bevy::log::LogPlugin {
        layer: Box::new(|| Box::new(bevy::log::tracing_subscriber::fmt::layer().pretty().without_time()))
        ..default()
    }))
    .run()
```

prints

```
INFO bevy_render::renderer: [important text!]
```

The main idea comes from @SonicZentropy and their pull request bevyengine#3778,
but it was stale since may. This has all the changes and a fix since now
`LogSettings` are included in the plugin and not in a resource.

Since plugin builders are stateless and not mutable (bevyengine#8101), the old
solution of using an `Option<Box<Layer>>` no longer works. One solution
to fix this is to use a `Box<Fn() -> Box<Layer>>`. I am unaware of
another more idiomatic fix, so if you have something better please
revise it. Should plugins builders be mutable in the future, this can be
changed back to the original approach.

---

## Changelog

- Adds field `layer` to `LogPlugin` that takes a function returning a
`tracing_subscriber` custom layer.
- Reexports `tracing_subscriber` from log so it can be used to configure
the plugin.
- Modifies the `logs.rs` and `system_piping.rs` examples to include the
new configuration.

## Migration Guide

No breaking changes
@eerii eerii closed this by deleting the head repository Oct 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Diagnostics Logging, crash handling, error reporting and performance analysis C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants