#[instrument] - support use of the 'self' variable in async-trait contexts #875
Conversation
nightmared
changed the title
This comment has been minimized.
This comment has been minimized.
|
The CI failure is not related to this change, sorry about that. We just merged PR #882 that should fix the CI failure. Mind rebasing onto master? |
There was a problem hiding this comment.
Overall, this looks pretty good — I had some style suggestions.
I wrote up a reply to your questions about handling Self types on functions without a self parameter here: #864 (comment). Let me know what you think?
tracing-attributes/src/lib.rs
Outdated
| @@ -211,20 +211,21 @@ pub fn instrument( | |||
| let input: ItemFn = syn::parse_macro_input!(item as ItemFn); | |||
| let args = syn::parse_macro_input!(args as InstrumentArgs); | |||
|
|
|||
| let span_declared_name = input.sig.ident.to_string(); | |||
There was a problem hiding this comment.
This naming is not super clear to me...also, this is the name of the instrumented function, and not necessarily the name of the span itself: it may be overridden with name = "something_else" in the macro's arguments.
tracing-attributes/src/lib.rs
Outdated
| if let Some(async_trait_fun) = async_trait_fun { | ||
| if let Some(Fields(ref mut fields)) = args.fields { | ||
| for mut field in fields.iter_mut() { | ||
| if let Some(ref mut e) = field.value { | ||
| syn::visit_mut::visit_expr_mut( | ||
| &mut SelfReplacer { | ||
| ty: async_trait_fun.self_type.clone(), | ||
| }, | ||
| e, | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Take it or leave it: I think we can express this with a little less rightward drift, like this:
| if let Some(async_trait_fun) = async_trait_fun { | |
| if let Some(Fields(ref mut fields)) = args.fields { | |
| for mut field in fields.iter_mut() { | |
| if let Some(ref mut e) = field.value { | |
| syn::visit_mut::visit_expr_mut( | |
| &mut SelfReplacer { | |
| ty: async_trait_fun.self_type.clone(), | |
| }, | |
| e, | |
| ); | |
| } | |
| } | |
| } | |
| } | |
| if let (Some(async_trait_fun), Some(Fields(ref mut fields))) = (async_trait_fun, args.fields) { | |
| let mut replacer = SelfReplacer { | |
| ty: async_trait_fun.self_type.clone(), | |
| }; | |
| for e in fields.iter_mut().filter_map(|f| f.value.as_mut()) { | |
| syn::visit_mut::visit_expr_mut(&mut replacer, e); | |
| } | |
| } |
| #[instrument(fields(Self=std::any::type_name::<Self>()))] | ||
| async fn call_with_self(&self) {} | ||
|
|
||
| #[instrument(fields(Self=std::any::type_name::<Self>()))] | ||
| async fn call_with_mut_self(self: &mut Self) {} |
There was a problem hiding this comment.
this is great, thanks for testing this.
|
Turns out I was wrong, it's not even possible (as far as I can tell) to obtain that information through a type alias, as there is no valid place where we can put the type alias: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=5c0d0f71aa9d8fc0f18fa45d4987ff72 :/ Any other idea ? Otherwise we will be forced to accept that in some cases the user have to supply the |
Ugh, yeah, you're right...it looks like this may not be possible no matter what we do. Let's make sure that's documented. I don't think it's a terribly common case, at least. |
Several minor documentation edits.
|
Nice ;) |
No problem at all! |
Added - Support for using `self` in field expressions when instrumenting `async-trait` functions (#875) - Several documentation improvements (#832, #897, #911, #913) Thanks to @anton-dutov and @nightmared for contributing to this release!
### Added - Support for using `self` in field expressions when instrumenting `async-trait` functions (#875) - Several documentation improvements (#832, #897, #911, #913) Thanks to @anton-dutov and @nightmared for contributing to this release!
Fixed - Updated `tracing-core` to fix incorrect calculation of the global max level filter (#908) Added - **attributes**: Support for using `self` in field expressions when instrumenting `async-trait` functions (#875) - Several documentation improvements (#832, #881, #896, #897, #911, #913) Thanks to @anton-dutov, @nightmared, @mystor, and @toshokan for contributing to this release!
### Fixed - Updated `tracing-core` to fix incorrect calculation of the global max level filter (#908) ### Added - **attributes**: Support for using `self` in field expressions when instrumenting `async-trait` functions (#875) - Several documentation improvements (#832, #881, #896, #897, #911, #913) Thanks to @anton-dutov, @nightmared, @mystor, and @toshokan for contributing to this release! Signed-off-by: Eliza Weisman <eliza@buoyant.io>
PR #875 added code to `tracing-attributes` that uses `Option::flatten`, which was only added to the standard library in Rust 1.40. This broke our MSRV, but we missed it because the MSRV CI checks weren't working correctly (fixed in #934). This commit removes the use of `Option::flatten`, and replaces it with a manual implementation. It's a little less pretty, but it builds on our MSRV (1.39.0).
PR #875 added code to `tracing-attributes` that uses `Option::flatten`, which was only added to the standard library in Rust 1.40. This broke our MSRV, but we missed it because the MSRV CI checks weren't working correctly (fixed in #934). This commit removes the use of `Option::flatten`, and replaces it with a manual implementation. It's a little less pretty, but it builds on our MSRV (1.39.0).
Motivation
Fix #864.
Solution
Visit the fields and replace 'self' with '_self', and 'Self' with the type of the impl (when we can acces it, see #864 (comment) for current limitations).