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

tracing: check log max level prior to dispatch check #1175

Merged
merged 4 commits into from
Jan 28, 2021

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Jan 6, 2021

Currently, when the log feature is enabled but the log-always
feature is not, we check the (dynamic, but inexpensive)
dispatch::has_been_set variable before checking if the event's
level would be enabled by log's static max level features.

Although this check is not terribly costly in terms of performance, it
does result in code being generated even when the log crate disables
some levels statically. This means that we will always generate some
code when the log feature is disabled. This isn't ideal --- the static
max level features should result in us generating no code whatsoever
for disabled trace points.

This commit moves the static max level check outside of the dispatch
check. Now, we should generate 0 bytes of assembly when both log and
tracing disable an event with their static filtering features.

I've also updated the docs to explain the relationship between tracing
and log's separate static filtering.

Fixes #1174

Currently, when the `log` feature is enabled but the `log-always`
feature is not, we check the (dynamic, but inexpensive)
`dispatch::has_been_set` variable **before** checking if the event's
level would be enabled by `log`'s static max level features.

Although this check is not terribly costly in terms of performance, it
*does* result in code being generated even when the `log` crate disables
some levels statically. This means that we will always generate *some*
code when the `log` feature is disabled. This isn't ideal --- the static
max level features should result in us generating *no* code whatsoever
for disabled trace points.

This commit moves the static max level check outside of the dispatch
check. Now, we should generate 0 bytes of assembly when both `log` and
`tracing` disable an event with their static filtering features.
@hawkw hawkw requested a review from a team as a code owner January 6, 2021 22:29
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw force-pushed the eliza/fix-static-log-lfilters branch from 6b44d9c to 991cdfc Compare January 7, 2021 18:50
@hawkw hawkw requested a review from davidbarsky as a code owner January 28, 2021 00:25
@hawkw hawkw merged commit b79704e into master Jan 28, 2021
hawkw added a commit that referenced this pull request Jan 28, 2021
Currently, when the `log` feature is enabled but the `log-always`
feature is not, we check the (dynamic, but inexpensive)
`dispatch::has_been_set` variable **before** checking if the event's
level would be enabled by `log`'s static max level features.

Although this check is not terribly costly in terms of performance, it
*does* result in code being generated even when the `log` crate disables
some levels statically. This means that we will always generate *some*
code when the `log` feature is disabled. This isn't ideal --- the static
max level features should result in us generating *no* code whatsoever
for disabled trace points.

This commit moves the static max level check outside of the dispatch
check. Now, we should generate 0 bytes of assembly when both `log` and
`tracing` disable an event with their static filtering features.

I've also updated the docs to explain the relationship between `tracing`
and `log`'s separate static filtering.

Fixes #1174

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this pull request Jan 28, 2021
Currently, when the `log` feature is enabled but the `log-always`
feature is not, we check the (dynamic, but inexpensive)
`dispatch::has_been_set` variable **before** checking if the event's
level would be enabled by `log`'s static max level features.

Although this check is not terribly costly in terms of performance, it
*does* result in code being generated even when the `log` crate disables
some levels statically. This means that we will always generate *some*
code when the `log` feature is disabled. This isn't ideal --- the static
max level features should result in us generating *no* code whatsoever
for disabled trace points.

This commit moves the static max level check outside of the dispatch
check. Now, we should generate 0 bytes of assembly when both `log` and
`tracing` disable an event with their static filtering features.

I've also updated the docs to explain the relationship between `tracing`
and `log`'s separate static filtering.

Fixes #1174

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
$if_log
($lvl:expr, $if_log:block else $else_block:block) => {
if $crate::level_to_log!($lvl) <= $crate::log::STATIC_MAX_LEVEL {
if !$crate::dispatcher::has_been_set() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This fails, similar to #1191:

error[E0433]: failed to resolve: maybe a missing crate `dispatcher`?
    --> /Users/ivan/projects/tracing/tracing/src/macros.rs:2352:25
     |
2352 |               if !$crate::dispatcher::has_been_set() {
     |                           ^^^^^^^^^^ maybe a missing crate `dispatcher`?
     |
    ::: /Users/ivan/projects/tracing/tracing/src/span.rs:765:9
     |
765  | /         if_log_enabled! { crate::Level::TRACE, {
766  | |             if let Some(ref meta) = self.meta {
767  | |                 self.log(ACTIVITY_LOG_TARGET, log::Level::Trace, format_args!("-> {}", meta.name()));
768  | |             }
769  | |         }}
     | |__________- in this macro invocation

Copy link
Contributor

Choose a reason for hiding this comment

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

I opened #1217 to address this.

hawkw added a commit that referenced this pull request Feb 4, 2021
Fixed

- **attributes**: Compiler error when using `#[instrument(err)]` on
  functions with mutable parameters ([#1167])
- **attributes**: Missing function visibility modifier when using
  `#[instrument]` with `async-trait` ([#977])
- **attributes** Removed unused `syn` features ([#928])
- **log**: Fixed an issue where the `tracing` macros would generate code
  for events whose levels are disabled statically by the `log` crate's
  `static_max_level_XXX` features ([#1175])
- Fixed deprecations and clippy lints ([#1195])
- Several documentation fixes and improvements ([#941], [#965], [#981],
  [#1146], [#1215])

Changed

- **attributes**: `tracing-futures` dependency is no longer required
  when using `#[instrument]` on async functions ([#808])
- **attributes**: Updated `tracing-attributes` minimum dependency to
  v0.1.12 ([#1222])

Thanks to @nagisa, @Txuritan, @TaKO8Ki, @okready, and @krojew for
contributing to this release!
hawkw added a commit that referenced this pull request Feb 4, 2021
### Fixed

- **attributes**: Compiler error when using `#[instrument(err)]` on
  functions with mutable parameters ([#1167])
- **attributes**: Missing function visibility modifier when using
  `#[instrument]` with `async-trait` ([#977])
- **attributes** Removed unused `syn` features ([#928])
- **log**: Fixed an issue where the `tracing` macros would generate code
  for events whose levels are disabled statically by the `log` crate's
  `static_max_level_XXX` features ([#1175])
- Fixed deprecations and clippy lints ([#1195])
- Several documentation fixes and improvements ([#941], [#965], [#981],
  [#1146], [#1215])

### Changed

- **attributes**: `tracing-futures` dependency is no longer required
  when using `#[instrument]` on async functions ([#808])
- **attributes**: Updated `tracing-attributes` minimum dependency to
  v0.1.12 ([#1222])

Thanks to @nagisa, @Txuritan, @TaKO8Ki, @okready, and @krojew for
contributing to this release!
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
### Fixed

- **attributes**: Compiler error when using `#[instrument(err)]` on
  functions with mutable parameters ([tokio-rs#1167])
- **attributes**: Missing function visibility modifier when using
  `#[instrument]` with `async-trait` ([tokio-rs#977])
- **attributes** Removed unused `syn` features ([tokio-rs#928])
- **log**: Fixed an issue where the `tracing` macros would generate code
  for events whose levels are disabled statically by the `log` crate's
  `static_max_level_XXX` features ([tokio-rs#1175])
- Fixed deprecations and clippy lints ([tokio-rs#1195])
- Several documentation fixes and improvements ([tokio-rs#941], [tokio-rs#965], [tokio-rs#981],
  [tokio-rs#1146], [tokio-rs#1215])

### Changed

- **attributes**: `tracing-futures` dependency is no longer required
  when using `#[instrument]` on async functions ([tokio-rs#808])
- **attributes**: Updated `tracing-attributes` minimum dependency to
  v0.1.12 ([tokio-rs#1222])

Thanks to @nagisa, @Txuritan, @TaKO8Ki, @okready, and @krojew for
contributing to this release!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

debug! statements still codegen despite release_max_level_off
2 participants