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

implement filter for the reload layer/subscriber #2159

Merged
merged 2 commits into from Jun 15, 2022

Conversation

guswynn
Copy link
Contributor

@guswynn guswynn commented Jun 9, 2022

Motivation

The reload layer doesn't (and can't) implement downcasting correctly, which breaks certain subscribers like the opentelemetry one.

Solution

. Most usages of this type (including mine) are just to change the filter, so this pr implements Filter on the reload type to allow users to not need to wrap the whole layer trait. Another advantage of this is that the common-case critical sections are shortened

@guswynn guswynn requested review from hawkw, davidbarsky and a team as code owners June 9, 2022 23:05
@guswynn guswynn changed the title implement for the layer/subscriber implement filter for the reload layer/subscriber Jun 9, 2022
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.

looks good to me overall, thanks for adding a test! i had some minor suggestions.

tracing-subscriber/src/reload.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/reload.rs Outdated Show resolved Hide resolved
tracing-subscriber/src/reload.rs Show resolved Hide resolved
tracing-subscriber/src/reload.rs Outdated Show resolved Hide resolved
@guswynn
Copy link
Contributor Author

guswynn commented Jun 13, 2022

@hawkw thanks for the review! This is RFAL!

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.

made a few requested changes as part of our adherence to our style guide. broadly speaking, we don't use the second-person tense in reference documentation, but do use it in longer-term guides (e.g., https://tokio.rs/tokio/tutorial). I apologize for being a stickler for this stuff and I wish we can automate this better :)

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

hawkw commented Jun 13, 2022

made a few requested changes as part of our adherence to our style guide.

...we should probably actually write down a style guide for docs, huh? maybe in CONTRIBUTING.md?

@davidbarsky
Copy link
Member

...we should probably actually write down a style guide for docs, huh? maybe in CONTRIBUTING.md?

Probably. An example of some documentation on a method, a struct, and some do's and do not's would probably go a long way.

@guswynn
Copy link
Contributor Author

guswynn commented Jun 13, 2022

RFAL! I feel like I should re-export reload::Subscriber as reload::Filter as well, or name it something else and re-export as Subscriber

@guswynn guswynn force-pushed the reload-filter branch 3 times, most recently from 74dcb78 to 60088ac Compare June 13, 2022 21:51
@hawkw
Copy link
Member

hawkw commented Jun 13, 2022

RFAL! I feel like I should re-export reload::Subscriber as reload::Filter as well, or name it something else and re-export as Subscriber

instead of re-exporting it under two different names, i would probably just rename it to something more generic (like reload::Reload) and deprecate the existing name. Let's do that in a follow-up, though, because we can just do a hard rename on v0.2.x, and we'll need to do a deprecation on v0.1.x.

@guswynn
Copy link
Contributor Author

guswynn commented Jun 13, 2022

@hawkw okay! after this merges Ill do that rename!

@hawkw
Copy link
Member

hawkw commented Jun 14, 2022

@davidbarsky do you want to review the docs again, or shall i merge this?

@davidbarsky
Copy link
Member

@davidbarsky do you want to review the docs again, or shall i merge this?

I realized I missed a bit on the module documentation; lemme fix that. I think it should be good to go after, though!

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.

missed a few things, sorry!

tracing-subscriber/src/reload.rs Show resolved Hide resolved
tracing-subscriber/src/reload.rs Outdated Show resolved Hide resolved
@guswynn
Copy link
Contributor Author

guswynn commented Jun 15, 2022

Is this merge-able?

@hawkw hawkw enabled auto-merge (squash) June 15, 2022 18:22
@hawkw
Copy link
Member

hawkw commented Jun 15, 2022

Is this merge-able?

Yup, I'll get it merged and then backport to v0.1.x!

@hawkw hawkw merged commit 3046981 into tokio-rs:master Jun 15, 2022
@guswynn guswynn deleted the reload-filter branch June 16, 2022 15:26
hawkw pushed a commit that referenced this pull request Jun 22, 2022
## Motivation

The `reload` layer doesn't (and can't) implement downcasting correctly,
which breaks certain layers like the opentelemetry one.

## Solution

Most uses of the `reload` module (including mine) are just to change the
filter. Therefore, this PR implements `Filter` for `reload::Layer`
to allow users to not need to wrap the whole layer trait. Another
advantage of this is that the common-case critical sections are
shortened.
guswynn added a commit to guswynn/tracing that referenced this pull request Jun 23, 2022
…kio-rs#2159)

The `reload` subscriber doesn't (and can't) implement downcasting correctly,
which breaks certain subscribers like the opentelemetry one.

Most uses of the `reload` module (including mine) are just to change the
filter. Therefore, this PR implements `Filter` for `reload::Subscriber`
to allow users to not need to wrap the whole layer trait. Another
advantage of this is that the common-case critical sections are
shortened.
hawkw added a commit that referenced this pull request Jun 23, 2022
## Motivation

The `reload` layer doesn't (and can't) implement downcasting correctly,
which breaks certain `Layer`s which require downcasting (such as
`tracing-opentelemetry`'s `OpenTelemetryLayer`).

## Solution

Most usages of `reload` (including mine) are just to change a `Filter`,
so this PR implements `Filter` on the `reload::Layer` type to allow
users to not need to wrap the whole `Filtered` layer. Another advantage
of this is that the common-case critical sections are shortened

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this pull request Jun 29, 2022
# 0.3.12 (Jun 29, 2022)

This release of `tracing-subscriber` adds a new `Layer::event_enabled`
method, which allows `Layer`s to filter events *after* their field
values are recorded; a `Filter` implementation for `reload::Layer`, to
make using `reload` with per-layer filtering more ergonomic, and
additional inherent method downcasting APIs for the `Layered` type. In
addition, it includes dependency updates, and minor fixes for
documentation and feature flagging.

### Added

- **layer**: `Layer::event_enabled` method, which can be implemented to
  filter events based on their field values (#2008)
- **reload**: `Filter` implementation for `reload::Layer` (#2159)
- **layer**: `Layered::downcast_ref` and `Layered::is` inherent methods
  (#2160)

### Changed

- **parking_lot**: Updated dependency on `parking_lot` to 0.13.0
  (#2143)
- Replaced `lazy_static` dependency with `once_cell` (#2147)

### Fixed

- Don't enable `tracing-core` features by default (#2107)
- Several documentation link and typo fixes (#2064, #2068, #2077,
  #2161, #1088)

Thanks to @ben0x539, @jamesmunns, @georgemp, @james7132, @jswrenn,
@CAD97, and @guswynn for contributing to this release!
hawkw added a commit that referenced this pull request Jun 29, 2022
# 0.3.12 (Jun 29, 2022)

This release of `tracing-subscriber` adds a new `Layer::event_enabled`
method, which allows `Layer`s to filter events *after* their field
values are recorded; a `Filter` implementation for `reload::Layer`, to
make using `reload` with per-layer filtering more ergonomic, and
additional inherent method downcasting APIs for the `Layered` type. In
addition, it includes dependency updates, and minor fixes for
documentation and feature flagging.

### Added

- **layer**: `Layer::event_enabled` method, which can be implemented to
  filter events based on their field values (#2008)
- **reload**: `Filter` implementation for `reload::Layer` (#2159)
- **layer**: `Layered::downcast_ref` and `Layered::is` inherent methods
  (#2160)

### Changed

- **parking_lot**: Updated dependency on `parking_lot` to 0.13.0
  (#2143)
- Replaced `lazy_static` dependency with `once_cell` (#2147)

### Fixed

- Don't enable `tracing-core` features by default (#2107)
- Several documentation link and typo fixes (#2064, #2068, #2077,
  #2161, #1088)

Thanks to @ben0x539, @jamesmunns, @georgemp, @james7132, @jswrenn,
@CAD97, and @guswynn for contributing to this release!
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

3 participants