subscriber: add Targets::targets method to iterate directives

[connec]

There's currently no way to get the registered directives from a Targets instance, which is a barrier to some use-cases such as combining user-supplied directives with application-supplied defaults.

This commit adds a Targets::targets method, which returns an iterator of (&str, LevelFilter) pairs, one for each directive (excluding the default level, whose target is None).

Items are yielded as per the underlying DirectiveSet::directives, which would yield itms in specifity order (as constructed by
DirectiveSet::add). However, the order has been left explicitly undefined in the documentation for Targets::targets on the basis that this is an implementation detail that may change.

Motivation

As hinted in the commit message, my use-case is to have an application-supplied 'base' Targets filter, which can then be extended/overridden by a user-supplied filter parsed from RUST_LOG (e.g.).

Sadly, there's currently no way of getting the directives out of a Targets instance, nor to re-serialize to the string representation. Thus, the only way to implement the above would be to manually parse the user-supplied filters in the application, which is shame since it duplicates the implementation here and would be prone to drift/subtle incompatibilities.

Solution

The proposed solution is to add a method to Targets to retrieve the underlying directives.

// application-defined defaults, for a useful level of information
let mut filter = Targets::new()
    .with_target("my_crate", LevelFilter::INFO)
    .with_target("important_dependency", LevelFilter::INFO);

if let Ok(overrides) = std::env::var("RUST_LOG") {
    let overrides: Targets = overrides.parse().unwrap();

    // merge user-supplied overrides, which can enable additional tracing or disable default tracing
    filter.extend(overrides.targets());
    //                      ^^^^^^^^^ this is new
}

For this PR, I've gone for fn targets(&self) -> impl Iterator<Item = (&str, LevelFilter)> + '_, e.g. a method targets that returns an iterator over the pairs of (target, filter) in the underlying directives. The iterator excludes any default level(s) where target would be None. This matches nicely with the FromIterator and Extend impls, which use (impl Into<String>, impl Into<LevelFilter>).

There are, of course, a number of other ways this could be achieved. Some alternatives I considered:

• An iterator over (String, LevelFilter), whilst a more direct correspondence to FromIterator and Extend impls, would lead to redundant cloning. In particular, &str impl Into<String>, so the iterator is compatible with Targets::with_targets, Extend, and FromIterator.
• Different method names, such as directives. directives is perhaps more aligned with the implementation, but the API talks of adding targets, so the chosen name seems a better fit.
• Implementing IntoIterator for &Targets. This felt like a more heavyweight option, and to be usable there would need to be some kind of iter(&self) method anyway. Given that the default level (if any) isn't included in the iteration, it felt better to go for some target-specific.
• Implementing Display for Targets. This would allow multiple Targets to be stringified and joined with ,, then parsed back up to a combined Targets. However this approach feels inefficient and introduces additional type-level fallibility (hypothetical parse errors have to be dealt with even if the combined targets were all valid).

Obviously happy to bike-shed anything here, opening a PR seemed like a good way to explain the use-case along with a possible solution!

[connec]

I should add I've targeted the v0.1.x branch. I'm not sure if that's the correct process, but Targets doesn't currently exist on master.

[connec]

As an additional anecdote, I originally faced this issue with EnvFilter which has a similar limitation. However there is a workaround since it implements Display, so you can stringify the base and concat the overrides:

let base: EnvFilter = todo!();
let overrides: EnvFilter = todo!();

let filter: EnvFilter = format!("{},{}", base, overrides).parse().unwrap();

It still seems preferable to allow iteration, since this feels wasteful and introduces unncecesary type-level fallibility (e.g. when parsing a filter composed from multiple valid filters, the potential for error must be dealt with).

If this PR gets accepted I'd be happy to follow-up with an equivalent for EnvFilter, if desired.

[hawkw]

Overall, this looks good to me! I had one small suggestion, that we might want to consider adding an IntoIterator impl for Targets, but I'm not sure if this is worth the effort.

In re: your other comments:

I should add I've targeted the v0.1.x branch. I'm not sure if that's the correct process, but Targets doesn't currently exist on master.

Yes, this is correct. We'll make sure this change gets ported forward to v0.2.x when the other changes it depends on are ported.

However, the order has been left explicitly undefined in the documentation for Targets::targets on the basis that this is an implementation detail that may change.

This is correct IMO.

• Implementing Display for Targets. This would allow multiple Targets to be stringified and joined with ,, then parsed back up to a combined Targets. However this approach feels inefficient and introduces additional type-level fallibility (hypothetical parse errors have to be dealt with even if the combined targets were all valid).

I think we probably should have a Display implementation for Targets, but I completely agree that it's not the right way to combine multiple Targets filters. Instead, it should just be used in cases where it's necessary to display the filter to a user in the filter string format.

Regarding combining multiple Targets filters, I have some additional thoughts. It seems like using the iterator + extend is probably a good working solution for now. However, we might want to consider adding a new method that specifically takes a Targets instance and combines it with an existing Targets. That would let us use Vec::append rather than iterating, and we'd be able to move the Strings out of the added Targets rather than allocating new strings for the borrowed &strs in the iterator. However, I don't think this is all that important...typically, I don't think a program is going to be combining lots of Targets filters over and over again in a hot loop; it's typically something that happens once on application startup, so the inefficiency is probably fine.

[hawkw]

WDYT about also adding an impl IntoIterator for &'_ Targets that calls this? it seems like it might be nice to be able to write

let targets = // ...
for (target, level) in &targets {
     // ...
}

however, the downside is that this would probably require returning a named type rather than an impl Iterator. the best way to do that would probably be to change DirectiveSet::directives to return a std::slice::Iter rather than impl Iterator, here

tracing/tracing-subscriber/src/filter/directive.rs

Line 65 in b917104

pub(crate) fn directives(&self) -> impl Iterator<Item = &T> {

and then adding an Iter type in this module like

pub struct Iter<'a>(
    std::iter::FilterMap<
        std::slice::Iter<'a, StaticDirective>,
        fn(&'a StaticDirective) -> Option<(&'a str, LevelFilter)>,
    >
);

and an impl Iterator for it.

this is a big pile of boilerplate code, so I'm not totally convinced that it's worth the effort just to be able to add an IntoIterator impl so people can for loop over an &Targets...

however, a named iterator type might also be nice if users also want to be able to return a named iterator type when composing the returned iterator from Targets with other iterator adapters. i'm not sure if there's enough of a use-case for that to matter, though. and, I think it's probably okay to go back and change this in the future if anyone does need a named return type: replacing an impl Trait return value with a named type is probably not a breaking change as long as the new return type implements the same trait (and has the correct lifetime etc).

what do you think?

[connec]

Thank you for your rapid and thorough feedback!

Not at all opposed to adding this. My two reservations with IntoIterator, which may be moot, were:

1. I was influenced by the implementation to consider whether the default LevelFilter should be yielded by an IntoIterator implementation, in order to properly iterate all the contained directives, but I don't think that would be very useful (but perhaps a getter for the default level would be useful? – as an orthogonal concern).

2. To have the tracing_subscriber::filter::targets::Iter type, the tracing_subscriber::filter::targets module would have to be made public. That would change the canonical location Targets itself. Not a breaking change since the re-export would remain, but something that felt 'too big' for a speculative PR 😅

As I say, if these are both non-issues I'm happy to add this as described.

[connec]

Thinking more on this and your additional thoughts above, if we did add IntoIterator for &Targets, I'd be inclined to add IntoIterator for Targets as well, yielding (String, LevelFilter). That way we would also tick the box for having a way to merge Targets without cloning.

[hawkw]

Yeah, that could be nice. But, I'm on the fence about whether or not this is really worth the effort. It's up to you --- I'd happily merge this change as-is.

I'm not concerned about the canonical module path change (as long as the re-export doesn't go away), but I agree that we should probably add a way to access the default level if we're making the targets iterable. It would be fine to do that separately, though.

[connec]

No worries – I'll put in a commit with both, and we can see how it looks.

[connec]

So I wound up amending the first commit, to replace Targets::targets with Targets::iter, which delegates to IntoIterator for &Targets.

Implementing the owned iterator turned out to be a little less straightforward, since the underlying iterator depends on the smallvec feature. The neatest solution was to implement IntoIterator for DirectiveSet as well, to keep the feature handling inside the directive module. The actual iteration of pairs was also a little awkward – I'm not sure if there's a better way than the loop-based approach I've gone for.

(On the flip side, I'm not entirely sure if constructing a FilterMap on every call to next in the borrowed iterator is great either, but figured I'd lean into 'zero-cost abstractions'...)

Anyway, this seems a bit more consistent and unlocks access to the owned targets, which is nice! Let me know what you think.

[connec]

Ah, I just noticed your suggestion embedded the FilterMap. Wasn't sure if that would work, but let me try it out.

[connec]

OK, sorry for the kerfuffle – pushed again using FilterMap inside targets::Iter and targets::IntoIter, much cleaner I think.

[connec]

Pushed again, fixing all your comments I think.

(I also noted the point about rebasing in the CONTRIBUTING.md, so I've added the changes as separate commits.)

[hawkw]

lovely, thanks for addressing the last feedback! I'm going to go ahead and merge this.