tracing: Instrument std::future::Future in tracing #808

davidbarsky · 2020-07-12T01:26:23Z

This branch introduces tracing::Instrument, a trait borrowed from tracing-futures. Instrument is implemented on std::future::Future, allowing tracing users instrument async functions and futures with spans without needing to depend on tracing-futures. Additionally, tracing_macros::instrument macro will now just work when used with async functions.

Few notes:

tracing depend depends on pin-project-lite, inline with just as Tokio does.
Before merging this branch, I'd like to look over the docs to ensure that inaccurate statements along the lines of "use tracing-futures for std::future::Future" are removed. However, since some of these changes would refer to as-of-yet non-existent links on docs.rs, I'd like to get this shipped as quickly as possible.

…strument

…lankets.

hawkw · 2020-07-12T16:44:46Z

* Should `tracing` depend on `pin-project` or `pin-project-lite`? While `tracing` compiles syn and quote by default, `tracing`'s [proposed inclusion](https://github.com/tokio-rs/tokio/pull/2655) into Tokio might become problematic.

If we don't need any of the fancier stuff from pin-project (and I don't believe we do, we just need basic field projection), I think it's better to use pin-project-lite, IMO. Remember that the syn/quote dependency can be disabled by disabling the attributes feature flag...

hawkw

This looks basically right. I commented on a few things.

Also, note that my proposal for making Span::in_scope work with async blocks as well as closures (PR #734) requires some changes to the Instrument trait:

tracing/tracing/src/span.rs

Lines 317 to 320 in 85995a5

    
           pub trait Instrument<I: Instrumented> { 
        
               fn instrument(self, span: &Span) -> I::Output 
        
               where 
        
                   Self: Sized,

If we decide to make this change, we may want to consider making the API changes in Instrument necessary for this idea to work before we publish a version of tracing with the Instrument trait, so we don't need to break it later...

tracing/src/instrument.rs

hawkw · 2020-07-20T21:34:12Z

tracing/src/instrument.rs

+    /// [`Subscriber`]: https://docs.rs/tracing/latest/tracing/subscriber/trait.Subscriber.html
+    /// [default]: https://docs.rs/tracing/latest/tracing/dispatcher/index.html#setting-the-default-subscriber


probably should be in-crate links

hawkw · 2020-07-20T21:34:27Z

tracing/src/instrument.rs

+    /// [`Subscriber`]: https://docs.rs/tracing/latest/tracing/subscriber/trait.Subscriber.html
+    /// [default]: https://docs.rs/tracing/latest/tracing/dispatcher/index.html#setting-the-default-subscriber


probably should be in-crate links.

tracing/src/instrument.rs

hawkw · 2020-07-20T21:36:59Z

tracing/src/instrument.rs

+/// a `tracing` [`Subscriber`].
+///
+/// [`Subscriber`]: https://docs.rs/tracing/latest/tracing/subscriber/trait.Subscriber.html
+pub trait WithSubscriber: Sized {


i'm on the fence about whether or not this needs to be in tracing proper. it isn't a whole lot of code, but I also think it's probably fine for it to stay in tracing-futures...

Co-authored-by: Eliza Weisman <eliza@buoyant.io>

davidbarsky · 2020-07-21T16:57:11Z

Resolved the in-crate links, but I'm not sure how to link to an HTML section using intra-doc links in Rustdoc.

I'm a little on the fence on the instrument trait change, but I don't feel too strongly.

davidbarsky · 2020-07-21T16:58:40Z

Actually, let's do the change. Do you want to land it as part of your (rebased) PR or in this PR?

hawkw · 2020-07-22T16:56:11Z

examples/examples/echo.rs

+        .instrument(info_span!("echo", %peer_addr))
+        .await?;


hawkw · 2020-07-22T17:55:47Z

Windows CI failure for 2c4f3d1 was definitely not your fault:

error: could not download file from 'https://static.rust-lang.org/rustup/release-stable.toml' to 'C:\Users\RUNNER~1\AppData\Local\Temp\rustup-updateMwhrLq\release-stable.toml'
error: caused by: failed to make network request
error: caused by: error sending request for url (https://static.rust-lang.org/rustup/release-stable.toml): connection error: An existing connection was forcibly closed by the remote host. (os error 10054)
error: caused by: connection error: An existing connection was forcibly closed by the remote host. (os error 10054)
error: caused by: An existing connection was forcibly closed by the remote host. (os error 10054)

I restarted it.

hawkw · 2020-07-22T17:59:01Z

Actually, let's do the change. Do you want to land it as part of your (rebased) PR or in this PR?

Hmm.

Let's not make the change to the in_scope function as part of this PR --- I think it might be considered semver breaking, since it changes a function signature. It won't break callers of the function, since any previously-accepted input types would still be accepted, but it changes the actual signature of the function, which would potentially be an issue if it was e.g. being used as a function pointer.

We can land the version of the Instrument trait as proposed in that PR as part of this branch, since it's an additive change. If you like, I can open a PR against this branch to make those changes here?

hawkw · 2020-07-22T18:01:18Z

Let's not make the change to the in_scope function as part of this PR --- I think it might be considered semver breaking, since it changes a function signature. It won't break callers of the function, since any previously-accepted input types would still be accepted, but it changes the actual signature of the function, which would potentially be an issue if it was e.g. being used as a function pointer.

A potential option for making the whole change without breaking is to just add a new Span method for the version of in_scope that also works for async blocks, under a new name, and (potentially) deprecating in_scope in favor of that new method. The downside of that is that we would have to think of a better name than in_scope...

davidbarsky · 2020-07-22T18:55:44Z

The proposed changes feels like a breaking change due to the signature change and I'd rather be conservative with these changes. Since we can intend to ship tracing 0.2 soon (TM), I'm okay with bundling that change into that release instead of this PR.

hawkw · 2020-07-22T20:27:30Z

The proposed changes feels like a breaking change due to the signature change and I'd rather be conservative with these changes. Since we can intend to ship tracing 0.2 soon (TM), I'm okay with bundling that change into that release instead of this PR.

Okay, that seems fine to me. Let's just make sure that we land the Instrument trait in a form that will allow us to make that change without breaking Instrument (e.g.: land the Instrument trait from that PR).

LegNeato · 2020-09-03T08:07:34Z

Any update on this?

hawkw · 2020-09-24T22:46:45Z

tracing/src/lib.rs

+/// Attach a span to a `std::future::Future`.
+pub mod instrument;


IDK if I really think this needs its own module, I might either reexport it from the root and make the instrument module hidden, or put it in the span module. but, i don't really care.

@nagisa

Fixed - Incorrect inlining of `Span::new`, `Span::new_root`, and `Span::new_child_of`, which could result in `dispatcher::get_default` being inlined at the callsite ([#994]) - Regression where using a struct field as a span or event field when other fields on that struct are borrowed mutably would fail to compile ([#987]) Changed - Updated `tracing-core` to 0.1.17 ([#992]) Added - `Instrument` trait and `Instrumented` type for attaching a `Span` to a `Future` ([#808]) - `Copy` implementations for `Level` and `LevelFilter` ([#992]) - Multiple documentation fixes and improvements ([#964], [#980], [#981]) Thanks to @nagisa, and new contributors @securityinsanity, @froydnj, @jyn514 and @TaKO8Ki for contributing to this release! [#994]: #994 [#992]: #992 [#987]: #987 [#980]: #980 [#981]: #981 [#964]: #964 [#808]: #808 Signed-off-by: Eliza Weisman <eliza@buoyant.io>

@nagisa

### Fixed - Incorrect inlining of `Span::new`, `Span::new_root`, and `Span::new_child_of`, which could result in `dispatcher::get_default` being inlined at the callsite ([#994]) - Regression where using a struct field as a span or event field when other fields on that struct are borrowed mutably would fail to compile ([#987]) ### Changed - Updated `tracing-core` to 0.1.17 ([#992]) ### Added - `Instrument` trait and `Instrumented` type for attaching a `Span` to a `Future` ([#808]) - `Copy` implementations for `Level` and `LevelFilter` ([#992]) - Multiple documentation fixes and improvements ([#964], [#980], [#981]) Thanks to @nagisa, and new contributors @securityinsanity, @froydnj, @jyn514 and @TaKO8Ki for contributing to this release! [#994]: #994 [#992]: #992 [#987]: #987 [#980]: #980 [#981]: #981 [#964]: #964 [#808]: #808 Signed-off-by: Eliza Weisman <eliza@buoyant.io>

* master: tracing: Instrument std::future::Future (tokio-rs#808) tracing: move `ValueSet` construction out of closures (tokio-rs#987)

@nagisa

Fixed - Compiler error when using `#[instrument(err)]` on functions with mutable parameters ([#1167]) - Missing function visibility modifier when using `#[instrument]` with `async-trait` ([#977]) - Multiple documentation fixes and improvements ([#965], [#981], [#1215]) Changed - `tracing-futures` dependency is no longer required when using `#[instrument]` on async functions ([#808]) Thanks to @nagisa, @Txuritan, @TaKO8Ki, and @okready for contributing to this release! [#1167]: #1167 [#977]: #977 [#965]: #965 [#981]: #981 [#1215]: #1215 [#808]: #808

@nagisa

Fixed - Compiler error when using `#[instrument(err)]` on functions with mutable parameters ([#1167]) - Missing function visibility modifier when using `#[instrument]` with `async-trait` ([#977]) - Multiple documentation fixes and improvements ([#965], [#981], [#1215]) Changed - `tracing-futures` dependency is no longer required when using `#[instrument]` on async functions ([#808]) Thanks to @nagisa, @Txuritan, @TaKO8Ki, and @okready for contributing to this release! [#1167]: #1167 [#977]: #977 [#965]: #965 [#981]: #981 [#1215]: #1215 [#808]: #808

@nagisa

### Fixed - Compiler error when using `#[instrument(err)]` on functions with mutable parameters ([#1167]) - Missing function visibility modifier when using `#[instrument]` with `async-trait` ([#977]) - Multiple documentation fixes and improvements ([#965], [#981], [#1215]) ### Changed - `tracing-futures` dependency is no longer required when using `#[instrument]` on async functions ([#808]) Thanks to @nagisa, @Txuritan, @TaKO8Ki, and @okready for contributing to this release! [#1167]: #1167 [#977]: #977 [#965]: #965 [#981]: #981 [#1215]: #1215 [#808]: #808

@nagisa

Fixed - **attributes**: Compiler error when using `#[instrument(err)]` on functions with mutable parameters ([#1167]) - **attributes**: Missing function visibility modifier when using `#[instrument]` with `async-trait` ([#977]) - **attributes** Removed unused `syn` features ([#928]) - **log**: Fixed an issue where the `tracing` macros would generate code for events whose levels are disabled statically by the `log` crate's `static_max_level_XXX` features ([#1175]) - Fixed deprecations and clippy lints ([#1195]) - Several documentation fixes and improvements ([#941], [#965], [#981], [#1146], [#1215]) Changed - **attributes**: `tracing-futures` dependency is no longer required when using `#[instrument]` on async functions ([#808]) - **attributes**: Updated `tracing-attributes` minimum dependency to v0.1.12 ([#1222]) Thanks to @nagisa, @Txuritan, @TaKO8Ki, @okready, and @krojew for contributing to this release!

@nagisa

### Fixed - **attributes**: Compiler error when using `#[instrument(err)]` on functions with mutable parameters ([#1167]) - **attributes**: Missing function visibility modifier when using `#[instrument]` with `async-trait` ([#977]) - **attributes** Removed unused `syn` features ([#928]) - **log**: Fixed an issue where the `tracing` macros would generate code for events whose levels are disabled statically by the `log` crate's `static_max_level_XXX` features ([#1175]) - Fixed deprecations and clippy lints ([#1195]) - Several documentation fixes and improvements ([#941], [#965], [#981], [#1146], [#1215]) ### Changed - **attributes**: `tracing-futures` dependency is no longer required when using `#[instrument]` on async functions ([#808]) - **attributes**: Updated `tracing-attributes` minimum dependency to v0.1.12 ([#1222]) Thanks to @nagisa, @Txuritan, @TaKO8Ki, @okready, and @krojew for contributing to this release!

@nagisa

### Fixed - Compiler error when using `#[instrument(err)]` on functions with mutable parameters ([tokio-rs#1167]) - Missing function visibility modifier when using `#[instrument]` with `async-trait` ([tokio-rs#977]) - Multiple documentation fixes and improvements ([tokio-rs#965], [tokio-rs#981], [tokio-rs#1215]) ### Changed - `tracing-futures` dependency is no longer required when using `#[instrument]` on async functions ([tokio-rs#808]) Thanks to @nagisa, @Txuritan, @TaKO8Ki, and @okready for contributing to this release! [tokio-rs#1167]: tokio-rs#1167 [tokio-rs#977]: tokio-rs#977 [tokio-rs#965]: tokio-rs#965 [tokio-rs#981]: tokio-rs#981 [tokio-rs#1215]: tokio-rs#1215 [tokio-rs#808]: tokio-rs#808

@nagisa

### Fixed - **attributes**: Compiler error when using `#[instrument(err)]` on functions with mutable parameters ([tokio-rs#1167]) - **attributes**: Missing function visibility modifier when using `#[instrument]` with `async-trait` ([tokio-rs#977]) - **attributes** Removed unused `syn` features ([tokio-rs#928]) - **log**: Fixed an issue where the `tracing` macros would generate code for events whose levels are disabled statically by the `log` crate's `static_max_level_XXX` features ([tokio-rs#1175]) - Fixed deprecations and clippy lints ([tokio-rs#1195]) - Several documentation fixes and improvements ([tokio-rs#941], [tokio-rs#965], [tokio-rs#981], [tokio-rs#1146], [tokio-rs#1215]) ### Changed - **attributes**: `tracing-futures` dependency is no longer required when using `#[instrument]` on async functions ([tokio-rs#808]) - **attributes**: Updated `tracing-attributes` minimum dependency to v0.1.12 ([tokio-rs#1222]) Thanks to @nagisa, @Txuritan, @TaKO8Ki, @okready, and @krojew for contributing to this release!

davidbarsky added 5 commits July 11, 2020 20:55

Add Instrument and WithDispatch to tracing

ee0fc2b

Update tracing-tower and tracing-attributes to depend on tracing's In…

5e3905a

…strument

Update examples

6de0d8a

BROKEN: update tracing-futures; not sure how to resolve conflict in b…

6841eea

…lankets.

It "works"

83e15dd

davidbarsky requested a review from a team July 12, 2020 01:26

davidbarsky added 2 commits July 12, 2020 18:03

switch to pin-project-lite

58481f8

Fix build issues

65818d9

davidbarsky marked this pull request as ready for review July 20, 2020 21:24

davidbarsky requested a review from hawkw as a code owner July 20, 2020 21:24

hawkw reviewed Jul 20, 2020

View reviewed changes

davidbarsky and others added 7 commits July 20, 2020 17:48

Update tracing/src/instrument.rs

8e7b0a0

Co-authored-by: Eliza Weisman <eliza@buoyant.io>

Update tracing/src/instrument.rs

388a3d9

Co-authored-by: Eliza Weisman <eliza@buoyant.io>

Update tracing/src/instrument.rs

a261cdb

Co-authored-by: Eliza Weisman <eliza@buoyant.io>

Update tracing/src/instrument.rs

7d54948

Co-authored-by: Eliza Weisman <eliza@buoyant.io>

Run with tokio::spawn in doc test

ed11567

update doc links

28ddc27

Update tracing/src/instrument.rs

ea5c7b5

Co-authored-by: Eliza Weisman <eliza@buoyant.io>

davidbarsky requested a review from hawkw July 21, 2020 18:43

uh oh

2c4f3d1

hawkw reviewed Jul 22, 2020

View reviewed changes

davidbarsky mentioned this pull request Jul 27, 2020

Measure function call latency #848

Open

LegNeato mentioned this pull request Jul 29, 2020

Add tracing support graphql-rust/juniper#720

Open

bz2 mentioned this pull request Aug 5, 2020

Improve README for first time users #825

Merged

Merge branch 'master' into davidbarsky/add-instrument-trait-to-tracing

1f847a4

davidbarsky requested a review from hawkw September 24, 2020 22:43

hawkw approved these changes Sep 24, 2020

View reviewed changes

davidbarsky merged commit a8cc977 into master Sep 25, 2020

davidbarsky deleted the davidbarsky/add-instrument-trait-to-tracing branch September 25, 2020 20:18

hawkw mentioned this pull request Sep 28, 2020

tracing: prepare to release 0.1.21 #997

Merged

dvdplm added a commit to dvdplm/tracing that referenced this pull request Oct 5, 2020

Merge branch 'master' into dp-use-cow

183f75f

* master: tracing: Instrument std::future::Future (tokio-rs#808) tracing: move `ValueSet` construction out of closures (tokio-rs#987)

davidbarsky mentioned this pull request Nov 15, 2020

examples: Fix echo by Reintroducing Concurrency #1107

Merged

hawkw mentioned this pull request Feb 4, 2021

attributes: prepare to release v0.1.12 #1221

Merged

hawkw mentioned this pull request Feb 4, 2021

tracing: prepare to release v0.1.23 #1223

Merged

ilslv mentioned this pull request Mar 30, 2023

impl<T: Stream> Stream for Instrumented<T> #2540

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tracing: Instrument std::future::Future in tracing #808

tracing: Instrument std::future::Future in tracing #808

davidbarsky commented Jul 12, 2020 •

edited

Loading

hawkw commented Jul 12, 2020

hawkw left a comment

hawkw Jul 20, 2020

hawkw Jul 20, 2020

hawkw Jul 20, 2020

davidbarsky commented Jul 21, 2020 •

edited

Loading

davidbarsky commented Jul 21, 2020

hawkw Jul 22, 2020

davidbarsky Jul 22, 2020

hawkw commented Jul 22, 2020

hawkw commented Jul 22, 2020

hawkw commented Jul 22, 2020

davidbarsky commented Jul 22, 2020

hawkw commented Jul 22, 2020

LegNeato commented Sep 3, 2020

hawkw Sep 24, 2020

	pub trait Instrument<I: Instrumented> {
	fn instrument(self, span: &Span) -> I::Output
	where
	Self: Sized,

		/// [`Subscriber`]: https://docs.rs/tracing/latest/tracing/subscriber/trait.Subscriber.html
		/// [default]: https://docs.rs/tracing/latest/tracing/dispatcher/index.html#setting-the-default-subscriber

		/// Attach a span to a `std::future::Future`.
		pub mod instrument;

tracing: Instrument std::future::Future in tracing #808

tracing: Instrument std::future::Future in tracing #808

Conversation

davidbarsky commented Jul 12, 2020 • edited Loading

hawkw commented Jul 12, 2020

hawkw left a comment

Choose a reason for hiding this comment

hawkw Jul 20, 2020

Choose a reason for hiding this comment

hawkw Jul 20, 2020

Choose a reason for hiding this comment

hawkw Jul 20, 2020

Choose a reason for hiding this comment

davidbarsky commented Jul 21, 2020 • edited Loading

davidbarsky commented Jul 21, 2020

hawkw Jul 22, 2020

Choose a reason for hiding this comment

davidbarsky Jul 22, 2020

Choose a reason for hiding this comment

hawkw commented Jul 22, 2020

hawkw commented Jul 22, 2020

hawkw commented Jul 22, 2020

davidbarsky commented Jul 22, 2020

hawkw commented Jul 22, 2020

LegNeato commented Sep 3, 2020

hawkw Sep 24, 2020

Choose a reason for hiding this comment

davidbarsky commented Jul 12, 2020 •

edited

Loading

davidbarsky commented Jul 21, 2020 •

edited

Loading