From 192ead3ecfd37f056a40e0d8770e10c66deda551 Mon Sep 17 00:00:00 2001 From: Henry de Valence Date: Mon, 23 Sep 2019 13:05:41 -0700 Subject: [PATCH 1/6] Allow skipping arguments in `#[instrument]`. 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. --- tracing-attributes/src/lib.rs | 42 ++++++++++++++++++++++++-- tracing-attributes/tests/instrument.rs | 10 +++--- 2 files changed, 46 insertions(+), 6 deletions(-) diff --git a/tracing-attributes/src/lib.rs b/tracing-attributes/src/lib.rs index 381f2a511c..324c1b5dc0 100644 --- a/tracing-attributes/src/lib.rs +++ b/tracing-attributes/src/lib.rs @@ -39,11 +39,13 @@ //! [instrument]: attr.instrument.html extern crate proc_macro; +use std::collections::HashSet; + use proc_macro::TokenStream; use quote::{quote, quote_spanned, ToTokens}; use syn::{ - spanned::Spanned, AttributeArgs, FnArg, Ident, ItemFn, Lit, LitInt, Meta, MetaNameValue, - NestedMeta, Pat, PatIdent, PatType, Signature, + spanned::Spanned, AttributeArgs, FnArg, Ident, ItemFn, Lit, LitInt, Meta, MetaList, + MetaNameValue, NestedMeta, Pat, PatIdent, PatType, Signature, }; /// Instruments a function to create and enter a `tracing` [span] every time @@ -143,6 +145,9 @@ pub fn instrument(args: TokenStream, item: TokenStream) -> TokenStream { // function name let ident_str = ident.to_string(); + // Pull out the arguments-to-be-skipped first, so we can filter results below. + let skips = skips(&args); + let param_names: Vec = params .clone() .into_iter() @@ -153,6 +158,7 @@ pub fn instrument(args: TokenStream, item: TokenStream) -> TokenStream { }, _ => None, }) + .filter(|ident| !skips.contains(ident)) .collect(); let param_names_clone = param_names.clone(); @@ -203,6 +209,38 @@ pub fn instrument(args: TokenStream, item: TokenStream) -> TokenStream { .into() } +fn skips(args: &AttributeArgs) -> HashSet { + let mut skips = args.iter().filter_map(|arg| match arg { + NestedMeta::Meta(Meta::List(MetaList { + ref path, + ref nested, + .. + })) if path.is_ident("skip") => Some(nested), + _ => None, + }); + let skip = skips.next(); + + // Ensure there's only one skip directive. + if let Some(_list) = skips.next() { + /* + return quote_spanned! { + 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."); + } + + // Collect the Idents inside the `skip(...)`, if it exists + skip.iter() + .map(|list| list.iter()) + .flatten() + .filter_map(|meta| match meta { + NestedMeta::Meta(Meta::Path(p)) => p.get_ident().map(Clone::clone), + _ => None, + }) + .collect() +} + fn level(args: &AttributeArgs) -> impl ToTokens { let mut levels = args.iter().filter_map(|arg| match arg { NestedMeta::Meta(Meta::NameValue(MetaNameValue { diff --git a/tracing-attributes/tests/instrument.rs b/tracing-attributes/tests/instrument.rs index 569b191643..05c4bb159f 100644 --- a/tracing-attributes/tests/instrument.rs +++ b/tracing-attributes/tests/instrument.rs @@ -43,8 +43,10 @@ fn override_everything() { #[test] fn fields() { - #[instrument(target = "my_target", level = "debug")] - fn my_fn(arg1: usize, arg2: bool) {} + struct UnDebug(pub u32); + + #[instrument(target = "my_target", level = "debug", skip(_arg3, _arg4))] + fn my_fn(arg1: usize, arg2: bool, _arg3: UnDebug, _arg4: UnDebug) {} let span = span::mock() .named("my_fn") @@ -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)); + my_fn(3, true, UnDebug(0), UnDebug(1)); }); handle.assert_finished(); From a8760d666a2e6771cc062091eaeea7e1abff876b Mon Sep 17 00:00:00 2001 From: Henry de Valence Date: Mon, 23 Sep 2019 15:22:16 -0700 Subject: [PATCH 2/6] Fix error-handling in skip directive parsing --- tracing-attributes/src/lib.rs | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/tracing-attributes/src/lib.rs b/tracing-attributes/src/lib.rs index 324c1b5dc0..2db84963c2 100644 --- a/tracing-attributes/src/lib.rs +++ b/tracing-attributes/src/lib.rs @@ -146,7 +146,10 @@ pub fn instrument(args: TokenStream, item: TokenStream) -> TokenStream { let ident_str = ident.to_string(); // Pull out the arguments-to-be-skipped first, so we can filter results below. - let skips = skips(&args); + let skips = match skips(&args) { + Ok(skips) => skips, + Err(err) => return quote!(#err).into(), + }; let param_names: Vec = params .clone() @@ -209,7 +212,7 @@ pub fn instrument(args: TokenStream, item: TokenStream) -> TokenStream { .into() } -fn skips(args: &AttributeArgs) -> HashSet { +fn skips(args: &AttributeArgs) -> Result, impl ToTokens> { let mut skips = args.iter().filter_map(|arg| match arg { NestedMeta::Meta(Meta::List(MetaList { ref path, @@ -221,24 +224,22 @@ fn skips(args: &AttributeArgs) -> HashSet { let skip = skips.next(); // Ensure there's only one skip directive. - if let Some(_list) = skips.next() { - /* - return quote_spanned! { - 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."); + if let Some(list) = skips.next() { + return Err(quote_spanned! { + list.span() => compile_error!("expected only a single `skip` argument!") + }); } // Collect the Idents inside the `skip(...)`, if it exists - skip.iter() + Ok(skip + .iter() .map(|list| list.iter()) .flatten() .filter_map(|meta| match meta { NestedMeta::Meta(Meta::Path(p)) => p.get_ident().map(Clone::clone), _ => None, }) - .collect() + .collect()) } fn level(args: &AttributeArgs) -> impl ToTokens { From cc15bad26eaf0f813c5038043fe91c60bd67249a Mon Sep 17 00:00:00 2001 From: Henry de Valence Date: Mon, 23 Sep 2019 15:24:09 -0700 Subject: [PATCH 3/6] Add a separate test for skip directives. --- tracing-attributes/tests/instrument.rs | 62 +++++++++++++++++++++++--- 1 file changed, 56 insertions(+), 6 deletions(-) diff --git a/tracing-attributes/tests/instrument.rs b/tracing-attributes/tests/instrument.rs index 05c4bb159f..10603dd5ee 100644 --- a/tracing-attributes/tests/instrument.rs +++ b/tracing-attributes/tests/instrument.rs @@ -45,8 +45,8 @@ fn override_everything() { fn fields() { struct UnDebug(pub u32); - #[instrument(target = "my_target", level = "debug", skip(_arg3, _arg4))] - fn my_fn(arg1: usize, arg2: bool, _arg3: UnDebug, _arg4: UnDebug) {} + #[instrument(target = "my_target", level = "debug")] + fn my_fn(arg1: usize, arg2: bool) {} let span = span::mock() .named("my_fn") @@ -62,7 +62,57 @@ fn fields() { span.clone().with_field( field::mock("arg1") .with_value(&format_args!("2")) - .and(field::mock("arg2").with_value(&format_args!("false"))), + .and(field::mock("arg2").with_value(&format_args!("false"))) + .only(), + ), + ) + .enter(span.clone()) + .exit(span.clone()) + .drop_span(span) + .new_span( + span2.clone().with_field( + field::mock("arg1") + .with_value(&format_args!("3")) + .and(field::mock("arg2").with_value(&format_args!("true"))) + .only(), + ), + ) + .enter(span2.clone()) + .exit(span2.clone()) + .drop_span(span2) + .done() + .run_with_handle(); + + with_default(subscriber, || { + my_fn(2, false); + my_fn(3, true); + }); + + handle.assert_finished(); +} + +#[test] +fn skip() { + struct UnDebug(pub u32); + + #[instrument(target = "my_target", level = "debug", skip(_arg2, _arg3))] + fn my_fn(arg1: usize, _arg2: UnDebug, _arg3: UnDebug) {} + + let span = span::mock() + .named("my_fn") + .at_level(Level::DEBUG) + .with_target("my_target"); + + let span2 = span::mock() + .named("my_fn") + .at_level(Level::DEBUG) + .with_target("my_target"); + let (subscriber, handle) = subscriber::mock() + .new_span( + span.clone().with_field( + field::mock("arg1") + .with_value(&format_args!("2")) + .only(), ), ) .enter(span.clone()) @@ -72,7 +122,7 @@ fn fields() { span2.clone().with_field( field::mock("arg1") .with_value(&format_args!("3")) - .and(field::mock("arg2").with_value(&format_args!("true"))), + .only(), ), ) .enter(span2.clone()) @@ -82,8 +132,8 @@ fn fields() { .run_with_handle(); with_default(subscriber, || { - my_fn(2, false, UnDebug(0), UnDebug(1)); - my_fn(3, true, UnDebug(0), UnDebug(1)); + my_fn(2, UnDebug(0), UnDebug(1)); + my_fn(3, UnDebug(0), UnDebug(1)); }); handle.assert_finished(); From f91c1ce7c92ece705714f05c0d4487c67c262ae5 Mon Sep 17 00:00:00 2001 From: Henry de Valence Date: Mon, 23 Sep 2019 15:33:39 -0700 Subject: [PATCH 4/6] Update tracing-attributes/tests/instrument.rs Co-Authored-By: Eliza Weisman --- tracing-attributes/tests/instrument.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/tracing-attributes/tests/instrument.rs b/tracing-attributes/tests/instrument.rs index 10603dd5ee..8fd7f120f1 100644 --- a/tracing-attributes/tests/instrument.rs +++ b/tracing-attributes/tests/instrument.rs @@ -43,7 +43,6 @@ fn override_everything() { #[test] fn fields() { - struct UnDebug(pub u32); #[instrument(target = "my_target", level = "debug")] fn my_fn(arg1: usize, arg2: bool) {} From 5e8346b4ebc3377c7880808c635e870cb73436bd Mon Sep 17 00:00:00 2001 From: Henry de Valence Date: Mon, 23 Sep 2019 15:55:28 -0700 Subject: [PATCH 5/6] fmt --- tracing-attributes/tests/instrument.rs | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/tracing-attributes/tests/instrument.rs b/tracing-attributes/tests/instrument.rs index 8fd7f120f1..41af6f8d63 100644 --- a/tracing-attributes/tests/instrument.rs +++ b/tracing-attributes/tests/instrument.rs @@ -108,21 +108,16 @@ fn skip() { .with_target("my_target"); let (subscriber, handle) = subscriber::mock() .new_span( - span.clone().with_field( - field::mock("arg1") - .with_value(&format_args!("2")) - .only(), - ), + span.clone() + .with_field(field::mock("arg1").with_value(&format_args!("2")).only()), ) .enter(span.clone()) .exit(span.clone()) .drop_span(span) .new_span( - span2.clone().with_field( - field::mock("arg1") - .with_value(&format_args!("3")) - .only(), - ), + span2 + .clone() + .with_field(field::mock("arg1").with_value(&format_args!("3")).only()), ) .enter(span2.clone()) .exit(span2.clone()) From 11c3623de80bf4ce46bc5f975f28b30247899c51 Mon Sep 17 00:00:00 2001 From: Henry de Valence Date: Mon, 23 Sep 2019 16:04:25 -0700 Subject: [PATCH 6/6] fmt --- tracing-attributes/tests/instrument.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/tracing-attributes/tests/instrument.rs b/tracing-attributes/tests/instrument.rs index 41af6f8d63..2045a47f61 100644 --- a/tracing-attributes/tests/instrument.rs +++ b/tracing-attributes/tests/instrument.rs @@ -43,7 +43,6 @@ fn override_everything() { #[test] fn fields() { - #[instrument(target = "my_target", level = "debug")] fn my_fn(arg1: usize, arg2: bool) {}