subscriber: set the max log LevelFilter in init
#1248
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This branch adds a check against the current value of the max global
`LevelFilter` hint in `tracing_log`'s `LogTracer::enabled` method. This
should prevent `log::${LEVEL}_enabled!` from always returning `true`
when `tracing` is in use, fixing a performance issue when consuming
`log` records from tracing (see bytecodealliance/wasmtime#2662).
This does, however, mean `LogTracer::enabled` will always be called if
the max `log` level is not set. We can't determine whether we can set
`log`'s max level in `tracing_log`, since we don't know if subscribers
will change (resulting in the current max `tracing` level filter
changing), or if the current subscriber is set as the global default.
Higher-level code in `tracing_subscriber` can do this, though, in
`init()`, because it _does_ know that it's setting a global default.
I'll add that in a follow-up PR.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Since `tracing-subscriber`'s `init` and `try_init` functions set the global default collector, we can use the collector's max-level hint as the max level for the log crate, as well. This should significantly improve performance for skipping `log` records that fall below the collector's max level, as they will not have to call the `LogTracer::enabled` method. In order to implement this, I also changed the `FmtCollector`'s `try_init` to just use `util::SubscriberInitExt`'s `try_init` function, so that the same code isn't duplicated in multiple places. Signed-off-by: Eliza Weisman <eliza@buoyant.io>
davidbarsky
approved these changes
Feb 18, 2021
hawkw
added a commit
that referenced
this pull request
Feb 18, 2021
Depends on #1247. Since `tracing-subscriber`'s `init` and `try_init` functions set the global default subscriber, we can use the subscriber's max-level hint as the max level for the log crate, as well. This should significantly improve performance for skipping `log` records that fall below the collector's max level, as they will not have to call the `LogTracer::enabled` method. This will prevent issues like bytecodealliance/wasmtime#2662 from occurring in the future. See also #1249. In order to implement this, I also changed the `FmtSubscriber`'s `try_init` to just use `util::SubscriberInitExt`'s `try_init` function, so that the same code isn't duplicated in multiple places. I also added `AsLog` and `AsTrace` conversions for `LevelFilter`s in the `tracing-log` crate. Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw
added a commit
that referenced
this pull request
Feb 19, 2021
Depends on #1247. Since `tracing-subscriber`'s `init` and `try_init` functions set the global default subscriber, we can use the subscriber's max-level hint as the max level for the log crate, as well. This should significantly improve performance for skipping `log` records that fall below the collector's max level, as they will not have to call the `LogTracer::enabled` method. This will prevent issues like bytecodealliance/wasmtime#2662 from occurring in the future. See also #1249. In order to implement this, I also changed the `FmtSubscriber`'s `try_init` to just use `util::SubscriberInitExt`'s `try_init` function, so that the same code isn't duplicated in multiple places. I also added `AsLog` and `AsTrace` conversions for `LevelFilter`s in the `tracing-log` crate. Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw
added a commit
that referenced
this pull request
Feb 19, 2021
PRs #1247, #1248, and #1251 improve `tracing`'s behavior for the `log` crate's `log_enabled!` macro when a max-level hint is set. However, this *doesn't* get us correct behavior when a particular target is not enabled but the level is permitted by the max-level filter. In this case, `log_enabled!` will still return `true` when using `LogTracer`, because it doesn't currently call `Collect::enabled` on the current collector in its `Log::enabled` method. Instead, `Collect::enabled` is only checked when actually recording an event. This means that when a target is disabled by a target-specific filter but it's below the max level, `log::log_enabled!` will erroneously return `true`. This also means that skipping disabled `log` records in similar cases will construct the `log::Record` struct even when it isn't necessary to do so. This PR improves this behavior by adding a call to `Collect::enabled` in `LogTracer`'s `Log::enabled` method. I've also added to the existing tests for filtering `log` records to ensure that we also handle the `log_enabled!` macro correctly. While I was here, I noticed that the `Log::log` method for `LogTracer` is somewhat inefficient --- it gets the current dispatcher *three* times, and checks `enabled` twice. Currently, we check if the event would be enabled, and then call the`format_trace` function, which *also* checks if the event is enabled, and then dispatches it if it is. This is not great. :/ I fixed this by moving the check-and-dispatch inside of a single `dispatcher::get_default` block, and removing the duplicate check. Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw
added a commit
that referenced
this pull request
Feb 19, 2021
PRs #1247, #1248, and #1251 improve `tracing`'s behavior for the `log` crate's `log_enabled!` macro when a max-level hint is set. However, this *doesn't* get us correct behavior when a particular target is not enabled but the level is permitted by the max-level filter. In this case, `log_enabled!` will still return `true` when using `LogTracer`, because it doesn't currently call `Subscriber::enabled` on the current subscriber in its `Log::enabled` method. Instead, `Subscriber::enabled` is only checked when actually recording an event. This means that when a target is disabled by a target-specific filter but it's below the max level, `log::log_enabled!` will erroneously return `true`. This also means that skipping disabled `log` records in similar cases will construct the `log::Record` struct even when it isn't necessary to do so. This PR improves this behavior by adding a call to `Subscriber::enabled` in `LogTracer`'s `Log::enabled` method. I've also added to the existing tests for filtering `log` records to ensure that we also handle the `log_enabled!` macro correctly. While I was here, I noticed that the `Log::log` method for `LogTracer` is somewhat inefficient --- it gets the current dispatcher *three* times, and checks `enabled` twice. Currently, we check if the event would be enabled, and then call the`format_trace` function, which *also* checks if the event is enabled, and then dispatches it if it is. This is not great. :/ I fixed this by moving the check-and-dispatch inside of a single `dispatcher::get_default` block, and removing the duplicate check. Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw
added a commit
that referenced
this pull request
Feb 19, 2021
PRs #1247, #1248, and #1251 improve `tracing`'s behavior for the `log` crate's `log_enabled!` macro when a max-level hint is set. However, this *doesn't* get us correct behavior when a particular target is not enabled but the level is permitted by the max-level filter. In this case, `log_enabled!` will still return `true` when using `LogTracer`, because it doesn't currently call `Subscriber::enabled` on the current subscriber in its `Log::enabled` method. Instead, `Subscriber::enabled` is only checked when actually recording an event. This means that when a target is disabled by a target-specific filter but it's below the max level, `log::log_enabled!` will erroneously return `true`. This also means that skipping disabled `log` records in similar cases will construct the `log::Record` struct even when it isn't necessary to do so. This PR improves this behavior by adding a call to `Subscriber::enabled` in `LogTracer`'s `Log::enabled` method. I've also added to the existing tests for filtering `log` records to ensure that we also handle the `log_enabled!` macro correctly. While I was here, I noticed that the `Log::log` method for `LogTracer` is somewhat inefficient --- it gets the current dispatcher *three* times, and checks `enabled` twice. Currently, we check if the event would be enabled, and then call the`format_trace` function, which *also* checks if the event is enabled, and then dispatches it if it is. This is not great. :/ I fixed this by moving the check-and-dispatch inside of a single `dispatcher::get_default` block, and removing the duplicate check. Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw
added a commit
that referenced
this pull request
Feb 19, 2021
PRs #1247, #1248, and #1251 improve `tracing`'s behavior for the `log` crate's `log_enabled!` macro when a max-level hint is set. However, this *doesn't* get us correct behavior when a particular target is not enabled but the level is permitted by the max-level filter. In this case, `log_enabled!` will still return `true` when using `LogTracer`, because it doesn't currently call `Subscriber::enabled` on the current subscriber in its `Log::enabled` method. Instead, `Subscriber::enabled` is only checked when actually recording an event. This means that when a target is disabled by a target-specific filter but it's below the max level, `log::log_enabled!` will erroneously return `true`. This also means that skipping disabled `log` records in similar cases will construct the `log::Record` struct even when it isn't necessary to do so. This PR improves this behavior by adding a call to `Subscriber::enabled` in `LogTracer`'s `Log::enabled` method. I've also added to the existing tests for filtering `log` records to ensure that we also handle the `log_enabled!` macro correctly. While I was here, I noticed that the `Log::log` method for `LogTracer` is somewhat inefficient --- it gets the current dispatcher *three* times, and checks `enabled` twice. Currently, we check if the event would be enabled, and then call the`format_trace` function, which *also* checks if the event is enabled, and then dispatches it if it is. This is not great. :/ I fixed this by moving the check-and-dispatch inside of a single `dispatcher::get_default` block, and removing the duplicate check. Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw
added a commit
that referenced
this pull request
Feb 19, 2021
Added - Re-export the `log` crate so that users can ensure consistent versions ([#602]) - `AsLog` implementation for `tracing::LevelFilter` ([#1248]) - `AsTrace` implementation for `log::LevelFilter` ([#1248]) Fixed - **log-tracer**: Fixed `Log::enabled` implementation for `LogTracer` not calling `Subscriber::enabled` ([#1254]) - **log-tracer**: Fixed `Log::enabled` implementation for `LogTracer` not checking the max level hint ([#1247]) - Several documentation fixes ([#483], [#485], [#537], [#595], [#941], [#981]) [#483]: https://github.com/tokio-rs/tracing/pulls/483 [#485]: https://github.com/tokio-rs/tracing/pulls/485 [#537]: https://github.com/tokio-rs/tracing/pulls/537 [#595]: https://github.com/tokio-rs/tracing/pulls/595 [#605]: https://github.com/tokio-rs/tracing/pulls/604 [#941]: https://github.com/tokio-rs/tracing/pulls/941 [#1247]: https://github.com/tokio-rs/tracing/pulls/1247 [#1248]: https://github.com/tokio-rs/tracing/pulls/1248 [#1254]: https://github.com/tokio-rs/tracing/pulls/1254 Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw
added a commit
to linkerd/linkerd2-proxy
that referenced
this pull request
Feb 22, 2021
This picks up tokio-rs/tracing#1247, tokio-rs/tracing#1248, tokio-rs/tracing#1251, and tokio-rs/tracing#1254. Collectively, these changes should result in a significant performance improvement when using `tracing-log`'s compatibility layer for converting legacy `log` records to `tracing` events. Since we have some dependencies which use the `log` crate, this should hopefully reduce the overhead of those dependencies' logging. I'm not sure how much `log` we have in the hot path by this point, but it's probably a good idea to pick up the change regardless. Signed-off-by: Eliza Weisman <eliza@buoyant.io>
olix0r
pushed a commit
to linkerd/linkerd2-proxy
that referenced
this pull request
Feb 22, 2021
This picks up tokio-rs/tracing#1247, tokio-rs/tracing#1248, tokio-rs/tracing#1251, and tokio-rs/tracing#1254. Collectively, these changes should result in a significant performance improvement when using `tracing-log`'s compatibility layer for converting legacy `log` records to `tracing` events. Since we have some dependencies which use the `log` crate, this should hopefully reduce the overhead of those dependencies' logging. I'm not sure how much `log` we have in the hot path by this point, but it's probably a good idea to pick up the change regardless. Signed-off-by: Eliza Weisman <eliza@buoyant.io>
This was referenced Mar 14, 2021
bors bot
referenced
this pull request
in comit-network/comit-rs
Mar 17, 2021
3542: Bump tracing from 0.1.22 to 0.1.25 r=mergify[bot] a=dependabot[bot] Bumps [tracing](https://github.com/tokio-rs/tracing) from 0.1.22 to 0.1.25. <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/tokio-rs/tracing/commit/4ad1e62a2dd9f3e97a06ead14285993a9df99ea5"><code>4ad1e62</code></a> tracing: prepare to release v0.1.25</li> <li><a href="https://github.com/tokio-rs/tracing/commit/c22b62e28121b66522beeff516f8a1528ec8ff36"><code>c22b62e</code></a> tracing: highlight <code>Span::entered</code> in more docs</li> <li><a href="https://github.com/tokio-rs/tracing/commit/a358728b1ef321751acead4580643b1150863c62"><code>a358728</code></a> tracing: simplify common case of immediately entering the span (<a href="https://github.com/tokio-rs/tracing/issues/1252">#1252</a>)</li> <li><a href="https://github.com/tokio-rs/tracing/commit/4538d74d228e4a6856c0176ade5f01fc26129b8e"><code>4538d74</code></a> subscriber: prepare to release v0.2.16 (<a href="https://github.com/tokio-rs/tracing/issues/1256">#1256</a>)</li> <li><a href="https://github.com/tokio-rs/tracing/commit/a0201ba798b86739ba633d15d297ec233b905e4e"><code>a0201ba</code></a> log: prepare to release v0.1.2</li> <li><a href="https://github.com/tokio-rs/tracing/commit/0cdd5e88ac3478ae91223e9c3b53a1c3598a7893"><code>0cdd5e8</code></a> log: forward <code>LogTracer::enabled</code> to the subscriber (<a href="https://github.com/tokio-rs/tracing/issues/1254">#1254</a>)</li> <li><a href="https://github.com/tokio-rs/tracing/commit/8d83326a5fddfdd68dc8865680f8663dad76a2b4"><code>8d83326</code></a> subscriber: fix FmtCollector not forwarding max level (<a href="https://github.com/tokio-rs/tracing/issues/1251">#1251</a>)</li> <li><a href="https://github.com/tokio-rs/tracing/commit/31aa6afecc1fb97b1f2d4eb15cefdd456c120142"><code>31aa6af</code></a> subscriber: set the max <code>log</code> <code>LevelFilter</code> in <code>init</code> (<a href="https://github.com/tokio-rs/tracing/issues/1248">#1248</a>)</li> <li><a href="https://github.com/tokio-rs/tracing/commit/2a9d17f73f8499b7e73cf6e8bee225190aac6990"><code>2a9d17f</code></a> log: compare <code>log</code> record <code>Level</code>s against the max level (<a href="https://github.com/tokio-rs/tracing/issues/1247">#1247</a>)</li> <li><a href="https://github.com/tokio-rs/tracing/commit/d173c2de9af913e2b4ebb8555e46d65b416ca730"><code>d173c2d</code></a> tracing: prepare to release v0.1.24 (<a href="https://github.com/tokio-rs/tracing/issues/1244">#1244</a>)</li> <li>Additional commits viewable in <a href="https://github.com/tokio-rs/tracing/compare/tracing-0.1.22...tracing-0.1.25">compare view</a></li> </ul> </details> <br /> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually </details> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
kaffarell
pushed a commit
to kaffarell/tracing
that referenced
this pull request
May 22, 2024
Depends on tokio-rs#1247. Since `tracing-subscriber`'s `init` and `try_init` functions set the global default subscriber, we can use the subscriber's max-level hint as the max level for the log crate, as well. This should significantly improve performance for skipping `log` records that fall below the collector's max level, as they will not have to call the `LogTracer::enabled` method. This will prevent issues like bytecodealliance/wasmtime#2662 from occurring in the future. See also tokio-rs#1249. In order to implement this, I also changed the `FmtSubscriber`'s `try_init` to just use `util::SubscriberInitExt`'s `try_init` function, so that the same code isn't duplicated in multiple places. I also added `AsLog` and `AsTrace` conversions for `LevelFilter`s in the `tracing-log` crate. Signed-off-by: Eliza Weisman <eliza@buoyant.io>
kaffarell
pushed a commit
to kaffarell/tracing
that referenced
this pull request
May 22, 2024
Added - Re-export the `log` crate so that users can ensure consistent versions ([tokio-rs#602]) - `AsLog` implementation for `tracing::LevelFilter` ([tokio-rs#1248]) - `AsTrace` implementation for `log::LevelFilter` ([tokio-rs#1248]) Fixed - **log-tracer**: Fixed `Log::enabled` implementation for `LogTracer` not calling `Subscriber::enabled` ([tokio-rs#1254]) - **log-tracer**: Fixed `Log::enabled` implementation for `LogTracer` not checking the max level hint ([tokio-rs#1247]) - Several documentation fixes ([tokio-rs#483], [tokio-rs#485], [tokio-rs#537], [tokio-rs#595], [tokio-rs#941], [tokio-rs#981]) [tokio-rs#483]: https://github.com/tokio-rs/tracing/pulls/483 [tokio-rs#485]: https://github.com/tokio-rs/tracing/pulls/485 [tokio-rs#537]: https://github.com/tokio-rs/tracing/pulls/537 [tokio-rs#595]: https://github.com/tokio-rs/tracing/pulls/595 [tokio-rs#605]: https://github.com/tokio-rs/tracing/pulls/604 [tokio-rs#941]: https://github.com/tokio-rs/tracing/pulls/941 [tokio-rs#1247]: https://github.com/tokio-rs/tracing/pulls/1247 [tokio-rs#1248]: https://github.com/tokio-rs/tracing/pulls/1248 [tokio-rs#1254]: https://github.com/tokio-rs/tracing/pulls/1254 Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Depends on #1247.
Since
tracing-subscriber'sinitandtry_initfunctions set theglobal default collector, we can use the collector's max-level hint as
the max level for the log crate, as well. This should significantly
improve performance for skipping
logrecords that fall below thecollector's max level, as they will not have to call the
LogTracer::enabledmethod.This will prevent issues like bytecodealliance/wasmtime#2662
from occurring in the future. See also #1249.
In order to implement this, I also changed the
FmtCollector'stry_initto just useutil::SubscriberInitExt'stry_initfunction,so that the same code isn't duplicated in multiple places. I also
added
AsLogandAsTraceconversions forLevelFilters inthe
tracing-logcrate.Signed-off-by: Eliza Weisman eliza@buoyant.io