implement `enabled_span` and `enabled_event` #1900

guswynn · 2022-02-04T17:08:35Z

Motivation

Solution

I went with sharing as much of the macro as possible. This means in the docs, the macro only shows the ($($rest:tt)*)=> (...) variant, but I think linking to the enabled! docs should be fine. Also, I find the macro pattern part of rustdoc to be very hard to understand, and I think many people dont look at it

@hawkw I went ahead and put this pr against the v0.1 branch, let me know if you prefer master and backporting

hawkw · 2022-02-04T17:11:57Z

@hawkw I went ahead and put this pr against the v0.1 branch, let me know if you prefer master and backporting

in general, please prefer to open PRs against master and backporting them to v0.1.x, rather than the other way around. normally, I backport large numbers of PRs from master in one go as part of the release prep process.

hawkw

overall, this seems like a good direction. i'm unconvinced about the benefits of making a separate internal macro, versus just making all the cases with kinds be part of the main enabled! macro and have the event- and span-specific versions call into that --- open to discussion on that, though.

also, I had some minor naming and documentation suggestions.

thanks for working on this!

tracing/src/macros.rs

guswynn · 2022-02-04T19:25:12Z

@hawkw I think all review is covered now.
I had some trouble with the macro branch ordering, for reasons I dont understand, but I got it working

currently, I pass through $kind:expr in the main enabled! macro, and span_enabled! and event_enabled! pass through the correct expression

this could be 1. left as is 2. or switched to allow users to pass kind: span and kind: event as optional parameters in front, if you prefer that over the specific macros. I like the simplicity of this impl, and the parallel with the event! and span! macros tho

I think #2 could be done backwards-compatibly in the future, as those parameters on the macro are un-documented as is

hawkw

overall, this seems good; i had some minor comments

tracing/tests/macros.rs

tracing/tests/enabled.rs

tracing/src/macros.rs

davidbarsky · 2022-02-08T21:37:43Z

I only just saw this PR; sorry about that. To quote @hawkw from the issue:

Alternatively, we could add this to the existing enabled! macro, like enabled!(span, ...) and enabled!(event, ...). IMO, just making separate macros is a bit more obvious from a UX perspective, but I'm open to being convinced otherwise.

I think I'd prefer to add this to the existing enabled! macro for the following reasons:

There's already a large amount of macros in tracing, and I'd prefer to avoid adding new ones that might overwhelm newcomers.
enabled! feels like a relatively niche tool—tracing was able to get away without adding it for years. By the time a user knows they'd benefit from using enabled!, I think that they'd check enabled!'s documentation to see if they can pass event/span data. I don't think that enabled! needs the additional discoverability that enabled_span! and enabled_event! will implicitly add.
- While I don't really use span macros like info_span!, I can see their value. I worry that increasing the prominence of enabled! (by having more enabled!-like macros in the top-level macros section of the Rustdoc) will lead people to over-rely on enabled! instead of leaning on filters such as, e.g., Targets (I made this point last since I have the least amount of data to support this assertion and I might be the most wrong on this).

That being said, I don't feel too strongly about these points, but I should've mentioned it before @guswynn implemented it.

hawkw · 2022-02-08T21:48:37Z

I only just saw this PR; sorry about that. To quote @hawkw from the issue:

Alternatively, we could add this to the existing enabled! macro, like enabled!(span, ...) and enabled!(event, ...). IMO, just making separate macros is a bit more obvious from a UX perspective, but I'm open to being convinced otherwise.

I think I'd prefer to add this to the existing enabled! macro for the following reasons:

This PR already does that, it just also adds span_enabled! and event_enabled! macros that expand to invoke the enabled! macro. I think this is correct because it's (a) consistent with the general design of other macros, and (b) makes it very clear that there is the possibility that the result of asking if a span is enabled and if an event is enabled may differ.

enabled! feels like a relatively niche tool—tracing was able to get away without adding it for years. By the time a user knows they'd benefit from using enabled!, I think that they'd check enabled!'s documentation to see if they can pass event/span data. I don't think that enabled! needs the additional discoverability that enabled_span! and enabled_event! will implicitly add.

While I don't really use span macros like info_span!, I can see their value. I worry that increasing the prominence of enabled! (by having more enabled!-like macros in the top-level macros section of the Rustdoc) will lead people to over-rely on enabled! instead of leaning on filters such as, e.g., Targets (I made this point last since I have the least amount of data to support this assertion and I might be the most wrong on this).

Is your argument here essentially that because you don't think that people should use the API, you don't think it should be easy to use? I don't think this is a good approach. If we're going to provide something, we should make it straightforward and easy to use. If we don't think it should exist at all, we shouldn't provide the API, but if we've made the decision to have it, we shouldn't intentionally make it worse in order to discourage its use. If we commit to providing an API, stopping people from using it is solely a documentation problem.

As a side note, I still don't understand where the idea that using enabled! is somehow in competition with "using filters such as e.g., Targets", when the only thing the macro does is ask the filter if it would enable something.

My final argument in favor of separate macros is that they ensure that the callsite kind always has its HINT bit set. If we just accept an arbitrary callsite kind, we can't stop the user from calling enabled! with a callsite that isn't a hint.

tracing/src/macros.rs

guswynn · 2022-02-08T22:42:53Z

all review should be handled!
cleaned up the tests, moved the .hint (but left $kind as :expr not :ident, left it as separate macros

hawkw

this seems fine to me. i have a slight preference for putting all the tests in one file, but it's not a big deal

hawkw · 2022-02-08T23:18:25Z

tracing/tests/specific_enabled.rs

+        .done()
+        .run();
+
+    tracing::collect::set_global_default(collector).unwrap();


i would probably use the local default (set_default) in these tests, so they can all be in the same file...having to put it in a separate file is kind of annoying. not a hard blocker though.

Ill clean this up then merge!

rather, ill clean this up, then yall can squash-and-merge

davidbarsky · 2022-02-09T16:17:51Z

As a side note, I still don't understand where the idea that using enabled! is somehow in competition with "using filters such as e.g., Targets", when the only thing the macro does is ask the filter if it would enable something.

Spoke to Eliza on Discord. Quick summary:

this PR is reasonable and tracing should require users of enabled! to specify whether they're querying over a span or event. enabled! should've either required a kind parameter (with the downside that a caller can provide an invalid bitset) or not have shipped, meaning that enabled_span! or enabled_event! should be the primary API. This can be revisited in tracing 0.2, but it won't be figured out on this PR.
I over-indexed on my fear that people would incorrectly rely on/misuse enabled! instead of realizing a deactivated span or event's field values are not evaluated.

guswynn · 2022-02-09T22:42:18Z

I cant recreate that compilation failure, locally, on the same nightly...

hawkw · 2022-02-09T23:59:26Z

I cant recreate that compilation failure, locally, on the same nightly...

it appears to be a compiler regression, i've already opened rust-lang/rust#93821 for it. we can move forward with this PR regardless, since it's not our fault.

## Motivation Closes #1892 ## Solution This branch adds new `span_enabled!` and `event_enabled!` macros. I went withe sharing as much of the macro as possible. The new macros were implemented by adding a `kind:` parameter to `enabled!`, and invoking `enabled!` with the appropriate `kind:` argument. This means in the docs, the macro only shows the `($($rest:tt)*)=> (...)` variant, but I think linking to the `enabled!` docs should be fine. Also, I find the macro pattern part of rustdoc to be very hard to understand, and I think many people dont look at it. Co-authored-by: Eliza Weisman <eliza@buoyant.io>

@guswynn

# 0.1.33 (April 9, 2022) This release adds new `span_enabled!` and `event_enabled!` variants of the `enabled!` macro, for testing whether a subscriber would specifically enable a span or an event. ### Added - `span_enabled!` and `event_enabled!` macros ([#1900]) - Several documentation improvements ([#2010], [#2012]) ### Fixed - Compilation warning when compiling for <=32-bit targets (including `wasm32`) ([#2060]) Thanks to @guswynn, @arifd, @hrxi, @CAD97, and @name1e5s for contributing to this release! [#1900]: #1900 [#2010]: #2010 [#2012]: #2012 [#2060]: #2060

@guswynn

# 0.1.33 (April 9, 2022) This release adds new `span_enabled!` and `event_enabled!` variants of the `enabled!` macro, for testing whether a subscriber would specifically enable a span or an event. ### Added - `span_enabled!` and `event_enabled!` macros ([#1900]) - Several documentation improvements ([#2010], [#2012]) ### Fixed - Compilation warning when compiling for <=32-bit targets (including `wasm32`) ([#2060]) Thanks to @guswynn, @arifd, @hrxi, @CAD97, and @name1e5s for contributing to this release! [#1900]: #1900 [#2010]: #2010 [#2012]: #2012 [#2060]: #2060

guswynn requested review from hawkw and a team as code owners February 4, 2022 17:08

guswynn force-pushed the span_enabled branch from cc49774 to 59ba34e Compare February 4, 2022 17:09

hawkw changed the base branch from v0.1.x to master February 4, 2022 17:14

hawkw requested review from carllerche, davidbarsky, jtescher and yaahc as code owners February 4, 2022 17:14

guswynn force-pushed the span_enabled branch from 59ba34e to bb52b2d Compare February 4, 2022 17:28

hawkw reviewed Feb 4, 2022

View reviewed changes

guswynn mentioned this pull request Feb 4, 2022

add tracing feature fede1024/rust-rdkafka#440

Merged

hawkw reviewed Feb 8, 2022

View reviewed changes

hawkw requested changes Feb 8, 2022

View reviewed changes

tracing/src/macros.rs Outdated Show resolved Hide resolved

guswynn added 8 commits February 8, 2022 14:27

implement enabled_span and enabled_event

ef715b9

enabled_event -> event_enable

04075d5

improve {event,span}_enabled docs

8f5a94e

cleanup rustdoc links for enabled

8716d98

switch macro impl to all being enabled

497f318

docs and tests

43d54b5

more doc tests

704ecf7

cleanup .hint usage

1b0d66e

guswynn force-pushed the span_enabled branch from 1a2210f to 1b0d66e Compare February 8, 2022 22:43

hawkw approved these changes Feb 8, 2022

View reviewed changes

hawkw approved these changes Feb 9, 2022

View reviewed changes

merge tests with set_default

1d3e8db

guswynn force-pushed the span_enabled branch from 87eac2e to 1d3e8db Compare February 9, 2022 22:42

Merge branch 'master' into span_enabled

e5b20bc

hawkw approved these changes Mar 31, 2022

View reviewed changes

hawkw enabled auto-merge (squash) March 31, 2022 18:22

hawkw merged commit 16577e1 into tokio-rs:master Mar 31, 2022

hawkw mentioned this pull request Apr 9, 2022

tracing: prepare to release v0.1.33 #2061

Merged

guswynn deleted the span_enabled branch June 23, 2022 18:11

davidbarsky mentioned this pull request Sep 26, 2023

chore: backport roughly a year's worth of changes #2728

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement `enabled_span` and `enabled_event` #1900

implement `enabled_span` and `enabled_event` #1900

guswynn commented Feb 4, 2022

hawkw commented Feb 4, 2022

hawkw left a comment

guswynn commented Feb 4, 2022

hawkw left a comment

davidbarsky commented Feb 8, 2022

hawkw commented Feb 8, 2022

guswynn commented Feb 8, 2022

hawkw left a comment

hawkw Feb 8, 2022

guswynn Feb 9, 2022

guswynn Feb 9, 2022

davidbarsky commented Feb 9, 2022

guswynn commented Feb 9, 2022

hawkw commented Feb 9, 2022

implement enabled_span and enabled_event #1900

implement enabled_span and enabled_event #1900

Conversation

guswynn commented Feb 4, 2022

Motivation

Solution

hawkw commented Feb 4, 2022

hawkw left a comment

Choose a reason for hiding this comment

guswynn commented Feb 4, 2022

hawkw left a comment

Choose a reason for hiding this comment

davidbarsky commented Feb 8, 2022

hawkw commented Feb 8, 2022

guswynn commented Feb 8, 2022

hawkw left a comment

Choose a reason for hiding this comment

hawkw Feb 8, 2022

Choose a reason for hiding this comment

guswynn Feb 9, 2022

Choose a reason for hiding this comment

guswynn Feb 9, 2022

Choose a reason for hiding this comment

davidbarsky commented Feb 9, 2022

guswynn commented Feb 9, 2022

hawkw commented Feb 9, 2022

implement `enabled_span` and `enabled_event` #1900

implement `enabled_span` and `enabled_event` #1900