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

Correctly instrument fns returning an async move block #1866

Merged
merged 5 commits into from Jan 27, 2022

Conversation

Swatinem
Copy link
Contributor

This should take care of half of #1863, meaning it should instrument functions returning an unboxed async move block.

This unfortunately does not cover generic impl Future returns, such as std::future::ready() and similar :-(

@Swatinem Swatinem requested review from davidbarsky, hawkw and a team as code owners January 26, 2022 19:17
@hawkw
Copy link
Member

hawkw commented Jan 26, 2022

Oops, I left a review on the your draft PR (#1865): #1865 (review)

I was expecting that the draft PR would be marked as ready for review, rather than opening a new PR...sorry about that!

@hawkw
Copy link
Member

hawkw commented Jan 26, 2022

Ah, I see that the main difference between this PR and #1865 is that #1865 also adds tests. Personally, I would prefer to also merge the tests, rather than removing them --- it would be nice to have tests that catch regressions in the future. Can we add the tests to this PR (or mark #1865 as "ready for review" instead?)

@Swatinem
Copy link
Contributor Author

Ah yes, sorry.
I meant to split that up into smaller incremental changes. The larger issue (the call not being instrumented) is still not solved, but I started with a failing testcase to understand how things work.

So this here (support for manual async blocks) works and can land already.

Learning how the matching and code generation works, I think I have an idea how to solve my other problems as well.

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, thanks for the docs changes!

If you'd prefer, I'm fine with merging this PR now, and making the additional change to enter the span for the duration of calling the function in a separate PR? I have some thoughts about how we could do that, if you're interested, but if you've already figured it out, that's great too!

One other note: I don't believe this will work correctly for code where a different impl Future is returned in one of multiple branches, such as

#[instrument] 
fn probably_doesnt_work(which_fut: bool) -> Pin<Box<dyn Future<Output = ()>>> {
    if which_fut {
        Box::pin(async move { /* ... */ })
    } else {
        Box::pin(async move { /* ... */ })
    }
}

it might be nice to handle that case, as well, in a follow-up PR, but the implementation might be significantly more complex. We could probably do that transformation using syn's Fold API.

@hawkw hawkw enabled auto-merge (squash) January 27, 2022 19:04
@hawkw hawkw merged commit 820613c into tokio-rs:master Jan 27, 2022
hawkw pushed a commit that referenced this pull request Feb 3, 2022
…1866)

This should take care of half of #1863, meaning it should instrument
functions returning an unboxed `async move` block.

This unfortunately does not cover generic `impl Future` returns, such as
`std::future::ready()` and similar :-(
hawkw pushed a commit that referenced this pull request Feb 3, 2022
…1866)

This should take care of half of #1863, meaning it should instrument
functions returning an unboxed `async move` block.

This unfortunately does not cover generic `impl Future` returns, such as
`std::future::ready()` and similar :-(
hawkw pushed a commit that referenced this pull request Feb 4, 2022
…1866)

This should take care of half of #1863, meaning it should instrument
functions returning an unboxed `async move` block.

This unfortunately does not cover generic `impl Future` returns, such as
`std::future::ready()` and similar :-(
hawkw added a commit that referenced this pull request Feb 4, 2022
# 0.1.19 (February 3, 2022)

This release introduces a new `#[instrument(ret)]` argument to emit an
event with the return value of an instrumented function.

### Added

- `#[instrument(ret)]` to record the return value of a function
  ([#1716])
- added `err(Debug)` argument to cause `#[instrument(err)]` to record
  errors with `Debug` rather than `Display ([#1631])

### Fixed

- incorrect code generation for functions returning async blocks
  ([#1866])
- incorrect diagnostics when using `rust-analyzer` ([#1634])

Thanks to @Swatinem, @hkmatsumoto, @cynecx, and @ciuncan for
contributing to this release!

[#1716]: #1716
[#1631]: #1631
[#1634]: #1634
[#1866]: #1866
hawkw added a commit that referenced this pull request Feb 4, 2022
# 0.1.19 (February 3, 2022)

This release introduces a new `#[instrument(ret)]` argument to emit an
event with the return value of an instrumented function.

### Added

- `#[instrument(ret)]` to record the return value of a function
  ([#1716])
- added `err(Debug)` argument to cause `#[instrument(err)]` to record
  errors with `Debug` rather than `Display ([#1631])

### Fixed

- incorrect code generation for functions returning async blocks
  ([#1866])
- incorrect diagnostics when using `rust-analyzer` ([#1634])

Thanks to @Swatinem, @hkmatsumoto, @cynecx, and @ciuncan for
contributing to this release!

[#1716]: #1716
[#1631]: #1631
[#1634]: #1634
[#1866]: #1866
@Swatinem Swatinem deleted the fix/impl-future branch June 10, 2022 14:55
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.

None yet

2 participants