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

Adds bevy_tracing crate #13556

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

mnmaita
Copy link
Member

@mnmaita mnmaita commented May 28, 2024

Objective

Solution

  • Added a new crate called bevy_tracing with all tracing utilities that were present in bevy_utils.

TODO:

We should decide if we:

a) decide to make this a breaking change and remove these utilities from bevy_utils in a follow-up or
b) deprecate the utilities in bevy_utils and migrate crate by crate to reduce diff footprint in subsequent PRs. We would need to remove the utilities from bevy_utils at some point though.

We also need to reserve the crate name in crates.io.


Migration Guide

  • TBD

@alice-i-cecile alice-i-cecile added the A-Diagnostics Logging, crash handling, error reporting and performance analysis label May 28, 2024
@alice-i-cecile
Copy link
Member

alice-i-cecile commented May 28, 2024

My preference is for b here. I do like doing this though.

@alice-i-cecile alice-i-cecile requested a review from cart May 28, 2024 14:07
@alice-i-cecile alice-i-cecile added the C-Code-Quality A section of code that is hard to understand or change label May 28, 2024
@mockersf
Copy link
Member

I think this will contribute to the crate inflation and the feeling of "way too many dependencies" without any actual benefits

@mockersf mockersf added the X-Controversial There is active debate or serious implications around merging this PR label May 28, 2024
@BD103
Copy link
Member

BD103 commented May 28, 2024

Can this just be moved to bevy_log instead? (Maybe behind a feature gate, or in a utilities module?) I feel like both crates overlap too much for them to be made separate.

Furthermore, the benefits provided by info, error, once, detailed_trace, and more are limited in my opinion. I don't feel like I would use them if it involved pulling in an extra crate.

@hymm
Copy link
Contributor

hymm commented May 28, 2024

I think the problem with bevy_log is that it'll create a circular dependency. bevy_ecs needs the tracing macros, but bevy_log needs bevy_app which uses bevy_ecs. We could make bevy_ecs depend directly on the tracing crate again if we want to go in this direction.

@mockersf
Copy link
Member

mockersf commented May 28, 2024

this seems to have three different things:

@mnmaita
Copy link
Member Author

mnmaita commented May 29, 2024

this seems to have three different things:

I'm writing an issue in the tracing repo right now. Regarding the other points, I'm 100% in removing code that is badly designed and/or redundant. I can create a separate PR removing these and documenting the breaking changes. Wondering if we're ok with the potential overhead that removing detailed_trace would reintroduce as per the linked PR.

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-Code-Quality A section of code that is hard to understand or change S-Needs-SME Decision or review from an SME is required 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.

5 participants