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

remove the detailed_trace macro #16450

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

Conversation

mockersf
Copy link
Member

Objective

Solution

  • Remove the detailed_trace macro, and replace its use by the trace macro
  • The macro doesn't work: it doesn't check for the detailed_trace feature
  • If it checked for the feature, as the macro is expanded where it's used, the feature should be added to all crates
  • Log levels are already controllable by features

@mockersf mockersf added this to the 0.15 milestone Nov 20, 2024
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

I agree with removing this in the current broken form, especially since it's now breaking CI.

@alice-i-cecile alice-i-cecile added X-Contentious There are nontrivial implications that should be thought through D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward A-Build-System Related to build systems or continuous integration A-Diagnostics Logging, crash handling, error reporting and performance analysis A-Utils Utility functions and types P-Critical This must be fixed immediately or contributors or users will be severely impacted labels Nov 20, 2024
@alice-i-cecile
Copy link
Member

@mockersf you're missing some imports, as CI helpfully points out: https://github.com/bevyengine/bevy/actions/runs/11940939339/job/33284703598?pr=16450#step:6:749

@rparrett
Copy link
Contributor

Log levels are already controllable by features

Just linking those here for visibility: https://docs.rs/tracing/latest/tracing/level_filters/index.html#compile-time-filters

Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

This does mean people need to remember to disable tracing (via compile time flags) or they will have significant performance degradation (even in release mode). I'm pretty worried that we'll be tanking performance by default now / a high percentage of games will ship with high-traffic / high cost trace logs. That being said, I do like the simplicity of just using normal log levels / removing this plumbing .

@cart
Copy link
Member

cart commented Nov 20, 2024

Actually hold on. Can we just fix the macro for 0.15? Maybe that would be better than rushing this in. It would be good to have a full cycle to evaluate the performance implications of this.

@cart cart mentioned this pull request Nov 20, 2024
@mockersf
Copy link
Member Author

In that case, I would prefer to remove those logs completely.

They were not possible to enable through the feature, so no one should miss them

@cart
Copy link
Member

cart commented Nov 20, 2024

I think we should discuss that as the permanent solution (ex: evaluate each log for value / do performance cost-benefit / decide the correct long term path for this category of thing). Imo we should minimally fix this for 0.15 without rocking the boat. It feels like removing logs indiscriminately is rocking the boat (even if they don't currently have users).

github-merge-queue bot pushed a commit that referenced this pull request Nov 20, 2024
Alternative to #16450 

# Objective

detailed_trace! in its current form does not work  (and breaks CI)

## Solution

Fix detailed_trace by checking for the feature properly, adding it to
the correct crates, and removing it from the incorrect crates
@cart cart removed this from the 0.15 milestone Nov 22, 2024
mockersf pushed a commit that referenced this pull request Nov 22, 2024
Alternative to #16450 

# Objective

detailed_trace! in its current form does not work  (and breaks CI)

## Solution

Fix detailed_trace by checking for the feature properly, adding it to
the correct crates, and removing it from the incorrect crates
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Build-System Related to build systems or continuous integration A-Diagnostics Logging, crash handling, error reporting and performance analysis A-Utils Utility functions and types D-Straightforward Simple bug fixes and API improvements, docs, test and examples P-Critical This must be fixed immediately or contributors or users will be severely impacted S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants