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 to v0.1.x #2059

Merged
merged 7 commits into from Apr 9, 2022
Merged

backport changes to v0.1.x #2059

merged 7 commits into from Apr 9, 2022

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Apr 8, 2022

This branch backports:

hawkw and others added 6 commits April 8, 2022 14:30
## Motivation

The Rust 1.60 release introduced a few new lints that trigger on the
`tracing` codebase. In particular, `clippy` added some new lints for
method naming, and the `unreachable_pub` lint now seems to be triggered
incorrectly by `pub use foo as _` re-exports.

## Solution

This branch fixes the lints.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
…#2052)

## Motivation

See issue #2046. When using calling [`Builder::parse`] or
[`Builder::parse_lossy`] with  an empty string an error is produced.
This happens for example when `EnvFilter::from_default_env()` is called,
but the `RUST_LOG` variable is unset. This regression was introduced by
#2035.

## Solution

Filter any empty directives. This allows the whole string to be empty,
as well as leading and trailing commas. A unit test was added for
[`Builder::parse`], but not [`Builder::parse_lossy`] because it (per
definition) doesn't produce any side effects visible from tests when
erroring.

Fixes #2046

[`Builder::parse`]: https://github.com/tokio-rs/tracing/blob/cade7e311848227348c9b3062e4a33db827a0390/tracing-subscriber/src/filter/env/builder.rs#L151=
[`Builder::parse_lossy`]: https://github.com/tokio-rs/tracing/blob/cade7e311848227348c9b3062e4a33db827a0390/tracing-subscriber/src/filter/env/builder.rs#L135=
## Motivation

Currently, there is a potential namespace resolution issue when calling
`EnvFilter` methods that have the same name in the `Filter` and
`Layer` traits (such as `enabled` and `max_level_hint`). When both
`Filter` and `Layer` are in scope, the method resolution is
ambiguous.

See #1983 (comment)

## Solution

This commit solves the problem by making the inherent method versions of
these methods public. When the traits are in scope, name resolution will
always select the inherent method prefer`entially, preventing the name
clash.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Depends on #2057

## Motivation

Currently, `EnvFilter`'s `Layer` implementation provides a
`Layer::on_record` method, but its `Filter` implementation is
missing the corresponding `Filter::on_record` implementation. This means
that when using `EnvFilter` as a per-layer filter, recording span
fields after the spans were created will not update the filter state.

## Solution

This commit factors out the `on_record` implementation for `Layer`
into an inherent method, and adds a new `Filter::on_record` method that
calls it as well.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
…ta (#1968)

## Motivation

Currently, `tracing-journald` prefixes event fields with the number
of parent spans, if present, in order to namespace field values. This
turned out to be unnecessary as journald preserves values for duplicated
field names.

## Solution

This branch removes event field prefixes and emits parent span names
and their field/value pairs as additional key/value pairs.
This adds a minimal-versions check to the tracing project. Adapted from
`tokio-rs/tokio`. Adding this avoids breaking downstream dependencies
from accidentally under-constraining minimal versions of dependencies
when they depend on tracing.

I've currently just introduced the check. I will try to and do encourage
others to add patches to fix this where possible since it can be a fair
bit of work to chase down a version of all dependencies that passes
minimal-versions and is msrv. I've also seen some really odd
windows-specific issues (which are not being tested for here).

This is currently only testing `tracing`, `tracing-core`, and
`tracing-subscriber`. Packages such as `tracing-futures` are proving to
be a bit harder to deal with due to having features which enable very
old dependencies.

Steps to test the build minimal versions locally:

```sh
cargo install cargo-hack
rustup default nightly
cargo hack --remove-dev-deps --workspace
cargo update -Z minimal-versions
cargo hack check --all-features --ignore-private
```

CC: tokio-rs/tokio#4513
@hawkw hawkw requested review from yaahc and a team as code owners April 8, 2022 21:40
@hawkw hawkw enabled auto-merge (rebase) April 8, 2022 21:40
@hawkw hawkw disabled auto-merge April 8, 2022 23:11
@hawkw hawkw enabled auto-merge (rebase) April 8, 2022 23:12
@hawkw hawkw disabled auto-merge April 9, 2022 17:42
This changes the "unregistered" interest state from `0xDEADFACED`
to`0xDEAD`, which should fit in a `usize` even on 16-bit platforms. The
actual value of this thing doesn't matter at all, it just has to be "not
0, 1, or 2", and it's good for it to be something weird to make it more
obvious in the event of stuff going wrong.

This should fix a warning being emitted when building for wasm (and
other <=32-bit platforms) because the previous literal would be
truncated.

Also, the `wasm_bindgen_test` macro apparently messes with the 
`macros_redefined_core` tests, so skip them on wasm.
@hawkw hawkw merged commit d91af65 into v0.1.x Apr 9, 2022
@hawkw hawkw deleted the eliza/backport-2058 branch April 9, 2022 17:43
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