tracing_subscriber: Expose .with_ansi() to custom event/field formatters

[lilyball]

Feature Request

Crates

tracing-subscriber

Motivation

The SubscriberBuilder::with_ansi() method only affects the default event formatter (tracing_subscriber::fmt::format::Format). This means tracing_subscriber::fmt().with_ansi(true).event_format(fmt_event).init() silently throws away the with_ansi flag. It also means that this flag isn't available to custom event formatters. And this flag isn't exposed to field formatters (not even the default field formatter, though DefaultFields doesn't currently care about this).

It would be great if instead of being a property of the default event formatter, it was a flag that was exposed to arbitrary FormatEvent and FormatFields implementations. This way if I do have a custom field formatter that cares about ANSI, there's a single source of truth for this, as well as having .event_format() not silently overwrite the .with_ansi() flag. This also makes it easier to integrate logic for only outputting colors to terminals as anything provided by tracing_subscriber directly would then propagate to custom event/field formatters.

Proposal

The simplest solution is probably to change FormatEvent and FormatFields to take a custom wrapper type instead of a fork &mut dyn Write. This wrapper type would still implement Write but would also expose the ansi flag. This way the event and field formatters will get the same flag, controlled by the .with_ansi() method, and custom field formatters can now be provided by crates as drop-in replacements easily.

Alternatives

The proposed change is a breaking one, but is the cleanest solution that comes to mind. A couple of alternatives come to mind:

Change Write to a trait with more semantic meaning

Right now, changing things like terminal colors requires replacing the event and field formatters, potentially discarding any custom logic there (e.g. if I have a field formatter that handles certain fields specially, and another one that adds color to fields, I can't combine the two). We could consider replacing Write with a trait that requires you to specify the nature of the data you're writing, such as write_key(_: &fmt::Arguments<'_>), write_value(_: &fmt::Arguments<'_>), write_separator(_: &fmt::Arguments<'_>), write_delimiter(_: &fmt::Arguments<'_>), etc. Then we can have a separate configurable "format decorator" that provides these implementations and picks appropriate escapes to use. See slog_term::RecordDecorator for an example of this approach. The complexity here is we'd need to figure out all possible semantic meanings that a format decorator would want to distinguish, but it would also allow a format decorator to e.g. emit XML instead (although such a decorator would have to handle e.g. a value with no key, and there's a bit of confusion there over who controls delimiter/separator strings, as an XML decorator would likely want to just use tags there but terminal output wants strings).

This is of course also a breaking change, and a much more significant one. It's unclear to me if it's worth the complexity in order to separate colors from text, though at the moment I would actually love to be able to do this as I'd like to change colors without losing any default behavior around custom handling of fields (such as DefaultFields skipping log.* fields and stripping r# prefixes and including error sources).

Expose the ansi flag via additional defaulted trait methods

We could extend RecordFields and FormatEvent with an additional defaulted fn set_ansi(enabled: Bool) (default impl doing nothing). This is classified as a "possibly breaking" change, in that code that has the trait in scope might throw ambiguity errors when calling the method if another in-scope trait defines the same method, and I suspect this is unlikely to affect downstream code. It's also not a particularly great API as it requires the implementation to maintain state.

Switch traits with a compatibility shim

We could do the original proposal but only expose this in new traits that replace FormatEvents and FormatFields. The new traits could have blanket impls for anything that implements the old traits, thus allowing existing code to keep working, and anything that cares about the ANSI flag would implement the new trait instead.

This should be backwards-compatible but is potentially confusing and requires coming up with new trait names.

[hawkw]

I agree that your proposed approach, wrapping the dyn Write trait object in a newtype, seems like the best one --- it would also give us a nice place to potentially put other things that may need to be passed in, in the future.

@lilyball, are you able to work on a PR for this change in the near future? We're currently working on preparing tracing-subscriber 0.3, which is necessary to resolve the recent security advisory for the chrono crate (currently a public API dependency in 0.2). This means that we have the opportunity to make other breaking API changes as well. So, if it's possible to get a branch together for that change soon, we can probably do this as part of 0.3.

[lilyball]

@hawkw Perhaps. I'm not sure yet. I'll see if it's something I can do.

[hawkw]

If you're not able to work on it, that's totally fine --- no pressure! I just figured I'd ask because it would certainly be nice to be able to get this change into 0.3, but if not, we can do it in the next breaking release.

[lilyball]

Sounds like based on the intended release time of 0.3 that there's not enough time for me to even find out if I can do a PR.

[hawkw]

@lilyball PR #1661 implements the breaking API change necessary for your proposed solution so that it can be merged in time for 0.3. Please feel free to take a look, if you like!

The actual propagation of the ANSI color configuration can be added in a non-breaking change once #1661 has been released as part of 0.3, so there's no rush for that part.

[hawkw]

Implemented in #1696