Allow skipping arguments in `#[instrument]`. #359

hdevalence · 2019-09-23T20:12:55Z

Motivation

This adds a skip directive to the #[instrument] macro that allows
specifying one or more arguments to the function which will not appear
in the generated span. In particular, this means that it's possible to
use #[instrument] on functions with a non-Debug parameter.

Solution

The interior of a skip(arg1, arg2) argument is scanned for identifiers and these are filtered out of the identifier list used to generate the span.

This hopefully closes #11.

This adds a `skip` directive to the `#[instrument]` macro that allows specifying one or more arguments to the function which will not appear in the generated span. In particular, this means that it's possible to use `#[instrument]` on functions with a non-`Debug` parameter.

hdevalence · 2019-09-23T20:13:26Z

tracing-attributes/src/lib.rs

+            lit.span() => compile_error!("expected only a single `skip` argument!")
+        };
+        */
+        panic!("not sure how to make this return an error -- the examples below return impl ToTokens and get stuck into the output.");


This should be fixed before merging but I'm not sure what the appropriate way to return an error in a proc macro is.

The commented-out code above is correct with regards to how you'd display the error to the user — you want to return a quoteed block that expands to the compile_error! macro.

However, unlike the other functions that return impl ToTokens, this function isn't actually returning anything that expands to tokens; it's returning a HashSet. I'd probably make this return a Result<HashSet<Ident>, impl ToTokens> or something? Then, at the call site, you could match the result and insert the compiler error if it's an Err.

hawkw

@hdevalence this is great, thanks so much!

I'd be very happy to merge this once the error handling thing is resolved --- I tried to answer your question in my review. I also commented on a couple of nits that aren't really blockers.

hawkw · 2019-09-23T21:23:48Z

tracing-attributes/src/lib.rs

+            lit.span() => compile_error!("expected only a single `skip` argument!")
+        };
+        */
+        panic!("not sure how to make this return an error -- the examples below return impl ToTokens and get stuck into the output.");


The commented-out code above is correct with regards to how you'd display the error to the user — you want to return a quoteed block that expands to the compile_error! macro.

However, unlike the other functions that return impl ToTokens, this function isn't actually returning anything that expands to tokens; it's returning a HashSet. I'd probably make this return a Result<HashSet<Ident>, impl ToTokens> or something? Then, at the call site, you could match the result and insert the compiler error if it's an Err.

hawkw · 2019-09-23T21:25:43Z

tracing-attributes/tests/instrument.rs

@@ -80,8 +82,8 @@ fn fields() {
        .run_with_handle();

    with_default(subscriber, || {
-        my_fn(2, false);
-        my_fn(3, true);
+        my_fn(2, false, UnDebug(0), UnDebug(1));


can we also add a .only() to the mock fields above, so that we assert no other fields are also added to the generated span? like:

span.clone().with_field( field::mock("arg1") .with_value(&format_args!("2")) .and(field::mock("arg2").with_value(&format_args!("false"))) .only(), ),

hawkw · 2019-09-23T21:27:31Z

tracing-attributes/tests/instrument.rs

-    fn my_fn(arg1: usize, arg2: bool) {}
+    struct UnDebug(pub u32);
+
+    #[instrument(target = "my_target", level = "debug", skip(_arg3, _arg4))]


Take it or leave it, but: rather than adding the skip behavior to this test, I might add a new test for the skip attribute? That way, if there's a regression that breaks only the skip attribute, we could see what behavior is broken based on which test fails. Not a blocker though!

I'd be happy to add a new test, but I didn't do it mostly because I don't really understand how the span and subscriber mocking works, so adding into the existing test seemed easier.

hawkw

A couple minor nits if you're interested in addressing them as well. Otherwise, this looks great, thanks!

tracing-attributes/src/lib.rs

tracing-attributes/tests/instrument.rs

Co-Authored-By: Eliza Weisman <[email protected]>

hawkw · 2019-09-23T22:51:01Z

@hdevalence I think the CI build for this is failing because rustfmt needs to be run. If you don't mind running cargo fmt --all & pushing a commit with the result, I'll merge this as soon as it lands.

hdevalence · 2019-09-23T22:56:00Z

Fixed!

hdevalence · 2019-09-23T23:03:59Z

Hmm, I ran cargo fmt --all but the CI is still upset about formatting. Perhaps there's a version mismatch between my local version and the one in CI, or..?

hdevalence · 2019-09-23T23:05:04Z

Oh, maybe it was that I ran cargo fmt before merging changes from suggestions on Github.

hawkw · 2019-09-23T23:23:27Z

And merged! Thanks @hdevalence!

Added: - Optional `skip` argument to `#[instrument]` for excluding function parameters from generated spans (#359) Signed-off-by: Eliza Weisman <[email protected]>

hawkw · 2019-09-26T21:08:46Z

@hdevalence just wanted to let you know that this has now been published in tracing-attributes version 0.1.4!

hdevalence · 2019-09-26T23:18:36Z

Thanks!

Added - Support for destructuring in arguments to `#[instrument]`ed functions (#397) - Generated field for `self` parameters when `#[instrument]`ing methods (#397) - Optional `skip` argument to `#[instrument]` for excluding function parametersfrom generated spans (#359) - Added `dispatcher::set_default` and `subscriber::set_default` APIs, which return a drop guard (#388) Fixed - Some minor documentation errors (#356, #370) Signed-off-by: Eliza Weisman <[email protected]>

* tracing: bump attrs and core versions Signed-off-by: Eliza Weisman <[email protected]> * tracing: prepare to release 0.1.10 Added - Support for destructuring in arguments to `#[instrument]`ed functions (#397) - Generated field for `self` parameters when `#[instrument]`ing methods (#397) - Optional `skip` argument to `#[instrument]` for excluding function parametersfrom generated spans (#359) - Added `dispatcher::set_default` and `subscriber::set_default` APIs, which return a drop guard (#388) Fixed - Some minor documentation errors (#356, #370) Signed-off-by: Eliza Weisman <[email protected]>

hdevalence commented Sep 23, 2019

View reviewed changes

hawkw reviewed Sep 23, 2019

View reviewed changes

hdevalence added 2 commits September 23, 2019 15:22

Fix error-handling in skip directive parsing

a8760d6

Add a separate test for skip directives.

cc15bad

hawkw approved these changes Sep 23, 2019

View reviewed changes

tracing-attributes/src/lib.rs Show resolved Hide resolved

tracing-attributes/tests/instrument.rs Outdated Show resolved Hide resolved

Update tracing-attributes/tests/instrument.rs

f91c1ce

Co-Authored-By: Eliza Weisman <[email protected]>

fmt

5e8346b

fmt

11c3623

hawkw merged commit 3552c6c into tokio-rs:master Sep 23, 2019

hdevalence deleted the skip branch September 23, 2019 23:29

hawkw added a commit that referenced this pull request Sep 25, 2019

attributes: prepare to release 0.1.4

4a7b8c9

Added: - Optional `skip` argument to `#[instrument]` for excluding function parameters from generated spans (#359) Signed-off-by: Eliza Weisman <[email protected]>

hawkw added a commit that referenced this pull request Sep 25, 2019

attributes: prepare to release 0.1.4

9d2aca3

Added: - Optional `skip` argument to `#[instrument]` for excluding function parameters from generated spans (#359) Signed-off-by: Eliza Weisman <[email protected]>

hawkw added a commit that referenced this pull request Sep 25, 2019

attributes: prepare to release 0.1.4

e1d5835

Added: - Optional `skip` argument to `#[instrument]` for excluding function parameters from generated spans (#359) Signed-off-by: Eliza Weisman <[email protected]>

hawkw mentioned this pull request Sep 25, 2019

attributes: prepare to release 0.1.4 #364

Merged

hawkw added a commit that referenced this pull request Sep 26, 2019

attributes: prepare to release 0.1.4

31e4035

Added: - Optional `skip` argument to `#[instrument]` for excluding function parameters from generated spans (#359) Signed-off-by: Eliza Weisman <[email protected]>

hawkw added a commit that referenced this pull request Sep 26, 2019

attributes: prepare to release 0.1.4 (#364)

5310bd0

Added: - Optional `skip` argument to `#[instrument]` for excluding function parameters from generated spans (#359) Signed-off-by: Eliza Weisman <[email protected]>

hdevalence mentioned this pull request Sep 26, 2019

Move tracing dependencies off of git ZcashFoundation/zebra#33

Closed

couchand mentioned this pull request Oct 22, 2019

attributes: make parameter fields optional #396

Closed

hawkw mentioned this pull request Oct 23, 2019

tracing: prepare to release 0.1.10 #402

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow skipping arguments in `#[instrument]`. #359

Allow skipping arguments in `#[instrument]`. #359

hdevalence commented Sep 23, 2019

hdevalence Sep 23, 2019

hawkw Sep 23, 2019 •

edited

Loading

hawkw left a comment

hawkw Sep 23, 2019 •

edited

Loading

hawkw Sep 23, 2019

hawkw Sep 23, 2019

hdevalence Sep 23, 2019

hawkw left a comment

hawkw commented Sep 23, 2019

hdevalence commented Sep 23, 2019

hdevalence commented Sep 23, 2019

hdevalence commented Sep 23, 2019

hawkw commented Sep 23, 2019

hawkw commented Sep 26, 2019

hdevalence commented Sep 26, 2019

Allow skipping arguments in #[instrument]. #359

Allow skipping arguments in #[instrument]. #359

Conversation

hdevalence commented Sep 23, 2019

Motivation

Solution

hdevalence Sep 23, 2019

Choose a reason for hiding this comment

hawkw Sep 23, 2019 • edited Loading

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment

hawkw Sep 23, 2019 • edited Loading

Choose a reason for hiding this comment

hawkw Sep 23, 2019

Choose a reason for hiding this comment

hawkw Sep 23, 2019

Choose a reason for hiding this comment

hdevalence Sep 23, 2019

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment

hawkw commented Sep 23, 2019

hdevalence commented Sep 23, 2019

hdevalence commented Sep 23, 2019

hdevalence commented Sep 23, 2019

hawkw commented Sep 23, 2019

hawkw commented Sep 26, 2019

hdevalence commented Sep 26, 2019

Allow skipping arguments in `#[instrument]`. #359

Allow skipping arguments in `#[instrument]`. #359

hawkw Sep 23, 2019 •

edited

Loading

hawkw Sep 23, 2019 •

edited

Loading