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 19 commits into
base: master
Choose a base branch
from

Conversation

Tamschi
Copy link
Contributor

@Tamschi Tamschi commented Jan 5, 2022

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.

@@ -68,6 +70,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.

@Tamschi Tamschi force-pushed the instrument-tracing-override branch from d331d95 to fabca1a Compare January 8, 2022 15:37
…ent the former

This doesn't technically make the parameter more narrow, as the generated code would fail to compile with any other sort of path due to macro calls on it, but it maybe makes some error messages better.
Comment on lines +34 to +47
#[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() {}
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.

@Tamschi Tamschi marked this pull request as ready for review January 8, 2022 20:37
@Tamschi Tamschi requested review from davidbarsky, hawkw and a team as code owners January 8, 2022 20:37
@davidbarsky
Copy link
Member

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
Copy link
Contributor Author

Tamschi commented Jan 11, 2022

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

Tamschi added a commit to Tamschi/Asteracea that referenced this pull request Jan 14, 2022
…o instead of using `#[instrument]`

This skips waiting for <tokio-rs/tracing#1819> to land and should also compile a bit faster in some cases.
@bryangarza
Copy link
Member

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

@Tamschi
Copy link
Contributor Author

Tamschi commented May 9, 2022

@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
Copy link
Contributor Author

Tamschi commented May 9, 2022

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

@bryangarza
Copy link
Member

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

@bryangarza bryangarza self-requested a review May 9, 2022 23:25
@Tamschi
Copy link
Contributor Author

Tamschi commented Sep 24, 2022

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
Copy link
Contributor Author

Tamschi commented Apr 1, 2023

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.

@benluelo
Copy link

any progress here? this is currently required for any crate that tries to reexport tracing (generated code trying to reexport everything under one crate, etc)

@Tamschi
Copy link
Contributor Author

Tamschi commented Sep 23, 2024

I just resolved the new merge conflict (and a copy-paste mistake in an error message that I'd missed).

But yeah, this would still be good to get merged. I've added support for coloured async to Asteracea since, and supporting tracing for that without relying on tracing-attributes would be cumbersome.

any progress here? this is currently required for any crate that tries to reexport tracing (generated code trying to reexport everything under one crate, etc)

The implementation here has been complete for two and a half years, though I've had to fix merge conflicts occasionally.
(I'm not notified when one occurs due to the master branch moving ahead. I check back here occasionally if there's any updates.)

Maybe this fifth opportunity's a charm to get a review for it 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tracing override for #[instrument]
5 participants