log: forward LogTracer::enabled to the collector
#1254
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
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
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
added a commit
to comit-network/comit-rs
that referenced
this pull request
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
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.
PRs #1247, #1248, and #1251 improve
tracing's behavior for thelogcrate's
log_enabled!macro when a max-level hint is set. However, thisdoesn'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 returntruewhen usingLogTracer,because it doesn't currently call
Collect::enabledon the currentcollector in its
Log::enabledmethod. Instead,Collect::enabledisonly 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 erroneouslyreturn
true. This also means that skipping disabledlogrecords insimilar cases will construct the
log::Recordstruct even when it isn'tnecessary to do so.
This PR improves this behavior by adding a call to
Collect::enabledinLogTracer'sLog::enabledmethod. I've also added to the existingtests for filtering
logrecords to ensure that we also handle thelog_enabled!macro correctly.While I was here, I noticed that the
Log::logmethod forLogTraceris somewhat inefficient --- it gets the current dispatcher three
times, and checks
enabledtwice. Currently, we check if the eventwould be enabled, and then call the
format_tracefunction, which alsochecks 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_defaultblock, and removing the duplicatecheck.