attributes: implement #[instrument(tracing)]

[Tamschi]

This adds an optional tracing = Path parameter to the #[instrument] attribute, which can be used to override where the generated code looks for the tracing crate. This defaults to just quote!(tracing), as before.

Closes #1818.

Motivation

Please see the linked issue for details.

In short, this change makes #[instrument] more useful in proc macros (①¹) where tracing is available as optional dependency of the proc macro's library's main crate (②). With the override, ② can re-export tracing for use in ①'s output, which means library crates using ① via ② don't have to include tracing as optional dependency/feature.

¹ Sorry if this is odd. English isn't my first language and it's sometimes hard to use it unambiguously.

Solution

I tried to stay close to the style of the existing code.

So far, I've

• added the new parameter,
• defaulted it like target,
• updated the quotes to replace tracing with #tracing
  (and added the required let tracing = args.tracing(); lookup where possible).

To do:

• add the last args.tracing() lookup,
• write tests: (I think these cover all relevant code paths.)
  • plain,
  • async,
  • each level (both by str and int literal each)
    • and a Level by ident for good measure,
  • a quoted field through tracing::field::debug (i.e. implicit field with non-Value type),
  • format modes (default, Debug, Display) for ret and err each and
  • a custom field without value
• write documentation,
• ensure MSRV is as documented.

---

It's getting late here and I have a question regarding an implementation detail (I'll comment about that in a moment), so I'm making a draft PR for now.

[Tamschi]

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.

[Tamschi]

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.)

[hawkw]

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.

[davidbarsky]

I'll try to review this PR shortly, but looking over how you're generating components in Asteracea (to use proc-macro-definitions/src/component_declaration.rs as an example), you can mimic what tracing-attributes does to generate spans by creating a span within the function body by writing something like this instead of using the #[instrument] macro:

let _guard = tracing::info_span!(#new_span_name).enter();

I still think it's probably a good idea for us to figure out the hygiene issues with the #[instrument], however.

[Tamschi]

Thank you for the hint, I may eventually do that to keep the proc macro requirements low (or to work around the debug/display name collision issue).

[bryangarza]

@Tamschi is this PR something you will continue working on, or should we close?

[Tamschi]

@Tamschi is this PR something you will continue working on, or should we close?

Well I mean, I had completed this in January from my end. I was only waiting for a review.
(No problem though, I don't exactly have expectations to OSS projects in general. (Edit: Not sarcasm. It's totally fine.))

I can look into those conflicts though, one moment.

[Tamschi]

There. This should be good to merge again, but if anything's amiss please let me know!

[bryangarza]

Thanks for fixing the conflicts, we'll go through the code in the next few days.

[Tamschi]

I just resolved the two merge conflicts that had appeared at some point since May, so this should be ready to merge again.

(I wish there was a notification for this, that way I could have fixed it much earlier. Maybe I just missed it.)

[Tamschi]

New year, new attempt 😅

Yes, I'm still (very slowly) building my framework on top of this branch.
I actually hit a snag where I can't use the tracing::info_span! trick easily anymore, since I'm now conditionally emitting async component constructors. It would be great to see this land in that regard.

There is one clippy warning, but it's in a different crate untouched by this MR, so I didn't fix it on my end.