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

More ergonomic common case of immediately entering the span. #1246

Closed
matklad opened this issue Feb 18, 2021 · 4 comments · Fixed by #1252
Closed

More ergonomic common case of immediately entering the span. #1246

matklad opened this issue Feb 18, 2021 · 4 comments · Fixed by #1252

Comments

@matklad
Copy link
Contributor

matklad commented Feb 18, 2021

When writing non-async code, the common case is that a span precisely corresponds to a lexical scope. At the moment, this case requires an introduction of otherwise unnecessary local variable:

let span = tracing::info_span!("spam");
let _span = span.enter();

I wish the following worked:

let _span = tracing::info_span_enter!("spam");

To do this, I think we can introduce a second version of Entered:

pub struct EnteredScope {
    span: Span, // note: owns the span
    _not_send: PhantomData<*mut ()>,
}

after that, all the lifetimes should work out.

@davidbarsky
Copy link
Member

I was wondering why let _span = tracing::info_span!("spam").enter(); didn't work, but it turns out that this is a borrowck error:

error[E0716]: temporary value dropped while borrowed
 --> src/main.rs:4:17
  |
4 |     let _span = tracing::info_span!("spam").enter();
  |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^        - temporary value is freed at the end of this statement
  |                 |
  |                 creates a temporary which is freed while still in use
5 | }
  | - borrow might be used here, when `_span` is dropped and runs the `Drop` code for type `Entered`
  |
  = note: consider using a `let` binding to create a longer lived value
  = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

I'm not necessarily opposed to this change, but I'm curious if this borrowck bug can be fixed in rustc anytime soon. I'll poke around and update you.

@hawkw
Copy link
Member

hawkw commented Feb 18, 2021

@matklad I'm definitely in favour of improving ergonomics for this case! It's mostly just something I haven't really had the time to work on, so I'd happily welcome a PR. There have been some previous discussions around similar changes, so you may also want to check out issues #158 and #192.

IMO, the best approach is probably not to add a whole new set of macros, but just to add a new Span method that enters the span by value, as proposed in #192. This would return an owned version of the Entered guard, like you described. That way, users could write something like this:

let _e = tracing::info_span!("my_span").entered();

For the purposes of this discussion I'm going to call that method Span::entered, but it could also be something like into_entered or enter_owned...those might be more descriptive but also feel kind of awkward to me.

The owned guard would also be usable in other cases, such as storing the span in a struct while also keeping it entered. With the current API, you can't do this, because the span is borrowed by the Entered guard.

Since the owned enter guard would take the span by value, it might make sense to give it methods for accessing the span (which the borrowed guard doesn't have). For example, you might want to be able to borrow the span from the owned guard:

// create & enter the span
let entered = tracing::info_span!("my_span").entered();

// ... a bunch of code  inside the span ...

// now, suppose we want to spawn a task inside that span.
let span = entered.span().clone();
tokio::spawn(some_future.instrument(span));

// ...

The Entered guard only exits the span when it's dropped, but it might make sense for the owned guard to also have an explicit exit method that returns the un-entered span. Like so:

let entered = tracing::info_span!("whatever").entered();

// inside the span ...

// exit the span, returning the `Span` struct:
let span = entered.exit(); 

// outside the span ...

// enter the span by-ref, using the `enter` method:
let _e = span.enter(); 

// ...

The alternative method is to add macros that immediately enter the span. We could actually do this without a new guard type, by just having the macros expand to

let span = tracing::info_span!("...");
let _e = span.enter();

in the scope where they're expanded, rather than creating a subscope. Or, we could have them return a second guard type. Adding a new guard introduces some API complexity, but it does have the advantage of still allowing access to the span once it has been entered. It's also a bit more flexible than new macros.

What do you think?

@hawkw
Copy link
Member

hawkw commented Feb 18, 2021

As a side note, @davidbarsky

I was wondering why let _span = tracing::info_span!("spam").enter(); didn't work, but it turns out that this is a borrowck error:

... snip ...

I'm not necessarily opposed to this change, but I'm curious if this borrowck bug can be fixed in rustc anytime soon. I'll poke around and update you.

I don't think this is actually a borrowck bug --- I think it's a consequence of the semantics around temporary variable lifetimes being applied correctly. So while it's possible that future editions of the language could add additional semantics that makes this kind of code legal, it's not just a compiler bug that might be fixed in the next stable release or something. It's probably not a great idea to bet on this code ever compiling --- we should focus on adding new APIs that do work as a one-liner.

@davidbarsky
Copy link
Member

@hawkw I asked @pnkfelix about this (thank you so much for your time, Felix!) and he pointed me to rust-lang/rust#15023, which tracks a (currently unimplemented) RFC that would support let _span = tracing::info_span!("spam").enter();. You're right that it's not a borrowck bug, but it is a change to the rules pertaining to rvalues in Rust. Felix noted that supporting the above one-liner cause a change to visible semantics and could make the destructor timing lifetime dependent.

I'm making the above point such that when we forget why this behavior exists, we can remember why pretty quickly. That being said, I think that @hawkw's right in that we should focus on adding new APIs that work as one-liners.

matklad added a commit to matklad/tracing that referenced this issue Feb 19, 2021
This PR allows the following API:

```
let _guard = tracing::span!("foo").entered();
```

See tokio-rs#1246 for an extended
discussion.
matklad added a commit to matklad/tracing that referenced this issue Feb 19, 2021
This PR allows the following API:

```
let _guard = tracing::span!("foo").entered();
```

See tokio-rs#1246 for an extended
discussion.
hawkw added a commit that referenced this issue Feb 19, 2021
* Simplify common case of immediately entering the span

This PR allows the following API:

```
let _guard = tracing::span!("foo").entered();
```

See #1246 for an extended
discussion.

* Add missing inlines

This fns are monomorphic, so an `#[inline]` is necessary to alow
cross-crate inlining at all.

* improve method documentation

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this issue Feb 20, 2021
* Simplify common case of immediately entering the span

This PR allows the following API:

```
let _guard = tracing::span!("foo").entered();
```

See #1246 for an extended
discussion.

* Add missing inlines

This fns are monomorphic, so an `#[inline]` is necessary to alow
cross-crate inlining at all.

* improve method documentation

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this issue Feb 22, 2021
* Simplify common case of immediately entering the span

This PR allows the following API:

```
let _guard = tracing::span!("foo").entered();
```

See #1246 for an extended
discussion.

* Add missing inlines

This fns are monomorphic, so an `#[inline]` is necessary to alow
cross-crate inlining at all.

* improve method documentation

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this issue Feb 22, 2021
* Simplify common case of immediately entering the span

This PR allows the following API:

```
let _guard = tracing::span!("foo").entered();
```

See #1246 for an extended
discussion.

* Add missing inlines

This fns are monomorphic, so an `#[inline]` is necessary to alow
cross-crate inlining at all.

* improve method documentation

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this issue Feb 23, 2021
* Simplify common case of immediately entering the span

This PR allows the following API:

```
let _guard = tracing::span!("foo").entered();
```

See #1246 for an extended
discussion.

* Add missing inlines

This fns are monomorphic, so an `#[inline]` is necessary to alow
cross-crate inlining at all.

* improve method documentation

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this issue Feb 23, 2021
* Simplify common case of immediately entering the span

This PR allows the following API:

```
let _guard = tracing::span!("foo").entered();
```

See #1246 for an extended
discussion.

* Add missing inlines

This fns are monomorphic, so an `#[inline]` is necessary to alow
cross-crate inlining at all.

* improve method documentation

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
kaffarell pushed a commit to kaffarell/tracing that referenced this issue May 22, 2024
…-rs#1252)

* Simplify common case of immediately entering the span

This PR allows the following API:

```
let _guard = tracing::span!("foo").entered();
```

See tokio-rs#1246 for an extended
discussion.

* Add missing inlines

This fns are monomorphic, so an `#[inline]` is necessary to alow
cross-crate inlining at all.

* improve method documentation

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
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 a pull request may close this issue.

3 participants