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

Add a method to Targets to get the default level #2242

Merged
merged 4 commits into from Jul 25, 2022
Merged

Add a method to Targets to get the default level #2242

merged 4 commits into from Jul 25, 2022

Conversation

connec
Copy link
Contributor

@connec connec commented Jul 23, 2022

Motivation

This makes it possible to fully "override" some base Targets filter with another (e.g. user-supplied) filter. Without some way to obtain the default, only explicit targets can be overridden (via IntoIter and friends).

Solution

We can add a method to Targets that filters the underlying DirectiveSet for the default directive. This works because DirectiveSet::add will replace directives with the same target/field_names, which is always None/vec![] for the directive added by with_default (and in fact we are only concerned with target, since no other Targets API allows adding directives with a None target).

Ideally the method would be named default, corresponding to with_default, however this conflicts with Default::default and so would be a breaking change (and harm ergonomics). default_level seemed a better name than get_default, since "getters" of this style are generally considered unidiomatic[citation needed].

Example usage:

let mut filter = Targets::new().with_target("some_module", LevelFilter::INFO);

// imagine this came from `RUST_LOG` or similar
let override: Targets = "trace".parse().unwrap();

// merge the override
if let Some(default) = override.default_level() {
    filter = filter.with_default(default);
}
for (target, level) in override.iter() {
    filter = filter.with_target(target, level);
}

This makes it possible to fully "override" some base `Targets` filter
with another (e.g. user-supplied) filter. Without some way to obtain the
default, only explicit targets can be overridden (via `IntoIter` and
friends).

Ideally the method would be named `default`, corresponding to
`with_default`, however this conflicts with `Default::default` and so
would be a breaking change (and harm ergonomics). `default_level` seemed
a better name than `get_default`, since "getters" of this style are
generally considered unidiomatic[citation needed].
@connec connec requested review from hawkw, davidbarsky and a team as code owners July 23, 2022 17:56
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

This definitely seems worth having, and I think the default_level naming is fine. I had some very minor suggestions about the API docs, but once those are addressed I'll be very happy to merge this!

Thank you!

tracing-subscriber/src/filter/targets.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/filter/targets.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/filter/targets.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/filter/targets.rs Outdated Show resolved Hide resolved
connec and others added 2 commits July 23, 2022 23:50
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
The new documentation includes more complete examples showing defaults
set by `with_default` or parsed, as well as examples of no defaults and
a specifically set `LevelFilter::OFF` default. You may also notice that
2nd person language has been removed.
@connec
Copy link
Contributor Author

connec commented Jul 23, 2022

Thanks for the fast review! I think I've responded to all your comments.

I did end up second-guessing the naming a little bit, since what we really return is a LevelFilter rather than a Level (and LevelFilter is already an Option<Level> under the hood). It does match up with the level argument for with_default though so perhaps it's fine (I'm not sure default_level_filter() is much better?).

@hawkw
Copy link
Member

hawkw commented Jul 25, 2022

I did end up second-guessing the naming a little bit, since what we really return is a LevelFilter rather than a Level (and LevelFilter is already an Option<Level> under the hood). It does match up with the level argument for with_default though so perhaps it's fine (I'm not sure default_level_filter() is much better?).

Personally, I think default_level is fine --- the return type should be pretty obvious just from looking at the function signature; I wouldn't worry about it too much. :)

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

this looks good to me, thank you again!

tracing-subscriber/src/filter/targets.rs Show resolved Hide resolved
tracing-subscriber/src/filter/targets.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/filter/targets.rs Outdated Show resolved Hide resolved
@hawkw
Copy link
Member

hawkw commented Jul 25, 2022

I believe this also closes #1790, as it implements the solution discussed in #1790 (comment).

@hawkw hawkw enabled auto-merge (squash) July 25, 2022 17:00
@hawkw hawkw merged commit 0081037 into tokio-rs:master Jul 25, 2022
@connec connec deleted the targets-default branch July 25, 2022 19:02
hawkw pushed a commit that referenced this pull request Jul 29, 2022
## Motivation

This makes it possible to fully "override" some base `Targets` filter
with another (e.g. user-supplied) filter. Without some way to obtain the
default, only explicit targets can be overridden (via `IntoIter` and
friends). 

See also #1790 (comment)

## Solution

We can add a method to `Targets` that filters the underlying
`DirectiveSet` for the default directive. This works because
`DirectiveSet::add` will replace directives with the same
`target`/`field_names`, which is always `None`/`vec![]` for the
directive added by `with_default` (and in fact we are only concerned
with `target`, since no other `Targets` API allows adding directives
with a `None` target).

Ideally the method would be named `default`, corresponding to
`with_default`, however this conflicts with `Default::default` and so
would be a breaking change (and harm ergonomics). `default_level` seemed
a better name than `get_default`, since "getters" of this style are
generally considered unidiomatic<sup>[citation needed]</sup>.

Example usage:

```rust
let mut filter = Targets::new().with_target("some_module", LevelFilter::INFO);

// imagine this came from `RUST_LOG` or similar
let override: Targets = "trace".parse().unwrap();

// merge the override
if let Some(default) = override.default_level() {
    filter = filter.with_default(default);
}
for (target, level) in override.iter() {
    filter = filter.with_target(target, level);
}
```

Closes #1790
@hawkw hawkw mentioned this pull request Jul 29, 2022
hawkw pushed a commit that referenced this pull request Jul 29, 2022
## Motivation

This makes it possible to fully "override" some base `Targets` filter
with another (e.g. user-supplied) filter. Without some way to obtain the
default, only explicit targets can be overridden (via `IntoIter` and
friends). 

See also #1790 (comment)

## Solution

We can add a method to `Targets` that filters the underlying
`DirectiveSet` for the default directive. This works because
`DirectiveSet::add` will replace directives with the same
`target`/`field_names`, which is always `None`/`vec![]` for the
directive added by `with_default` (and in fact we are only concerned
with `target`, since no other `Targets` API allows adding directives
with a `None` target).

Ideally the method would be named `default`, corresponding to
`with_default`, however this conflicts with `Default::default` and so
would be a breaking change (and harm ergonomics). `default_level` seemed
a better name than `get_default`, since "getters" of this style are
generally considered unidiomatic<sup>[citation needed]</sup>.

Example usage:

```rust
let mut filter = Targets::new().with_target("some_module", LevelFilter::INFO);

// imagine this came from `RUST_LOG` or similar
let override: Targets = "trace".parse().unwrap();

// merge the override
if let Some(default) = override.default_level() {
    filter = filter.with_default(default);
}
for (target, level) in override.iter() {
    filter = filter.with_target(target, level);
}
```

Closes #1790
hawkw added a commit that referenced this pull request Oct 6, 2022
# 0.3.16 (October 6, 2022)

This release of `tracing-subscriber` fixes a regression introduced in
[v0.3.15][subscriber-0.3.15] where `Option::None`'s `Layer`
implementation would set the max level hint to `OFF`. In addition, it
adds several new APIs, including the `Filter::event_enabled` method for
filtering events based on fields values, and the ability to log internal
errors that occur when writing a log line.

This release also replaces the dependency on the unmaintained
[`ansi-term`] crate with the [`nu-ansi-term`] crate, resolving an
*informational* security advisory ([RUSTSEC-2021-0139] for
[`ansi-term`]'s maintainance status. This increases the minimum
supported Rust version (MSRV) to Rust 1.50+, although the crate should
still compile for the previous MSRV of Rust 1.49+ when the `ansi`
feature is not enabled.

### Fixed

- **layer**: `Option::None`'s `Layer` impl always setting the
  `max_level_hint` to `LevelFilter::OFF` (#2321)
- Compilation with `-Z minimal versions` (#2246)
- **env-filter**: Clarify that disabled level warnings are emitted by
  `tracing-subscriber` (#2285)

### Added

- **fmt**: Log internal errors to `stderr` if writing a log line fails
  (#2102)
- **fmt**: `FmtLayer::log_internal_errors` and
  `FmtSubscriber::log_internal_errors` methods for configuring whether
  internal writer errors are printed to `stderr` (#2102)
- **fmt**: `#[must_use]` attributes on builders to warn if a
  `Subscriber` is configured but not set as the default subscriber
  (#2239)
- **filter**: `Filter::event_enabled` method for filtering an event
  based on its fields (#2245, #2251)
- **filter**: `Targets::default_level` accessor (#2242)

### Changed

- **ansi**: Replaced dependency on unmaintained `ansi-term` crate with
  `nu-ansi-term` ((#2287, fixes informational advisory
  [RUSTSEC-2021-0139])
- `tracing-core`: updated to [0.1.30][core-0.1.30]
- Minimum Supported Rust Version (MSRV) increased to Rust 1.50+ (when
  the `ansi`) feature flag is enabled (#2287)

### Documented

- **fmt**: Correct inaccuracies in `fmt::init` documentation (#2224)
- **filter**: Fix incorrect doc link in `filter::Not` combinator
  (#2249)

Thanks to new contributors @cgbur, @DesmondWillowbrook, @RalfJung, and
@poliorcetics, as well as returning contributors @CAD97, @connec,
@jswrenn, @guswynn, and @bryangarza, for contributing to this release!

[nu-ansi-term]: https://github.com/nushell/nu-ansi-term
[ansi_term]: https://github.com/ogham/rust-ansi-term
[RUSTSEC-2021-0139]: https://rustsec.org/advisories/RUSTSEC-2021-0139.html
[core-0.1.30]: https://github.com/tokio-rs/tracing/releases/tag/tracing-core-0.1.30
[subscriber-0.3.15]: https://github.com/tokio-rs/tracing/releases/tag/tracing-subscriber-0.3.15
hawkw added a commit that referenced this pull request Oct 6, 2022
# 0.3.16 (October 6, 2022)

This release of `tracing-subscriber` fixes a regression introduced in
[v0.3.15][subscriber-0.3.15] where `Option::None`'s `Layer`
implementation would set the max level hint to `OFF`. In addition, it
adds several new APIs, including the `Filter::event_enabled` method for
filtering events based on fields values, and the ability to log internal
errors that occur when writing a log line.

This release also replaces the dependency on the unmaintained
[`ansi-term`] crate with the [`nu-ansi-term`] crate, resolving an
*informational* security advisory ([RUSTSEC-2021-0139] for
[`ansi-term`]'s maintainance status. This increases the minimum
supported Rust version (MSRV) to Rust 1.50+, although the crate should
still compile for the previous MSRV of Rust 1.49+ when the `ansi`
feature is not enabled.

### Fixed

- **layer**: `Option::None`'s `Layer` impl always setting the
  `max_level_hint` to `LevelFilter::OFF` (#2321)
- Compilation with `-Z minimal versions` (#2246)
- **env-filter**: Clarify that disabled level warnings are emitted by
  `tracing-subscriber` (#2285)

### Added

- **fmt**: Log internal errors to `stderr` if writing a log line fails
  (#2102)
- **fmt**: `FmtLayer::log_internal_errors` and
  `FmtSubscriber::log_internal_errors` methods for configuring whether
  internal writer errors are printed to `stderr` (#2102)
- **fmt**: `#[must_use]` attributes on builders to warn if a
  `Subscriber` is configured but not set as the default subscriber
  (#2239)
- **filter**: `Filter::event_enabled` method for filtering an event
  based on its fields (#2245, #2251)
- **filter**: `Targets::default_level` accessor (#2242)

### Changed

- **ansi**: Replaced dependency on unmaintained `ansi-term` crate with
  `nu-ansi-term` ((#2287, fixes informational advisory
  [RUSTSEC-2021-0139])
- `tracing-core`: updated to [0.1.30][core-0.1.30]
- Minimum Supported Rust Version (MSRV) increased to Rust 1.50+ (when
  the `ansi`) feature flag is enabled (#2287)

### Documented

- **fmt**: Correct inaccuracies in `fmt::init` documentation (#2224)
- **filter**: Fix incorrect doc link in `filter::Not` combinator
  (#2249)

Thanks to new contributors @cgbur, @DesmondWillowbrook, @RalfJung, and
@poliorcetics, as well as returning contributors @CAD97, @connec,
@jswrenn, @guswynn, and @bryangarza, for contributing to this release!

[nu-ansi-term]: https://github.com/nushell/nu-ansi-term
[ansi_term]: https://github.com/ogham/rust-ansi-term
[RUSTSEC-2021-0139]: https://rustsec.org/advisories/RUSTSEC-2021-0139.html
[core-0.1.30]: https://github.com/tokio-rs/tracing/releases/tag/tracing-core-0.1.30
[subscriber-0.3.15]: https://github.com/tokio-rs/tracing/releases/tag/tracing-subscriber-0.3.15
hawkw added a commit that referenced this pull request Oct 6, 2022
# 0.3.16 (October 6, 2022)

This release of `tracing-subscriber` fixes a regression introduced in
[v0.3.15][subscriber-0.3.15] where `Option::None`'s `Layer`
implementation would set the max level hint to `OFF`. In addition, it
adds several new APIs, including the `Filter::event_enabled` method for
filtering events based on fields values, and the ability to log internal
errors that occur when writing a log line.

This release also replaces the dependency on the unmaintained
[`ansi-term`] crate with the [`nu-ansi-term`] crate, resolving an
*informational* security advisory ([RUSTSEC-2021-0139] for
[`ansi-term`]'s maintainance status. This increases the minimum
supported Rust version (MSRV) to Rust 1.50+, although the crate should
still compile for the previous MSRV of Rust 1.49+ when the `ansi`
feature is not enabled.

### Fixed

- **layer**: `Option::None`'s `Layer` impl always setting the
  `max_level_hint` to `LevelFilter::OFF` (#2321)
- Compilation with `-Z minimal versions` (#2246)
- **env-filter**: Clarify that disabled level warnings are emitted by
  `tracing-subscriber` (#2285)

### Added

- **fmt**: Log internal errors to `stderr` if writing a log line fails
  (#2102)
- **fmt**: `FmtLayer::log_internal_errors` and
  `FmtSubscriber::log_internal_errors` methods for configuring whether
  internal writer errors are printed to `stderr` (#2102)
- **fmt**: `#[must_use]` attributes on builders to warn if a
  `Subscriber` is configured but not set as the default subscriber
  (#2239)
- **filter**: `Filter::event_enabled` method for filtering an event
  based on its fields (#2245, #2251)
- **filter**: `Targets::default_level` accessor (#2242)

### Changed

- **ansi**: Replaced dependency on unmaintained `ansi-term` crate with
  `nu-ansi-term` ((#2287, fixes informational advisory
  [RUSTSEC-2021-0139])
- `tracing-core`: updated to [0.1.30][core-0.1.30]
- Minimum Supported Rust Version (MSRV) increased to Rust 1.50+ (when
  the `ansi`) feature flag is enabled (#2287)

### Documented

- **fmt**: Correct inaccuracies in `fmt::init` documentation (#2224)
- **filter**: Fix incorrect doc link in `filter::Not` combinator
  (#2249)

Thanks to new contributors @cgbur, @DesmondWillowbrook, @RalfJung, and
@poliorcetics, as well as returning contributors @CAD97, @connec,
@jswrenn, @guswynn, and @bryangarza, for contributing to this release!

[nu-ansi-term]: https://github.com/nushell/nu-ansi-term
[ansi_term]: https://github.com/ogham/rust-ansi-term
[RUSTSEC-2021-0139]: https://rustsec.org/advisories/RUSTSEC-2021-0139.html
[core-0.1.30]: https://github.com/tokio-rs/tracing/releases/tag/tracing-core-0.1.30
[subscriber-0.3.15]: https://github.com/tokio-rs/tracing/releases/tag/tracing-subscriber-0.3.15
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

2 participants