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

EnvFilter does not implement Filter #1868

Closed
lilyball opened this issue Jan 26, 2022 · 7 comments
Closed

EnvFilter does not implement Filter #1868

lilyball opened this issue Jan 26, 2022 · 7 comments
Labels
crate/subscriber Related to the `tracing-subscriber` crate kind/feature New feature or request

Comments

@lilyball
Copy link
Contributor

Bug Report

Version

tracing-subscriber v0.3.7

Crates

tracing-subscriber

Description

tracing_subscriber::layer::Filter is implemented for most various filter-like things I can think of, including LevelFilter, Targets, FilterFn, DynFilterFn, and the new combinators. But what is notably missing from this list is EnvFilter. I can apply a filter on top of EnvFilter using <EnvFilter as Layer>::with_filter(), but I can't apply an EnvFilter on top of an existing filter. This is a weird limitation and feels like a simple oversight.

@Eugeny
Copy link

Eugeny commented Feb 12, 2022

Workaround:

    let env_filter = Arc::new(EnvFilter::from_default_env());
    let fmt_layer = tracing_subscriber::fmt::layer()
        .with_filter(dynamic_filter_fn(move |m, c| {
            env_filter.enabled(m, c.clone())
        }));

@hawkw
Copy link
Member

hawkw commented Feb 17, 2022

This is a weird limitation and feels like a simple oversight.

Unfortunately, this is more than a simple oversight. EnvFilter does not implement the Filter trait because it implements dynamic filtering that requires keeping track of the currently entered span scope. At the moment, it does that through its implementation of the Layer trait, which allows it to implement methods like new_span, on_enter, and on_exit hooks.

In order to make EnvFilter implement Filter, it would need to be re-written to fit the Filter interface. Currently, Filters don't have callbacks for recording spans being created, entered, exited, or closed. Instead, the current trait requires that they query the wrapped Subscriber to get the current span scope. We could potentially change the implementation of EnvFilter to do this, but it hasn't happened yet because it's a large rewrite.

Alternatively, we could consider adding optional new_span, on_enter, on_exit, and on_close callbacks to the Filter trait, as well. This might be a good idea, as it would make it easier for Filters to implement dynamic span based filtering without having to ask the wrapped subscriber for the current context.

@Eugeny

Workaround:

    let env_filter = Arc::new(EnvFilter::from_default_env());
    let fmt_layer = tracing_subscriber::fmt::layer()
        .with_filter(dynamic_filter_fn(move |m, c| {
            env_filter.enabled(m, c.clone())
        }));

This workaround will work, _provided that the EnvFIlter is also added as a global filter layer. If it's not, it will not receive the new_span, on_enter, on_exit, and on_close callbacks it requires to perform dynamic filtering based on the span context. But, in that case, it will also perform global filtering, which defeats the point of also using it as a per-layer filter. In order for this workaround to actually work correctly, you would also need to wrap the EnvFilter that's added as a Layer in a wrapper struct that forwards all the Layer methods, but doesn't return false from enabled to disable a span or event globally. That way, you could use this workaround to use EnvFilter as a per-layer filter while still having dynamic span filtering work correctly.

@hawkw hawkw added crate/subscriber Related to the `tracing-subscriber` crate kind/feature New feature or request labels Feb 17, 2022
@Eugeny
Copy link

Eugeny commented Feb 17, 2022

@hawkw I can't explain why, but it works without adding it as a filter. Here's my complete setup:

fn init_logging() {
    if std::env::var("RUST_LOG").is_err() {
        std::env::set_var("RUST_LOG", "xxxxx=info")
    }

    let offset =
        UtcOffset::current_local_offset().unwrap_or(UtcOffset::from_whole_seconds(0).unwrap());

    let env_filter = Arc::new(EnvFilter::from_default_env());
    let fmt_layer = tracing_subscriber::fmt::layer()
        .with_timer(OffsetTime::new(
            offset,
            format_description::parse("[day].[month].[year] [hour]:[minute]:[second]").unwrap(),
        ))
        .with_filter(dynamic_filter_fn(move |m, c| {
            env_filter.enabled(m, c.clone())
        }));

    let r = tracing_subscriber::registry();

    #[cfg(debug_assertions)]
    let console_layer = console_subscriber::spawn();

    #[cfg(debug_assertions)]
    let r = r.with(console_layer);

    r.with(fmt_layer).init();
}

I'm using Registry to enable both FmtLayer and tokio-console.

With the above code, I can use RUST_LOG to filter things that go into the fmt layer, while console-subscriber still gets all spans (including trace level which is required for tokio-console to work).

@lilyball
Copy link
Contributor Author

@Eugeny Does it work for filter strings that depend on span names/fields?

@Eugeny
Copy link

Eugeny commented Feb 18, 2022

I've just started out using tracing, so not sure about that. It works when filtering by module name/prefix if that's what you mean.

@hawkw
Copy link
Member

hawkw commented Feb 18, 2022

It works when filtering by module name/prefix if that's what you mean.

I would expect this to work with the approach you are using.

It's specifically filters that match on the current spans (anything using the [...] syntax as described here) that will not work with this approach. If you don't need dynamic filtering based on spans, though, you could just use the Targets filter, instead; it implements the same module name prefix-based filtering as EnvFilter, but does not implement dynamic filtering based on spans.

Thus, the workaround does work with basic usage, but not for the features that are EnvFilter-specific. If dynamic filtering based on spans is not needed, though, you can just use Targets, which does implement the Filter trait, so there isn't actually a reason to use this workaround without the additional changes I described.

Does that help clear things up?

@Eugeny
Copy link

Eugeny commented Feb 22, 2022

Yep that explains it, thanks!

hawkw pushed a commit that referenced this issue Mar 8, 2022
Closes #1955

Call those methods in the `Layer` methods for `Filtered` and delegate them
in the filter combinators

## Motivation

Currently, if a `Filter` is going to implement dynamic filtering, it must
do this by querying the wrapped Subscriber for the current span context.
Since `Filter` only has callsite_enabled and enabled methods, a `Filter`
implementation cannot track the span context internally, the way a Layer
can.

Unfortunately, this means that the current implementation of `EnvFilter`
can only implement `Layer` (and provide global filtering). It cannot
currently implement `Filter`, because it stores span context data
internally. See #1868 for
details.

## Proposal

We should add `on_new_span`, `on_enter`, `on_exit`, and `on_close` methods to
`Filter`. This would allow implementing `Filter`s (such as `EnvFilter`) that
internally track span states.
hawkw pushed a commit that referenced this issue Mar 23, 2022
Closes #1955

Call those methods in the `Subscribe` methods for `Filtered` and
delegate them  in the filter combinators

## Motivation

Currently, if a `Filter` is going to implement dynamic filtering, it must
do this by querying the wrapped Subscriber for the current span context.
Since `Filter` only has callsite_enabled and enabled methods, a `Filter`
implementation cannot track the span context internally, the way a
subscriber can.

Unfortunately, this means that the current implementation of `EnvFilter`
can only implement `Subscribe` (and provide global filtering). It cannot
currently implement `Filter`, because it stores span context data
internally. See #1868 for
details.

## Proposal

We should add `on_new_span`, `on_enter`, `on_exit`, and `on_close` methods to
`Filter`. This would allow implementing `Filter`s (such as `EnvFilter`) that
internally track span states.
hawkw pushed a commit that referenced this issue Mar 23, 2022
Closes #1955

Call those methods in the `Subscribe` methods for `Filtered` and
delegate them  in the filter combinators

## Motivation

Currently, if a `Filter` is going to implement dynamic filtering, it must
do this by querying the wrapped Subscriber for the current span context.
Since `Filter` only has callsite_enabled and enabled methods, a `Filter`
implementation cannot track the span context internally, the way a
subscriber can.

Unfortunately, this means that the current implementation of `EnvFilter`
can only implement `Subscribe` (and provide global filtering). It cannot
currently implement `Filter`, because it stores span context data
internally. See #1868 for
details.

## Proposal

We should add `on_new_span`, `on_enter`, `on_exit`, and `on_close` methods to
`Filter`. This would allow implementing `Filter`s (such as `EnvFilter`) that
internally track span states.
hawkw pushed a commit that referenced this issue Mar 23, 2022
Closes #1955

Call those methods in the `Subscribe` methods for `Filtered` and
delegate them  in the filter combinators

## Motivation

Currently, if a `Filter` is going to implement dynamic filtering, it must
do this by querying the wrapped Subscriber for the current span context.
Since `Filter` only has callsite_enabled and enabled methods, a `Filter`
implementation cannot track the span context internally, the way a
subscriber can.

Unfortunately, this means that the current implementation of `EnvFilter`
can only implement `Subscribe` (and provide global filtering). It cannot
currently implement `Filter`, because it stores span context data
internally. See #1868 for
details.

## Proposal

We should add `on_new_span`, `on_enter`, `on_exit`, and `on_close` methods to
`Filter`. This would allow implementing `Filter`s (such as `EnvFilter`) that
internally track span states.
hawkw pushed a commit that referenced this issue Mar 24, 2022
Closes #1955

Call those methods in the `Subscribe` methods for `Filtered` and
delegate them  in the filter combinators

## Motivation

Currently, if a `Filter` is going to implement dynamic filtering, it must
do this by querying the wrapped Subscriber for the current span context.
Since `Filter` only has callsite_enabled and enabled methods, a `Filter`
implementation cannot track the span context internally, the way a
subscriber can.

Unfortunately, this means that the current implementation of `EnvFilter`
can only implement `Subscribe` (and provide global filtering). It cannot
currently implement `Filter`, because it stores span context data
internally. See #1868 for
details.

## Proposal

We should add `on_new_span`, `on_enter`, `on_exit`, and `on_close` methods to
`Filter`. This would allow implementing `Filter`s (such as `EnvFilter`) that
internally track span states.
hawkw pushed a commit that referenced this issue Mar 24, 2022
Closes #1955

Call those methods in the `Subscribe` methods for `Filtered` and
delegate them  in the filter combinators

## Motivation

Currently, if a `Filter` is going to implement dynamic filtering, it must
do this by querying the wrapped Subscriber for the current span context.
Since `Filter` only has callsite_enabled and enabled methods, a `Filter`
implementation cannot track the span context internally, the way a
subscriber can.

Unfortunately, this means that the current implementation of `EnvFilter`
can only implement `Subscribe` (and provide global filtering). It cannot
currently implement `Filter`, because it stores span context data
internally. See #1868 for
details.

## Proposal

We should add `on_new_span`, `on_enter`, `on_exit`, and `on_close` methods to
`Filter`. This would allow implementing `Filter`s (such as `EnvFilter`) that
internally track span states.
hawkw pushed a commit that referenced this issue Mar 24, 2022
Closes #1955

Call those methods in the `Subscribe` methods for `Filtered` and
delegate them  in the filter combinators

## Motivation

Currently, if a `Filter` is going to implement dynamic filtering, it must
do this by querying the wrapped Subscriber for the current span context.
Since `Filter` only has callsite_enabled and enabled methods, a `Filter`
implementation cannot track the span context internally, the way a
subscriber can.

Unfortunately, this means that the current implementation of `EnvFilter`
can only implement `Subscribe` (and provide global filtering). It cannot
currently implement `Filter`, because it stores span context data
internally. See #1868 for
details.

## Proposal

We should add `on_new_span`, `on_enter`, `on_exit`, and `on_close` methods to
`Filter`. This would allow implementing `Filter`s (such as `EnvFilter`) that
internally track span states.
hawkw pushed a commit that referenced this issue Mar 24, 2022
Closes #1955

Call those methods in the `Subscribe` methods for `Filtered` and
delegate them  in the filter combinators

## Motivation

Currently, if a `Filter` is going to implement dynamic filtering, it must
do this by querying the wrapped Subscriber for the current span context.
Since `Filter` only has callsite_enabled and enabled methods, a `Filter`
implementation cannot track the span context internally, the way a
subscriber can.

Unfortunately, this means that the current implementation of `EnvFilter`
can only implement `Subscribe` (and provide global filtering). It cannot
currently implement `Filter`, because it stores span context data
internally. See #1868 for
details.

## Proposal

We should add `on_new_span`, `on_enter`, `on_exit`, and `on_close` methods to
`Filter`. This would allow implementing `Filter`s (such as `EnvFilter`) that
internally track span states.
hawkw pushed a commit that referenced this issue Mar 24, 2022
Closes #1955

Call those methods in the `Subscribe` methods for `Filtered` and
delegate them  in the filter combinators

## Motivation

Currently, if a `Filter` is going to implement dynamic filtering, it must
do this by querying the wrapped Subscriber for the current span context.
Since `Filter` only has callsite_enabled and enabled methods, a `Filter`
implementation cannot track the span context internally, the way a
subscriber can.

Unfortunately, this means that the current implementation of `EnvFilter`
can only implement `Subscribe` (and provide global filtering). It cannot
currently implement `Filter`, because it stores span context data
internally. See #1868 for
details.

## Proposal

We should add `on_new_span`, `on_enter`, `on_exit`, and `on_close` methods to
`Filter`. This would allow implementing `Filter`s (such as `EnvFilter`) that
internally track span states.
hawkw pushed a commit that referenced this issue Mar 24, 2022
Closes #1955

Call those methods in the `Subscribe` methods for `Filtered` and
delegate them  in the filter combinators

## Motivation

Currently, if a `Filter` is going to implement dynamic filtering, it must
do this by querying the wrapped Subscriber for the current span context.
Since `Filter` only has callsite_enabled and enabled methods, a `Filter`
implementation cannot track the span context internally, the way a
subscriber can.

Unfortunately, this means that the current implementation of `EnvFilter`
can only implement `Subscribe` (and provide global filtering). It cannot
currently implement `Filter`, because it stores span context data
internally. See #1868 for
details.

## Proposal

We should add `on_new_span`, `on_enter`, `on_exit`, and `on_close` methods to
`Filter`. This would allow implementing `Filter`s (such as `EnvFilter`) that
internally track span states.
hawkw pushed a commit that referenced this issue Mar 24, 2022
Closes #1955

Call those methods in the `Subscribe` methods for `Filtered` and
delegate them  in the filter combinators

## Motivation

Currently, if a `Filter` is going to implement dynamic filtering, it must
do this by querying the wrapped Subscriber for the current span context.
Since `Filter` only has callsite_enabled and enabled methods, a `Filter`
implementation cannot track the span context internally, the way a
subscriber can.

Unfortunately, this means that the current implementation of `EnvFilter`
can only implement `Subscribe` (and provide global filtering). It cannot
currently implement `Filter`, because it stores span context data
internally. See #1868 for
details.

## Proposal

We should add `on_new_span`, `on_enter`, `on_exit`, and `on_close` methods to
`Filter`. This would allow implementing `Filter`s (such as `EnvFilter`) that
internally track span states.
hawkw pushed a commit that referenced this issue Mar 24, 2022
Closes #1955

Call those methods in the `Subscribe` methods for `Filtered` and
delegate them  in the filter combinators

## Motivation

Currently, if a `Filter` is going to implement dynamic filtering, it must
do this by querying the wrapped Subscriber for the current span context.
Since `Filter` only has callsite_enabled and enabled methods, a `Filter`
implementation cannot track the span context internally, the way a
subscriber can.

Unfortunately, this means that the current implementation of `EnvFilter`
can only implement `Subscribe` (and provide global filtering). It cannot
currently implement `Filter`, because it stores span context data
internally. See #1868 for
details.

## Proposal

We should add `on_new_span`, `on_enter`, `on_exit`, and `on_close` methods to
`Filter`. This would allow implementing `Filter`s (such as `EnvFilter`) that
internally track span states.
hawkw added a commit that referenced this issue Mar 25, 2022
## Motivation

Filtering by span and field requires using `EnvFilter` rather than
`Targets`. Per-layer filtering requires the `Filter` trait, which
`EnvFilter` does not implement.

## Solution

Implement the `Filter` trait for `EnvFilter`. PR #1973 adds additiional
methods to the `Filter` trait, which are necessary for `EnvFilter` to
implement dynamic span filtering. Now that those methods are added, we
can provide a `Filter` impl for `EnvFilter`.

In addition, we changed the globally-scoped `thread_local!` macro to use
a `ThreadLocal` struct as a field, so that multiple `EnvFilter`s used as
per-layer filters don't share a single span scope.

Fixes #1868
Follow-up on #1973

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this issue Mar 29, 2022
## Motivation

Filtering by span and field requires using `EnvFilter` rather than
`Targets`. Per-subscriber filtering requires the `Filter` trait, which
`EnvFilter` does not implement.

## Solution

Implement the `Filter` trait for `EnvFilter`. PR #1973 adds additiional
methods to the `Filter` trait, which are necessary for `EnvFilter` to
implement dynamic span filtering. Now that those methods are added, we
can provide a `Filter` impl for `EnvFilter`.

In addition, we changed the globally-scoped `thread_local!` macro to use
a `ThreadLocal` struct as a field, so that multiple `EnvFilter`s used as
per-subscriber filters don't share a single span scope.

Fixes #1868
Follow-up on #1973

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this issue Mar 29, 2022
## Motivation

Filtering by span and field requires using `EnvFilter` rather than
`Targets`. Per-subscriber filtering requires the `Filter` trait, which
`EnvFilter` does not implement.

## Solution

Implement the `Filter` trait for `EnvFilter`. PR #1973 adds additiional
methods to the `Filter` trait, which are necessary for `EnvFilter` to
implement dynamic span filtering. Now that those methods are added, we
can provide a `Filter` impl for `EnvFilter`.

In addition, we changed the globally-scoped `thread_local!` macro to use
a `ThreadLocal` struct as a field, so that multiple `EnvFilter`s used as
per-subscriber filters don't share a single span scope.

Fixes #1868
Follow-up on #1973

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this issue Mar 29, 2022
## Motivation

Filtering by span and field requires using `EnvFilter` rather than
`Targets`. Per-subscriber filtering requires the `Filter` trait, which
`EnvFilter` does not implement.

## Solution

Implement the `Filter` trait for `EnvFilter`. PR #1973 adds additiional
methods to the `Filter` trait, which are necessary for `EnvFilter` to
implement dynamic span filtering. Now that those methods are added, we
can provide a `Filter` impl for `EnvFilter`.

In addition, we changed the globally-scoped `thread_local!` macro to use
a `ThreadLocal` struct as a field, so that multiple `EnvFilter`s used as
per-subscriber filters don't share a single span scope.

Fixes #1868
Follow-up on #1973

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this issue Mar 29, 2022
## Motivation

Filtering by span and field requires using `EnvFilter` rather than
`Targets`. Per-subscriber filtering requires the `Filter` trait, which
`EnvFilter` does not implement.

## Solution

Implement the `Filter` trait for `EnvFilter`. PR #1973 adds additiional
methods to the `Filter` trait, which are necessary for `EnvFilter` to
implement dynamic span filtering. Now that those methods are added, we
can provide a `Filter` impl for `EnvFilter`.

In addition, we changed the globally-scoped `thread_local!` macro to use
a `ThreadLocal` struct as a field, so that multiple `EnvFilter`s used as
per-subscriber filters don't share a single span scope.

Fixes #1868
Follow-up on #1973

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw closed this as completed in f58019e Mar 29, 2022
liuhaozhan added a commit to liuhaozhan/tracing that referenced this issue Nov 17, 2022
Closes #1955

Call those methods in the `Subscribe` methods for `Filtered` and
delegate them  in the filter combinators

## Motivation

Currently, if a `Filter` is going to implement dynamic filtering, it must
do this by querying the wrapped Subscriber for the current span context.
Since `Filter` only has callsite_enabled and enabled methods, a `Filter`
implementation cannot track the span context internally, the way a
subscriber can.

Unfortunately, this means that the current implementation of `EnvFilter`
can only implement `Subscribe` (and provide global filtering). It cannot
currently implement `Filter`, because it stores span context data
internally. See tokio-rs/tracing#1868 for
details.

## Proposal

We should add `on_new_span`, `on_enter`, `on_exit`, and `on_close` methods to
`Filter`. This would allow implementing `Filter`s (such as `EnvFilter`) that
internally track span states.
kaffarell pushed a commit to kaffarell/tracing that referenced this issue May 22, 2024
…#1973)

Closes tokio-rs#1955

Call those methods in the `Layer` methods for `Filtered` and delegate them
in the filter combinators

## Motivation

Currently, if a `Filter` is going to implement dynamic filtering, it must
do this by querying the wrapped Subscriber for the current span context.
Since `Filter` only has callsite_enabled and enabled methods, a `Filter`
implementation cannot track the span context internally, the way a Layer
can.

Unfortunately, this means that the current implementation of `EnvFilter`
can only implement `Layer` (and provide global filtering). It cannot
currently implement `Filter`, because it stores span context data
internally. See tokio-rs#1868 for
details.

## Proposal

We should add `on_new_span`, `on_enter`, `on_exit`, and `on_close` methods to
`Filter`. This would allow implementing `Filter`s (such as `EnvFilter`) that
internally track span states.
kaffarell pushed a commit to kaffarell/tracing that referenced this issue May 22, 2024
Filtering by span and field requires using `EnvFilter` rather than
`Targets`. Per-layer filtering requires the `Filter` trait, which
`EnvFilter` does not implement.

Implement the `Filter` trait for `EnvFilter`. PR tokio-rs#1973 adds additiional
methods to the `Filter` trait, which are necessary for `EnvFilter` to
implement dynamic span filtering. Now that those methods are added, we
can provide a `Filter` impl for `EnvFilter`.

In addition, we changed the globally-scoped `thread_local!` macro to use
a `ThreadLocal` struct as a field, so that multiple `EnvFilter`s used as
per-layer filters don't share a single span scope.

Fixes tokio-rs#1868
Follow-up on tokio-rs#1973

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate/subscriber Related to the `tracing-subscriber` crate kind/feature New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants