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.

Sign up for GitHub

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

Jump to bottom

fix max_level_hint for empty cases for Option/Vec Subscribe impls #2195

Merged
merged 10 commits into from Jul 1, 2022
Merged

fix max_level_hint for empty cases for Option/Vec Subscribe impls #2195

merged 10 commits into from Jul 1, 2022

Conversation

guswynn
Copy link
Contributor

@guswynn guswynn commented Jun 30, 2022

Motivation

These are incorrect; Currently, when you have a None layer, the None hint it returns causes the default TRACE to win, which is inaccurate. Similarly, Vec should default to OFF if it has no Subscribes in it

Solution

Change the defaults hints to Some(OFF)

@guswynn guswynn requested review from hawkw, davidbarsky and a team as code owners June 30, 2022 21:56
@guswynn guswynn changed the title max_level_hint for empty cases for Option/Vec Subscribe impls fix max_level_hint for empty cases for Option/Vec Subscribe impls Jun 30, 2022
hawkw
hawkw requested changes Jun 30, 2022
View reviewed changes
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.

some minor nits on the tests etc, but the fix looks correct to me

tracing-subscriber/tests/subscriber_filters/vec.rs Show resolved Hide resolved
tracing-subscriber/tests/subscriber_filters/option.rs Outdated Show resolved Hide resolved
tracing-subscriber/tests/subscriber_filters/option.rs Outdated
Comment on lines 1 to 45
use super::*;
use tracing::Collect;

// This test is just used to compare to the tests below
#[test]
fn just_layer() {
let info = subscriber::named("info")
.run()
.with_filter(LevelFilter::INFO)
.boxed();

let collector = tracing_subscriber::registry().with(info);
assert_eq!(collector.max_level_hint(), Some(LevelFilter::INFO));
}

#[test]
fn layer_and_option_layer() {
let info = subscriber::named("info")
.run()
.with_filter(LevelFilter::INFO)
.boxed();

let debug = subscriber::named("debug")
.run()
.with_filter(LevelFilter::DEBUG)
.boxed();

let mut collector = tracing_subscriber::registry().with(info).with(Some(debug));
assert_eq!(collector.max_level_hint(), Some(LevelFilter::DEBUG));

let error = subscriber::named("error")
.run()
.with_filter(LevelFilter::ERROR)
.boxed();
// None means the other layer takes control
collector = tracing_subscriber::registry().with(error).with(None);
assert_eq!(collector.max_level_hint(), Some(LevelFilter::ERROR));
}

#[test]
fn just_option_layer() {
// Just a None means everything is off
let collector = tracing_subscriber::registry().with(None::<ExpectSubscriber>);
assert_eq!(collector.max_level_hint(), Some(LevelFilter::OFF));
}
Copy link
Member

Choose a reason for hiding this comment

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

we should probably have the same tests for the Subscribe implementation for Option, as well as the Filter impl.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There isn't a filter impl is there? I can move these tests out of this directory

Copy link
Member

Choose a reason for hiding this comment

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

yeah, i misspoke: there isn't a Filter impl for Option<impl Filter>, but some of these tests do also exercise per-layer filtering, because they use with_filter on the inner subscribers. thus, those tests do need to be in this test module because of feature flagging. however, we could have other tests in a different module that don't use with_filter, and instead use the Layer impl for LevelFilter...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ooh, I like the Layer impl of LevelFilter, let me push that up!

tracing-subscriber/tests/subscriber_filters/option.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/subscribe/mod.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/subscribe/mod.rs Outdated Show resolved Hide resolved
hawkw
hawkw approved these changes Jun 30, 2022
View reviewed changes
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.

lgtm once CI passes

tracing-subscriber/tests/option.rs Show resolved Hide resolved
tracing-subscriber/tests/vec.rs Show resolved Hide resolved
@hawkw hawkw enabled auto-merge (squash) June 30, 2022 23:27
@hawkw
oops lol
8429efd 
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw merged commit 2c568b5 into tokio-rs:master Jul 1, 2022
hawkw added a commit that referenced this pull request Jul 1, 2022
@guswynn @hawkw
subscriber: fix max_level_hint for empty Option/Vec Layer imp…
d864366 
…ls (#2195)

## Motivation

These are incorrect: currently, when you have a `None` layer, the `None`
hint it returns causes the default `TRACE` to win, which is inaccurate.
Similarly, `Vec` should default to `OFF` if it has no `Layer`s in it

## Solution

Change the default hints to `Some(OFF)`

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this pull request Jul 1, 2022
@guswynn @hawkw
subscriber: fix max_level_hint for empty Option/Vec Layer imp…
e0b3f79 
…ls (#2195)

## Motivation

These are incorrect: currently, when you have a `None` layer, the `None`
hint it returns causes the default `TRACE` to win, which is inaccurate.
Similarly, `Vec` should default to `OFF` if it has no `Layer`s in it

## Solution

Change the default hints to `Some(OFF)`

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this pull request Jul 1, 2022
@hawkw
subscriber: prepare to release v0.3.14
dd130f9 
# 0.3.14 (Jul 1, 2022)

This release fixes multiple filtering bugs in the `Layer`
implementations for `Option<impl Layer>` and `Vec<impl Layer>`.

### Fixed

- **layer**: `Layer::event_enabled` implementation for `Option<impl
  Layer<S>>` returning `false` when the `Option` is `None`, disabling
  all events globally ([#2193])
- **layer**: `Layer::max_level_hint` implementation for `Option<impl
  Layer<S>>` incorrectly disabling max level filtering when the option
  is `None` ([#2195])
- **layer**: `Layer::max_level_hint` implementation for `Vec<impl
  Layer<S>>` returning `LevelFilter::ERROR` rather than
  `LevelFilter::OFF` when the `Vec` is empty ([#2195])

Thanks to @CAD97 and @guswynn for contributing to this release!

[#2193]: #2193
[#2195]: #2195
@hawkw hawkw mentioned this pull request Jul 1, 2022
Merged
hawkw added a commit that referenced this pull request Jul 1, 2022
@hawkw
subscriber: prepare to release v0.3.14 (#2201)
9638587 
# 0.3.14 (Jul 1, 2022)

This release fixes multiple filtering bugs in the `Layer`
implementations for `Option<impl Layer>` and `Vec<impl Layer>`.

### Fixed

- **layer**: `Layer::event_enabled` implementation for `Option<impl
  Layer<S>>` returning `false` when the `Option` is `None`, disabling
  all events globally ([#2193])
- **layer**: `Layer::max_level_hint` implementation for `Option<impl
  Layer<S>>` incorrectly disabling max level filtering when the option
  is `None` ([#2195])
- **layer**: `Layer::max_level_hint` implementation for `Vec<impl
  Layer<S>>` returning `LevelFilter::ERROR` rather than
  `LevelFilter::OFF` when the `Vec` is empty ([#2195])

Thanks to @CAD97 and @guswynn for contributing to this release!

[#2193]: #2193
[#2195]: #2195
@fredr fredr mentioned this pull request Aug 4, 2022
Closed
@guswynn guswynn deleted the option-max-level branch September 21, 2022 20:06
@davidbarsky davidbarsky mentioned this pull request Sep 26, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hawkw hawkw hawkw approved these changes

@davidbarsky davidbarsky Awaiting requested review from davidbarsky davidbarsky is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@guswynn @hawkw @davidbarsky