subscriber: unify interface of `enabled` functions

[har7an]

across all implementations in traits (for Filter and Context) and associated functions for structs.

The API from the Filter trait takes a &Context, while individual implementations previously took Context by value. This was likely an oversight because a) the Context argument is ignored in a few places and not used at all and b) Context is Clone so can be turned into a value from a reference when required.

Motivation

I noticed this inconsistency while trying to implement a complex filter with DynFilterFn. The DynFilterFn::new function passes a &Context to the closure, which conforms to the API defined in the Filter trait. Part of my filter is an EnvFilter, which has an associated function enabled defined on the struct (i.e. next to the Filter impl). Quoting from the docs:

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.

Since the Context argument is taken by value in some implementations but by reference in others, this is not true. So I decided to make a proposal to change this. Since it is part of the public API, I'm afraid it will break existing code that has already arranged with this.

Solution

Changes all enabled functions in the codebase to take their Context argument by reference.

Here's a quick regex to check if all have been caught (although it's not multiline):

rg -e "fn enabled\(.*Context"

[har7an]

I know the CONTRIBUTING says to add tests, but I wouldn't know what a sensible test for this looks like. If someone has a suggestion I'll add it.

[hawkw]

Thanks for the PR. Since this is an API-breaking change, it will have to wait until the next SemVer-incompatible release of tracing-subscriber. But, I think it's a good idea to make this more consistent, so we'll want to include this patch in v0.4 when we're ready to publish it!

I know the CONTRIBUTING says to add tests, but I wouldn't know what a sensible test for this looks like. If someone has a suggestion I'll add it.

I don't think it's necessary to add tests for this change, because, you're right, there's nothing here that it would make sense to test. Generally, tests are more of a requirement for PRs that add new features or fix bugs; this kind of API change is not really possible to test.

[mladedav]

@hawkw

Since this is an API-breaking change, it will have to wait until the next SemVer-incompatible release of tracing-subscriber

To make it clear, this is ready to be merged since there are breaking changes in master, it will just take some time for this to be actually realeased, right?

[davidbarsky]

To make it clear, this is ready to be merged since there are breaking changes in master, it will just take some time for this to be actually realeased, right?

We don't want to merge this into master beyond the already existing set of tracing 0.2 changes that exist on master. Once we're closer to release, we can merge this in. I've added this to the tracing 0.2 milestone so that we don't forget.