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 5 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
91 changes: 91 additions & 0 deletions tracing/src/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -783,6 +783,97 @@ macro_rules! event {
);
}

/// Check's whether an equivalent `event!` invocation would result in an enabled
/// event.
///
/// If you are using this macro to guard a log line that requires expensive computation, it can
/// result in false-negative's, if the default collector has filters based on line numbers or field
/// names.
///
/// This macro operates similarly to [`event!`], with some extensions:
/// - Allows passing just a level.
/// - Allows passing just a level and a target.
///
/// See [the top-level documentation][lib] for details on the syntax accepted by
/// this macro.
///
/// [lib]: crate#using-the-macros
/// [`event!`]: event!
///
/// # Examples
///
/// ```rust
/// use tracing::{enabled, Level};
///
/// # fn main() {
/// # if enabled!(Level::DEBUG, "Debug loggin") {
/// # // Do something expensive
/// # }
/// # }
Copy link
Member

Choose a reason for hiding this comment

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

let's make sure we have some examples showing all of the following:

  • level only
  • level + target
  • level + field names
  • level + field names + target

also, it looks like the current example shows a "debug loggin" message, which shouldn't be necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will wait on guswynn#1

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, $($k:ident).+) => (
$crate::enabled!(target: $target, $lvl, $($k).+,)
);
(target: $target:expr, $lvl:expr, $($k:ident).+, $($field:tt)*) => (
$crate::enabled!(
target: $target,
$lvl,
{ $($k).+, $($field)*}
)
);

($lvl:expr, $($k:ident).+, $($field:tt)*) => (
$crate::enabled!(
target: module_path!(),
$lvl,
{ $($k).+, $($field)*}
)
);
($lvl:expr, $($k:ident).+) => (
$crate::enabled!($lvl, $($k).+,)
);
guswynn marked this conversation as resolved.
Show resolved Hide resolved

// 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();
}
15 changes: 14 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,19 @@ 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, messaged,);
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