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

backport changes from master #1609

Merged
merged 9 commits into from
Oct 1, 2021
Merged

backport changes from master #1609

merged 9 commits into from
Oct 1, 2021

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Oct 1, 2021

this backports the following changes from master to v0.1.x:

Of these commits, only #1602 required meaningful modification for v0.1.x
(beyond tracking renamings). It was necessary to change the feature
flagging of WithDispatch/WithSubscriber from the version on
master, so that the Future impl requires the "std" feature flag,
but the WithSubscriber trait and WithDispatch type do not. This is
because requiring the feature flag for these definitions would
technically be a breaking change, since they were previously published
without the feature flag and could e.g. be imported...even though they
were totally useless. Sigh.

hawkw and others added 8 commits October 1, 2021 11:34
It turns out panic hooks also work nicely even when panics are captured.
I figured we may as well have an example demoing this!
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
## Motivation
Looks like doc is missing a parenthesis


## Solution
Add a parenthesis
Allow the deprecated API for now, until the next MSRV bump.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
This should *actually* fix the build on no-std.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
## Motivation

When a `RollingFileAppender` is refreshed, the previous `BufWriter` may
encounter and suppress errors in `drop`.

From https://doc.rust-lang.org/std/io/struct.BufWriter.html:

> It is critical to call flush before BufWriter<W> is dropped. Though
> dropping will attempt to flush the contents of the buffer, any errors
> that happen in the process of dropping will be ignored.

## Solution

Explicitly flush the previous buffer before dropping, printing any error
to stderr.
The version of `WithSubscriber` in `tracing::instrument` (rather than in
`tracing-futures`) is currently...useless, since there is no `Future`
impl for the `WithDispatch` type. This means that calling
`with_collector` on a `Future` returns a value that _isn't_ a `Future`.
Additionally, the `WithSubscriber` trait isn't actually implemented for
anything (although this was fixed on v0.1.x).

This branch adds the missing implementations. I also improved the docs a
bit.

Note that the `Future` impl requires the "std" feature flag, but the
`WithSubscriber` trait and `WithDispatch` type do not. This is because
requiring the feature flag for these definitions would *technically* be
a breaking change, since they were previously published without the
feature flag and could e.g. be imported...even though they were totally
useless. Sigh.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
; Conflicts:
;	tracing/src/instrument.rs
## Motivation

In #1600, the `instrument` code generation was changed to avoid ever
constructing a `Span` struct if the level is explicitly disabled.
However, for `async` functions, `#[instrument]` will currently still
create the span, but simply skips constructing an `Instrument` future if
the level is disabled.

## Solution

This branch changes the `#[instrument]` code generation for async blocks
to totally skip constructing the span if the level is disabled. I also
simplfied the code generation a bit by combining the shared code between
the `err` and non-`err` cases, reducing code duplication a bit.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw requested a review from davidbarsky October 1, 2021 18:51
@hawkw hawkw requested a review from a team as a code owner October 1, 2021 18:51
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw merged commit 84c1c26 into v0.1.x Oct 1, 2021
@hawkw hawkw deleted the eliza/backport-1602 branch October 1, 2021 21:34
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

4 participants