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

attributes: implement #[instrument(tracing)] #1819

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
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
95 changes: 77 additions & 18 deletions tracing-attributes/src/attr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ pub(crate) struct InstrumentArgs {
pub(crate) fields: Option<Fields>,
pub(crate) err_args: Option<EventArgs>,
pub(crate) ret_args: Option<EventArgs>,
tracing: Option<Path>,
/// Errors describing any unrecognized parse inputs that we skipped.
parse_warnings: Vec<syn::Error>,
}
Expand All @@ -42,6 +43,17 @@ impl InstrumentArgs {
}
}

/// The location of the `tracing` crate.
///
/// For backwards-compatibility, this must default to the plain `tracing` identifier with call site resolution.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not entirely sure if this should go into the doc comment, on second thought. A comment above the quote!(tracing) may be more apt.

I'd actually like to suggest adding one of those breaking change comments there: A default of quote!(::tracing) would avoid collisions with consumer-defined names just about entirely by default, but unfortunately it would also break the "hidden override" that's theoretically available right now.

pub(crate) fn tracing(&self) -> impl ToTokens {
if let Some(ref tracing) = self.tracing {
quote!(#tracing)
} else {
quote!(tracing)
}
}

/// Generate "deprecation" warnings for any unrecognized attribute inputs
/// that we skipped.
///
Expand Down Expand Up @@ -134,6 +146,14 @@ impl Parse for InstrumentArgs {
let _ = input.parse::<kw::ret>()?;
let ret_args = EventArgs::parse(input)?;
args.ret_args = Some(ret_args);
} else if lookahead.peek(kw::tracing) {
if args.tracing.is_some() {
return Err(input.error("expected only a single `tracing` argument"));
}
let _ = input.parse::<kw::tracing>()?;
let _ = input.parse::<Token![=]>()?;
let tracing = Path::parse(input)?;
args.tracing = Some(tracing);
} else if lookahead.peek(Token![,]) {
let _ = input.parse::<Token![,]>()?;
} else {
Expand Down Expand Up @@ -306,6 +326,11 @@ pub(crate) enum FieldKind {
Value,
}

pub(crate) struct WithArgs<'a, T> {
value: &'a T,
args: &'a InstrumentArgs,
}

impl Parse for Fields {
fn parse(input: ParseStream<'_>) -> syn::Result<Self> {
let _ = input.parse::<kw::fields>();
Expand All @@ -316,9 +341,18 @@ impl Parse for Fields {
}
}

impl ToTokens for Fields {
impl Fields {
pub(crate) fn with_args<'a>(&'a self, args: &'a InstrumentArgs) -> WithArgs<'a, Self> {
WithArgs { value: self, args }
}
}

impl ToTokens for WithArgs<'_, Fields> {
fn to_tokens(&self, tokens: &mut TokenStream) {
self.0.to_tokens(tokens)
for pair in self.value.0.pairs() {
pair.value().with_args(self.args).to_tokens(tokens);
pair.punct().to_tokens(tokens);
}
}
}

Expand Down Expand Up @@ -350,25 +384,34 @@ impl Parse for Field {
}
}

impl ToTokens for Field {
impl Field {
fn with_args<'a>(&'a self, args: &'a InstrumentArgs) -> WithArgs<'a, Self> {
WithArgs { value: self, args }
}
}

impl ToTokens for WithArgs<'_, Field> {
fn to_tokens(&self, tokens: &mut TokenStream) {
if let Some(ref value) = self.value {
let name = &self.name;
let kind = &self.kind;
let field = self.value;

if let Some(ref value) = field.value {
let name = &field.name;
let kind = &field.kind;
tokens.extend(quote! {
#name = #kind #value
})
} else if self.kind == FieldKind::Value {
} else if field.kind == FieldKind::Value {
// XXX(eliza): I don't like that fields without values produce
// empty fields rather than local variable shorthand...but,
// we've released a version where field names without values in
// `instrument` produce empty field values, so changing it now
// is a breaking change. agh.
let name = &self.name;
tokens.extend(quote!(#name = tracing::field::Empty))
let name = &field.name;
let tracing = self.args.tracing();
tokens.extend(quote!(#name = #tracing::field::Empty))
} else {
self.kind.to_tokens(tokens);
self.name.to_tokens(tokens);
field.kind.to_tokens(tokens);
field.name.to_tokens(tokens);
}
}
}
Expand Down Expand Up @@ -438,14 +481,29 @@ impl Parse for Level {
}
}

impl ToTokens for Level {
impl Level {
pub(crate) fn with_tracing<T: ToTokens>(self, tracing: T) -> LevelWithTracing<T> {
LevelWithTracing {
level: self,
tracing,
}
}
}

pub(crate) struct LevelWithTracing<T: ToTokens> {
level: Level,
tracing: T,
}

impl<T: ToTokens> ToTokens for LevelWithTracing<T> {
fn to_tokens(&self, tokens: &mut TokenStream) {
match self {
Level::Trace => tokens.extend(quote!(tracing::Level::TRACE)),
Level::Debug => tokens.extend(quote!(tracing::Level::DEBUG)),
Level::Info => tokens.extend(quote!(tracing::Level::INFO)),
Level::Warn => tokens.extend(quote!(tracing::Level::WARN)),
Level::Error => tokens.extend(quote!(tracing::Level::ERROR)),
let tracing = &self.tracing;
match self.level {
Level::Trace => tokens.extend(quote!(#tracing::Level::TRACE)),
Level::Debug => tokens.extend(quote!(#tracing::Level::DEBUG)),
Level::Info => tokens.extend(quote!(#tracing::Level::INFO)),
Level::Warn => tokens.extend(quote!(#tracing::Level::WARN)),
Level::Error => tokens.extend(quote!(#tracing::Level::ERROR)),
Level::Path(ref pat) => tokens.extend(quote!(#pat)),
}
}
Expand All @@ -461,4 +519,5 @@ mod kw {
syn::custom_keyword!(name);
syn::custom_keyword!(err);
syn::custom_keyword!(ret);
syn::custom_keyword!(tracing);
}
25 changes: 13 additions & 12 deletions tracing-attributes/src/expand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,9 @@ fn gen_block<B: ToTokens>(
.map(|name| quote!(#name))
.unwrap_or_else(|| quote!(#instrumented_function_name));

let tracing = args.tracing();
let args_level = args.level();
let level = args_level.clone();
let level = args_level.clone().with_tracing(&tracing);

let follows_from = args.follows_from.iter();
let follows_from = quote! {
Expand Down Expand Up @@ -199,7 +200,7 @@ fn gen_block<B: ToTokens>(
})
.map(|(user_name, (real_name, record_type))| match record_type {
RecordType::Value => quote!(#user_name = #real_name),
RecordType::Debug => quote!(#user_name = tracing::field::debug(&#real_name)),
RecordType::Debug => quote!(#user_name = #tracing::field::debug(&#real_name)),
})
.collect();

Expand All @@ -221,9 +222,9 @@ fn gen_block<B: ToTokens>(
}
}

let custom_fields = &args.fields;
let custom_fields = args.fields.as_ref().map(|fields| fields.with_args(&args));

quote!(tracing::span!(
quote!(#tracing::span!(
target: #target,
#(parent: #parent,)*
#level,
Expand All @@ -238,13 +239,13 @@ fn gen_block<B: ToTokens>(

let err_event = match args.err_args {
Some(event_args) => {
let level_tokens = event_args.level(Level::Error);
let level_tokens = event_args.level(Level::Error).with_tracing(&tracing);
match event_args.mode {
FormatMode::Default | FormatMode::Display => Some(quote!(
tracing::event!(target: #target, #level_tokens, error = %e)
#tracing::event!(target: #target, #level_tokens, error = %e)
)),
FormatMode::Debug => Some(quote!(
tracing::event!(target: #target, #level_tokens, error = ?e)
#tracing::event!(target: #target, #level_tokens, error = ?e)
)),
}
}
Expand All @@ -253,13 +254,13 @@ fn gen_block<B: ToTokens>(

let ret_event = match args.ret_args {
Some(event_args) => {
let level_tokens = event_args.level(args_level);
let level_tokens = event_args.level(args_level).with_tracing(&tracing);
match event_args.mode {
FormatMode::Display => Some(quote!(
tracing::event!(target: #target, #level_tokens, return = %x)
#tracing::event!(target: #target, #level_tokens, return = %x)
)),
FormatMode::Default | FormatMode::Debug => Some(quote!(
tracing::event!(target: #target, #level_tokens, return = ?x)
#tracing::event!(target: #target, #level_tokens, return = ?x)
)),
}
}
Expand Down Expand Up @@ -320,7 +321,7 @@ fn gen_block<B: ToTokens>(
let __tracing_instrument_future = #mk_fut;
if !__tracing_attr_span.is_disabled() {
#follows_from
tracing::Instrument::instrument(
#tracing::Instrument::instrument(
__tracing_instrument_future,
__tracing_attr_span
)
Expand All @@ -344,7 +345,7 @@ fn gen_block<B: ToTokens>(
// regression in case the level is enabled.
let __tracing_attr_span;
let __tracing_attr_guard;
if tracing::level_enabled!(#level) || tracing::if_log_enabled!(#level, {true} else {false}) {
if #tracing::level_enabled!(#level) || #tracing::if_log_enabled!(#level, {true} else {false}) {
__tracing_attr_span = #span;
#follows_from
__tracing_attr_guard = __tracing_attr_span.enter();
Expand Down
16 changes: 16 additions & 0 deletions tracing-attributes/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,9 @@ mod expand;
/// Note that overlap between the names of fields and (non-skipped) arguments
/// will result in a compile error.
///
/// Also note that this macro will look for [`tracing`] locally with call site resolution by default.
/// Specify `tracing = ::path::to::tracing` to override.
///
/// # Examples
/// Instrumenting a function:
/// ```
Expand Down Expand Up @@ -310,6 +313,19 @@ mod expand;
/// }
/// ```
///
/// To resolve [`tracing`] at a specific path, assign it to `tracing`:
///
/// ```
/// # use tracing_attributes::instrument;
/// /// Shadows [`tracing`](`::tracing`).
/// mod tracing {}
///
/// #[instrument(tracing = ::tracing)]
/// fn my_function() {
/// // ...
/// }
/// ```
///
/// `async fn`s may also be instrumented:
///
/// ```
Expand Down
76 changes: 76 additions & 0 deletions tracing-attributes/tests/tracing_override.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
//! Ensures that all unqualified uses of `tracing` can be overridden.
//!
//! As compilation fails directly when `tracing` is shadowed, no test functions are run here.

use ::tracing::instrument;

/// Shadows the crate `tracing` locally.
#[allow(dead_code)]
mod tracing {}

#[instrument(tracing = ::tracing)]
pub fn plain() {}

#[instrument(tracing = ::tracing)]
pub async fn async_fn() {}

#[derive(Debug)]
pub struct Custom;

#[instrument(tracing = ::tracing)]
pub fn implicit_field_debug(custom: Custom) {}

#[instrument(tracing = ::tracing, level = "error")]
pub fn level_error() {}
#[instrument(tracing = ::tracing, level = "warn")]
pub fn level_warn() {}
#[instrument(tracing = ::tracing, level = "info")]
pub fn level_info() {}
#[instrument(tracing = ::tracing, level = "debug")]
pub fn level_debug() {}
#[instrument(tracing = ::tracing, level = "trace")]
pub fn level_trace() {}

#[instrument(tracing = ::tracing, level = 5)]
pub fn level_5() {}
#[instrument(tracing = ::tracing, level = 4)]
pub fn level_4() {}
#[instrument(tracing = ::tracing, level = 3)]
pub fn level_3() {}
#[instrument(tracing = ::tracing, level = 2)]
pub fn level_2() {}
#[instrument(tracing = ::tracing, level = 1)]
pub fn level_1() {}

const A_LEVEL: ::tracing::Level = ::tracing::Level::INFO;
#[instrument(tracing = ::tracing, level = A_LEVEL)]
pub fn level_ident() {}
Comment on lines +34 to +47
Copy link
Contributor Author

@Tamschi Tamschi Jan 8, 2022

Choose a reason for hiding this comment

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

The ability to specify the level through an integer constant or Level-identifier seems to be undocumented, as far as the immediate ///-documentation on #[instrument] goes. Should I document it or is this deprecated/out of scope for this PR?

(I'd still like to keep these tests here regardless of the answer to that question, since they all have distinct code paths.)

Copy link
Member

Choose a reason for hiding this comment

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

It should probably be documented, but in a follow-up PR. We should probably document it on the Level type as well, since I believe its FromStr implementation will also accept numbers.


#[::tracing::instrument(tracing = ::tracing, fields(empty))]
pub fn empty() {}

#[::tracing::instrument(tracing = ::tracing, ret)]
pub fn ret() {}

#[instrument(tracing = ::tracing, ret(Debug))]
pub fn ret_debug() {}

#[instrument(tracing = ::tracing, ret(Display))]
pub fn ret_display() -> &'static str {
""
}

#[instrument(tracing = ::tracing, err)]
pub fn err() -> Result<(), &'static str> {
Ok(())
}

#[instrument(tracing = ::tracing, err(Debug))]
pub fn err_debug() -> Result<(), &'static str> {
Ok(())
}

#[instrument(tracing = ::tracing, err(Display))]
pub fn err_display() -> Result<(), &'static str> {
Ok(())
}
Loading