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

Add Log Events: A simple way to view tracing logs #10737

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

doonv
Copy link
Contributor

@doonv doonv commented Nov 25, 2023

Objective

Some plugins (such as my W.I.P. bevy_dev_console plugin) require access to tracing's logs (logs created with info!, warn!, error!, etc.). However, it is quite difficult to access tracing's logs. You need to replace the default LogPlugin with your own implementation that adds a custom Layer for handling logs.

This PR adds a LogMessage event which makes getting tracing logs much easier.

Solution

I created LogEventLayer, this is a struct that implements Layer and stores an Arc<Mutex<Vec<LogMessage>>> (which is shared with a LogEvents resource). The LogEventLayer layer uses LogEventVisitor to record the message string. LogEventLayer pushes the message along with it's metadata to it's Arc<Mutex<Vec<LogMessage>>>. Then a system called transfer_log_events reads those log events with the LogEvents resource and writes them to Events<LogMessage> via EventWriter<LogEvent>.

This implementation isn't the best and it's quite long, but I'm not sure if there's a simpler way to transfer the log events from the LogEventLayer to the Events<LogMessage>. If you have an idea of how to simplify it that would be appreciated.

I also added an example of how to use it.


Changelog

  • Added Log Message Events: A simple way to view tracing logs

Copy link
Contributor

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

@nicopap nicopap self-requested a review November 25, 2023 19:27
@Nilirad Nilirad added A-ECS Entities, components, systems, and events A-Diagnostics Logging, crash handling, error reporting and performance analysis and removed A-ECS Entities, components, systems, and events labels Nov 25, 2023
@mockersf
Copy link
Member

I don't think logs should be forwarded as events

Either you need events from some of the things that are logged, then add events at those points, or you need to process logs, then you should add a subscriber to tracing (#7682)

@Nilirad Nilirad added the C-Usability A targeted quality-of-life change that makes Bevy easier to use label Nov 25, 2023
@doonv
Copy link
Contributor Author

doonv commented Nov 25, 2023

I actually did have a nearly identical solution to #7682, but I ended scrapping it for 3 reasons:

  1. Implementing a Layer and Visitor yourself is too verbose. (Especially the part where you transfer the information from the Subscriber to Bevy ECS)
  2. You don't gain much by implementing your own Layer and Visitor instead of just reading from a EventReader<LogMessage>.
  3. Reading from a EventReader<LogMessage> makes much more sense to an average user than learning about tracing's Layers and Visitors.

@doonv doonv closed this Nov 25, 2023
@doonv doonv reopened this Nov 25, 2023
@doonv
Copy link
Contributor Author

doonv commented Nov 25, 2023

I accidentally closed it, whoops.

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Nov 26, 2023
@james7132
Copy link
Member

Instead of listening in via Bevy events, it may be better to use tracing's infrastructure (i.e. tracing_subscriber's Layer). bevy_log currently does not support this kind of use, but adding support for adding additional tracing_subscriber layers during initialization might be a good idea. Though supporting this in an extendable way might be a bit tough since LogPlugin is only initialized once.

@nicopap
Copy link
Contributor

nicopap commented Nov 27, 2023

I think this kind of feature would be useful to display error logs on screen such as with https://github.com/nicopap/bevy-debug-text-overlay/

@doonv
Copy link
Contributor Author

doonv commented Nov 27, 2023

@james7132 It should be possible to add additional layers by reinitializing the plug-in and all its layers.

But, as I said earlier I don't see the benefit of creating your own layers for the reasons I already talked about above.

  1. Implementing a Layer and Visitor yourself is too verbose. (Especially the part where you transfer the information from the Subscriber to Bevy ECS)
  2. You don't gain much by implementing your own Layer and Visitor instead of just reading from a EventReader<LogMessage>.
  3. Reading from a EventReader<LogMessage> makes much more sense to an average user than learning about tracing's Layers and Visitors.

@ActuallyHappening
Copy link
Contributor

ActuallyHappening commented Jun 10, 2024

I'm also slightly concerned about performance issues, unless of course tracing (through the normal LogPlugin.filter / EnvFilter) eliminates all useless logs as expected. Obviously we don't want every trace inside of wgpu being sent as a bevy event!

It would be great to be able to have some way of reading tracing events, maybe very intentionally feature flagging this on an opt-in basis for the bevy_log crate? I don't want to have to re-implement bevy_log::LogPlugin to display a debug stream of tracing events.

And I'm probably naive, but what's stopping this from being implemented using LogPlugin.update_subscriber here? Maybe making LogPlugin.update_subscriber a Vec<_> would allow libraries (such as bevy_dev_console) to add their own layers?

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 X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants