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

Bevy_utils should not force tracing's release_max_level_info crate feature #4069

Closed
shanesveller opened this issue Feb 28, 2022 · 6 comments
Closed
Labels
A-Utils Utility functions and types C-Bug An unexpected or incorrect behavior C-Usability A targeted quality-of-life change that makes Bevy easier to use

Comments

@shanesveller
Copy link

Bevy version

0.6.1 and current main

Operating system & version

Applicable to all

What you did

Include a direct/transitive dependency on bevy_utils, which is a dependency of most bevy_* internal crates, some of which are a literal or de facto required dependency of the top-level bevy crate.

What you expected to happen

I should be able to include my preferred level of compile-time verbosity features on the tracing crate, including something more verbose than release_max_level_info or no limit at all.

What actually happened

I am forced to use the release_max_info crate feature on tracing which sets a limit on how much verbosity I can produce in release binaries, as tracing calls and attribute macros that are lower verbosity (debug or trace in this example) are fully compiled away prior to runtime.

Additional information

As a workaround, I would need to either fork-and-patch bevy_utils, or produce a Cargo profile that mimics release profile but will not receive the effects of this tracing crate flag, which currently requires re-enabling debug_assertions. If the linked line could be converted to an on-by-default crate feature of bevy_utils that was not also rigidly enforced anywhere else in the bevy crate tree, I think I could continue to use upstream code but get the verbosity I situationally desire.

Obviously this is not a common scenario and the performance overhead of each verbosity level is observable, but I think encouraging via documentation and defaults would be preferable to choosing inflexibly for your downstream users.

@shanesveller shanesveller added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Feb 28, 2022
@alice-i-cecile alice-i-cecile added A-Utils Utility functions and types C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled labels Feb 28, 2022
@alice-i-cecile
Copy link
Member

Seems like a sensible change. Want to make a PR?

@shanesveller
Copy link
Author

@alice-i-cecile Sure, I'll try to take a stab at the required code changes this weekend. Would you consider the documentation additions to be required in order to merge the change?

@mockersf
Copy link
Member

mockersf commented Mar 1, 2022

Bevy is quite opinionated on how it sets up logs, and its good that it adds that feature by default...

It would be better if it was possible to disable it though

@alice-i-cecile
Copy link
Member

Would you consider the documentation additions to be required in order to merge the change?

Generally yes, although I'll need to examine the PR in detail (I'm not super familiar with that area of the code).

@shanesveller
Copy link
Author

shanesveller commented Mar 1, 2022

Bevy is quite opinionated on how it sets up logs, and its good that it adds that feature by default...

It would be better if it was possible to disable it though

Totally in agreement there - as a default it's a fine stance to take, but the difficulty is that tracing is approaching the point of being a foundational ecosystem crate for Rust development in general. Folks who work with Rust outside of Bevy are perhaps more accustomed to working with it / configuring it directly, as I am.

@tamasfe
Copy link
Contributor

tamasfe commented Apr 7, 2022

I've also run into this, right now I'm using bevy_ecs in an application which also depends on bevy_utils.

Bevy is quite opinionated on how it sets up logs, and its good that it adds that feature by default...

It would be better if it was possible to disable it though

Being opinionated in design is one thing, this however just forces the user into an optimization step that affects a lot more than Bevy itself, cannot be opted-out from, and could be achieved by the user easily as well if desired.

The log crate with the same feature even warns about this:

Libraries should avoid using the max level features because they’re global and can’t be changed once they’re set.

In #1206 it was suggested that feature flags from tracing should be reexported and this one enabled by default, however I think
tracing has become such a common library now that chances are whoever works with Bevy had already worked with it or is using it, it would be more or less pointless.

I believe the best way forward for this would be simply removing the constraint and adding an entry about it in a performance/release-oriented section of the documentation.

Alternatively if this feature flag is really desired, it could be moved up from bevy_utils to the bevy crate itself so that only the main target audience of the project would be affected.

bors bot pushed a commit that referenced this issue Apr 25, 2022
# Objective

- Debug logs are useful in release builds, but `tracing` logs are hard-capped (`release_max_level_info`) at the `info` level by `bevy_utils`.

## Solution

- This PR simply removes the limit in `bevy_utils` with no further actions.
- If any out-of-the box performance regressions arise, the steps to enable this `tracing` feature should be documented in a user guide in the future.

This PR closes #4069 and closes #1206.

## Alternatives considered

- Instruct the user to build with `debug-assertions` enabled: this is just a workaround, as it obviously enables all `debug-assertions` that affect more than logging itself.
- Re-exporting the feature from `tracing` and enabling it by default: I believe it just adds complexity and confusion, the `tracing` feature can also be re-enabled with one line in userland.

---

## Changelog

### Fixed

- Log level is not hard capped at `info` for release builds anymore.

## Migration Guide

- Maximum log levels for release builds is not enforced by Bevy anymore, to omit "debug" and "trace" level logs entirely from release builds, `tracing` must be added as a dependency with its `release_max_level_info` feature enabled in `Cargo.toml`. (`tracing = { version = "0.1", features = ["release_max_level_info"] }`)
exjam pushed a commit to exjam/bevy that referenced this issue May 22, 2022
# Objective

- Debug logs are useful in release builds, but `tracing` logs are hard-capped (`release_max_level_info`) at the `info` level by `bevy_utils`.

## Solution

- This PR simply removes the limit in `bevy_utils` with no further actions.
- If any out-of-the box performance regressions arise, the steps to enable this `tracing` feature should be documented in a user guide in the future.

This PR closes bevyengine#4069 and closes bevyengine#1206.

## Alternatives considered

- Instruct the user to build with `debug-assertions` enabled: this is just a workaround, as it obviously enables all `debug-assertions` that affect more than logging itself.
- Re-exporting the feature from `tracing` and enabling it by default: I believe it just adds complexity and confusion, the `tracing` feature can also be re-enabled with one line in userland.

---

## Changelog

### Fixed

- Log level is not hard capped at `info` for release builds anymore.

## Migration Guide

- Maximum log levels for release builds is not enforced by Bevy anymore, to omit "debug" and "trace" level logs entirely from release builds, `tracing` must be added as a dependency with its `release_max_level_info` feature enabled in `Cargo.toml`. (`tracing = { version = "0.1", features = ["release_max_level_info"] }`)
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
# Objective

- Debug logs are useful in release builds, but `tracing` logs are hard-capped (`release_max_level_info`) at the `info` level by `bevy_utils`.

## Solution

- This PR simply removes the limit in `bevy_utils` with no further actions.
- If any out-of-the box performance regressions arise, the steps to enable this `tracing` feature should be documented in a user guide in the future.

This PR closes bevyengine#4069 and closes bevyengine#1206.

## Alternatives considered

- Instruct the user to build with `debug-assertions` enabled: this is just a workaround, as it obviously enables all `debug-assertions` that affect more than logging itself.
- Re-exporting the feature from `tracing` and enabling it by default: I believe it just adds complexity and confusion, the `tracing` feature can also be re-enabled with one line in userland.

---

## Changelog

### Fixed

- Log level is not hard capped at `info` for release builds anymore.

## Migration Guide

- Maximum log levels for release builds is not enforced by Bevy anymore, to omit "debug" and "trace" level logs entirely from release builds, `tracing` must be added as a dependency with its `release_max_level_info` feature enabled in `Cargo.toml`. (`tracing = { version = "0.1", features = ["release_max_level_info"] }`)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Utils Utility functions and types C-Bug An unexpected or incorrect behavior C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants