An experimental rewrite of tracing-subscriber filter providers

[CAD97]

Repository with backing code: https://github.com/CAD97/tracing-filter
See preview docs live: https://cad97.github.io/tracing-filter/tracing_filter/

So, I've implemented a new effectively replacement for EnvFilter. First off:

Why not use tracing-filter?

• tracing-filter is highly experimental
• tracing-filter is not officially supported by the tracing team
• tracing-filter is not published to crates-io
• tracing-filter works with the unpublished tracing 0.2.0 ecosystem
  • backport coming eventually

Why use tracing-filter?

• More configurable than [email protected]'s EnvFilter
• You want your runtime filter syntax to work for serialized event queries
• You like the author and want them to feel proud of themself
• We have nice miette-powered errors 😄

And now that that's out of the way,

Wait, what is this?

What it says on the tin: I want to replace EnvFilter. To that end, I've implemented a 100% bug-for-bug compatible version of EnvFilter, along with another filter provider 100% compatible with env_filter's filter format. (Unfortunately, they aren't quite drop-in compatible, but this is in service of better Developer eXperience using them.)

Motivation

Congratulations, you have the dubious honor of knowing this code [EnvFilter] better than I do... — @hawkw

Due to what could be described as "historical accident," EnvFilter's format is... underspecified, to say the least, and its parser leaves much to be desired. While target[span] is the normal filter we all know and love, EnvFilter also allows such unusual filter directives as [span]target, [span][span], [[[[[[[[[[[span], and [{/* you can put anything here; it's ignored */}]. Having a more principled implementation allows us to understand and perhaps improve some of these "incidental features" and give EnvFilter some love.

Also, the new parser gives nice miette-powered diagnostics for ignored directives:

Error:
  × some directives had no effect

Error:
  ⚠ 2 directives were ignored as invalid

Error: tracing_filter::legacy::UnclosedSpan

  ⚠ unclosed span directive
   ╭────
 1 │ target[span,[{key=(?:wow){2}}]
   ·       ┬    ▲
   ·       │    ╰── stopped looking here
   ·       ╰── opened here
   ╰────
Error: tracing_filter::legacy::InvalidRegex

  ⚠ invalid regex specified
   ╭────
 1 │ target[span,[{key=(?:wow){2}}]
   ·                   ────┬────
   ·                       ╰── regex parse error:
    (?:wow){2
           ^^
error: unclosed counted repetition
   ╰────

To note: these diagnostics are first pass, and can likely be improved further with some more effort. (Notably, zkat/miette#139.) Send some ⭐s zkat's way for the wonderful library!

(Side note: while writing this I'm jumping around in tracing-filter's code pulling notes and examples, and I keep finding things, going "wait, that can't be right", testing them, and finding out that no, this is in fact the upstream behavior. There is a laundry list of low-hanging fruit for improvement to the EnvFilter format exposed by this port, and they're all technically breaking...)

Future 🦄 👀

A new filter string format?

While working on tracing-filter, I've been sketching a "query¹" language that can be used to better specify what events and spans to include (or exclude!) and would love to see it developed. Notably, it's much more structured than the existing ad-hoc extension to env_logger's format, which puts it in a great place to integrate with the experimental valuable support. Of course, 100% backwards compatibility with common² env_logger filters will be provided; just the legacy EnvFilter syntax will be replaced³.

Improvements to the legacy format?

While I personally am more interested and invested in the "query" format, having the much more principled implementation will allow for targeted improvements (say, multiple fields in a given [span] directive?) — and, given the improved understanding of the parser's behavior, perhaps these can even be non-breaking improvements for everyone.

Post-facto filtering?

The real meat on the bone. While this is a stretch goal to a stretch goal, I'd love to shim the current Filter trait with a version that can work on serialized forms of events. This would allow cool new applications, such as recording the structured event stream in the json format, and then later using query filters to filter down the data set to the events actually necessary to diagnose a traced incident.

Other?

I have an open tracking discussion where I want to hear your potential use cases!

Integration

There's three main approaches (and one "status quo" approach) that I see:

Upstream into tracing-subscriber

Pros

• Everyone benefits from the improvements
• Fully official
• Can replace EnvFilter directly (in-place, even, with the compatible format)
• Benefits from tracing's doubleporting efforts for 0.1 and 0.2 ecosystems

Cons

• Filter formats are orthogonal enough a concern from the subscriber machinery that they're plug-and-play with the Filter trait
• tracing-filter is already decently complicated to support two formats, and the third will require even more involved support (efficient evaluation of the general format basically requires NFA compilation⁴)
• Currently uses a public dependency on miette, which might not be desirable
• Takes velocity/credit away from the primary author (@CAD97) living inside another crate

Live in the tracing repo as tracing-filter (or similar)

Pros

• Officially endorsed, documentation can reference it freely
• Decently discoverable as an official component
• More reviewing eyes to keep code quality up to par
• Benefits from tokio/tracing's CI infrastructure
• Benefits from tracing's doubleporting efforts for 0.1 and 0.2 ecosystems

Cons

• Velocity limited by tracing core developers' time
  • @CAD97 currently has a surplus of time due to circumstances
  • But this only really matters until the two stretch goals are done, being the query filter and post-facto filtering

Live in external repo, officially endorsed/recommended solution

Pros

• Velocity not limited by tracing dev's bandwidth
• Can migrate to be part of the repo in the future

Cons

• No second pair of reviewing eyes
• Limited support for backporting from tracing 0.2 to tracing 0.1

External crate, no official endorsement (but a note in the docs)

Pros

• @CAD97 gets full control
• MAXIMUM velocity

Cons

• Discoverability is limited
• @CAD97 has full responsibility (bus factor = 1)
  • I've been known to procrastinate publishing updates for my crates for months at a time... 😅

What next?

Ideally, four things:

• We agree that this is a good direction
• We determine how we want to treat tracing-filter (this includes the future of EnvFilter)
• Land Add Subscribe::event_enabled to conditionally dis/enable events based on fields #2008 so filtering on event field contents is possible
• I bring forward a proposal for the structured query format, and we decide how to precede with that

Also, disclaimer: assuming this work is "greenlit" to continue with official tracing's blessing, I currently plan to apply for a (small, individual) Project Grant from the Rust Foundation for this work (convenient timing!). I honestly expect to not get one (this is at best of tertiary benefit to the Rust Ecosystem, and it currently looks more focused on more directly rust-lang/rust adjacent work), but might as well through my hat in the ring.

Footnotes

1. Strictly speaking, it's not a real query language, as it cannot do data mapping (it only does the "filter" in map/filter/reduce), but it's a decent informal description of how it feels, so the moniker has stuck. ↩

2. Unfortunately, true 100% compatibility is untractable, due to env_logger's very permissive format. However, any directive of form rust::module::path=level will continue to work as in env_logger (i.e. only custom specified targets are potentially impacted). Also, /regex filters from env_logger apply globally, and should be replaced with the query filters' support for the same functionality. Of course, you can always use the simple filter for true 100% compatibility with env_logger. ↩

3. I could be talked into supporting a sensible subset of EnvFilter's syntax, as it is disjoint to common env_logger syntax (see previous footnote) and also doesn't overlap the planned way of specifying query filters. However, the point of a new format is to remove legacy jank, so (hopefully) no legacy jank is getting inherited. ↩

4. I have compiler engineer brain. Every program is a compiler. Every data structure is a graph. I am not taking questions w.r.t. this footnote. ↩

[hawkw]

Thanks for working on this, this is lovely!

Another option for officially endorsing the new tracing-filter code, if we don't upstream it into tracing-subscriber directly, is that we could have tracing-filter live in a separate repository in the tokio-rs github organization. In general, I would like to prefer that new "officially-endorsed" tracing crates have their own separate repositories under tokio-rs, and we've been considering moving some of the existing non-core crates to their own new repositories.

The primary reason for this is that the current tokio-rs/tracing CI configuration builds and tests every crate in the repository (some of which also have additional special build and test steps) on all pull requests, although a vast majority of branches only change a single crate. Moving most crates to their own repos would decrease the amount of work that happens on CI significantly, reducing time spent waiting for builds. Also, it would reduce the frequency of situations where a build failure for one crate prevents totally unrelated PRs from being merged due to CI failures, which occurs a lot when e.g. a new Clippy version introduces new lints, or an API used by one tracing crate is deprecated externally.

[hawkw]

Regarding the question of whether or not to upstream tracing-filter into tracing-subscriber, I think the primary benefit is by far the ability to replace the existing EnvFilter code with tracing-filter. If tracing-filter is a strictly better replacement for EnvFilter, it really seems like a shame to continue maintaining EnvFilter and having it be in a place where it's immediately visible to new users, while the superior alternative tracing-filter is less discoverable.

Regarding the cons you listed:

• Currently uses a public dependency on miette, which might not be desirable

If we're going to upstream the code into tracing-subscriber directly, I think we probably don't want to expose miette directly in the public API if possible. If we can easily wrap the types exposed from miette with our own wrappers that expose the required functionality, that would be preferable.

• Takes velocity/credit away from the primary author (@CAD97) living inside another crate

Regarding this, it's definitely important to me that as much credit is given for this work as possible, and I'd be happy to do whatever you think is appropriate to ensure that you receive credit for all your work on this. Also, if any tracing-filter code is merged into a tracing repository, I would ask that we add @CAD97 to the .github/CODEOWNERS file for that code --- I would like to ensure that any new changes to your code are reviewed by that code's original author whenever possible.

One of the biggest arguments against upstreaming into tracing-subscriber is that the tracing-subscriber crate already contains a lot of code, and is kind of a heavyweight dependency (depending on filter flags). In particular, it's unfortunate that the crate currently plays kind of a dual role, containing both core trait definitions (Layer, Filter) and various implementations of those traits (e.g. fmt, EnvFilter, etc). I don't love the current situation where crates offering alternative implementations of those traits have to depend on a crate that contains existing implementations (e.g. it doesn't make sense that tracing-filter has to depend on the crate that contains the implementation of EnvFilter).

One solution I've been considering is factoring out the core trait definitions for Layer and Filter into a separate tracing-subscriber-core crate (or similar) which tracing-subscriber would depend on and re-export. That way, tracing-subscriber would become a sort of facade crate offering a large selection of implementations as a sort of "one stop shop" for including tracing in an application, while implementations of the Layer and Filter traits would be able to depend only on tracing-subscriber-core and not have to worry about disabling all of tracing-subscriber's other feature flags.

This would also allow us to have a separate tracing-filter crate that depends on the trait definitions from tracing-subscriber-core and is re-exported by tracing-subscriber, improving discoverability and letting us replace EnvFilter while still allowing users to depend on just tracing-filter as a separate crate. If we did this, we could even have tracing-filter live in its own repository while still being part of tracing-subscriber.

[CAD97]

One solution I've been considering is factoring out the core trait definitions for Layer and Filter into a separate tracing-subscriber-core crate (or similar) which tracing-subscriber would depend on and re-export. That way, tracing-subscriber would become a sort of facade crate offering a large selection of implementations as a sort of "one stop shop" for including tracing in an application, while implementations of the Layer and Filter traits would be able to depend only on tracing-subscriber-core and not have to worry about disabling all of tracing-subscriber's other feature flags.

This would also allow us to have a separate tracing-filter crate that depends on the trait definitions from tracing-subscriber-core and is re-exported by tracing-subscriber, improving discoverability and letting us replace EnvFilter while still allowing users to depend on just tracing-filter as a separate crate. If we did this, we could even have tracing-filter live in its own repository while still being part of tracing-subscriber.

This sounds useful! Definitely the "everything bagel" of tracing-subscriber is my main worry of tracing-filter getting "lost" in it, so explicitly factoring it out into a diamond shape¹ would help directly mitigate that.

have tracing-filter live in a separate repository in the tokio-rs github organization

This sounds like the best of both worlds deal! Honestly, I'm not sure why I didn't think of that as a possibility.

If y'all are open to it, I'm down to set up CI jobs in the tracing-recommended manner and transfer the repository over to the org, if that's a fair representation of the state of things (roughly, experimental, but an endorsed experiment?).

The one limitation to both of the above, though, is that I'm not sure how tractable it is in tracing-0.1, or if this is a when-tracing-0.2 kind of thing... but you know more about that (both tracing-0.2 plans and GitHub organization organization) than I do.

encapsulating miette

Yeah, this is definitely desirable and something that's been on the back of my mind for the best way to handle. The way miette is currently recommended to use in libraries is to expose your impl Error + miette::Diagnostic error type. Notably, formatting for this type works as it does for normal #[derive(Debug, Error)]; it's only when you format a Diagnostic with a ReportHandler that you get formatted output; the common way to do so is with miette's Report type.

There's really not strong existing practice/consensus for how to handle impl Errors which are more involved than a single line description² plus optional pointer to the source impl Error; the only real existing practice here is for application error types (anyhow, eyre). (And with provide-any coming down the pipeline, that'll change best practice a bit as well, so...)

That said, I think providing an opaque error type and writing some examples is probably the next thing to do for tracing-filter, while event_enabled cooks and I stew on a proposal for the "query filter" format some more.

Footnotes

1. probably: subscriber->fmt,filter,log, fmt,filter; fmt,filter->subscriber-core. (Interesting that tracing-fmt used to exist and is now bundled with subscriber!) ↩

2. Plus, it seems like an ideal way to handle parsing warnings from RUST_LOG would be to set up the tracing collector and then warn! the warnings, and multiiline messages always look funny as well. ↩

[CAD97]

One notable thing splitting into multiple repos loses is a unified docs preview, though I suppose that might be able to be recovered with some clever GHA work?

[davidbarsky]

If y'all are open to it, I'm down to set up CI jobs in the tracing-recommended manner and transfer the repository over to the org, if that's a fair representation of the state of things (roughly, experimental, but an endorsed experiment?).

The one limitation to both of the above, though, is that I'm not sure how tractable it is in tracing-0.1, or if this is a when-tracing-0.2 kind of thing... but you know more about that (both tracing-0.2 plans and GitHub organization organization) than I do.

Thankfully, this is independent of tracing 0.1/0.2, but if you're able to do this, we'd greatly appreciate it. It might just take a second for the repo to moved under Tokio, but feel free to add Eliza and I under your repo until it's under the Tokio org.

One notable thing splitting into multiple repos loses is a unified docs preview, though I suppose that might be able to be recovered with some clever GHA work?

I'm not sure that this is the biggest loss, but we can probably get 90% of the way there through some clever Netlify routing. I wouldn't worry about it, tbh.