-
Notifications
You must be signed in to change notification settings - Fork 722
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-subscriber: impl LookupSpan
for Box<LS>
and Arc<LS>
#2247
Conversation
These implementations mirror those provided by tracing-core for `Collect` on `Box<C>` and `Arc<C>`.
@@ -1656,7 +1656,7 @@ feature! { | |||
} | |||
|
|||
|
|||
impl<C, S> Subscribe<C> for Vec<S> | |||
impl<C, S> Subscribe<C> for alloc::vec::Vec<S> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is unrelated, but included here because I noticed that
$ cargo build --no-default-features --features alloc
was failing without it.
@hawkw Would it also be possible to backport this and release in the near future? My only real two blockers for tokio-rs/console#363 are this PR and #2229 (which you've already backported, and is just pending release)! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me, thank you! i'll take care of the backports
These implementations mirror those provided by tracing-core for `Collect` on `Box<C>` and `Arc<C>`.
These implementations mirror those provided by tracing-core for `Collect` on `Box<C>` and `Arc<C>`.
These implementations mirror those provided by tracing-core for `Subscriber` on `Box<S>` and `Arc<S>`.
@hawkw Hm, unfortunately, I just discovered a shortcoming of this PR. Since I think the impl provided by this PR is probably better-than-nothing for v0.1.x, but for the master branch, it might be more sensible to provide the impl instead for |
Ah, yeah, I had forgotten about that --- I believe there was actually a similar implementation previously that was removed for the exact reason you're referring to. I think we probably don't want to publish an implementation of However, I will note that
isn't actually correct: Alternatively, your other suggestion about changing |
)" This reverts commit a0824d3 (PR #2247). As discussed in [this comment][1], the implementation for `Arc`s may cause subtly incorrect behavior if actually used, due to the `&mut self` receiver of the `LookupSpan::register_filter` method, since the `Arc` cannot be mutably borrowed if any clones of it exist. The APIs added in PRs #2269 and #2293 offer an alternative solution to the same problems this change was intended to solve, and --- since this change hasn't been published yet --- it can safely be reverted. [1]: https://giethub.com/tokio-rs/tracing/pull/2247#issuecomment-1199924876
…kio-rs#2247)" This reverts commit a0824d3 (PR tokio-rs#2247). As discussed in [this comment][1], the implementation for `Arc`s may cause subtly incorrect behavior if actually used, due to the `&mut self` receiver of the `LookupSpan::register_filter` method, since the `Arc` cannot be mutably borrowed if any clones of it exist. The APIs added in PRs tokio-rs#2269 and tokio-rs#2293 offer an alternative solution to the same problems this change was intended to solve, and --- since this change hasn't been published yet --- it can safely be reverted. [1]: https://giethub.com/tokio-rs/tracing/pull/2247#issuecomment-1199924876
These implementations mirror those provided by tracing-core for
Collect
onBox<C>
andArc<C>
.