Skip to content

Adds user-definable log formatting#3778

Closed
SonicZentropy wants to merge 3 commits intobevyengine:mainfrom
SonicZentropy:main
Closed

Adds user-definable log formatting#3778
SonicZentropy wants to merge 3 commits intobevyengine:mainfrom
SonicZentropy:main

Conversation

@SonicZentropy
Copy link

Objective

  • Allow users to alter log formatting

Solution

  • tracing_subscriber offers a Subscriber trait to allow this customization, so I put one into LogSettings and check for it before making the prior default Subscriber.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jan 27, 2022
@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Log and removed S-Needs-Triage This issue needs to be labelled labels Jan 27, 2022
@SonicZentropy SonicZentropy marked this pull request as draft January 27, 2022 10:35
@SonicZentropy
Copy link
Author

Post-CI I'm not sure this is even worth draft status at the moment. It seems wasm and android builds both add required trait bounds on anything holding a Subscriber that enter the HRTB territory, which is fascinating but pretty beyond me at this point and perhaps better left to collect cobwebs until I understand what's going on better

@SonicZentropy SonicZentropy force-pushed the main branch 2 times, most recently from 366d467 to d8c205b Compare January 28, 2022 23:35
@SonicZentropy
Copy link
Author

Nevermind, found a better way, seems to work fine now!

@SonicZentropy SonicZentropy marked this pull request as ready for review January 29, 2022 00:02
@rparrett
Copy link
Contributor

rparrett commented Jan 29, 2022

I wonder how this potentially interacts with the trace_chrome and trace_tracy features, and if it makes sense to use a separate subscriber for logging and tracing.

Could be a bit of a footgun if defining a custom logging format prevents you from profiling your code in the future. I'm not really familiar enough with tracing to say.

I just want to point out another PR that touches this code here: #3583.

@SonicZentropy
Copy link
Author

SonicZentropy commented Jan 29, 2022

You actually were completely correct about that @rparrett, though I didn't see this until I just stumbled across it in use. I stuck my head into tokio's Discord and had them help me modify it to a more proper version. This one should take care of those issues

Link to Tokio discussion regarding this

@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 Feb 4, 2022
/// # use bevy_app::App;
/// # use bevy_log::LogSettings;
/// # use bevy_utils::tracing::Level;
/// # use bevy_utils::tracing::{ Level };
Copy link
Member

Choose a reason for hiding this comment

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

this looks like something that was part of an experiment and you didn't revert back completely?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, sorry about the delay, honestly forgot this PR existed after a while. That was probably my global config clashing and me not noticing, sorry

@SonicZentropy
Copy link
Author

I missed the super basic formatting problem and this didn't get merged, apologies. Looks like it has a conflict now, so I'll try to resolve it this weekend since I'm sure it'll be minor

/// # use bevy_app::App;
/// # use bevy_log::LogSettings;
/// # use bevy_utils::tracing::Level;
/// # use bevy_utils::tracing::{ Level };
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// # use bevy_utils::tracing::{ Level };
/// # use bevy_utils::tracing::Level;

@alice-i-cecile
Copy link
Member

@SonicZentropy I'd like to merge this :) While you're rebasing, there's a tiny formatting suggestion for the doc example too.

@alice-i-cecile alice-i-cecile added the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label May 9, 2022
@vladbat00
Copy link
Contributor

It would be great if we allowed our users to pass not just 1 layer but multiple ones. Or we maybe could allow passing a custom subscriber, like it was done in the initial attempt, I'm curious what was the problem exactly with that one.
This way users can also get the benefit of not only customizing the formatter but also passing additional layers - that would come in handy for the puffin integration for instance if we decide that it should exist outside of Bevy and be added by users via either a plugin or by passing a puffin layer manually.

@cart cart added A-Diagnostics Logging, crash handling, error reporting and performance analysis and removed A-Log labels Jun 1, 2022
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
@tygyh
Copy link
Contributor

tygyh commented Jan 28, 2024

I believe the functionality from this PR has already been added in other later changes. Is there a reason to keep it open?

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 S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants