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

Improve tracing layer customization #13159

Merged
merged 5 commits into from
May 12, 2024

Conversation

cBournhonesque
Copy link
Contributor

@cBournhonesque cBournhonesque commented May 1, 2024

Objective

The current tracing customization option (the update_subscriber field) was basically unusable because it provides a dyn Subscriber and most layers require a Subscriber that also implements for<'a> LookupSpan<'a, Data=Data<'a>>, so it was impossible to add a layer on top of the dyn Subscriber.

This PR provides an alternative way of adding additional tracing layers to the LogPlugin by instead creating an Option<Layer>.

This is enough for most situations because Option<Layer> and Vec<Layer> both implement Layer.

Solution

  • Replace the update_subscriber field of LogPlugin with a custom_layer field which is function pointer returning an Option<BoxedLayer>
  • Update the examples to showcase that this works:
    • with multiple additional layers
    • with Layers that were previously problematic, such as bevy::log::tracing_subscriber::fmt::layer().with_file(true) (mentioned in the issue)

Note that in the example this results in duplicate logs, since we have our own layer on top of the default fmt_layer added in the LogPlugin; maybe in the future we might want to provide a single one? Or to let the user customize the default fmt_layer ? I still think this change is an improvement upon the previous solution, which was basically broken.


Changelog

This section is optional. If this was a trivial fix, or has no externally-visible impact, you can delete this section.

  • The LogPlugin's update_subscriber field has been replaced with custom_layer to allow the user to flexibly add a tracing::Layer to the layer stack

Migration Guide

  • The LogPlugin's update_subscriber field has been replaced with custom_layer

@cBournhonesque cBournhonesque added A-Diagnostics Logging, crash handling, error reporting and performance analysis D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes D-Domain-Agnostic Can be tackled by anyone with generic programming or Rust skills and removed A-Utils Utility functions and types labels May 2, 2024
@cBournhonesque cBournhonesque requested review from BD103 and JMS55 May 5, 2024 03:46
cBournhonesque and others added 2 commits May 5, 2024 10:05
Co-authored-by: BD103 <59022059+BD103@users.noreply.github.com>
Copy link
Member

@BD103 BD103 left a comment

Choose a reason for hiding this comment

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

In general looks good! I'm a bit concerned about encouraging people to create Box<Vec<...>>, but since the function itself only gets called once it's probably fine.

@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 May 12, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 12, 2024
Merged via the queue into bevyengine:main with commit ded5d52 May 12, 2024
28 checks passed
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 D-Domain-Agnostic Can be tackled by anyone with generic programming or Rust skills D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes 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.

LogPlugin's update_subscriber is broken when adding extra FmtLayers
5 participants