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 support for adding custom log layers #7682

Closed
wants to merge 5 commits into from

Conversation

johanhelsing
Copy link
Contributor

Objective

  • Currently, custom log layers can not co-exist with LogPlugin
  • This makes it hard to integrate with other error diagnostics solutions like automatic error reporting, saving logs to files, etc. without also losing the normal functionality of bevy's logging
  • Enabler for Bevy Log Output to Files #5233, alternative to Log to file #5342

Solution

  • Add a number of user-specified, boxed Subscribers to LogPlugin's settings

Additional notes

Draft PR because how extra_layers is passed in (Arc<Mutex<Option<Vec>>>) is pretty ugly. I'm not sure what the current idiom of letting Plugin::build take ownership of something is? Should it be in a resource instead?

@alice-i-cecile alice-i-cecile added C-Usability A simple quality-of-life change that makes Bevy easier to use A-Diagnostics Logging, crash handling, error reporting and performance analysis labels Feb 15, 2023
@geieredgar
Copy link
Contributor

After this comment #8101 (comment) for my PR

note this potentially goes would go against cart's comment here that plugin build should be stateless.

#4212 (comment)

Something else worth calling out: consuming self would mean that the Plugin's init isn't a repeatable process, which might come in handy for things like editors, hot reloading, etc.
I think we should be moving in the direction of "plugins are completely stateless by default and configured with an instance of a specific type (ideally with a default impl)"

I think that the best approach would be to use Box<dyn Fn() -> Vec<_>> as type for extra_layers with Box::new(|| Vec::new()) as default value. And then branching on empty/non-empty Vec. Would that work for you?

@alice-i-cecile
Copy link
Member

Closing; similar work was completed in #10822.

github-merge-queue bot pushed a commit that referenced this pull request Jan 15, 2024
# Objective

This PR is heavily inspired by
#7682
It aims to solve the same problem: allowing the user to extend the
tracing subscriber with extra layers.

(in my case, I'd like to use `use
metrics_tracing_context::{MetricsLayer, TracingContextLayer};`)


## Solution

I'm proposing a different api where the user has the opportunity to take
the existing `subscriber` and apply any transformations on it.

---

## Changelog

- Added a `update_subscriber` option on the `LogPlugin` that lets the
user modify the `subscriber` (for example to extend it with more tracing
`Layers`


## Migration Guide

> This section is optional. If there are no breaking changes, you can
delete this section.

- Added a new field `update_subscriber` in the `LogPlugin`

---------

Co-authored-by: Charles Bournhonesque <cbournhonesque@snapchat.com>
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 simple 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