From 564ad32bbb584e8841b2aa49973094cbb7d09d8d Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Thu, 7 Apr 2022 17:07:40 -0700 Subject: [PATCH 1/2] subscriber: add inherent impls for `EnvFilter` methods ## Motivation Currently, there is a potential namespace resolution issue when calling `EnvFilter` methods that have the same name in the `Filter` and `Subscribe` traits (such as `enabled` and `max_level_hint`). When both `Filter` and `Subscribe` are in scope, the method resolution is ambiguous. See https://github.com/tokio-rs/tracing/pull/1983#issuecomment-1088984580 ## Solution This commit solves the problem by making the inherent method versions of these methods public. When the traits are in scope, name resolution will always select the inherent method prefer`entially, preventing the name clash. --- tracing-subscriber/src/filter/env/mod.rs | 156 ++++++++++++-------- tracing-subscriber/tests/env_filter/main.rs | 9 ++ 2 files changed, 106 insertions(+), 59 deletions(-) diff --git a/tracing-subscriber/src/filter/env/mod.rs b/tracing-subscriber/src/filter/env/mod.rs index a2fc0d112b..4c0b77c96a 100644 --- a/tracing-subscriber/src/filter/env/mod.rs +++ b/tracing-subscriber/src/filter/env/mod.rs @@ -226,6 +226,8 @@ impl EnvFilter { /// pub const DEFAULT_ENV: &'static str = "RUST_LOG"; + // === constructors, etc === + /// Returns a [builder] that can be used to configure a new [`EnvFilter`] /// instance. /// @@ -457,44 +459,19 @@ impl EnvFilter { self } - fn cares_about_span(&self, span: &span::Id) -> bool { - let spans = try_lock!(self.by_id.read(), else return false); - spans.contains_key(span) - } + // === filtering methods === - fn base_interest(&self) -> Interest { - if self.has_dynamics { - Interest::sometimes() - } else { - Interest::never() - } - } - - fn register_callsite(&self, metadata: &'static Metadata<'static>) -> Interest { - if self.has_dynamics && metadata.is_span() { - // If this metadata describes a span, first, check if there is a - // dynamic filter that should be constructed for it. If so, it - // should always be enabled, since it influences filtering. - if let Some(matcher) = self.dynamics.matcher(metadata) { - let mut by_cs = try_lock!(self.by_cs.write(), else return self.base_interest()); - by_cs.insert(metadata.callsite(), matcher); - return Interest::always(); - } - } - - // Otherwise, check if any of our static filters enable this metadata. - if self.statics.enabled(metadata) { - Interest::always() - } else { - self.base_interest() - } - } - - fn enabled(&self, metadata: &Metadata<'_>) -> bool { + /// Returns `true` if this `EnvFilter` would enable the provided `metadata` + /// in the current context. + /// + /// This is equivalent to calling the [`Subscribe::enabled`] or + /// [`Filter::enabled`] methods on `EnvFilter`'s implementations of those + /// traits, but it does not require the trait to be in scope. + pub fn enabled(&self, metadata: &Metadata<'_>, _: Context<'_, C>) -> bool { let level = metadata.level(); // is it possible for a dynamic filter directive to enable this event? - // if not, we can avoid the thread local access + iterating over the + // if not, we can avoid the thread loca'l access + iterating over the // spans in the current scope. if self.has_dynamics && self.dynamics.max_level >= *level { if metadata.is_span() { @@ -534,7 +511,15 @@ impl EnvFilter { false } - fn max_level_hint(&self) -> Option { + /// Returns an optional hint of the highest [verbosity level][level] that + /// this `EnvFilter` will enable. + /// + /// This is equivalent to calling the [`Subscribe::max_level_hint`] or + /// [`Filter::max_level_hint`] methods on `EnvFilter`'s implementations of those + /// traits, but it does not require the trait to be in scope. + /// + /// [level]: tracing_core::metadata::Level + pub fn max_level_hint(&self) -> Option { if self.dynamics.has_value_filters() { // If we perform any filtering on span field *values*, we will // enable *all* spans, because their field values are not known @@ -547,7 +532,12 @@ impl EnvFilter { ) } - fn on_new_span(&self, attrs: &span::Attributes<'_>, id: &span::Id) { + /// Informs the filter that a new span was created. + /// + /// This is equivalent to calling the [`Subscribe::on_new_span`] or + /// [`Filter::on_new_span`] methods on `EnvFilter`'s implementations of those + /// traits, but it does not require the trait to be in scope. + pub fn on_new_span(&self, attrs: &span::Attributes<'_>, id: &span::Id, _: Context<'_, C>) { let by_cs = try_lock!(self.by_cs.read()); if let Some(cs) = by_cs.get(&attrs.metadata().callsite()) { let span = cs.to_span_match(attrs); @@ -555,7 +545,12 @@ impl EnvFilter { } } - fn on_enter(&self, id: &span::Id) { + /// Informs the filter that the span with the provided `id` was entered. + /// + /// This is equivalent to calling the [`Subscribe::on_enter`] or + /// [`Filter::on_enter`] methods on `EnvFilter`'s implementations of those + /// traits, but it does not require the trait to be in scope. + pub fn on_enter(&self, id: &span::Id, _: Context<'_, C>) { // XXX: This is where _we_ could push IDs to the stack instead, and use // that to allow changing the filter while a span is already entered. // But that might be much less efficient... @@ -564,13 +559,23 @@ impl EnvFilter { } } - fn on_exit(&self, id: &span::Id) { + /// Informs the filter that the span with the provided `id` was exited. + /// + /// This is equivalent to calling the [`Subscribe::on_exit`] or + /// [`Filter::on_exit`] methods on `EnvFilter`'s implementations of those + /// traits, but it does not require the trait to be in scope. + pub fn on_exit(&self, id: &span::Id, _: Context<'_, C>) { if self.cares_about_span(id) { self.scope.get_or_default().borrow_mut().pop(); } } - fn on_close(&self, id: span::Id) { + /// Informs the filter that the span with the provided `id` was closed. + /// + /// This is equivalent to calling the [`Subscribe::on_close`] or + /// [`Filter::on_close`] methods on `EnvFilter`'s implementations of those + /// traits, but it does not require the trait to be in scope. + pub fn on_close(&self, id: span::Id, _: Context<'_, C>) { // If we don't need to acquire a write lock, avoid doing so. if !self.cares_about_span(&id) { return; @@ -579,6 +584,39 @@ impl EnvFilter { let mut spans = try_lock!(self.by_id.write()); spans.remove(&id); } + + fn cares_about_span(&self, span: &span::Id) -> bool { + let spans = try_lock!(self.by_id.read(), else return false); + spans.contains_key(span) + } + + fn base_interest(&self) -> Interest { + if self.has_dynamics { + Interest::sometimes() + } else { + Interest::never() + } + } + + fn register_callsite(&self, metadata: &'static Metadata<'static>) -> Interest { + if self.has_dynamics && metadata.is_span() { + // If this metadata describes a span, first, check if there is a + // dynamic filter that should be constructed for it. If so, it + // should always be enabled, since it influences filtering. + if let Some(matcher) = self.dynamics.matcher(metadata) { + let mut by_cs = try_lock!(self.by_cs.write(), else return self.base_interest()); + by_cs.insert(metadata.callsite(), matcher); + return Interest::always(); + } + } + + // Otherwise, check if any of our static filters enable this metadata. + if self.statics.enabled(metadata) { + Interest::always() + } else { + self.base_interest() + } + } } impl Subscribe for EnvFilter { @@ -593,13 +631,13 @@ impl Subscribe for EnvFilter { } #[inline] - fn enabled(&self, metadata: &Metadata<'_>, _: Context<'_, C>) -> bool { - self.enabled(metadata) + fn enabled(&self, metadata: &Metadata<'_>, ctx: Context<'_, C>) -> bool { + self.enabled(metadata, ctx) } #[inline] - fn on_new_span(&self, attrs: &span::Attributes<'_>, id: &span::Id, _: Context<'_, C>) { - self.on_new_span(attrs, id) + fn on_new_span(&self, attrs: &span::Attributes<'_>, id: &span::Id, ctx: Context<'_, C>) { + self.on_new_span(attrs, id, ctx) } fn on_record(&self, id: &span::Id, values: &span::Record<'_>, _: Context<'_, C>) { @@ -609,18 +647,18 @@ impl Subscribe for EnvFilter { } #[inline] - fn on_enter(&self, id: &span::Id, _: Context<'_, C>) { - self.on_enter(id); + fn on_enter(&self, id: &span::Id, ctx: Context<'_, C>) { + self.on_enter(id, ctx); } #[inline] - fn on_exit(&self, id: &span::Id, _: Context<'_, C>) { - self.on_exit(id); + fn on_exit(&self, id: &span::Id, ctx: Context<'_, C>) { + self.on_exit(id, ctx); } #[inline] - fn on_close(&self, id: span::Id, _: Context<'_, C>) { - self.on_close(id); + fn on_close(&self, id: span::Id, ctx: Context<'_, C>) { + self.on_close(id, ctx); } } @@ -629,8 +667,8 @@ feature! { impl subscribe::Filter for EnvFilter { #[inline] - fn enabled(&self, meta: &Metadata<'_>, _: &Context<'_, C>) -> bool { - self.enabled(meta) + fn enabled(&self, meta: &Metadata<'_>, ctx: &Context<'_, C>) -> bool { + self.enabled(meta, ctx.clone()) } #[inline] @@ -644,23 +682,23 @@ feature! { } #[inline] - fn on_new_span(&self, attrs: &span::Attributes<'_>, id: &span::Id, _: Context<'_, C>) { - self.on_new_span(attrs, id) + fn on_new_span(&self, attrs: &span::Attributes<'_>, id: &span::Id, ctx: Context<'_, C>) { + self.on_new_span(attrs, id, ctx) } #[inline] - fn on_enter(&self, id: &span::Id, _: Context<'_, C>) { - self.on_enter(id); + fn on_enter(&self, id: &span::Id, ctx: Context<'_, C>) { + self.on_enter(id, ctx); } #[inline] - fn on_exit(&self, id: &span::Id, _: Context<'_, C>) { - self.on_exit(id); + fn on_exit(&self, id: &span::Id, ctx: Context<'_, C>) { + self.on_exit(id, ctx); } #[inline] - fn on_close(&self, id: span::Id, _: Context<'_, C>) { - self.on_close(id); + fn on_close(&self, id: span::Id, ctx: Context<'_, C>) { + self.on_close(id, ctx); } } } diff --git a/tracing-subscriber/tests/env_filter/main.rs b/tracing-subscriber/tests/env_filter/main.rs index 3f06594769..d47c57e17e 100644 --- a/tracing-subscriber/tests/env_filter/main.rs +++ b/tracing-subscriber/tests/env_filter/main.rs @@ -189,3 +189,12 @@ fn span_name_filter_is_dynamic() { finished.assert_finished(); } + +#[test] +fn method_name_resolution() { + #[allow(unused_imports)] + use tracing_subscriber::subscribe::{Filter, Subscribe}; + + let filter = EnvFilter::new("hello_world=info"); + filter.max_level_hint(); +} From a02042e189dd6fcd012d2e799ac312aad5c2985d Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Fri, 8 Apr 2022 09:26:58 -0700 Subject: [PATCH 2/2] fix broken links Signed-off-by: Eliza Weisman --- tracing-subscriber/src/filter/env/mod.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tracing-subscriber/src/filter/env/mod.rs b/tracing-subscriber/src/filter/env/mod.rs index 4c0b77c96a..aafb2ea254 100644 --- a/tracing-subscriber/src/filter/env/mod.rs +++ b/tracing-subscriber/src/filter/env/mod.rs @@ -11,7 +11,7 @@ mod field; use crate::{ filter::LevelFilter, - subscribe::{self, Context, Subscribe}, + subscribe::{Context, Subscribe}, sync::RwLock, }; use directive::ParseError; @@ -664,8 +664,9 @@ impl Subscribe for EnvFilter { feature! { #![all(feature = "registry", feature = "std")] + use crate::subscribe::Filter; - impl subscribe::Filter for EnvFilter { + impl Filter for EnvFilter { #[inline] fn enabled(&self, meta: &Metadata<'_>, ctx: &Context<'_, C>) -> bool { self.enabled(meta, ctx.clone())