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

feat(subscriber) add filter env var builder parameter #276

Merged
merged 8 commits into from
Feb 18, 2022

Conversation

mujpao
Copy link
Contributor

@mujpao mujpao commented Feb 8, 2022

Add a new builder method to console_subscriber::Builder to configure the filter environment variable to use for tracing events.

closes #206

Add a new builder method to `console_subscriber::Builder` to configure
the filter environment variable to use for `tracing` events.

closes tokio-rs#206
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.

Overall, the implementation looks right. I had some naming and documentation suggestions.

It would probably also be good to mention something about this method in the documentation for Builder::init, where it discusses the RUST_LOG environment variable:

/// In addition to the [`ConsoleLayer`], which collects instrumentation data
/// consumed by the console, the default [`Subscriber`][sub] initialized by this
/// function also includes a [`tracing_subscriber::fmt`] layer, which logs
/// tracing spans and events to stdout. Which spans and events are logged will
/// be determined by the `RUST_LOG` environment variable.

and maybe also here:

/// | `RUST_LOG` | Configures what events are logged events. See [`Targets`] for details. | "error" |

it would be good if that documentation mentioned that the environment variable can be changed using this method.

Thanks for working on this; once the docs and naming changes are addressed, I think this will be good to merge!

/// Sets the filter environment variable for tracing events.
///
/// By default, this is `RUST_LOG`.
pub fn filter_env_variable(self, filter_env_variable: impl Into<String>) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

I think maybe we should rename this to filter_env_var rather than filter_env_variable? It feels a little bit weird to me that we abbreviate "environment" to "env" but we don't abbreviate "variable" to "var", especially since the stdlib function is std::env::var rather than std::env::variable.

i know i was the one who suggested this name originally, sorry about that.

console-subscriber/src/builder.rs Outdated Show resolved Hide resolved
@hawkw hawkw enabled auto-merge (squash) February 18, 2022 18:31
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.

i went ahead and made the remaining changes, this LGTM!

@hawkw hawkw merged commit dbdb149 into tokio-rs:main Feb 18, 2022
hawkw added a commit that referenced this pull request Feb 18, 2022
#### Features

*  add `Builder::filter_env_var` builder parameter (#276)
   ([dbdb149](dbdb149), closes [#206](206))

#### Bug Fixes

*  record timestamps for updates last (#289) ([703f1aa](703f1aa),
   closes [#266](266))
*  use monotonic `Instant`s for all timestamps (#288)
   ([abc0830](abc0830), closes [#286](286))
*  bail rather than panic when encountering clock skew (#287)
   ([24db8c6](24db8c6), closes [#286](286))
*  fix compilation on targets without 64-bit atomics (#282)
   ([5590fdb](5590fdb), closes [#279](279))
hawkw added a commit that referenced this pull request Feb 18, 2022
#### Features

*  add `Builder::filter_env_var` builder parameter (#276)
   ([dbdb149](dbdb149), closes [#206](206))

#### Bug Fixes

*  record timestamps for updates last (#289) ([703f1aa](703f1aa),
   closes [#266](266))
*  use monotonic `Instant`s for all timestamps (#288)
   ([abc0830](abc0830), closes [#286](286))
*  bail rather than panic when encountering clock skew (#287)
   ([24db8c6](24db8c6), closes [#286](286))
*  fix compilation on targets without 64-bit atomics (#282)
   ([5590fdb](5590fdb), closes [#279](279))
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.

subscriber: consider adding filter env var builder parameter
2 participants