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

<None as Subscribe>::event_enabled should be true #2193

Merged
merged 4 commits into from
Jun 30, 2022
Merged

<None as Subscribe>::event_enabled should be true #2193

merged 4 commits into from
Jun 30, 2022

Conversation

CAD97
Copy link
Contributor

@CAD97 CAD97 commented Jun 30, 2022

Sorry @guswynn @hawkw

Motivation

This is wrong.

Solution

Make it unwrong.

@CAD97 CAD97 requested review from hawkw, davidbarsky and a team as code owners June 30, 2022 21:03
CAD97 added a commit to CAD97/tracing that referenced this pull request Jun 30, 2022
@CAD97
Backport tokio-rs#2193
1f748d6
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.

thanks for fixing this, this looks good to me.

would it make sense to add a test that would have caught the bug?

@hawkw hawkw enabled auto-merge (squash) June 30, 2022 21:09
@CAD97
Copy link
Contributor Author

CAD97 commented Jun 30, 2022

Actual context:

As described by the documentation on Subscribe::event_enabled, the return value sets the global filtering of an event. This code used to say that that Option::<impl Subscribe>::None existing in the subscriber stack should disable any subscriber in the stack seeing events 🙃

A test is a good idea, I'll make another follow-up PR for that so this can merge ASAP.

It looks like unrelated code is failing a clippy lint.

@hawkw
Copy link
Member

hawkw commented Jun 30, 2022

It looks like unrelated code is failing a clippy lint.

fixed that part on master, CI should pass now when it runs.

@hawkw hawkw disabled auto-merge June 30, 2022 21:44
@hawkw
Copy link
Member

hawkw commented Jun 30, 2022 •
edited
Loading

looks like CI has somehow failed to run at all for the most recent commit? @CAD97, would you mind pushing a new empty commit to try and re-trigger CI?
EDIT: nevermind, master changed again so i was able to fix this by merging master, and it looks like CI is running again.

@hawkw hawkw enabled auto-merge (squash) June 30, 2022 22:28
@hawkw hawkw merged commit 665ad11 into tokio-rs:master Jun 30, 2022
@CAD97 CAD97 deleted the patch-1 branch June 30, 2022 23:40
hawkw pushed a commit that referenced this pull request Jul 1, 2022
@CAD97 @hawkw
subscriber: <None as Layer>::event_enabled should be true (#2193)
6d30a01 
## Motivation

This is wrong.

## Solution

Make it unwrong.

As described by the documentation on `Layer::event_enabled`, the
return value sets the global filtering of an event. This code used to
say that that `Option::<impl Layer>::None` existing in the
layer stack should disable any layer in the stack seeing
events
hawkw pushed a commit that referenced this pull request Jul 1, 2022
@CAD97 @hawkw
subscriber: <None as Layer>::event_enabled should be true (#2193)
0012a41 
## Motivation

This is wrong.

## Solution

Make it unwrong.

As described by the documentation on `Layer::event_enabled`, the
return value sets the global filtering of an event. This code used to
say that that `Option::<impl Layer>::None` existing in the
layer stack should disable any layer in the stack seeing
events
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
@guswynn guswynn mentioned this pull request Aug 23, 2022
Closed
@davidbarsky davidbarsky mentioned this pull request Sep 26, 2023
Merged
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
@CAD97 @kaffarell
subscriber: <None as Layer>::event_enabled should be true (tokio-…
8bdd972 
…rs#2193)

## Motivation

This is wrong.

## Solution

Make it unwrong.

As described by the documentation on `Layer::event_enabled`, the
return value sets the global filtering of an event. This code used to
say that that `Option::<impl Layer>::None` existing in the
layer stack should disable any layer in the stack seeing
events
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
@hawkw @kaffarell
subscriber: prepare to release v0.3.14 (tokio-rs#2201)
636211c 
# 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 ([tokio-rs#2193])
- **layer**: `Layer::max_level_hint` implementation for `Option<impl
  Layer<S>>` incorrectly disabling max level filtering when the option
  is `None` ([tokio-rs#2195])
- **layer**: `Layer::max_level_hint` implementation for `Vec<impl
  Layer<S>>` returning `LevelFilter::ERROR` rather than
  `LevelFilter::OFF` when the `Vec` is empty ([tokio-rs#2195])

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

[tokio-rs#2193]: tokio-rs#2193
[tokio-rs#2195]: tokio-rs#2195
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidbarsky davidbarsky davidbarsky approved these changes

@hawkw hawkw hawkw approved these changes

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
@CAD97 @hawkw @davidbarsky