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

remaining backports for tracing-subscriber 0.3 #1676

Merged
merged 10 commits into from
Oct 22, 2021

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Oct 22, 2021

  • misc stuff

0b01 and others added 8 commits October 22, 2021 11:46
This PR adds tracing-etw which is a new crate.

* Update README.md

* Update lib.rs
## Motivation

This PR attempts to solve #1630 by introducing `err(Debug)` meta to
`intrument` attribute macro. As `err` meta causes the error (`e`)
returned by instrumented function to be passed to `tracing::error!(error
= %e)` i.e. makes it use the `Display` implementation of `e`, the newly
added `err(Debug)` makes expands to `tracing::error!(error = ?e)` which
makes the `error!` macro to use `Debug` implementation for `e`. `err`
and `err(Debug)` are mutually exclusive, adding both will create a
compilation error.

`err(Display)` is also supported to specify `Display` explicitly.

As tried to describe, for some types implementing `Error` it might be
more suitable to use `Debug` implementation as in the case of
`eyre::Result`. This frees us to manually go over the error chain and
print them all, so that `instrument` attribute macro would do it for us.

## Solution

- Added a custom keyword `err(Debug)` similar to `err`,
- Add `err(Debug)` field to `InstrumentArgs`,
- Add parsing for `err(Debug)` arg and check for conflicts with `err`,
- Generate `tracing::error!(error = ?e)` when `err(Debug)` is `true` and
  `tracing::error!(error = %e)` when `err(Display)` or `err` is `true`,
- Interpolate generated `err_block` into `Err` branches in both async
  and sync return positions, if `err` or `err(Debug)` is `true`.
## Motivation

Recent `rust-analyzer` versions enabled automatic expansion of proc
macro attributes by default. This is a problem with `#[instrument]`,
because it currently produces a `compile_error!` when parsing the code
inside the `#[instrument]`ed function fails, and *discards* those
tokens. This means that if the `#[instrument]` attribute is placed on a
function whose implementation fails to parse, recent versions of
`rust-analyzer` will no longer be able to display diagnostics for those
errors. In some cases, this can also break autocompletion.

## Solution

This branch changes `#[instrument]` to always expand to the tokens
contained in the `#[instrument]`ed function body, regardless of whether
or not they could be parsed successfully. Now, an error is only emitted
when the `#[instrument]` attribute *itself* could not be parsed. Since
the instrumented function is always expanded, any errors within that
function can be displayed properly by `rust-analyzer`.

Fixes #1633.
## Motivation

Currently, `tracing-attributes` consists of one very large `lib.rs`
module that's 1358 lines long. In my opinion, this makes the code
somewhat hard to navigate.

## Solution

This branch refactors `tracing-attributes` so that most of the code is
split into two separate modules: `attrs.rs`, which contains the types
representing the attribute arguments and the code for parsing them, and
`expand.rs`, which contains the code for actually expanding an
`#[instrument]` macro to generate the instrumented code. For the most
part, this was a pretty clean split.

I also did a small change to the way `async-trait` support is
implemented; moving the two steps for generating instrumented code for
`async-trait` to two methods on the `AsyncTraitInfo` struct; one for
determining if the instrumented function is also an `async-trait` method
and finding the necessary information to instrument it, and one for
generating the instrumented code. This feels a bit neater than doing all
of that in the `gen_function` function.

There shouldn't be any functional changes or significant
implementatation changes (besides the `async-trait` change) in this
branch; just moving code around.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
`Event::parent()` and `Span::parent()` returns an `Option`, not boolean.
This branch adds some environment variables to configure *all* CI jobs.
In particular, we:

- disable incremental compilation
- increase the number of retries for network requests in `cargo` and
  `rustup`
- emit shortened backtraces from panics

This config was blatantly stolen from linkerd/linkerd2-proxy#7137. :)

Incremental compilation is useful as part of an edit-build-test-edit
cycle, as it lets the compiler avoid recompiling code that hasn't
changed. However, on CI, we're not making small edits; we're almost
always building the entire project from scratch. Thus, incremental
compilation on CI actually introduces *additional* overhead to support
making future builds faster...but no future builds will ever occur in
any given CI environment.

See https://matklad.github.io/2021/09/04/fast-rust-builds.html#ci-workflow
for details.

Increasing retries for network requests should help reduce flakiness a
bit.
## Motivation

This PR continues the work started in
#1646 to replace `chrono` with
`time`. I'll refer to @hawkw's motivation:

> Currently, `tracing-subscriber` supports the `chrono` crate for
> timestamp formatting, via a default-on feature flag. When this code
> was initially added to `tracing-subscriber`, the `time` crate did not
> have support for the timestamp formatting options we needed.
>
> Unfortunately, the `chrono` crate's maintenance status is now in
> question (see #1598). Furthermore, `chrono` depends on version 0.1 of
> the `time` crate, which contains a security vulnerability
> (https://rustsec.org/advisories/RUSTSEC-2020-0071.html). This
> vulnerability is fixed in more recent releases of `time`, but `chrono`
> still uses v0.1.

## Solution

I've replaced chrono with time 0.3. Unfortunately, some of chrono's
builders for DateTimes are not present in `time`, which required the
usage of `macro_rules!` macros to construct some of the hard-coded
times. I also took the opportunity to tidy some of the tests and change
the internal representation of `Rotation::NEVER` from year 9,999 to an
`Option::None`.

This branch changes `tracing-appender`'s MSRV from Rust 1.42 to Rust
1.51, the MSRV for the `time` crate when certain required feature flags
are enabled. This does *not* effect the MSRV for other crates in this
repository.
@hawkw hawkw requested review from jtescher, yaahc and a team as code owners October 22, 2021 19:49
hawkw and others added 2 commits October 22, 2021 13:13
While we're breaking things, we may as well do this as well.

Closes #630
Closes #662
This branch is @dzvon's PR #1508, with the following changes:

* Add a newtype wrapping the error counter to hide that it's internally
  an `Arc<AtomicUsize>`. This would allow us to make additional changes
  to the implementation without potentially causing breaking changes.

* Use saturating arithmetic when incrementing the counter to avoid
  wrapping to 0 on overflows. This is more likely to be an issue on
  32-bit platforms.

This is a breaking change that will be released as part of 
`tracing-appender` 0.2.

Closes #1508

Description from @dzvon's original PR:

## Motivation

Currently, tracing-appender crate cannot be compiled on PowerPC
platform. Because

> PowerPC and MIPS platforms with 32-bit pointers do not have
> `AtomicU64` or `AtomicI64` types.

quote from std library docs.
(https://doc.rust-lang.org/std/sync/atomic/index.html#portability)

## Solution

Change `AtomicU64` to `AtomicUsize`.

Co-authored-by: Dezhi Wu <wu543065657@163.com>
@hawkw hawkw force-pushed the eliza/subscriber-0.3-backports branch from 3634847 to 6d261cc Compare October 22, 2021 20:13
@hawkw hawkw merged commit f461c5b into v0.1.x Oct 22, 2021
@hawkw hawkw deleted the eliza/subscriber-0.3-backports branch October 22, 2021 20:26
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.

7 participants