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 recent changes to v0.1.x #2002

Merged
merged 3 commits into from
Mar 22, 2022
Merged

backport recent changes to v0.1.x #2002

merged 3 commits into from
Mar 22, 2022

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Mar 18, 2022

This branch backports the following changes:

hawkw and others added 2 commits March 18, 2022 13:00
## Motivation

The `tracing`' crate's feature powerset is kind of unmanageably huge due
to the large number of `max_level_XXX` and `release_max_level_XXX`
feature flags. This is why we currently _don't_ run a `cargo-hack`
feature powerset check for it on CI. However, I forgot about that when I
added feature powerset checks to the `bin/publish` script, so publishing
a `tracing` release results in a combinatorial explosion that takes a
*very* long time to complete.

## Solution

It turns out that `cargo-hack` actually has flags for controlling what
features are included in the powerset (`--include-features` and
`--exclude-features`). This branch modifies `bin/publish` to use those
flags when checking the `tracing` and `tracing-subscriber` crate.
Additionally, I've modified the CI `cargo-hack` job to use the same
flags, so that it can now check `tracing` and `tracing-subscriber`.

This allows us to remove the manual feature check jobs for those crates
from CI.

Signed-off-by: Eliza Weisman <[email protected]>
This branch removes some unnecessary uses of the `format_args!`
and `write!` macros in `tracing-core`. Using `fmt::Display::fmt` and
similar rather than the macros may be slightly more efficient.

Co-authored-by: David Barsky <[email protected]>
@hawkw hawkw requested a review from a team as a code owner March 18, 2022 20:07
@hawkw hawkw enabled auto-merge (rebase) March 18, 2022 20:07
## Motivation

Currently, it is not actually possible to use `set_default(NoSubscriber)`
or similar to temporarily disable the global default subscriber (see
#1999).

This is because `NoSubscriber` is currently used as a placeholder value
when the thread-local cell that stores the current scoped default
subscriber is initialized. Therefore, we currently check if the current
scoped subscriber is `NoSubscriber`, and if it is, we fall back to
returning the global default instead.

This was fine, _when `NoSubscriber` was a private internal type only_.
However, PR #1549 makes `NoSubscriber` into a public API type. When users
can publicly construct `NoSubscriber` instances, it makes sense to want
to be able to use `NoSubscriber` to disable the current subscriber. This
is not possible when there is a global default set, because the local
default being `NoSubscriber` will cause the global default to be
returned.

## Solution

This branch changes the thread-local cell to store an `Option<Dispatch>`
instead, and use the `None` case to indicate no local default is set.
This way, when the local default is explicitly set to `NoSubscriber`, we
will return `NoSubscriber` rather than falling back.

This may also be a slight performance improvement, because we now check
if there's no global default by checking if the `Option` is `None`,
rather than downcasting it to a `NoSubscriber`.

I've also added a test reproducing #1999.

Fixes #1999

Signed-off-by: Eliza Weisman <[email protected]>
@hawkw hawkw force-pushed the eliza/backport-2001 branch from 35ab8c0 to 8f1222c Compare March 18, 2022 20:56
@hawkw hawkw disabled auto-merge March 22, 2022 17:37
@hawkw hawkw enabled auto-merge (rebase) March 22, 2022 17:37
@hawkw hawkw disabled auto-merge March 22, 2022 17:38
@hawkw hawkw merged commit b1123e4 into v0.1.x Mar 22, 2022
@hawkw hawkw deleted the eliza/backport-2001 branch March 22, 2022 17:39
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.

3 participants