Skip to content

Commit

Permalink
subscriber: add inherent impls for EnvFilter methods (#2057)
Browse files Browse the repository at this point in the history 
## Motivation

Currently, there is a potential namespace resolution issue when calling
`EnvFilter` methods that have the same name in the `Filter` and
`Layer` traits (such as `enabled` and `max_level_hint`). When both
`Filter` and `Layer` are in scope, the method resolution is
ambiguous.

See #1983 (comment)

## 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.

Signed-off-by: Eliza Weisman <[email protected]>
  • Loading branch information
@hawkw
hawkw committed Apr 8, 2022
1 parent 6f8f56d commit cd8d613
Show file tree
Hide file tree
Showing 2 changed files with 109 additions and 61 deletions.
161 changes: 100 additions & 61 deletions tracing-subscriber/src/filter/env/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ mod field;

use crate::{
filter::LevelFilter,
layer::{self, Context, Layer},
layer::{Context, Layer},
sync::RwLock,
};
use directive::ParseError;
Expand Down Expand Up @@ -228,6 +228,8 @@ impl EnvFilter {
/// [`EnvFilter::try_from_default_env`]: #method.try_from_default_env
pub const DEFAULT_ENV: &'static str = "RUST_LOG";

// === constructors, etc ===

/// Returns a [builder] that can be used to configure a new [`EnvFilter`]
/// instance.
///
Expand Down Expand Up @@ -460,44 +462,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 [`Layer::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<S>(&self, metadata: &Metadata<'_>, _: Context<'_, S>) -> 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() {
Expand Down Expand Up @@ -537,7 +514,15 @@ impl EnvFilter {
false
}

fn max_level_hint(&self) -> Option<LevelFilter> {
/// Returns an optional hint of the highest [verbosity level][level] that
/// this `EnvFilter` will enable.
///
/// This is equivalent to calling the [`Layer::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<LevelFilter> {
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
Expand All @@ -550,15 +535,25 @@ 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 [`Layer::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<S>(&self, attrs: &span::Attributes<'_>, id: &span::Id, _: Context<'_, S>) {
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);
try_lock!(self.by_id.write()).insert(id.clone(), span);
}
}

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 [`Layer::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<S>(&self, id: &span::Id, _: Context<'_, S>) {
// 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...
Expand All @@ -567,13 +562,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 [`Layer::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<S>(&self, id: &span::Id, _: Context<'_, S>) {
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 [`Layer::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<S>(&self, id: span::Id, _: Context<'_, S>) {
// If we don't need to acquire a write lock, avoid doing so.
if !self.cares_about_span(&id) {
return;
Expand All @@ -582,6 +587,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<S: Subscriber> Layer<S> for EnvFilter {
Expand All @@ -596,13 +634,13 @@ impl<S: Subscriber> Layer<S> for EnvFilter {
}

#[inline]
fn enabled(&self, metadata: &Metadata<'_>, _: Context<'_, S>) -> bool {
self.enabled(metadata)
fn enabled(&self, metadata: &Metadata<'_>, ctx: Context<'_, S>) -> bool {
self.enabled(metadata, ctx)
}

#[inline]
fn on_new_span(&self, attrs: &span::Attributes<'_>, id: &span::Id, _: Context<'_, S>) {
self.on_new_span(attrs, id)
fn on_new_span(&self, attrs: &span::Attributes<'_>, id: &span::Id, ctx: Context<'_, S>) {
self.on_new_span(attrs, id, ctx)
}

fn on_record(&self, id: &span::Id, values: &span::Record<'_>, _: Context<'_, S>) {
Expand All @@ -612,28 +650,29 @@ impl<S: Subscriber> Layer<S> for EnvFilter {
}

#[inline]
fn on_enter(&self, id: &span::Id, _: Context<'_, S>) {
self.on_enter(id);
fn on_enter(&self, id: &span::Id, ctx: Context<'_, S>) {
self.on_enter(id, ctx);
}

#[inline]
fn on_exit(&self, id: &span::Id, _: Context<'_, S>) {
self.on_exit(id);
fn on_exit(&self, id: &span::Id, ctx: Context<'_, S>) {
self.on_exit(id, ctx);
}

#[inline]
fn on_close(&self, id: span::Id, _: Context<'_, S>) {
self.on_close(id);
fn on_close(&self, id: span::Id, ctx: Context<'_, S>) {
self.on_close(id, ctx);
}
}

feature! {
#![all(feature = "registry", feature = "std")]
use crate::layer::Filter;

impl<S> layer::Filter<S> for EnvFilter {
impl<S> Filter<S> for EnvFilter {
#[inline]
fn enabled(&self, meta: &Metadata<'_>, _: &Context<'_, S>) -> bool {
self.enabled(meta)
fn enabled(&self, meta: &Metadata<'_>, ctx: &Context<'_, S>) -> bool {
self.enabled(meta, ctx.clone())
}

#[inline]
Expand All @@ -647,23 +686,23 @@ feature! {
}

#[inline]
fn on_new_span(&self, attrs: &span::Attributes<'_>, id: &span::Id, _: Context<'_, S>) {
self.on_new_span(attrs, id)
fn on_new_span(&self, attrs: &span::Attributes<'_>, id: &span::Id, ctx: Context<'_, S>) {
self.on_new_span(attrs, id, ctx)
}

#[inline]
fn on_enter(&self, id: &span::Id, _: Context<'_, S>) {
self.on_enter(id);
fn on_enter(&self, id: &span::Id, ctx: Context<'_, S>) {
self.on_enter(id, ctx);
}

#[inline]
fn on_exit(&self, id: &span::Id, _: Context<'_, S>) {
self.on_exit(id);
fn on_exit(&self, id: &span::Id, ctx: Context<'_, S>) {
self.on_exit(id, ctx);
}

#[inline]
fn on_close(&self, id: span::Id, _: Context<'_, S>) {
self.on_close(id);
fn on_close(&self, id: span::Id, ctx: Context<'_, S>) {
self.on_close(id, ctx);
}
}
}
Expand Down
9 changes: 9 additions & 0 deletions tracing-subscriber/tests/env_filter/main.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,15 @@ fn span_name_filter_is_dynamic() {
finished.assert_finished();
}

#[test]
fn method_name_resolution() {
#[allow(unused_imports)]
use tracing_subscriber::layer::{Filter, Layer};

let filter = EnvFilter::new("hello_world=info");
filter.max_level_hint();
}

// contains the same tests as the first half of this file
// but using EnvFilter as a `Filter`, not as a `Layer`
mod per_layer_filter {
Expand Down

0 comments on commit cd8d613

Please sign in to comment.