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

subscriber: backport tracing-subscriber v0.3 changes #1666

Merged
merged 11 commits into from
Oct 21, 2021

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Oct 21, 2021

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

In a few places, the changes couldn't be simply cherry-picked and required
additional changes due to differences between the v0.1.x and master versions.
In particular, this was necessary for the no_std support change.

Folyd and others added 9 commits October 21, 2021 10:23
I'm quite confident that no elsewhere using the
`tracing_subscriber::CurrentSpan`  in the present codebases. Do we need
to keep it for the future version? Otherwise, I think we can remove it
and the related `thread.rs`.

Feel free to point me out if I'm wrong. :)

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
Previously, the documentation explained that an `EnvFilter` consisted
of multiple directives, but didn't describe how to actually put
these directives together.  (My first guess was "with spaces", but
that was wrong.)

This patch changes the documentation to say that directives are
comma-separated, and gives an example of a comma-separated filter.
)

## 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 maintainance 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

Fortunately, the `time` crate now has its own timestamp formatting
support.

This branch replaces the `ChronoLocal` and `ChronoUtc` timestamp
formatters with new `LocalTime` and `UtcTime` formatters. These
formatters use the `time` crate's formatting APIs rather than
`chrono`'s. This removes the vulnerable dependency on `time` 0.1

Additionally, the new `time` APIs are feature flagged as an _opt-in_
feature, rather than as an _opt-out_ feature. This should make it easier
to avoid accidentally depending on the `time` crate when more
sophisticated timestamp formatting is _not_ required.

In a follow-up branch, we could also add support for `humantime` as an
option for timestamp formatting.

Naturally, since this removes existing APIs, this is a breaking change,
and will thus require publishing `tracing-subscriber` 0.3. We'll want to
do some other breaking changes as well.

Fixes #1598.
This changes `tracing-subscriber` so that the `env-filter`, `json`,
 and `chrono` features are not enabled by default, and instead require
users to opt in. This should significantly reduce the default
dependency footprint.

Of course, this is a breaking change, and therefore will be part of
`tracing-subscriber` v0.3.

Fixes #1258

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Implementing `Layer` for `Arc`s, which are immutable, breaks the
ability to implement `Layer::on_layer` with a mutable reference.
This is necessary for per-layer filtering. See
#1576 (comment) for
details. Therefore, the `Layer` impls for `Arc`s should not be used.

In 0.3, we have the opportunity to remove these APIs. Therefore, this PR
removes them.
Backports #1648 from `master`.

Depends on #1649

## Motivation

Presently, the `tracing-subscriber` crate requires the Rust standard
library and doesn't build with `#![no_std]` targets. For the most part,
this is fine, as much of `tracing-subscriber` inherently depends on
`std` APIs.

However, `tracing-subscriber` also contains some key abstractions that
are necessary for interoperability: the `Layer` and `LookupSpan`
traits. Since these traits are in `tracing-subscriber`, `no-std` users
cannot currently access them.

Some of the other utilities in this crate, such as the field visitor
combinators, may also be useful for `#![no_std]` projects.

## Solution

This branch adds "std" and "alloc" feature flags to
`tracing-subscriber`, for conditionally enabling `libstd` and
`liballoc`, respectively. The `registry`, `fmt`, `EnvFilter`, and
`reload` APIs all require libstd, and cannot be implemented without it,
but the core `Layer` and `LookupSpan` traits are now available with
`#![no_std]`.

Fixes #999

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

Currently, some `tracing-subscriber` dependencies in other crates use
`default-features = false`, to ensure that unneeded default features of
`tracing-subscriber` are not inadvertently enabled. However, some of the
features those crates enable also require the `std` feature flag. The
`default-features = false` config _disables_ the `std` feature, so the
required features are not available.

## Solution

This branch changes `tracing-subscriber`'s `fmt`, `registry`, and
`env-filter` feature flags to also enable the `std` feature flag
automatically.

## Alternatives

As an alternative solution, we could change crates that depend on
`tracing-subscriber` with `default-features = false` to also explicitly
ensure that the `std` feature is enabled (which might be worth doing
regardless). This is what PR #1659 does. However, I think the approach
in this branch is more correct; the feature flags that require standard
library support should automatically enable it. This provides a better
experience for most users, who are simply adding `default-features = false`
in order to avoid enabling un-needed features, not to support
`no_std`, and would like enabling a feature flag to *actually* enable
that feature.

`no_std` users will know not to enable `registry`, `fmt`, and
`env-filter` because the documentation notes that those features require
the standard library.
## Motivation

Currently, the `FormatEvent` and `FormatFields` traits in
`tracing-subscriber` are passed a `&mut dyn fmt::Write` trait object to
write formatted representations of events and fields to. This is fine,
but it doesn't give us the ability to easily provide additional
configuration to the formatter, such as whether ANSI color codes are
supported.

Issue #1651 describes some approaches for adding a way to expose the
ANSI color code configuration to custom formatters. In particular, the
proposed solution involves wrapping the `fmt::Write` trait object in an
opaque struct, which can then implement additional methods for exposing
information like "are ANSI colors enabled" to the formatter. Since this
changes the signature of the `FormatEvent::format_event` and
`FormatFields::format_fields` methods, it's a breaking change.
Therefore, we need to make this change _now_ if we want to get the API
change in for `tracing-subscriber` 0.3.

## Solution

This branch adds a `Writer` struct that wraps the `&mut dyn fmt::Write`
trait object, and changes the various method signatures as appropriate.
It does **not** actually implement the change related to ANSI color
formatting. Once we change these methods' signatures to accept a
`Writer` struct, we can add as many methods to that struct as we like
without making additional breaking API changes. Therefore, this branch
_just_ makes the breaking change that's necessary to get in before v0.3
is released.

Propagating the ANSI color configuration can be implemented in a future
branch.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
This fixes unused code warnings with the default featureset.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw added crate/subscriber Related to the `tracing-subscriber` crate meta/breaking This is a breaking change, and should wait until the next breaking release. labels Oct 21, 2021
@hawkw hawkw added this to the tracing-subscriber 0.3 milestone Oct 21, 2021
@hawkw hawkw self-assigned this Oct 21, 2021
@hawkw hawkw requested a review from a team as a code owner October 21, 2021 18:35
## Motivation

Currently, `cargo audit` checks are run on every push that modifies
`Cargo.toml` or lockfiles. The intention behind this was to fail changes
that introduce dependencies that have security advisories. However, it
turns out that this is not actually the primary use-case for `cargo audit`
runs. Typically, when a dependency of a `tracing` crate has a
security advisory, this isn't newly introduced by a PR, but a new
*advisory* that was just announced for a library we *already* depended
on. In this case, this isn't a failure that should block any particular
branch from merging; instead, it's a *new issue* that should block
effected crates from being *released*.

## Solution

This branch changes the audit workflow from running on pushes to running
on a schedule (nightly). When using `actions-rs/audit-check` in a
scheduled mode, it will automatically open new issues if any
dependencies have security advisories (see
https://github.com/actions-rs/audit-check#scheduled-audit for details).
This means those advisories can be fixed separately while still allowing
unrelated branches to pass CI. This is, IMO, a better workflow for
handling security advisories.

If we introduce release automation in the future, we should ensure that
the release automation process checks that the crate being released has
no open security advisory issues, and fails the *release* if any such
issues are still open.
Turns out that if `std` is enabled but `smallvec` is not, we have some
missing imports. This fixes the build when a crate passes
`default-features = "false` but then enables `std`.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
Copy link
Member Author

hawkw commented Oct 21, 2021

CI is failing due to...downloading cargo hack?
image
i restarted it and am hoping it goes away...

Copy link
Member

@davidbarsky davidbarsky left a comment

Choose a reason for hiding this comment

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

LGTM

@hawkw hawkw merged commit 7d3bff8 into v0.1.x Oct 21, 2021
@hawkw hawkw deleted the eliza/subscriber-backports branch October 21, 2021 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate/subscriber Related to the `tracing-subscriber` crate meta/breaking This is a breaking change, and should wait until the next breaking release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants