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

subscriber: warn if trying to enable a statically disabled level #990

Merged
merged 10 commits into from
Oct 6, 2020
Merged
3 changes: 2 additions & 1 deletion tracing-subscriber/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ keywords = ["logging", "tracing", "metrics", "subscriber"]
[features]

default = ["env-filter", "smallvec", "fmt", "ansi", "chrono", "tracing-log", "json"]
env-filter = ["matchers", "regex", "lazy_static"]
env-filter = ["matchers", "regex", "lazy_static", "tracing"]
Copy link
Member

Choose a reason for hiding this comment

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

Not sure tracing needs to be another feature flag on this. I don't see this flag being used elsewhere and I think that a tracing dependency is a forgone conclusion for tracing-subscriber.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This won't compile without adding a tracing dependency:

error[E0433]: failed to resolve: use of undeclared type or module `tracing`
   --> tracing-subscriber/src/filter/env/mod.rs:245:13
    |
245 |         use tracing::level_filters::STATIC_MAX_LEVEL;
    |             ^^^^^^^ use of undeclared type or module `tracing`
error[E0433]: failed to resolve: use of undeclared type or module `tracing`
   --> tracing-subscriber/src/filter/env/mod.rs:245:13
    |
245 |         use tracing::level_filters::STATIC_MAX_LEVEL;
    |             ^^^^^^^ use of undeclared type or module `tracing`

Copy link
Member

Choose a reason for hiding this comment

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

I'd like to hear what @hawkw says, but I'm not sure tracing merits being an optional dependency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see, you want to make it always a dependency instead of optional. Sure, I can do that.

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 the tracing dependency should be optional and only enabled by the "env-filter" feature. Otherwise, there's no reason to depend on tracing --- the rest of the crate only uses tracing-core.

I think @davidbarsky misinterpreted this change as adding a new feature flag "tracing" that users could independently enable. That's not what this change is doing --- it simply makes the "env-filter" feature enable a dependency on tracing that isn't otherwise necessary.

IMO the original code was correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I reverted it to the original code.

fmt = ["registry"]
ansi = ["fmt", "ansi_term"]
registry = ["sharded-slab", "thread_local"]
Expand All @@ -34,6 +34,7 @@ json = ["tracing-serde", "serde", "serde_json"]
tracing-core = { path = "../tracing-core", version = "0.2" }

# only required by the filter feature
tracing = { optional = true, path = "../tracing", version = "0.2" }
matchers = { optional = true, version = "0.0.1" }
regex = { optional = true, version = "1", default-features = false, features = ["std"] }
smallvec = { optional = true, version = "1" }
Expand Down
4 changes: 2 additions & 2 deletions tracing-subscriber/src/filter/env/directive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,10 @@ use tracing_core::{span, Level, Metadata};
// TODO(eliza): add a builder for programmatically constructing directives?
#[derive(Debug, Eq, PartialEq)]
pub struct Directive {
target: Option<String>,
jyn514 marked this conversation as resolved.
Show resolved Hide resolved
in_span: Option<String>,
fields: FilterVec<field::Match>,
level: LevelFilter,
pub(crate) target: Option<String>,
pub(crate) level: LevelFilter,
}

/// A directive which will statically enable or disable a given callsite.
Expand Down
90 changes: 90 additions & 0 deletions tracing-subscriber/src/filter/env/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,96 @@ impl EnvFilter {
}

fn from_directives(directives: impl IntoIterator<Item = Directive>) -> Self {
use tracing::level_filters::STATIC_MAX_LEVEL;
use tracing::Level;

let directives: Vec<_> = directives.into_iter().collect();

let disabled: Vec<_> = directives
.iter()
.filter(|directive| directive.level > STATIC_MAX_LEVEL)
.collect();

if !disabled.is_empty() {
#[cfg(feature = "ansi_term")]
use ansi_term::{Color, Style};
// NOTE: We can't use a configured `MakeWriter` because the EnvFilter
// has no knowledge of any underlying subscriber or collector, which
// may or may not use a `MakeWriter`.
let warn = |msg: &str| {
#[cfg(not(feature = "ansi_term"))]
let msg = format!("warning: {}", msg);
#[cfg(feature = "ansi_term")]
let msg = {
let bold = Style::new().bold();
let mut warning = Color::Yellow.paint("warning");
warning.style_ref_mut().is_bold = true;
format!("{}{} {}", warning, bold.clone().paint(":"), bold.paint(msg))
};
eprintln!("{}", msg);
};
let ctx_prefixed = |prefix: &str, msg: &str| {
#[cfg(not(feature = "ansi_term"))]
let msg = format!("note: {}", msg);
#[cfg(feature = "ansi_term")]
let msg = {
let mut equal = Color::Fixed(21).paint("="); // dark blue
equal.style_ref_mut().is_bold = true;
format!(" {} {} {}", equal, Style::new().bold().paint(prefix), msg)
};
eprintln!("{}", msg);
};
let ctx_help = |msg| ctx_prefixed("help:", msg);
let ctx_note = |msg| ctx_prefixed("note:", msg);
let ctx = |msg: &str| {
#[cfg(not(feature = "ansi_term"))]
let msg = format!("note: {}", msg);
#[cfg(feature = "ansi_term")]
let msg = {
let mut pipe = Color::Fixed(21).paint("|");
pipe.style_ref_mut().is_bold = true;
format!(" {} {}", pipe, msg)
};
eprintln!("{}", msg);
};
warn("some trace filter directives would enable traces that are disabled statically");
for directive in disabled {
let target = if let Some(target) = &directive.target {
format!("the `{}` target", target)
} else {
"all targets".into()
};
let level = directive
.level
.clone()
.into_level()
.expect("=off would not have enabled any filters");
ctx(&format!(
"`{}` would enable the {} level for {}",
directive, level, target
));
}
ctx_note(&format!("the static max level is `{}`", STATIC_MAX_LEVEL));
let help_msg = || {
let (feature, filter) = match STATIC_MAX_LEVEL.into_level() {
Some(Level::TRACE) => unreachable!(
"if the max level is trace, no static filtering features are enabled"
),
Some(Level::DEBUG) => ("max_level_debug", Level::TRACE),
Some(Level::INFO) => ("max_level_info", Level::DEBUG),
Some(Level::WARN) => ("max_level_warn", Level::INFO),
Some(Level::ERROR) => ("max_level_error", Level::WARN),
None => return ("max_level_off", String::new()),
};
(feature, format!("{} ", filter))
};
let (feature, earlier_level) = help_msg();
ctx_help(&format!(
"to enable {}logging, remove the `{}` feature",
earlier_level, feature
));
}

let (dynamics, mut statics) = Directive::make_tables(directives);
let has_dynamics = !dynamics.is_empty();

Expand Down