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: impl Clone for EnvFilter #2956

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Benjamin-L
Copy link

Motivation

In #2360, people have requested impl Clone for EnvFilter in order to use EnvFilter as a clap argument. In conduwuit, we want this in order to do something like this:

let filter_layer: EnvFilter = ...;
let subscriber = Registry::default()
    .with(console_subscriber::spawn())
    .with(logging_subscriber_a.with_filter(filter_layer.clone()))
    .with(logging_subscriber_b.with_filter(filter_layer.clone()))

tokio-console's console_subscriber layer needs to be unfiltered in order to pick up tokio trace events, but we have several "logging-type" layers that we want to apply the configured EnvFilter to.

We considered the workaround of constructing several fresh EnvFilters with EnvFilter::try_new(config.log.clone()), but would prefer to just clone the EnvFilter itself and not have to deal with repeating the parsing/error handling.

Solution

We generally expect users to be cloning an EnvFilter before attaching it to a subscriber, rather than cloning EnvFilters that are already attached. Because of this, we reset all the accumulated dynamic state when cloning. This means that some spans and callsites might be missed when an already-attached EnvFilter is cloned, but the presence of the dynamic state mean that detaching and attaching EnvFilters to existing subscribers (e.g. with reload) already doesn't work very well. This isn't a new class of problem.

There was a previous implementation of this in #2398, that shared the dynamic state between all cloned filters behind an Arc. I chose not do go for that approach because it causes inconsistencies if the cloned filters are attached to different subscribers.

A third option would be to clone the dynamic state, but this gets kinda messy with lock poisoning. I can't think of a scenario where that behavior would actually be better.

This is useful when using `EnvFilter` for multiple identical per-layer
filters, as well as with clap and similar libraries that have `Clone`
bounds.

We generally expect users to be cloning an `EnvFilter` before attaching it
to a subscriber, rather than cloning `EnvFilters` that are already
attached. Because of this, we reset all the accumulated dynamic state
when cloning. This means that some spans and callsites might be missed
when an already-attached `EnvFilter` is cloned, but the presence of the
dynamic state mean that detaching and attaching `EnvFilter`s to existing
subscribers (e.g. with `reload`) already doesn't work very well. This
isn't a new class of problem.

There was a previous implementation of this in tokio-rs#2398, that shared the
dynamic state between all cloned filters behind an `Arc`. I chose
not do go for that approach because it causes inconsistencies if the
cloned filters are attached to different subscribers.

Fixes: tokio-rs#2360
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.

None yet

1 participant