subscriber: update pretty formatter for no ansi

[cratelyn]

Background

Currently, when the Pretty event formatter is being used, it does not change
its output when the with_ansi flag is set to false by the CollectorBuilder.

Overview

While this formatter is generally used in situations such as local development,
where ANSI escape codes are more often acceptable, there are some situations in
which this can lead to mangled output.

This commit makes some minor changes to account for this ansi flag when
formatting events using Pretty.

Becuase ANSI codes were previously used to imply the event level using colors,
this commit additionally modifies Pretty so that it respects display_level
when formatting an event.

Now, installing a subscriber like so...

    tracing_subscriber::fmt()
        .pretty()
        .with_thread_names(true)
        .with_ansi(false)
        .with_level(true)
        .with_max_level(tracing::Level::TRACE)
        .init();

...will lead to logs that look like this:

Changes

• Changes to <Format<Pretty, T> as FormatEvent<C, N>>::format_event

• Implement LevelNames for Pretty, copying Full's
  implementation.

• Add a PrettyVisitor::ansi boolean flag, used in its Visit
  implementation.

  • Add a new PrettyVisitor::with_ansi builder pattern method to
    facilitate this.

Out of Scope

One detail worth nothing is that this does not solve the problem of fields
being formatted without ANSI codes. Configuring a subscriber using this snippet
would still lead to bolded fields in parent spans.

tracing_subscriber::fmt()
    .pretty()
    .with_ansi(false)
    .with_level(true)
    .with_max_level(tracing::Level::TRACE)
    .init();

This can be worked around by using a different field formatter, via
.fmt_fields(tracing_subscriber::fmt::format::DefaultFields::new()) in the
short-term.

In the long-term, #658 is worth investigating further.

[cratelyn]

Alas! I've missed some CI checks. I'll resolve these later today 🙂

[hawkw]

Alas! I've missed some CI checks. I'll resolve these later today slightly_smiling_face

I think the checks that are failing are the ones that test various feature combinations. In particular, it looks like the new code doesn't compile when the ansi feature flag is disabled. I think we want to add #[cfg(feature = "ansi")] attributes to the code that constructs Styles, similar to the other formatters:

tracing/tracing-subscriber/src/fmt/format/mod.rs

Lines 607 to 612 in 8720848

	#[cfg(feature = "ansi")]
	let dimmed = if self.ansi {
	Style::new().dimmed()
	} else {
	Style::new()
	};

[hawkw]

overall, this looks great --- I think the only issue is that we need to correctly handle the "ansi" feature flag being disabled at compile-time, which should fix CI. once that's taken care of, this should be good to merge!

One detail worth nothing is that this does not solve the problem of fields
being formatted without ANSI codes. Configuring a subscriber using this snippet
would still lead to bolded fields in parent spans.

tracing_subscriber::fmt()
    .pretty()
    .with_ansi(false)
    .with_level(true)
    .with_max_level(tracing::Level::TRACE)
    .init();

This can be worked around by using a different field formatter, via
.fmt_fields(tracing_subscriber::fmt::format::DefaultFields::new()) in the
short-term.

In the long-term, #658 is worth investigating further.

I think a potential short term solution to this is just adding a bit to the docs explaining that the Pretty field formatter always includes ANSI color codes, so that users who are using the Pretty event formatter know that if they want to disable ANSI colors, they need to provide a different field formatter. Ideally, we definitely want this to just work correctly, but in the meantime it would be good to at least document the current behavior.

We might also want to add a new public PrettyFields field formatter type that makes the PrettyVisitor type (which would have to be made public, but its constructors could remain private). This could have an option to enable/disable ANSI, so that users can get the same style formatting with ANSI color codes disabled, rather than using the default field formatter. Users would have to write something like this:

tracing_subscriber::fmt()
    .pretty()
    .with_ansi(false)
    .fmt_fields(format::PrettyFields::new().with_ansi(false))
    // ... other settings ...
    .init();

which is a little bit redundant, but at least it would be possible to get consistent punctuation without having to reimplement the Pretty field formatter in user code.

We could make that change in a separate, follow-up PR.

[hawkw]

nit, take it or leave it: we could factor this out into an inherent method on PrettyVisitor, since it shows up a couple of times. not a blocker though, up to you!

[cratelyn]

Fixed this in 3c6b46c ✨

[hawkw]

Any updates on this one?

[cratelyn]

Any updates on this one?

Oh my goodness was this actually 10 days ago? I'll address the feedback above and should have this ready for another look very soon. My apologies!

[hawkw]

No rush, just wanted to make sure it wasn't blocked on me :)

[cratelyn]

No rush, just wanted to make sure it wasn't blocked on me :)

Not at all! Blocked on "my brain, she is very sick."

[cratelyn]

Pardon, I'm a smidge unfamiliar with the CI setup for tracing. It looks like the failures in warnings, test-versions, test-msrv, et. al. are unrelated, mostly something to do with async_fn_with_async_trait cases?

Going off of the "Merging can be performed automatically with 1 approving review." I'm guessing those are optional CI jobs, but I wanted to make sure I was understanding that correctly.

[hawkw]

Pardon, I'm a smidge unfamiliar with the CI setup for tracing. It looks like the failures in warnings, test-versions, test-msrv, et. al. are unrelated, mostly something to do with async_fn_with_async_trait cases?

Sorry, this was an unrelated issue due to an upstream change that broke the async-trait support for tracing-attributes. It's been fixed now; if we update this with the latest master, i think we should be able to get a clean CI build.

[cratelyn]

Sounds good! I'll update this branch now 🚀

[hawkw]

Latest CI failure is a known flaky test that i should really probably just remove. I'll restart the build.

[hawkw]

overall, this looks good! it might be nice to have a test for whether or not ANSI escape codes are present in output when they're disabled this way, but it's not a blocker.

it would be nice to remove the "ansi" feature flag requirement from the Pretty formatter, but that's a bigger change, so we can do that in a future PR. currently the code in other formatters for handling the "ansi" feature flag is quite messy. we might want to consider an approach that would be a bit neater, like having a module with a no-op Style type in it (all the same methods as ansi_term's Style, but they just do nothing), when the feature is not set, and reexporting ansi_term::Style when it is set. this way, we wouldn't need to stick nearly as many cfg attributes in the actual formatter code to handle the feature-flagging.

anyway, that's just a thought, this looks great! thank you!

[hawkw]

can we remove the "ansi" feature flag now that this doesn't depend on ANSI formatting? we would have to change it to default to no-ANSI formatting when the ansi feature flag is disabled, and update the code so that all the use of Styles is feature flagged, like the other formatters.

it would be fine to do this in a follow-up PR after this change lands.

[cratelyn]

I would love to do that! If it's alright, I'd like to do that in a follow-up PR, as I suspect it'll be slightly involved. If it works for you, feel free to create and assign an issue for me 🙂