-
Notifications
You must be signed in to change notification settings - Fork 754
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: add err(Debug)
meta to use Debug
impl
#1631
Changes from 2 commits
b8d94fe
83e030a
0bfac9f
5768a50
db5a161
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -393,7 +393,6 @@ fn gen_block( | |||||||||||||||||
instrumented_function_name: &str, | ||||||||||||||||||
self_type: Option<&syn::TypePath>, | ||||||||||||||||||
) -> proc_macro2::TokenStream { | ||||||||||||||||||
let err = args.err; | ||||||||||||||||||
|
||||||||||||||||||
// generate the span's name | ||||||||||||||||||
let span_name = args | ||||||||||||||||||
|
@@ -507,29 +506,34 @@ fn gen_block( | |||||||||||||||||
)) | ||||||||||||||||||
})(); | ||||||||||||||||||
|
||||||||||||||||||
let err_block = match args.err_mode { | ||||||||||||||||||
Some(ErrorMode::Display) => Some(quote!(tracing::error!(error = %e))), | ||||||||||||||||||
Some(ErrorMode::Debug) => Some(quote!(tracing::error!(error = ?e))), | ||||||||||||||||||
_ => None, | ||||||||||||||||||
}; | ||||||||||||||||||
|
||||||||||||||||||
// Generate the instrumented function body. | ||||||||||||||||||
// If the function is an `async fn`, this will wrap it in an async block, | ||||||||||||||||||
// which is `instrument`ed using `tracing-futures`. Otherwise, this will | ||||||||||||||||||
// enter the span and then perform the rest of the body. | ||||||||||||||||||
// If `err` is in args, instrument any resulting `Err`s. | ||||||||||||||||||
if async_context { | ||||||||||||||||||
let mk_fut = if err { | ||||||||||||||||||
quote_spanned!(block.span()=> | ||||||||||||||||||
let mk_fut = match err_block { | ||||||||||||||||||
Some(err_block) => quote_spanned!(block.span()=> | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if you apply my naming suggestion from above, we would also need to change this
Suggested change
Suggested change
|
||||||||||||||||||
async move { | ||||||||||||||||||
match async move { #block }.await { | ||||||||||||||||||
#[allow(clippy::unit_arg)] | ||||||||||||||||||
Ok(x) => Ok(x), | ||||||||||||||||||
Err(e) => { | ||||||||||||||||||
tracing::error!(error = %e); | ||||||||||||||||||
#err_block; | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ...and we'd change this as well
Suggested change
|
||||||||||||||||||
Err(e) | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
) | ||||||||||||||||||
} else { | ||||||||||||||||||
quote_spanned!(block.span()=> | ||||||||||||||||||
), | ||||||||||||||||||
None => quote_spanned!(block.span()=> | ||||||||||||||||||
async move { #block } | ||||||||||||||||||
) | ||||||||||||||||||
), | ||||||||||||||||||
}; | ||||||||||||||||||
|
||||||||||||||||||
return quote!( | ||||||||||||||||||
|
@@ -566,15 +570,15 @@ fn gen_block( | |||||||||||||||||
} | ||||||||||||||||||
); | ||||||||||||||||||
|
||||||||||||||||||
if err { | ||||||||||||||||||
if let Some(err_block) = err_block { | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ...and we'd change this
Suggested change
|
||||||||||||||||||
return quote_spanned!(block.span()=> | ||||||||||||||||||
#span | ||||||||||||||||||
#[allow(clippy::redundant_closure_call)] | ||||||||||||||||||
match (move || #block)() { | ||||||||||||||||||
#[allow(clippy::unit_arg)] | ||||||||||||||||||
Ok(x) => Ok(x), | ||||||||||||||||||
Err(e) => { | ||||||||||||||||||
tracing::error!(error = %e); | ||||||||||||||||||
#err_block; | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||
Err(e) | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
|
@@ -603,7 +607,7 @@ struct InstrumentArgs { | |||||||||||||||||
target: Option<LitStr>, | ||||||||||||||||||
skips: HashSet<Ident>, | ||||||||||||||||||
fields: Option<Fields>, | ||||||||||||||||||
err: bool, | ||||||||||||||||||
err_mode: Option<ErrorMode>, | ||||||||||||||||||
/// Errors describing any unrecognized parse inputs that we skipped. | ||||||||||||||||||
parse_warnings: Vec<syn::Error>, | ||||||||||||||||||
} | ||||||||||||||||||
|
@@ -728,8 +732,8 @@ impl Parse for InstrumentArgs { | |||||||||||||||||
} | ||||||||||||||||||
args.fields = Some(input.parse()?); | ||||||||||||||||||
} else if lookahead.peek(kw::err) { | ||||||||||||||||||
let _ = input.parse::<kw::err>()?; | ||||||||||||||||||
args.err = true; | ||||||||||||||||||
let ErrorModes(mode) = input.parse()?; | ||||||||||||||||||
args.err_mode = Some(mode); | ||||||||||||||||||
} else if lookahead.peek(Token![,]) { | ||||||||||||||||||
let _ = input.parse::<Token![,]>()?; | ||||||||||||||||||
} else { | ||||||||||||||||||
|
@@ -787,6 +791,55 @@ impl Parse for Skips { | |||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
#[derive(Debug, Hash, PartialEq, Eq)] | ||||||||||||||||||
enum ErrorMode { | ||||||||||||||||||
Display, | ||||||||||||||||||
Debug, | ||||||||||||||||||
} | ||||||||||||||||||
impl Default for ErrorMode { | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: prefer a line of whitespace between definitions & impl blocks:
Suggested change
|
||||||||||||||||||
fn default() -> Self { | ||||||||||||||||||
ErrorMode::Display | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
impl Parse for ErrorMode { | ||||||||||||||||||
fn parse(input: ParseStream<'_>) -> syn::Result<Self> { | ||||||||||||||||||
let ident: Ident = input.parse()?; | ||||||||||||||||||
match ident.to_string().as_str() { | ||||||||||||||||||
"Debug" => Ok(ErrorMode::Debug), | ||||||||||||||||||
"Display" => Ok(ErrorMode::Display), | ||||||||||||||||||
_ => Err(syn::Error::new( | ||||||||||||||||||
ident.span(), | ||||||||||||||||||
"unknown error mode, must be Debug or Display", | ||||||||||||||||||
)), | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
struct ErrorModes(ErrorMode); | ||||||||||||||||||
|
||||||||||||||||||
impl Parse for ErrorModes { | ||||||||||||||||||
|
||||||||||||||||||
fn parse(input: ParseStream<'_>) -> syn::Result<Self> { | ||||||||||||||||||
let _ = input.parse::<kw::err>(); | ||||||||||||||||||
if input.peek(syn::token::Paren) { | ||||||||||||||||||
let content; | ||||||||||||||||||
let _ = syn::parenthesized!(content in input); | ||||||||||||||||||
let modes: Punctuated<ErrorMode, Token![,]> = content.parse_terminated(ErrorMode::parse)?; | ||||||||||||||||||
let modes = modes.into_iter().collect::<HashSet<_>>(); | ||||||||||||||||||
return match modes.len() { | ||||||||||||||||||
0 => Ok(Self(ErrorMode::Display)), | ||||||||||||||||||
1 => Ok(Self(modes.into_iter().next().unwrap())), | ||||||||||||||||||
_ => Err(syn::Error::new( | ||||||||||||||||||
content.span(), | ||||||||||||||||||
"at most one of Debug or Display can be given", | ||||||||||||||||||
)) | ||||||||||||||||||
} | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can simplify this a bit --- it shouldn't be necessary to use I believe that should allow us to replace the This should hopefully be much simpler! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I might have misunderstood but, I couldn't implement I went ahead and utilized Overall this looks simpler, but let me know if I further can improve it. |
||||||||||||||||||
} | ||||||||||||||||||
Ok(Self(ErrorMode::Display)) | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
#[derive(Debug)] | ||||||||||||||||||
struct Fields(Punctuated<Field, Token![,]>); | ||||||||||||||||||
|
||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -174,3 +174,23 @@ fn impl_trait_return_type() { | |||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
handle.assert_finished(); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
#[instrument(err(Debug))] | ||||||||||||||||||||||||||||
fn err_dbg() -> Result<u8, TryFromIntError> { | ||||||||||||||||||||||||||||
u8::try_from(1234) | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
#[test] | ||||||||||||||||||||||||||||
fn test_err_dbg() { | ||||||||||||||||||||||||||||
let span = span::mock().named("err_dbg"); | ||||||||||||||||||||||||||||
let (collector, handle) = collector::mock() | ||||||||||||||||||||||||||||
.new_span(span.clone()) | ||||||||||||||||||||||||||||
.enter(span.clone()) | ||||||||||||||||||||||||||||
.event(event::mock().at_level(Level::ERROR)) | ||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks for adding the test! it might be nice to change this line to also expect that the we could do that like this:
Suggested change
but, the other tests don't make assertions about the formatted value of the error, so this isn't strictly necessary --- I just thought it might be nice. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perfect! Honestly I didn't know what I was doing, and just copied one of the tests. This is what I was looking for. 👍 Will apply. |
||||||||||||||||||||||||||||
.exit(span.clone()) | ||||||||||||||||||||||||||||
.drop_span(span) | ||||||||||||||||||||||||||||
.done() | ||||||||||||||||||||||||||||
.run_with_handle(); | ||||||||||||||||||||||||||||
with_default(collector, || err_dbg().ok()); | ||||||||||||||||||||||||||||
handle.assert_finished(); | ||||||||||||||||||||||||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, take it or leave it: not sure if
err_block
is the best name for this; it's not actually a block, just an expression. maybe