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

implement enabled! #1821

Merged
merged 38 commits into from
Jan 28, 2022
Merged
Show file tree
Hide file tree
Changes from 33 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
0b4c3ac
implement `enabled!`
guswynn Jan 7, 2022
12474e1
simplify the macro
guswynn Jan 11, 2022
5ccc947
remove new event methods
guswynn Jan 11, 2022
289bfed
fix Kind::HINT's doc's
guswynn Jan 11, 2022
04b7fcc
Merge branch 'master' into enabled
hawkw Jan 11, 2022
7d5831d
implement `enabled!`
guswynn Jan 7, 2022
b3d042d
simplify the macro
guswynn Jan 11, 2022
ac87e42
remove new event methods
guswynn Jan 11, 2022
9b09230
fix Kind::HINT's doc's
guswynn Jan 11, 2022
7f9aee9
simplify dot cases
guswynn Jan 13, 2022
dee8846
Merge branch 'master' into enabled
davidbarsky Jan 24, 2022
14d8137
journald: Set syslog identifier (#1822)
swsnr Jan 11, 2022
9a1942f
docs: don't state that `tracing-futures` is required (#1827)
hawkw Jan 11, 2022
147d793
simplify dot cases
guswynn Jan 13, 2022
48240a1
subscriber: fix leading comma with `Pretty` formatter (#1833)
hawkw Jan 13, 2022
133154b
subscriber: add `Format::with_file` and `with_line_number` (#1773)
renecouto Jan 14, 2022
27c3633
appender: bump MSRV to 1.53.0 (#1851)
hawkw Jan 21, 2022
d88df7c
subscriber: update thread_local to 1.1.4 (#1858)
matze Jan 24, 2022
a0c69eb
WIP docs
davidbarsky Jan 12, 2022
55ee5c2
add examples
davidbarsky Jan 12, 2022
dd563c3
wip, sorry
davidbarsky Jan 21, 2022
61dbf07
okay, it covers stuff now
davidbarsky Jan 24, 2022
ad24e27
whoops, typo.
davidbarsky Jan 24, 2022
0b26585
rebase, i think?
davidbarsky Jan 24, 2022
18ae036
fix phrasing fuckups
davidbarsky Jan 24, 2022
b7f9ada
Merge branch 'enabled' into david/edit-documentation-for-enabled
davidbarsky Jan 24, 2022
8e68af0
implement `enabled!`
guswynn Jan 7, 2022
0bee395
simplify the macro
guswynn Jan 11, 2022
f68cc1f
remove new event methods
guswynn Jan 11, 2022
93657b7
fix Kind::HINT's doc's
guswynn Jan 11, 2022
405e61c
simplify dot cases
guswynn Jan 13, 2022
b620da7
Merge branch 'enabled' into david/edit-documentation-for-enabled
guswynn Jan 25, 2022
9395dc1
Merge pull request #1 from davidbarsky/david/edit-documentation-for-e…
guswynn Jan 25, 2022
50a2c87
Apply docs suggestions from code review
hawkw Jan 28, 2022
9cb95d5
fix wrong callsite kind for `enabled!`
hawkw Jan 28, 2022
f993288
Merge branch 'master' into enabled
hawkw Jan 28, 2022
fbe5f84
Update tracing/src/macros.rs
hawkw Jan 28, 2022
3cf628a
bleh rustfmt
hawkw Jan 28, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions tracing-core/src/metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,7 @@ impl<'a> fmt::Debug for Metadata<'a> {
enum KindInner {
Event,
Span,
Hint,
}

impl Kind {
Expand All @@ -382,6 +383,11 @@ impl Kind {
/// `Span` callsite
pub const SPAN: Kind = Kind(KindInner::Span);

/// `enabled!` callsite. [`Collect`][`crate::collect::Collect`]s can assume
/// this `Kind` means they will never recieve a
/// full event with this [`Metadata`].
pub const HINT: Kind = Kind(KindInner::Hint);
Copy link
Member

Choose a reason for hiding this comment

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

One thought I had: a Subscriber may have a filter that chooses to enable only spans with a particular metadata, or only events with particular metadata. Having a HINT callsite limits specificity with the enabled! macro, as we don't get a way to ask "would you enable a span in this module with the fields foo and bar?" or "would you enable an event with those fields?"

I wonder if we want to change the internal representation of callsite kinds to permit a callsite to be a hint and a span or an event. We could do that by changing the hint state to be a separate flag, like:

pub struct Kind {
    // This is an `Option` so a hint generated by `enabled!` invocations that
    // don't specifically specify span or event aren't considered one or the 
    // other.
    inner: Option<KindInner>,
    is_hint: bool,
}

impl Kind {
    pub fn is_span(&self) -> bool {
         // If the inner kind is `None`, this is an unspecific hint callsite, so
         // treat the callsite as both a span _and_ an event, in case the 
         // subscriber would enable either
         matches!(self.inner, Some(KindInner::Span) | None)
    }

    pub fn is_event(&self) -> bool {
         matches!(self.inner, Some(KindInner::Event) | None)
    }

    pub fn is_hint(&self) -> bool {
        self.is_hint
    }
}

Alternatively, if we wanted to avoid making the struct a bit larger, we could implement the same thing by changing the internal representation to bit flags:

pub struct Kind(u8);

impl Kind {
    const HINT_BIT: u8 = 1 << 3;
    const EVENT_BIT: u8 = 1 << 1;
    const SPAN_BIT: u8 = 1 << 1;

    pub fn is_span(&self) -> bool {
        self.0 & Self::SPAN_BIT != 0
    }

    pub fn is_event(&self) -> bool {
        self.0 & Self::EVENT_BIT != 0
    }

    pub fn is_hint(&self) -> bool {
        self.0 & Self::HINT_BIT != 0
    }
    // ...
}

This might be a bit less clear, though.

In either case, we can address this in a follow-up branch, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is quite interesting! I agree follow-up branch is better for that kind of change!


/// Return true if the callsite kind is `Span`
pub fn is_span(&self) -> bool {
matches!(self, Kind(KindInner::Span))
Expand All @@ -391,6 +397,11 @@ impl Kind {
pub fn is_event(&self) -> bool {
matches!(self, Kind(KindInner::Event))
}

/// Return true if the callsite kind is `Hint`
pub fn is_hint(&self) -> bool {
matches!(self, Kind(KindInner::Hint))
}
}

// ===== impl Level =====
Expand Down
117 changes: 117 additions & 0 deletions tracing/src/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -783,6 +783,123 @@ macro_rules! event {
);
}

/// Checks whether a span or event is enabled depending on the provided metadata.
hawkw marked this conversation as resolved.
Show resolved Hide resolved
///
/// This macro is a specialized tool: it is intended to be used prior
/// to an expensive computation required *just* for that event, but
/// *cannot* be done as part of an argument to that event, such as
/// when multiple events are emitted (e.g., iterating over a collection
/// and emitting an event for each item).
///
/// ## Usage
hawkw marked this conversation as resolved.
Show resolved Hide resolved
///
/// It is possible for `enabled!` to return a false positive or false negative. This might
/// occur when a subscriber is using a _more specific_ filter than what was (or could be)
/// provided to `enabled!`. Below are several examples where this might occur:
hawkw marked this conversation as resolved.
Show resolved Hide resolved
///
/// - If a subscriber is using a filter which may enable a span or event based
/// on field names, but `enabled!` is invoked without listing field names,
/// `enabled!` may return a false negative if a specific field name would
/// cause the subscriber to enable something that would otherwise be disabled.
/// - If a subscriber is using a filter which enables or disables specific events by
/// file path and line number, a particular event may be enabled/disabled
/// even if an `enabled!` invocation with the same level, target, and fields
/// indicated otherwise.
/// - The subscriber can choose to enable _only_ spans or _only_ events, which `enabled`
/// will not reflect.
hawkw marked this conversation as resolved.
Show resolved Hide resolved
///
/// ## Examples
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
/// ## Examples
/// # Examples

///
/// `enabled!()` requires a level argument, an optional `target:`
Copy link
Member

Choose a reason for hiding this comment

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

nit, take it or leave it: i might consider adding a link

Suggested change
/// `enabled!()` requires a level argument, an optional `target:`
/// `enabled!()` requires a [level](crate::Level) argument, an optional `target:`

/// argument, and an optional set of fields. If the fields are not provided,
Copy link
Member

Choose a reason for hiding this comment

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

nit: might be more explicit to say field names here, so that we don't incorrectly suggest you could pass field values

Suggested change
/// argument, and an optional set of fields. If the fields are not provided,
/// argument, and an optional set of field names. If the fields are not provided,

/// they are considered to be unknown. `enabled!` attempts to match the
/// syntax of `event!()` as closely as possible, which can be seen in the
/// examples below.
hawkw marked this conversation as resolved.
Show resolved Hide resolved
///
/// ```rust
/// use tracing::{enabled, Level};
///
/// # fn main() {
/// // If the underlying collector is interested in recording
/// // DEBUG-level spans and events, this will evaluate to true.
/// if enabled!(Level::DEBUG) {
/// // some expensive work...
/// }
///
/// // If the underlying collector is interested in recording spans and events
/// // with the target "my_crate" at the level DEBUG, this will evaluate to true.
/// if enabled!(target: "my_crate", Level::DEBUG) {
/// // some expensive work...
/// }
///
/// // If the underlying collector is interested in recording spans and events
/// // with the target "my_crate", at the level DEBUG, and the field name "hello",
/// // this will evaluate to true.
Copy link
Member

Choose a reason for hiding this comment

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

nit: i don't love "underlying" here. how about "current" or "default":

Suggested change
/// // If the underlying collector is interested in recording
/// // DEBUG-level spans and events, this will evaluate to true.
/// if enabled!(Level::DEBUG) {
/// // some expensive work...
/// }
///
/// // If the underlying collector is interested in recording spans and events
/// // with the target "my_crate" at the level DEBUG, this will evaluate to true.
/// if enabled!(target: "my_crate", Level::DEBUG) {
/// // some expensive work...
/// }
///
/// // If the underlying collector is interested in recording spans and events
/// // with the target "my_crate", at the level DEBUG, and the field name "hello",
/// // this will evaluate to true.
/// // If the current collector is interested in recording
/// // DEBUG-level spans and events, this will evaluate to true.
/// if enabled!(Level::DEBUG) {
/// // some expensive work...
/// }
///
/// // If the current collector is interested in recording spans and events
/// // with the target "my_crate" at the level DEBUG, this will evaluate to true.
/// if enabled!(target: "my_crate", Level::DEBUG) {
/// // some expensive work...
/// }
///
/// // If the current collector is interested in recording spans and events
/// // with the target "my_crate", at the level DEBUG, and the field name "hello",
/// // this will evaluate to true.

/// if enabled!(target: "my_crate", Level::DEBUG, hello) {
/// // some expensive work...
/// }
/// # }
hawkw marked this conversation as resolved.
Show resolved Hide resolved
/// ```
hawkw marked this conversation as resolved.
Show resolved Hide resolved
///
/// [`log::log_enabled`]: https://docs.rs/log/0.4.14/log/macro.log_enabled.html
/// [`Targets`]: https://docs.rs/tracing-subscriber/0.3.5/tracing_subscriber/filter/targets/struct.Targets.html
/// [`filter_fn`]: https://docs.rs/tracing-subscriber/0.3.5/tracing_subscriber/filter/fn.filter_fn.html
/// [`EnvFilter`]: https://docs.rs/tracing-subscriber/0.3.5/tracing_subscriber/struct.EnvFilter.html
hawkw marked this conversation as resolved.
Show resolved Hide resolved
#[macro_export]
macro_rules! enabled {
(target: $target:expr, $lvl:expr, { $($fields:tt)* } )=> ({
if $crate::level_enabled!($lvl) {
use $crate::__macro_support::Callsite as _;
static CALLSITE: $crate::__macro_support::MacroCallsite = $crate::callsite2! {
name: concat!(
"enabled ",
file!(),
":",
line!()
),
Comment on lines +869 to +874
Copy link
Member

Choose a reason for hiding this comment

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

hmm, i wonder if we should also add the option to pass a name: key or something, so that users can test if a span with a particular name would be enabled. we can do this in a follow-up branch though.

kind: $crate::metadata::Kind::EVENT,
hawkw marked this conversation as resolved.
Show resolved Hide resolved
target: $target,
level: $lvl,
fields: $($fields)*
};
let interest = CALLSITE.interest();
if !interest.is_never() && CALLSITE.is_enabled(interest) {
let meta = CALLSITE.metadata();
$crate::dispatch::get_default(|current| current.enabled(meta))
} else {
false
}
} else {
false
}
});
// Just target and level
(target: $target:expr, $lvl:expr ) => (
$crate::enabled!(target: $target, $lvl, { })
);

// These two cases handle fields with no values
(target: $target:expr, $lvl:expr, $($field:tt)*) => (
$crate::enabled!(
target: $target,
$lvl,
{ $($field)*}
)
);
($lvl:expr, $($field:tt)*) => (
$crate::enabled!(
target: module_path!(),
$lvl,
{ $($field)*}
)
);

// Simplest `enabled!` case
( $lvl:expr ) => (
$crate::enabled!(target: module_path!(), $lvl, { })
);
}

/// Constructs an event at the trace level.
///
/// This functions similarly to the [`event!`] macro. See [the top-level
Expand Down
30 changes: 30 additions & 0 deletions tracing/tests/enabled.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// liballoc is required because the test subscriber cannot be constructed
// statically
#![cfg(feature = "alloc")]

mod support;

use self::support::*;
use tracing::Level;

#[cfg_attr(target_arch = "wasm32", wasm_bindgen_test::wasm_bindgen_test)]
#[test]
fn level_and_target() {
let (collector, handle) = collector::mock()
.with_filter(|meta| {
if meta.target() == "debug_module" {
meta.level() <= &Level::DEBUG
} else {
meta.level() <= &Level::INFO
}
})
.done()
.run_with_handle();

tracing::collect::set_global_default(collector).unwrap();

assert!(tracing::enabled!(target: "debug_module", Level::DEBUG));
assert!(tracing::enabled!(Level::ERROR));
assert!(!tracing::enabled!(Level::DEBUG));
handle.assert_finished();
}
16 changes: 15 additions & 1 deletion tracing/tests/macros.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#![deny(warnings)]
use tracing::{
callsite, debug, debug_span, error, error_span, event, info, info_span, span, trace,
callsite, debug, debug_span, enabled, error, error_span, event, info, info_span, span, trace,
trace_span, warn, warn_span, Level,
};

Expand Down Expand Up @@ -334,6 +334,20 @@ fn event() {
event!(Level::DEBUG, foo);
}

#[cfg_attr(target_arch = "wasm32", wasm_bindgen_test::wasm_bindgen_test)]
#[test]
fn enabled() {
enabled!(Level::DEBUG, foo, bar.baz, quux,);
enabled!(Level::DEBUG, message);
enabled!(Level::INFO, foo, bar.baz, quux, message,);
enabled!(Level::INFO, foo, bar., message,);
enabled!(Level::DEBUG, foo);

enabled!(Level::DEBUG);
enabled!(target: "rando", Level::DEBUG);
enabled!(target: "rando", Level::DEBUG, field);
}

#[cfg_attr(target_arch = "wasm32", wasm_bindgen_test::wasm_bindgen_test)]
#[test]
fn locals_with_message() {
Expand Down