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

Update to track upstream changes #3

Merged
merged 7 commits into from
Feb 8, 2019
Merged

Update to track upstream changes #3

merged 7 commits into from
Feb 8, 2019

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Feb 7, 2019

tokio-rs/tokio@9dc0129 merged a fairly large change to the upstream tokio-trace in order to support more efficient batching of field values. This changed several of the tokio-trace and tokio-trace-core APIs that the crates in this repository depend on. This PR updates all the tokio-trace-nursery crates to compile against the latest upstream tokio-trace. Note that the tokio-trace-subscriber crate will still require a fairly significant rewrite to actually be functional, but that API has needed to be reworked for a while.

hawkw added 5 commits February 7, 2019 11:22
It still needs some work to actually *work* correctly, but the whole
subscriber crate needs to be rewritten. Now it won't break the build
anymore.

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 hawkw self-assigned this Feb 7, 2019
@hawkw hawkw requested a review from davidbarsky February 7, 2019 21:21
The upstream change that made `Dispatch` no longer implement `Subscriber`
broke this a bit, since a `Dispatch` can no longer be passed to
`with_subscriber`. To make this also work with unwrapped subscribers,
we'll probably want an impl of `From<Subscriber>` for `Dispatch`.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw merged commit b7c66b3 into master Feb 8, 2019
@hawkw hawkw deleted the eliza/update branch February 8, 2019 17:11
@hawkw hawkw restored the eliza/update branch February 8, 2019 17:11
@hawkw hawkw deleted the eliza/update branch June 18, 2019 23:57
hawkw added a commit that referenced this pull request Jan 28, 2022
## Motivation

In my library I define a `macro_rules! concat` macro, i.e.
[`callbag::concat`](https://docs.rs/callbag/latest/callbag/macro.concat.html).

When I try to call `tracing::info!(...)`, I get error output such as
this:

<details>
<summary>error output</summary>

<!-- leave a blank line above -->
```
> RUSTFLAGS='-Z macro-backtrace' cargo +nightly clippy --features trace
    Checking callbag v0.14.0 (/home/teohhanhui/projects/teohhanhui/callbag-rs)
error[E0308]: mismatched types
  --> src/concat.rs:89:9
   |
89 |         info!("from sink: {message:?}");
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `&str`, found `u32`

error[E0277]: the trait bound `std::sync::Arc<core::Callbag<never::Never, _>>: std::convert::From<&str>` is not satisfied
    --> src/concat.rs:58:9
     |
56   | / macro_rules! concat {
57   | |     ($($s:expr),* $(,)?) => {
58   | |         $crate::concat(::std::vec![$($s),*].into_boxed_slice())
     | |         ^^^^^^^^^^^^^^ the trait `std::convert::From<&str>` is not implemented for `std::sync::Arc<core::Callbag<never::Never, _>>`
59   | |     };
60   | | }
     | |_- in this expansion of `concat!` (#5)
...
89   |           info!("from sink: {message:?}");
     |           ------------------------------- in this macro invocation (#1)
     |
    ::: src/utils/tracing.rs:47:1
     |
47   | / macro_rules! info {
48   | |     ($($arg:tt)+) => {
49   | |         ::cfg_if::cfg_if! {
50   | |             if #[cfg(feature = "trace")] {
51   | |                 ::tracing::info!($($arg)+)
     | |                 -------------------------- in this macro invocation (#2)
...    |
54   | |     };
55   | | }
     | |_- in this expansion of `info!` (#1)
     |
    ::: /home/teohhanhui/.cargo/registry/src/gh.neting.cc-1ecc6299db9ec823/tracing-0.1.29/src/macros.rs:586:1
     |
586  |   macro_rules! event {
     |  _-
     | |_|
     | |
587  | |     (target: $target:expr, parent: $parent:expr, $lvl:expr, { $($fields:tt)* } )=> (
588  | |         $crate::__tracing_log!(
589  | |             target: $target,
...    |
644  |                   name: concat!(
     |  _______________________-
645  |                       "event ",
646  |                       file!(),
647  |                       ":",
648  |                       line!()
649  | |                 ),
     | |_________________- in this macro invocation (#5)
...
667  | /         $crate::event!(
668  |               target: $target,
669  |               $lvl,
670  |               { message = format_args!($($arg)+), $($fields)* }
671  | |         )
     | |_________- in this macro invocation (#4)
...
791  | |     );
792  | | }
     | | -
     | |_|
     | |_in this expansion of `$crate::event!` (#3)
     |   in this expansion of `$crate::event!` (#4)
...
1229 | / macro_rules! info {
1230 |        (target: $target:expr, parent: $parent:expr, { $($field:tt)* }, $($arg:tt)* ) => (
1231 |           $crate::event!(target: $target, parent: $parent, $crate::Level::INFO, { $($field)* }, $($arg)*)
1232 |       );
...
1398 | /         $crate::event!(
1399 | |             target: module_path!(),
1400 | |             $crate::Level::INFO,
1401 | |             {},
1402 | |             $($arg)+
1403 | |         )
     | |_________- in this macro invocation (#3)
1404 |       );
1405 | | }
     | |_- in this expansion of `::tracing::info!` (#2)
     |
     = help: the following implementations were found:
               <std::sync::Arc<B> as std::convert::From<std::borrow::Cow<'a, B>>>
               <std::sync::Arc<T> as std::convert::From<T>>
               <std::sync::Arc<T> as std::convert::From<std::boxed::Box<T>>>
               <std::sync::Arc<[T]> as std::convert::From<&[T]>>
             and 9 others
     = note: required because of the requirements on the impl of `std::convert::Into<std::sync::Arc<core::Callbag<never::Never, _>>>` for `&str`
note: required by a bound in `concat::concat`
    --> src/concat.rs:81:8
     |
72   | pub fn concat<
     |        ------ required by a bound in this
...
81   |     S: Into<Arc<Source<T>>> + Send + Sync,
     |        ^^^^^^^^^^^^^^^^^^^^ required by this bound in `concat::concat`

error[E0308]: mismatched types
    --> src/concat.rs:58:9
     |
56   | / macro_rules! concat {
57   | |     ($($s:expr),* $(,)?) => {
58   | |         $crate::concat(::std::vec![$($s),*].into_boxed_slice())
     | |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `&str`, found struct `core::Callbag`
59   | |     };
60   | | }
     | |_- in this expansion of `concat!` (#5)
...
89   |           info!("from sink: {message:?}");
     |           ------------------------------- in this macro invocation (#1)
     |
    ::: src/utils/tracing.rs:47:1
     |
47   | / macro_rules! info {
48   | |     ($($arg:tt)+) => {
49   | |         ::cfg_if::cfg_if! {
50   | |             if #[cfg(feature = "trace")] {
51   | |                 ::tracing::info!($($arg)+)
     | |                 -------------------------- in this macro invocation (#2)
...    |
54   | |     };
55   | | }
     | |_- in this expansion of `info!` (#1)
     |
    ::: /home/teohhanhui/.cargo/registry/src/gh.neting.cc-1ecc6299db9ec823/tracing-0.1.29/src/macros.rs:586:1
     |
586  |   macro_rules! event {
     |  _-
     | |_|
     | |
587  | |     (target: $target:expr, parent: $parent:expr, $lvl:expr, { $($fields:tt)* } )=> (
588  | |         $crate::__tracing_log!(
589  | |             target: $target,
...    |
644  |                   name: concat!(
     |  _______________________-
645  |                       "event ",
646  |                       file!(),
647  |                       ":",
648  |                       line!()
649  | |                 ),
     | |_________________- in this macro invocation (#5)
...
667  | /         $crate::event!(
668  |               target: $target,
669  |               $lvl,
670  |               { message = format_args!($($arg)+), $($fields)* }
671  | |         )
     | |_________- in this macro invocation (#4)
...
791  | |     );
792  | | }
     | | -
     | |_|
     | |_in this expansion of `$crate::event!` (#3)
     |   in this expansion of `$crate::event!` (#4)
...
1229 | / macro_rules! info {
1230 |        (target: $target:expr, parent: $parent:expr, { $($field:tt)* }, $($arg:tt)* ) => (
1231 |           $crate::event!(target: $target, parent: $parent, $crate::Level::INFO, { $($field)* }, $($arg)*)
1232 |       );
...
1398 | /         $crate::event!(
1399 | |             target: module_path!(),
1400 | |             $crate::Level::INFO,
1401 | |             {},
1402 | |             $($arg)+
1403 | |         )
     | |_________- in this macro invocation (#3)
1404 |       );
1405 | | }
     | |_- in this expansion of `::tracing::info!` (#2)
     |
     = note: expected reference `&'static str`
                   found struct `core::Callbag<never::Never, _>`

Some errors have detailed explanations: E0277, E0308.
For more information about an error, try `rustc --explain E0277`.
error: could not compile `callbag` due to 3 previous errors
```
</details>

This is because of my `concat` macro being in scope.

## Solution

This branch adds a re-export of `core::concat!` in the
`__macro_support` module, and changes all the `tracing` macros to use
that, rather than using an un-namespaced `concat!`. The re-export
ensures that everything still works even in a crate that redefines the
`core` name to something else.

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
hawkw pushed a commit that referenced this pull request Feb 9, 2022
Re-application of changes made in #1842 which were lost in f1cf1f1

Integration tests added for regression.

---

## Motivation

In my library I define a `macro_rules! concat` macro, i.e.
[`callbag::concat`](https://docs.rs/callbag/latest/callbag/macro.concat.html).

When I try to call `tracing::info!(...)`, I get error output such as
this:

<details>
<summary>error output</summary>

<!-- leave a blank line above -->
```
> RUSTFLAGS='-Z macro-backtrace' cargo +nightly clippy --features trace
    Checking callbag v0.14.0 (/home/teohhanhui/projects/teohhanhui/callbag-rs)
error[E0308]: mismatched types
  --> src/concat.rs:89:9
   |
89 |         info!("from sink: {message:?}");
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `&str`, found `u32`

error[E0277]: the trait bound `std::sync::Arc<core::Callbag<never::Never, _>>: std::convert::From<&str>` is not satisfied
    --> src/concat.rs:58:9
     |
56   | / macro_rules! concat {
57   | |     ($($s:expr),* $(,)?) => {
58   | |         $crate::concat(::std::vec![$($s),*].into_boxed_slice())
     | |         ^^^^^^^^^^^^^^ the trait `std::convert::From<&str>` is not implemented for `std::sync::Arc<core::Callbag<never::Never, _>>`
59   | |     };
60   | | }
     | |_- in this expansion of `concat!` (#5)
...
89   |           info!("from sink: {message:?}");
     |           ------------------------------- in this macro invocation (#1)
     |
    ::: src/utils/tracing.rs:47:1
     |
47   | / macro_rules! info {
48   | |     ($($arg:tt)+) => {
49   | |         ::cfg_if::cfg_if! {
50   | |             if #[cfg(feature = "trace")] {
51   | |                 ::tracing::info!($($arg)+)
     | |                 -------------------------- in this macro invocation (#2)
...    |
54   | |     };
55   | | }
     | |_- in this expansion of `info!` (#1)
     |
    ::: /home/teohhanhui/.cargo/registry/src/gh.neting.cc-1ecc6299db9ec823/tracing-0.1.29/src/macros.rs:586:1
     |
586  |   macro_rules! event {
     |  _-
     | |_|
     | |
587  | |     (target: $target:expr, parent: $parent:expr, $lvl:expr, { $($fields:tt)* } )=> (
588  | |         $crate::__tracing_log!(
589  | |             target: $target,
...    |
644  |                   name: concat!(
     |  _______________________-
645  |                       "event ",
646  |                       file!(),
647  |                       ":",
648  |                       line!()
649  | |                 ),
     | |_________________- in this macro invocation (#5)
...
667  | /         $crate::event!(
668  |               target: $target,
669  |               $lvl,
670  |               { message = format_args!($($arg)+), $($fields)* }
671  | |         )
     | |_________- in this macro invocation (#4)
...
791  | |     );
792  | | }
     | | -
     | |_|
     | |_in this expansion of `$crate::event!` (#3)
     |   in this expansion of `$crate::event!` (#4)
...
1229 | / macro_rules! info {
1230 |        (target: $target:expr, parent: $parent:expr, { $($field:tt)* }, $($arg:tt)* ) => (
1231 |           $crate::event!(target: $target, parent: $parent, $crate::Level::INFO, { $($field)* }, $($arg)*)
1232 |       );
...
1398 | /         $crate::event!(
1399 | |             target: module_path!(),
1400 | |             $crate::Level::INFO,
1401 | |             {},
1402 | |             $($arg)+
1403 | |         )
     | |_________- in this macro invocation (#3)
1404 |       );
1405 | | }
     | |_- in this expansion of `::tracing::info!` (#2)
     |
     = help: the following implementations were found:
               <std::sync::Arc<B> as std::convert::From<std::borrow::Cow<'a, B>>>
               <std::sync::Arc<T> as std::convert::From<T>>
               <std::sync::Arc<T> as std::convert::From<std::boxed::Box<T>>>
               <std::sync::Arc<[T]> as std::convert::From<&[T]>>
             and 9 others
     = note: required because of the requirements on the impl of `std::convert::Into<std::sync::Arc<core::Callbag<never::Never, _>>>` for `&str`
note: required by a bound in `concat::concat`
    --> src/concat.rs:81:8
     |
72   | pub fn concat<
     |        ------ required by a bound in this
...
81   |     S: Into<Arc<Source<T>>> + Send + Sync,
     |        ^^^^^^^^^^^^^^^^^^^^ required by this bound in `concat::concat`

error[E0308]: mismatched types
    --> src/concat.rs:58:9
     |
56   | / macro_rules! concat {
57   | |     ($($s:expr),* $(,)?) => {
58   | |         $crate::concat(::std::vec![$($s),*].into_boxed_slice())
     | |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `&str`, found struct `core::Callbag`
59   | |     };
60   | | }
     | |_- in this expansion of `concat!` (#5)
...
89   |           info!("from sink: {message:?}");
     |           ------------------------------- in this macro invocation (#1)
     |
    ::: src/utils/tracing.rs:47:1
     |
47   | / macro_rules! info {
48   | |     ($($arg:tt)+) => {
49   | |         ::cfg_if::cfg_if! {
50   | |             if #[cfg(feature = "trace")] {
51   | |                 ::tracing::info!($($arg)+)
     | |                 -------------------------- in this macro invocation (#2)
...    |
54   | |     };
55   | | }
     | |_- in this expansion of `info!` (#1)
     |
    ::: /home/teohhanhui/.cargo/registry/src/gh.neting.cc-1ecc6299db9ec823/tracing-0.1.29/src/macros.rs:586:1
     |
586  |   macro_rules! event {
     |  _-
     | |_|
     | |
587  | |     (target: $target:expr, parent: $parent:expr, $lvl:expr, { $($fields:tt)* } )=> (
588  | |         $crate::__tracing_log!(
589  | |             target: $target,
...    |
644  |                   name: concat!(
     |  _______________________-
645  |                       "event ",
646  |                       file!(),
647  |                       ":",
648  |                       line!()
649  | |                 ),
     | |_________________- in this macro invocation (#5)
...
667  | /         $crate::event!(
668  |               target: $target,
669  |               $lvl,
670  |               { message = format_args!($($arg)+), $($fields)* }
671  | |         )
     | |_________- in this macro invocation (#4)
...
791  | |     );
792  | | }
     | | -
     | |_|
     | |_in this expansion of `$crate::event!` (#3)
     |   in this expansion of `$crate::event!` (#4)
...
1229 | / macro_rules! info {
1230 |        (target: $target:expr, parent: $parent:expr, { $($field:tt)* }, $($arg:tt)* ) => (
1231 |           $crate::event!(target: $target, parent: $parent, $crate::Level::INFO, { $($field)* }, $($arg)*)
1232 |       );
...
1398 | /         $crate::event!(
1399 | |             target: module_path!(),
1400 | |             $crate::Level::INFO,
1401 | |             {},
1402 | |             $($arg)+
1403 | |         )
     | |_________- in this macro invocation (#3)
1404 |       );
1405 | | }
     | |_- in this expansion of `::tracing::info!` (#2)
     |
     = note: expected reference `&'static str`
                   found struct `core::Callbag<never::Never, _>`

Some errors have detailed explanations: E0277, E0308.
For more information about an error, try `rustc --explain E0277`.
error: could not compile `callbag` due to 3 previous errors
```
</details>

This is because of my `concat` macro being in scope.

## Solution

Change all the `tracing` macros to use the re-export of `core::concat!`
in the `__macro_support` module, rather than using an un-namespaced
`concat!`. The re-export ensures that everything still works even in a
crate that redefines the `core` name to something else.
hawkw added a commit that referenced this pull request Mar 23, 2022
; This is the 1st commit message:

subscriber: implement per-subscriber filtering (#1523)

## Motivation

Currently, filtering with `Subscribe`s is always _global_. If a
`Subscribe` performs filtering, it will disable a span or event for
_all_ subscribers that compose the current subscriber. In some cases,
however, it is often desirable for individual subscribers to see
different spans or events than the rest of the `Subscribe` stack.

Issues in other projects, such as tokio-rs/console#64 and
tokio-rs/console#76, linkerd/linkerd2-proxy#601,
influxdata/influxdb_iox#1012 and influxdata/influxdb_iox#1681,
jackwaudby/spaghetti#86, etc; as well as `tracing` feature requests like
#302, #597, and #1348, all indicate that there is significant demand for
the ability to add filters to individual `Subscribe`s.

Unfortunately, doing this nicely is somewhat complicated. Although a
naive implementation that simply skips events/spans in
`Subscribe::on_event` and `Subscribe::new_span` based on some filter is
relatively simple, this wouldn't really be an ideal solution, for a
number of reasons. A proper per-subscriber filtering implementation
would satisfy the following _desiderata_:

* If a per-subscriber filter disables a span, it shouldn't be present
  _for the subscriber that filter is attached to_ when iterating over
  span contexts (such as `Context::event_scope`, `SpanRef::scope`, etc),
  or when looking up a span's parents.
* When _all_ per-subscriber filters disable a span or event, it should
  be completely disabled, rather than simply skipped by those particular
  subscribers. This means that per-subscriber filters should participate
  in `enabled`, as well as being able to skip spans and events in
  `new_span` and `on_event`.
* Per-subscriber filters shouldn't interfere with non-filtered
  `Subscribe`s. If a subscriber contains subscribers without any
  filters, as well as subscribers with per-subscriber filters, the
  non-filtered `Subscribe`s should behave exactly as they would without
  any per-subscriber filters present.
* Similarly, per-subscriber filters shouldn't interfere with global
  filtering. If a `Subscribe` in a stack is performing global filtering
  (e.g. the current filtering behavior), per-subscriber filters should
  also be effected by the global filter.
* Per-subscriber filters _should_ be able to participate in `Interest`
  caching, _but only when doing so doesn't interfere with
  non-per-subscriber-filtered subscribers_.
* Per-subscriber filters should be _tree-shaped_. If a `Subscriber`
  consists of multiple subscribers that have been `Subscribeed` together
  to form new `Subscribe`s, and some of the `Subscribeed` subscribers
  have per-subscriber filters, those per-subscriber filters should
  effect all subscribers in that subtree. Similarly, if `Subscribe`s in
  a per-subscriber filtered subtree have their _own_ per-subscriber
  filters, those subscribers should be effected by the union of their
  own filters and any per-subscriber filters that wrap them at higher
  levels in the tree.

Meeting all these requirements means that implementing per-subscriber
filtering correctly is somewhat more complex than simply skipping events
and spans in a `Subscribe`'s `on_event` and `new_span` callbacks.

## Solution

This branch has a basic working implementation of per-subscriber
filtering for `tracing-subscriber` v0.2. It should be possible to add
this in a point release without a breaking change --- in particular, the
new per-subscriber filtering feature _doesn't_ interfere with the global
filtering behavior that subscribers currently have when not using the
new per-subscriber filtering APIs, so existing configurations should all
behave exactly as they do now.

### Implementation

The new per-subscriber filtering API consists of a `Filter` trait that
defines a filtering strategy and a `Filtered` type that combines a
`Filter` with a `Subscribe`. When `enabled` is called on a `Filtered`
subscriber, it checks the metadata against the `Filter` and stores
whether that filter enabled the span/event in a thread-local cell. If
every per-subscriber filter disables a span/event, and there are no
non-per-subscriber-filtered subscribers in use, the `Registry`'s
`enabled` will return `false`. Otherwise, when the span/event is
recorded, each `Filtered` subscriber's `on_event` and `new_span` method
checks with the `Registry` to see whether _it_ enabled or disabled that
span/event, and skips passing it to the wrapped subscriber if it was
disabled. When a span is recorded, the `Registry` which filters disabled
it, so that they can skip it when looking up spans in other callbacks.

 A new `on_subscriber` method was added to the `Subscribe` trait, which
 allows running a callback when adding a `Subscribe` to a `Subscriber`.
 This allows mutating both the `Subscribe` and the `Subscriber`, since
 they are both passed by value when subscribering a `Subscribe` onto a
 `Subscriber`, and a new `register_filter` method was added to
 `LookupSpan`, which returns a `FilterId` type. When a `Filtered`
 subscriber is added to a subscriber that implements `LookupSpan`, it
 calls `register_filter` in its `on_subscriber` hook to generate a
 unique ID for its filter. `FilterId` can be used to look up whether a
 span or event was enabled by the `Filtered` subscriber.

Internally, the filter results are stored in a simple bitset to reduce
the performance overhead of adding per-subscriber filtering as much as
possible. The `FilterId` type is actually a bitmask that's used to set
the bit corresponding to a `Filtered` subscriber in that bitmap when it
disables a span or event, and check whether the bit is set when querying
if a span or event was disabled. Setting a bit in the bitset and testing
whether its set is extremely efficient --- it's just a couple of bitwise
ops. Additionally, the "filter map" that tracks which filters disabled a
span or event is just a single `u64` --- we don't need to allocate a
costly `HashSet` or similar every time we want to filter an event. This
*does* mean that we are limited to 64 unique filters per
`Registry`...however, I would be _very_ surprised if anyone _ever_
builds a subscriber with 64 subscribers, let alone 64 separate
per-subscriber filters.

Additionally, the `Context` and `SpanRef` types now also potentially
carry `FilterId`s, so that they can filter span lookups and span
traversals for `Filtered` subscribers. When `Filtered` subscribers are
nested using the `Subscribeed`, the `Context` _combines_ the filter ID
of the inner and outer subscribers so that spans can be skipped if they
were disabled by either.

Finally, an implementation of the `Filter` trait was added for
`LevelFilter`, and new `FilterFn` and `DynFilterFn` structs were added
to produce a `Filter` implementation from a function pointer or closure.

### Notes

Some other miscellaneous factors to consider when reviewing this change
include:

* I did a bit of unrelated refactoring in the `subscriber` module. Since
  we were adding more code to the `Subscribe` module (the `Filter`
  trait), the `subscriber.rs` file got significantly longer and was
  becoming somewhat hard to navigate. Therefore, I split out the
  `Context` and `Subscribeed` types into their own files. Most of the
  code in those files is the same as it was before, although some of it
  was changed in order to support per-subscriber filtering. Apologies in
  advance to reviewers for this...
* In order to test this change, I added some new test-support code in
  `tracing-subscriber`. The `tracing` test support code provides a
  `Subscriber` implementation that can be configured to expect a
  particular set of spans and events, and assert that they occurred as
  expected. In order to test per-subscriber filtering, I added a new
  `Subscribe` implementation that does the same thing, but implements
  `Subscribe` rather than `Subscriber`. This allows testing
  per-subscriber filter behavior in the same way we test global filter
  behavior. Reviewers should feel free to just skim the test the test
  support changes, or skip reviewing them entirely.

## Future Work

There is a bunch of additional stuff we can do once this change lands.
In order to limit the size of this PR, I didn't make these changes yet,
but once this merges, we will want to consider some important follow up
changes:

* [ ] Currently, _only_ `tracing-subscriber`'s `Registry` is capable of
      serving as a root subscriber for per-subscriber filters. This is
      in order to avoid stabilizing a lot of the per-subscriber
      filtering design as public API without taking the time to ensure
      there are no serious issues. In the future, we will want to add
      new APIs to allow users to implement their own root `Subscriber`s
      that can host subscribers with per-subscriber filtering.
* [ ] The `EnvFilter` type doesn't implement the `Filter` trait and thus
      cannot currently be used as a per-subscriber filter. We should add
      an implementation of `Filter` for `EnvFilter`.
* [ ] The new `FilterFn` and `DynFilterFn` types could conceivably
      _also_ be used as global filters. We could consider adding
      `Subscribe` implementations to allow them to be used for global
      filtering.
* [ ] We could consider adding new `Filter` implementations for
      performing common filtering predicates. For example, "only enable
      spans/events with a particular target or a set of targets", "only
      enable spans", "only enable events", etc. Then, we could add a
      combinator API to combine these predicates to build new filters.
* [ ] It would be nice if the `Interest` system could be used to cache
      the result of multiple individual per-subscriber filter
      evaluations, so that we don't need to always use `enabled` when
      there are several per-subscriber filters that disagree on whether
      or not they want a given span/event. There are a lot of unused
      bits in `Interest` that could potentially be used for this
      purpose. However, this would require new API changes in
      `tracing-core`, and is therefore out of scope for this PR.

Closes #302
Closes #597
Closes #1348

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

; The commit message #2 will be skipped:

; fixup: start making changes for filter

; The commit message #3 will be skipped:

; fixup: move stuff to `subscribe` folder from `layer

; The commit message #4 will be skipped:

; fixup: additional compilation errors
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
Re-application of changes made in tokio-rs#1842 which were lost in f1cf1f1

Integration tests added for regression.

---

## Motivation

In my library I define a `macro_rules! concat` macro, i.e.
[`callbag::concat`](https://docs.rs/callbag/latest/callbag/macro.concat.html).

When I try to call `tracing::info!(...)`, I get error output such as
this:

<details>
<summary>error output</summary>

<!-- leave a blank line above -->
```
> RUSTFLAGS='-Z macro-backtrace' cargo +nightly clippy --features trace
    Checking callbag v0.14.0 (/home/teohhanhui/projects/teohhanhui/callbag-rs)
error[E0308]: mismatched types
  --> src/concat.rs:89:9
   |
89 |         info!("from sink: {message:?}");
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `&str`, found `u32`

error[E0277]: the trait bound `std::sync::Arc<core::Callbag<never::Never, _>>: std::convert::From<&str>` is not satisfied
    --> src/concat.rs:58:9
     |
56   | / macro_rules! concat {
57   | |     ($($s:expr),* $(,)?) => {
58   | |         $crate::concat(::std::vec![$($s),*].into_boxed_slice())
     | |         ^^^^^^^^^^^^^^ the trait `std::convert::From<&str>` is not implemented for `std::sync::Arc<core::Callbag<never::Never, _>>`
59   | |     };
60   | | }
     | |_- in this expansion of `concat!` (tokio-rs#5)
...
89   |           info!("from sink: {message:?}");
     |           ------------------------------- in this macro invocation (tokio-rs#1)
     |
    ::: src/utils/tracing.rs:47:1
     |
47   | / macro_rules! info {
48   | |     ($($arg:tt)+) => {
49   | |         ::cfg_if::cfg_if! {
50   | |             if #[cfg(feature = "trace")] {
51   | |                 ::tracing::info!($($arg)+)
     | |                 -------------------------- in this macro invocation (tokio-rs#2)
...    |
54   | |     };
55   | | }
     | |_- in this expansion of `info!` (tokio-rs#1)
     |
    ::: /home/teohhanhui/.cargo/registry/src/gh.neting.cc-1ecc6299db9ec823/tracing-0.1.29/src/macros.rs:586:1
     |
586  |   macro_rules! event {
     |  _-
     | |_|
     | |
587  | |     (target: $target:expr, parent: $parent:expr, $lvl:expr, { $($fields:tt)* } )=> (
588  | |         $crate::__tracing_log!(
589  | |             target: $target,
...    |
644  |                   name: concat!(
     |  _______________________-
645  |                       "event ",
646  |                       file!(),
647  |                       ":",
648  |                       line!()
649  | |                 ),
     | |_________________- in this macro invocation (tokio-rs#5)
...
667  | /         $crate::event!(
668  |               target: $target,
669  |               $lvl,
670  |               { message = format_args!($($arg)+), $($fields)* }
671  | |         )
     | |_________- in this macro invocation (tokio-rs#4)
...
791  | |     );
792  | | }
     | | -
     | |_|
     | |_in this expansion of `$crate::event!` (tokio-rs#3)
     |   in this expansion of `$crate::event!` (tokio-rs#4)
...
1229 | / macro_rules! info {
1230 |        (target: $target:expr, parent: $parent:expr, { $($field:tt)* }, $($arg:tt)* ) => (
1231 |           $crate::event!(target: $target, parent: $parent, $crate::Level::INFO, { $($field)* }, $($arg)*)
1232 |       );
...
1398 | /         $crate::event!(
1399 | |             target: module_path!(),
1400 | |             $crate::Level::INFO,
1401 | |             {},
1402 | |             $($arg)+
1403 | |         )
     | |_________- in this macro invocation (tokio-rs#3)
1404 |       );
1405 | | }
     | |_- in this expansion of `::tracing::info!` (tokio-rs#2)
     |
     = help: the following implementations were found:
               <std::sync::Arc<B> as std::convert::From<std::borrow::Cow<'a, B>>>
               <std::sync::Arc<T> as std::convert::From<T>>
               <std::sync::Arc<T> as std::convert::From<std::boxed::Box<T>>>
               <std::sync::Arc<[T]> as std::convert::From<&[T]>>
             and 9 others
     = note: required because of the requirements on the impl of `std::convert::Into<std::sync::Arc<core::Callbag<never::Never, _>>>` for `&str`
note: required by a bound in `concat::concat`
    --> src/concat.rs:81:8
     |
72   | pub fn concat<
     |        ------ required by a bound in this
...
81   |     S: Into<Arc<Source<T>>> + Send + Sync,
     |        ^^^^^^^^^^^^^^^^^^^^ required by this bound in `concat::concat`

error[E0308]: mismatched types
    --> src/concat.rs:58:9
     |
56   | / macro_rules! concat {
57   | |     ($($s:expr),* $(,)?) => {
58   | |         $crate::concat(::std::vec![$($s),*].into_boxed_slice())
     | |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `&str`, found struct `core::Callbag`
59   | |     };
60   | | }
     | |_- in this expansion of `concat!` (tokio-rs#5)
...
89   |           info!("from sink: {message:?}");
     |           ------------------------------- in this macro invocation (tokio-rs#1)
     |
    ::: src/utils/tracing.rs:47:1
     |
47   | / macro_rules! info {
48   | |     ($($arg:tt)+) => {
49   | |         ::cfg_if::cfg_if! {
50   | |             if #[cfg(feature = "trace")] {
51   | |                 ::tracing::info!($($arg)+)
     | |                 -------------------------- in this macro invocation (tokio-rs#2)
...    |
54   | |     };
55   | | }
     | |_- in this expansion of `info!` (tokio-rs#1)
     |
    ::: /home/teohhanhui/.cargo/registry/src/gh.neting.cc-1ecc6299db9ec823/tracing-0.1.29/src/macros.rs:586:1
     |
586  |   macro_rules! event {
     |  _-
     | |_|
     | |
587  | |     (target: $target:expr, parent: $parent:expr, $lvl:expr, { $($fields:tt)* } )=> (
588  | |         $crate::__tracing_log!(
589  | |             target: $target,
...    |
644  |                   name: concat!(
     |  _______________________-
645  |                       "event ",
646  |                       file!(),
647  |                       ":",
648  |                       line!()
649  | |                 ),
     | |_________________- in this macro invocation (tokio-rs#5)
...
667  | /         $crate::event!(
668  |               target: $target,
669  |               $lvl,
670  |               { message = format_args!($($arg)+), $($fields)* }
671  | |         )
     | |_________- in this macro invocation (tokio-rs#4)
...
791  | |     );
792  | | }
     | | -
     | |_|
     | |_in this expansion of `$crate::event!` (tokio-rs#3)
     |   in this expansion of `$crate::event!` (tokio-rs#4)
...
1229 | / macro_rules! info {
1230 |        (target: $target:expr, parent: $parent:expr, { $($field:tt)* }, $($arg:tt)* ) => (
1231 |           $crate::event!(target: $target, parent: $parent, $crate::Level::INFO, { $($field)* }, $($arg)*)
1232 |       );
...
1398 | /         $crate::event!(
1399 | |             target: module_path!(),
1400 | |             $crate::Level::INFO,
1401 | |             {},
1402 | |             $($arg)+
1403 | |         )
     | |_________- in this macro invocation (tokio-rs#3)
1404 |       );
1405 | | }
     | |_- in this expansion of `::tracing::info!` (tokio-rs#2)
     |
     = note: expected reference `&'static str`
                   found struct `core::Callbag<never::Never, _>`

Some errors have detailed explanations: E0277, E0308.
For more information about an error, try `rustc --explain E0277`.
error: could not compile `callbag` due to 3 previous errors
```
</details>

This is because of my `concat` macro being in scope.

## Solution

Change all the `tracing` macros to use the re-export of `core::concat!`
in the `__macro_support` module, rather than using an un-namespaced
`concat!`. The re-export ensures that everything still works even in a
crate that redefines the `core` name to something else.
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.

1 participant