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

implement enabled! #1821

merged 38 commits into from
Jan 28, 2022

Conversation

guswynn
Copy link
Contributor

@guswynn guswynn commented Jan 7, 2022

Motivation

Closes: #1668

My usecase is different than the referenced "avoid doing something expensive to log": I want to guard turning on debug mode for an ffi'd library, based on some target that represents the "module" we care about.

Solution

The macro is very similar to event!, but adds a few simplistic cases, and generates ever so slightly different code (to return the correct value always. It also skips anything to do with tracing-log. I considered (and tried), to share the impl between event! and enabled!, but must confess I am not good at macros and got stuck. I think they are sufficiently different, where copied impls, is easier to read. We already manage

Also, my project is on the crates.io version, so this would need to be backported to 0.1, I can help with that with guidance.

@guswynn guswynn force-pushed the enabled branch 2 times, most recently from f82d63e to 3be5c13 Compare January 7, 2022 17:53
`enabled!` is similar to `event!`, but never dispatches an event. It
also allows a simplistic version of calling it, filling in a fake
message in the metadata.
@guswynn
Copy link
Contributor Author

guswynn commented Jan 7, 2022

failures for netlify seem unrelated?

@hawkw
Copy link
Member

hawkw commented Jan 10, 2022

failures for netlify seem unrelated?

yup, just fixed that --- I believe it was related to a new RustDoc version finding some broken links it couldn't previously detect.

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

I really don't think implementing the enabled! hint as a special kind of event is the best approach. The enabled! macro is not a type of event. Instead, it's asking whether a span or event with a particular set of Metadata would be enabled. Therefore, I think we should only construct the Metadata, and not construct an entire event that we just throw away to get its metadata, which seems inefficient. Also, it just feels kind of weird to hang the constructors used by enabled! off of the Event type --- Event shouldn't be responsible for this.

Instead, we probably want to use the metadata! or callsite! macro to just generate the metadata, and ask if it's enabled.

Comment on lines 82 to 98
/// Constructs a new `Event` with the specified metadata and set of values,
/// and returns whether such an event would be enabled by the current collector.
pub fn child_of_enabled(
parent: impl Into<Option<Id>>,
metadata: &'static Metadata<'static>,
fields: &'a field::ValueSet<'_>,
) -> bool {
let event = Self::new_child_of(parent, metadata, fields);
crate::dispatch::get_default(|current| current.enabled(event.metadata()))
}

/// Constructs a new `Event` with the specified metadata and set of values,
/// and returns whether such an event would be enabled by the current collector.
pub fn enabled(metadata: &'static Metadata<'static>, fields: &'a field::ValueSet<'_>) -> bool {
let event = Event::new(metadata, fields);
crate::dispatch::get_default(|current| current.enabled(event.metadata()))
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that the enabled macro should create a new Event variant; that seems incorrect to me. Subscriber::enabled Instead, I think it should just create a callsite and metadata, and call Dispatch::enabled with that metadata. I guess the intention was to ask the subscriber if it would enable an event with specific field values...but filtering isn't performed on field values anyway, so there's no point in having this take field values.

Similarly, I don't believe child_of_enabled should exist, as it does nothing. enabled is only called with Metadata, and Metadata doesn't include the parent span.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we just not support parent: in the enabled! macro?

Copy link
Member

Choose a reason for hiding this comment

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

yeah, the macro shouldn't accept a parent or field values.

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

/// `enabled!` callsite. [`Collect`][`crate::collect::Collect`]'s can assume
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// `enabled!` callsite. [`Collect`][`crate::collect::Collect`]'s can assume
/// `enabled!` callsite. [`Collect`][`crate::collect::Collect`]s can assume

$crate::Event::child_of_enabled(
$parent,
meta,
&$crate::valueset!(meta.fields(), $($fields)*)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this should take field values, only field names. as I discussed in my comment on the new Event constructors you added, the Collect::enabled method doesn't care about field values, as it's only called with Metadata, not a full event. We should not include field values in the macro.

///
#[macro_export]
macro_rules! enabled {
(target: $target:expr, parent: $parent:expr, $lvl:expr, { $($fields:tt)* } )=> (
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this should take a parent span. As I discussed in my comment on the new Event constructors you added, the Collect::enabled method doesn't care about overridden span parents. The macro should not include a parent span.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh I see here now, sounds good!

Comment on lines 849 to 856
(target: $target:expr, parent: $parent:expr, $lvl:expr, { $($fields:tt)* }, $($arg:tt)+ ) => (
$crate::enabled!(
target: $target,
parent: $parent,
$lvl,
{ message = format_args!($($arg)+), $($fields)* }
)
);
Copy link
Member

Choose a reason for hiding this comment

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

Hints shouldn't have a message. They could have a message field name, if the intent is to test if an event with a message would be enabled. But they should never actually generate a formatted message, as nothing will ever format it. This macro arm should be removed.

Comment on lines 860 to 862
(target: $target:expr, parent: $parent:expr, $lvl:expr, $($arg:tt)+) => (
$crate::enabled!(target: $target, parent: $parent, $lvl, { $($arg)+ })
);
Copy link
Member

Choose a reason for hiding this comment

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

this macro arm is for formatting a message from format_args!, but we shouldn't do that. this should be removed.

Comment on lines 893 to 905
(target: $target:expr, $lvl:expr, { $($fields:tt)* }, $($arg:tt)+ ) => (
$crate::enabled!(
target: $target,
$lvl,
{ message = format_args!($($arg)+), $($fields)* }
)
);
(target: $target:expr, $lvl:expr, $($k:ident).+ = $($fields:tt)* ) => (
$crate::enabled!(target: $target, $lvl, { $($k).+ = $($fields)* })
);
(target: $target:expr, $lvl:expr, $($arg:tt)+ ) => (
$crate::enabled!(target: $target, $lvl, { $($arg)+ })
);
Copy link
Member

Choose a reason for hiding this comment

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

these are for formatting a format_args! message, and for field values. i don't think we should have these.

Comment on lines 1016 to 867
// This cases is added on top of how `event` works
( $lvl:expr ) => (
$crate::enabled!(target: module_path!(), $lvl, { "hint_message" })
);
Copy link
Member

Choose a reason for hiding this comment

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

we shouldn't need this if we don't include a message value.

Comment on lines 906 to 982
// This cases is added on top of how `event` works
(target: $target:expr, $lvl:expr ) => (
$crate::enabled!(target: $target, $lvl, { "hint_message" })
);
(parent: $parent:expr, $lvl:expr, { $($fields:tt)* }, $($arg:tt)+ ) => (
$crate::enabled!(
target: module_path!(),
parent: $parent,
$lvl,
{ message = format_args!($($arg)+), $($fields)* }
)
);
(parent: $parent:expr, $lvl:expr, $($k:ident).+ = $($field:tt)*) => (
$crate::enabled!(
target: module_path!(),
parent: $parent,
$lvl,
{ $($k).+ = $($field)*}
)
);
(parent: $parent:expr, $lvl:expr, ?$($k:ident).+ = $($field:tt)*) => (
$crate::enabled!(
target: module_path!(),
parent: $parent,
$lvl,
{ ?$($k).+ = $($field)*}
)
);
(parent: $parent:expr, $lvl:expr, %$($k:ident).+ = $($field:tt)*) => (
$crate::enabled!(
target: module_path!(),
parent: $parent,
$lvl,
{ %$($k).+ = $($field)*}
)
);
(parent: $parent:expr, $lvl:expr, $($k:ident).+, $($field:tt)*) => (
$crate::enabled!(
target: module_path!(),
parent: $parent,
$lvl,
{ $($k).+, $($field)*}
)
);
(parent: $parent:expr, $lvl:expr, %$($k:ident).+, $($field:tt)*) => (
$crate::enabled!(
target: module_path!(),
parent: $parent,
$lvl,
{ %$($k).+, $($field)*}
)
);
(parent: $parent:expr, $lvl:expr, ?$($k:ident).+, $($field:tt)*) => (
$crate::enabled!(
target: module_path!(),
parent: $parent,
$lvl,
{ ?$($k).+, $($field)*}
)
);
(parent: $parent:expr, $lvl:expr, $($arg:tt)+ ) => (
$crate::enabled!(target: module_path!(), parent: $parent, $lvl, { $($arg)+ })
);
( $lvl:expr, { $($fields:tt)* }, $($arg:tt)+ ) => (
$crate::enabled!(
target: module_path!(),
$lvl,
{ message = format_args!($($arg)+), $($fields)* }
)
);
($lvl:expr, $($k:ident).+ = $($field:tt)*) => (
$crate::enabled!(
target: module_path!(),
$lvl,
{ $($k).+ = $($field)*}
)
);
Copy link
Member

Choose a reason for hiding this comment

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

these are basically all for matching field values and messages, we should remove them.

Comment on lines 990 to 1009
($lvl:expr, ?$($k:ident).+, $($field:tt)*) => (
$crate::enabled!(
target: module_path!(),
$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).+,)
);
($lvl:expr, %$($k:ident).+) => (
$crate::enabled!($lvl, %$($k).+,)
);
Copy link
Member

Choose a reason for hiding this comment

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

i don't think we would need the ? and % sigils, which control how values are recorded, if we never remove values.

@guswynn
Copy link
Contributor Author

guswynn commented Jan 11, 2022

@hawkw I think the new commit implements what you asked
I am not 100% on what the difference between $k and $fields are in the macro, and how fieldset! works, so I may be passing the wrong field name through to the metadata, so itll need close eye. as mentioned, macro_rules! is not my forte

@guswynn
Copy link
Contributor Author

guswynn commented Jan 11, 2022

I rebased on master and it still fails, weird

tracing/src/macros.rs Outdated Show resolved Hide resolved
Comment on lines 803 to 812
/// # 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

`enabled!` is similar to `event!`, but never dispatches an event. It
also allows a simplistic version of calling it, filling in a fake
message in the metadata.
/// `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!

@hawkw
Copy link
Member

hawkw commented Jan 19, 2022

@davidbarsky any updates on the docs changes?

davidbarsky and others added 3 commits January 24, 2022 18:17
Set the `SYSLOG_IDENTIFIER` field on all events.  I noticed that the
subscriber didn't do this so far.

## Motivation

The identifier is used with `journalctl -t`, and while it's normally
better to filter by unit with `journalctl -u` the identifier is still
nice for processes that are not started through systemd units.

Upstream does this as well, see [here]:

![grafik](https://user-images.githubusercontent.com/224922/148660479-9525b21e-547f-4787-9bb7-db933963041a.png)

`program_invocation_short_name` is a glibc variable which holds the file
name of the current executable; I tried to replicate this behaviour in
Rust.

## Solution

Add a syslog identifier field to the subscriber, defaulting to the
filename of the current executable, and write this value as
`SYSLOG_IDENTIFIER` with every event.  It's not written for spans, because
it's a global value and inevitably the same for each span.

[here]: https://github.com/systemd/systemd/blob/81218ac1e14b4b50b4337938bcf55cacc76f0728/src/libsystemd/sd-journal/journal-send.c#L270
Currently, the documentation states in a few places that the
`tracing-futures` crate is required for asynchronous code to use
`tracing`. This is no longer the case, as `tracing` provides the
`Instrument` combinator for futures; `tracing-futures` is only needed
for things defined in the `futures` crate, such as `Stream` and `Sink`.

This branch updates the documentation so that it doesn't incorrectly
state that `tracing-futures` is required.
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

i went ahead and applied my docs suggestions, so this is good IMO pending CI

@hawkw hawkw enabled auto-merge (squash) January 28, 2022 18:51
auto-merge was automatically disabled January 28, 2022 18:51

Pull Request is not mergeable

@guswynn
Copy link
Contributor Author

guswynn commented Jan 28, 2022

@hawkw thanks for shepherding this through!

@guswynn
Copy link
Contributor Author

guswynn commented Jan 28, 2022

now I probably need to look into how to backport it to 0.1

@hawkw
Copy link
Member

hawkw commented Jan 28, 2022

@hawkw thanks for shepherding this through!

my pleasure, thanks for working on this!

now I probably need to look into how to backport it to 0.1

i'll take care about that, don't worry about it! :)

if you're interested in continuing to work on stuff, i think there might be a couple of follow-up changes to investigate, but i'm also happy to do them myself if you'd prefer:

tracing/src/macros.rs Outdated Show resolved Hide resolved
@hawkw hawkw enabled auto-merge (squash) January 28, 2022 19:00
@hawkw hawkw merged commit 71da8a5 into tokio-rs:master Jan 28, 2022
guswynn added a commit to guswynn/tracing that referenced this pull request Feb 1, 2022
## Motivation

Closes: tokio-rs#1668

My usecase is different than the referenced "avoid doing something
expensive to log": I want to guard turning on `debug` mode for an ffi'd
library, based on some `target` that represents the "module" we care
about.

## Solution

The macro is very similar to `event!`, but adds a few simplistic cases,
and generates ever so slightly different code (to return the correct
value always. It also skips anything to do with `tracing-log`. I
considered (and tried), to share the impl between `event!` and
`enabled!`, but must confess I am not good at macros and got stuck. I
think they are sufficiently different, where copied impls, is easier to
read. We already manage 

Also, my project is on the crates.io version, so this would need to be
backported to 0.1, I can help with that with guidance.

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
Co-authored-by: David Barsky <me@davidbarsky.com>
@guswynn
Copy link
Contributor Author

guswynn commented Feb 1, 2022

@hawkw If I find time this week I may look into those followups! as well as my own:

Query if a span/event/hint is enabled! for a specific layer. I need to think about this one for a bit

@hawkw
Copy link
Member

hawkw commented Feb 1, 2022

Query if a span/event/hint is enabled! for a specific layer. I need to think about this one for a bit

I think this would require a separate API in tracing-subscriber --- it isn't going to be possible in the enabled! macro, which is defined in the tracing crate, which doesn't know about any concept of Layers.

Something like that could certainly be nice to have, although I think figuring out how to do it may require a great deal of work --- I would probably prioritize some of the smaller changes to the macro first, since they'll be easier to do, and would be nice to add before making a release with the enabled! macro.

hawkw added a commit that referenced this pull request Feb 2, 2022
Backports #1821 to `v0.1.x`.

## Motivation

Closes: #1668

My usecase is different than the referenced "avoid doing something
expensive to log": I want to guard turning on `debug` mode for an ffi'd
library, based on some `target` that represents the "module" we care
about.

## Solution

The macro is very similar to `event!`, but adds a few simplistic cases,
and generates ever so slightly different code (to return the correct
value always. It also skips anything to do with `tracing-log`. I
considered (and tried), to share the impl between `event!` and
`enabled!`, but must confess I am not good at macros and got stuck. I
think they are sufficiently different, where copied impls, is easier to
read. We already manage 

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
Co-authored-by: David Barsky <me@davidbarsky.com>
hawkw added a commit that referenced this pull request Feb 3, 2022
This changes the `Kind` type to a bitflag, in order to represent
callsites that are hints but also count as spans or events. The goal
here is to allow variants of the `enabled!` macro that specifically
check if a span would be enabled, or if an event would be enabled.

See [this comment][1] for details.

This does not actually implement the `enabled!` variants, just the
`Kind` representation change. This way, we can add to the `enabled!`
macro in a subsequent `tracing` release without having to change
`tracing-core` again.

I went with the bitflag representation rather than adding a bool to the
`KindInner::Span` and `KindInner::Event` enum variants because it felt a
bit simpler and maybe more performant, although I don't think it's
particularly important to micro-optimize here. I'd consider changing
this to an enum-based representation if people think it's significantly
easier to understand.

[1]: #1821 (comment)
hawkw added a commit that referenced this pull request Feb 3, 2022
This changes the `Kind` type to a bitflag, in order to represent
callsites that are hints but also count as spans or events. The goal
here is to allow variants of the `enabled!` macro that specifically
check if a span would be enabled, or if an event would be enabled.

See [this comment][1] for details.

This does not actually implement the `enabled!` variants, just the
`Kind` representation change. This way, we can add to the `enabled!`
macro in a subsequent `tracing` release without having to change
`tracing-core` again.

I went with the bitflag representation rather than adding a bool to the
`KindInner::Span` and `KindInner::Event` enum variants because it felt a
bit simpler and maybe more performant, although I don't think it's
particularly important to micro-optimize here. I'd consider changing
this to an enum-based representation if people think it's significantly
easier to understand.

[1]: #1821 (comment)
hawkw added a commit that referenced this pull request Feb 3, 2022
This changes the `Kind` type to a bitflag, in order to represent
callsites that are hints but also count as spans or events. The goal
here is to allow variants of the `enabled!` macro that specifically
check if a span would be enabled, or if an event would be enabled.

See [this comment][1] for details.

This does not actually implement the `enabled!` variants, just the
`Kind` representation change. This way, we can add to the `enabled!`
macro in a subsequent `tracing` release without having to change
`tracing-core` again.

I went with the bitflag representation rather than adding a bool to the
`KindInner::Span` and `KindInner::Event` enum variants because it felt a
bit simpler and maybe more performant, although I don't think it's
particularly important to micro-optimize here. I'd consider changing
this to an enum-based representation if people think it's significantly
easier to understand.

[1]: #1821 (comment)
hawkw added a commit that referenced this pull request Feb 3, 2022
This changes the `Kind` type to a bitflag, in order to represent
callsites that are hints but also count as spans or events. The goal
here is to allow variants of the `enabled!` macro that specifically
check if a span would be enabled, or if an event would be enabled.

See [this comment][1] for details.

This does not actually implement the `enabled!` variants, just the
`Kind` representation change. This way, we can add to the `enabled!`
macro in a subsequent `tracing` release without having to change
`tracing-core` again.

I went with the bitflag representation rather than adding a bool to the
`KindInner::Span` and `KindInner::Event` enum variants because it felt a
bit simpler and maybe more performant, although I don't think it's
particularly important to micro-optimize here. I'd consider changing
this to an enum-based representation if people think it's significantly
easier to understand.

[1]: #1821 (comment)
hawkw added a commit that referenced this pull request Feb 3, 2022
This changes the `Kind` type to a bitflag, in order to represent
callsites that are hints but also count as spans or events. The goal
here is to allow variants of the `enabled!` macro that specifically
check if a span would be enabled, or if an event would be enabled.

See [this comment][1] for details.

This does not actually implement the `enabled!` variants, just the
`Kind` representation change. This way, we can add to the `enabled!`
macro in a subsequent `tracing` release without having to change
`tracing-core` again.

I went with the bitflag representation rather than adding a bool to the
`KindInner::Span` and `KindInner::Event` enum variants because it felt a
bit simpler and maybe more performant, although I don't think it's
particularly important to micro-optimize here. I'd consider changing
this to an enum-based representation if people think it's significantly
easier to understand.

[1]: #1821 (comment)
hawkw added a commit that referenced this pull request Feb 4, 2022
This changes the `Kind` type to a bitflag, in order to represent
callsites that are hints but also count as spans or events. The goal
here is to allow variants of the `enabled!` macro that specifically
check if a span would be enabled, or if an event would be enabled.

See [this comment][1] for details.

This does not actually implement the `enabled!` variants, just the
`Kind` representation change. This way, we can add to the `enabled!`
macro in a subsequent `tracing` release without having to change
`tracing-core` again.

I went with the bitflag representation rather than adding a bool to the
`KindInner::Span` and `KindInner::Event` enum variants because it felt a
bit simpler and maybe more performant, although I don't think it's
particularly important to micro-optimize here. I'd consider changing
this to an enum-based representation if people think it's significantly
easier to understand.

[1]: #1821 (comment)
@guswynn guswynn deleted the enabled branch February 4, 2022 17:10
liuhaozhan added a commit to liuhaozhan/tracing that referenced this pull request Nov 17, 2022
This changes the `Kind` type to a bitflag, in order to represent
callsites that are hints but also count as spans or events. The goal
here is to allow variants of the `enabled!` macro that specifically
check if a span would be enabled, or if an event would be enabled.

See [this comment][1] for details.

This does not actually implement the `enabled!` variants, just the
`Kind` representation change. This way, we can add to the `enabled!`
macro in a subsequent `tracing` release without having to change
`tracing-core` again.

I went with the bitflag representation rather than adding a bool to the
`KindInner::Span` and `KindInner::Event` enum variants because it felt a
bit simpler and maybe more performant, although I don't think it's
particularly important to micro-optimize here. I'd consider changing
this to an enum-based representation if people think it's significantly
easier to understand.

[1]: tokio-rs/tracing#1821 (comment)
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
Backports tokio-rs#1821 to `v0.1.x`.

## Motivation

Closes: tokio-rs#1668

My usecase is different than the referenced "avoid doing something
expensive to log": I want to guard turning on `debug` mode for an ffi'd
library, based on some `target` that represents the "module" we care
about.

## Solution

The macro is very similar to `event!`, but adds a few simplistic cases,
and generates ever so slightly different code (to return the correct
value always. It also skips anything to do with `tracing-log`. I
considered (and tried), to share the impl between `event!` and
`enabled!`, but must confess I am not good at macros and got stuck. I
think they are sufficiently different, where copied impls, is easier to
read. We already manage 

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
Co-authored-by: David Barsky <me@davidbarsky.com>
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
This changes the `Kind` type to a bitflag, in order to represent
callsites that are hints but also count as spans or events. The goal
here is to allow variants of the `enabled!` macro that specifically
check if a span would be enabled, or if an event would be enabled.

See [this comment][1] for details.

This does not actually implement the `enabled!` variants, just the
`Kind` representation change. This way, we can add to the `enabled!`
macro in a subsequent `tracing` release without having to change
`tracing-core` again.

I went with the bitflag representation rather than adding a bool to the
`KindInner::Span` and `KindInner::Event` enum variants because it felt a
bit simpler and maybe more performant, although I don't think it's
particularly important to micro-optimize here. I'd consider changing
this to an enum-based representation if people think it's significantly
easier to understand.

[1]: tokio-rs#1821 (comment)
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.

tracing::enabled!() macro for testing if events might be enabled given a static subset of event data
6 participants