subscriber: propagate ANSI config via `Writer` #1696

hawkw · 2021-11-03T00:17:02Z

Motivation

Currently, whether tracing-subscriber's fmt subscriber will ANSI
formatting escape codes use is configured on the Format type. This
means that the configuration is honored by the event formatters
implemented in tracing-subscriber, but is not easily exposed to those
in other crates. Additionally, it's not currently easy to expose the
configuration to the field formatter, so it's difficult to implement
field formatters that use ANSI escape codes conditionally.

Issue #1651 suggested a new API for this, where the writer that's passed
in to the event and field formatters provides a method for checking if
ANSI escape codes are supported.

Solution

This branch adds a new method to the Writer type added in #1661. The
FormatEvent and FormatFields implementations can call
Writer::has_ansi_escapes to determine whether ANSI escape codes are
supported. This is also propagated to FormattedFields, so that it can
be determined when adding new fields to a preexisting set of formatted
fields.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

hawkw · 2021-11-03T00:17:14Z

cc @lilyball

davidbarsky · 2021-11-03T00:23:26Z

will review tomorrow, but this is a breaking change, yeah? my skim of this seems to imply that.

hawkw · 2021-11-03T17:03:36Z

will review tomorrow, but this is a breaking change, yeah? my skim of this seems to imply that.

It shouldn't be a breaking change. The breaking change (replacing the &mut dyn Write with the Writer type) was already made in #1661 in preparation for this change.

The current APIs for configuring ANSI formatting support are still supported. All this branch does is add methods on Writer for exposing whether or not ANSI colors are supported to the FormatEvent and FormatFields implementations.

The fmt::Subscriber type will now expose the with_ansi method regardless of whether or not the formatter is an instance of the Format type, but this is not a breaking change, since that method will still be there in all cases where it previously was.

We may want to deprecate the Format::with_ansi method in favor of using the fmt::Subscriber::with_ansi method, which will expose the ANSI formatting configuration to all formatters, not just the Format type's formatters. But, enabling ANSI escapes via that method does still work.

hawkw · 2021-11-03T17:05:13Z

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

-impl<'a> fmt::Debug for PrettyVisitor<'a> {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        f.debug_struct("PrettyVisitor")
-            .field("writer", &format_args!("<dyn fmt::Write>"))
-            .field("is_empty", &self.is_empty)
-            .field("result", &self.result)
-            .field("style", &self.style)
-            .field("ansi", &self.ansi)
-            .finish()


This debug impl can now be derived.

hawkw · 2021-11-03T17:06:06Z

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

@@ -530,7 +550,10 @@ impl<F, T> Format<F, T> {

    /// Enable ANSI terminal colors for formatted output.
    pub fn with_ansi(self, ansi: bool) -> Format<F, T> {


we may want to deprecate this method, since the fmt::Subscriber::with_ansi method is probably more useful in most cases.

lilyball · 2021-11-06T07:48:54Z

Thanks for doing this! I'm curious if there's any potential issue with calling .json().with_ansi(true) since that will leave it enabled. I assume not, as .json() overrides both the event and field formatters and presumably those don't attempt to write ANSI escapes, so it would only affect cases where someone then overrides one or both formatters after the .json() call (which would be odd).

I suppose in theory the json formatters could be updated such that a subsequent call to .with_ansi(true) would actually make it syntax-highlight the json, but that would have a rather niche use and is unlikely to be worth implementing (not to mention it might be surprising in utility code that enables ansi regardless of the specific event/format fields).

hawkw · 2021-11-06T17:14:51Z

I suppose in theory the json formatters could be updated such that a subsequent call to .with_ansi(true) would actually make it syntax-highlight the json, but that would have a rather niche use and is unlikely to be worth implementing (not to mention it might be surprising in utility code that enables ansi regardless of the specific event/format fields).

Yeah...this is something we could do, but I think it might be better off left as a JSON-specific option, for the reason you mentioned --- the behavior could be somewhat surprising if it's not explicitly enabled. If we wanted to support a human-readable JSON mode, we could also expose the option to enable serde-json's pretty serializer, to pretty-print the JSON objects with whitespace.

I'm not sure if either of these are all that important, since a user could also do something like piping the output into a command like jq . to pretty-print the JSON. But, we could certainly add it as an option...

davidbarsky

this looks good to me.

This backports PR #1696 to v0.1.x. ## Motivation Currently, whether `tracing-subscriber`'s `fmt` subscriber will ANSI formatting escape codes use is configured on the `Format` type. This means that the configuration is honored by the event formatters implemented in `tracing-subscriber`, but is not easily exposed to those in other crates. Additionally, it's not currently easy to expose the configuration to the field formatter, so it's difficult to implement field formatters that use ANSI escape codes conditionally. Issue #1651 suggested a new API for this, where the writer that's passed in to the event and field formatters provides a method for checking if ANSI escape codes are supported. ## Solution This branch adds a new method to the `Writer` type added in #1661. The `FormatEvent` and `FormatFields` implementations can call `Writer::has_ansi_escapes` to determine whether ANSI escape codes are supported. This is also propagated to `FormattedFields`, so that it can be determined when adding new fields to a preexisting set of formatted fields. Fixes #1651 Signed-off-by: Eliza Weisman <eliza@buoyant.io>

Depends on #1696 ## Motivation PR #1696 adds a new API for propagating whether or not ANSI formatting escape codes are enabled to the event formatter and field formatter via the `Writer` type. This means that it's now quite easy to make field formatting also include ANSI formatting escape codes when ANSI formatting is enabled. Currently, `tracing-subscriber`'s default field formatter does not use ANSI formatting --- ANSI escape codes are currently only used for parts of the event log line controlled by the event formatter, not within fields. Adding ANSI colors and formatting in the fields could make the formatted output nicer looking and easier to read. ## Solution This branch adds support for ANSI formatting escapes in the default field formatter, when ANSI formatting is enabled. Now, field names will be italicized, and the `=` delimiter is dimmed. This should make it a little easier to visually scan the field lists, since the names and values are more clearly separated and should be easier to distinguish. Additionally, I cleaned up the code for conditionally using ANSI formatting significantly. Now, the `Writer` type has (crate-private) methods for returning `Style`s for bold, dimmed, and italicized text, when ANSI escapes are enabled, or no-op `Style`s when they are not. This is reused in all the formatter implementations, so it makes sense to have it in one place. I also added dimmed formatting of delimiters in a couple other places in the default event format, which I thought improved readability a bit by bringing the actual *text* to the forefront. Example of the default format with ANSI escapes enabled: ![image](https://user-images.githubusercontent.com/2796466/140166750-aaf64bd8-b051-4985-9e7d-168fe8757b11.png) Signed-off-by: Eliza Weisman <eliza@buoyant.io>

# 0.3.2 (Nov 19, 2021) ### Fixed - **fmt**: Fixed `MakeWriter` filtering not working with `BoxMakeWriter` ([#1694]) ### Added - **fmt**: `Writer::has_ansi_escapes` method to check if an output supports ANSI terminal formatting escape codes ([#1696]) - **fmt**: Added additional ANSI terminal formatting to field formatters when supported ([#1702]) - **fmt**: Added `FmtContext::span_scope`, `FmtContext::event_scope`, and `FmtContext::parent_span` methods for accessing the current span and its scope when formatting an event ([#1728]) - **fmt**: Improved documentation on implementing event formatters ([#1727]) [#1694]: #1694 [#1696]: #1696 [#1702]: #1702 [#1728]: #1728 [#1727]: #1727

The line of code that prints the event's fields (including its message) was accidentally deleted in 0a16972, while backporting PR #1696 from `master` (where the `Compact`) formatter implementation is significantly different. This branch puts it back. Fixes #1741

The line of code that prints the event's fields (including its message) was accidentally deleted in 0a16972, while backporting PR #1696 from `master` (where the `Compact` formatter implementation is significantly different). This branch puts it back. Also, I've added tests for the `Compact` formatter's output, to guard against accidentally breaking it in the future. Previously, we only had tests for the `Full` and `Json` formatters. Fixes #1741 Signed-off-by: Eliza Weisman <eliza@buoyant.io>

hawkw added 3 commits October 29, 2021 11:52

wip propagate ANSI color configuration via writer

0c21ccf

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

perhaps improve naming

5e476e3

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

also propagate ansi configuration to add_fields

8d788ee

Signed-off-by: Eliza Weisman <eliza@buoyant.io>

hawkw requested a review from davidbarsky November 3, 2021 00:17

hawkw requested a review from a team as a code owner November 3, 2021 00:17

Merge branch 'master' into eliza/ansi-writer

6f0abe3

hawkw self-assigned this Nov 3, 2021

hawkw commented Nov 3, 2021

View reviewed changes

Merge branch 'master' into eliza/ansi-writer

c1ee8e3

This was referenced Nov 3, 2021

subscriber: use ANSI formatting in field formatters #1702

Merged

[WIP] backport #1696 to v0.1.x #1704

Merged

davidbarsky approved these changes Nov 8, 2021

View reviewed changes

hawkw merged commit 8a7d0c5 into master Nov 8, 2021

hawkw deleted the eliza/ansi-writer branch November 8, 2021 18:40

hawkw mentioned this pull request Nov 8, 2021

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

Closed

hawkw mentioned this pull request Nov 18, 2021

Include Trace ID and Span ID in formatted logs #1481

Open

hawkw mentioned this pull request Nov 29, 2021

subscriber: fix missing event fields in Compact formatter #1755

Merged

davidbarsky mentioned this pull request Sep 26, 2023

chore: backport roughly a year's worth of changes #2728

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

subscriber: propagate ANSI config via `Writer` #1696

subscriber: propagate ANSI config via `Writer` #1696

hawkw commented Nov 3, 2021

hawkw commented Nov 3, 2021

davidbarsky commented Nov 3, 2021

hawkw commented Nov 3, 2021

hawkw Nov 3, 2021

hawkw Nov 3, 2021

lilyball commented Nov 6, 2021

hawkw commented Nov 6, 2021

davidbarsky left a comment

		@@ -530,7 +550,10 @@ impl<F, T> Format<F, T> {

		/// Enable ANSI terminal colors for formatted output.
		pub fn with_ansi(self, ansi: bool) -> Format<F, T> {

subscriber: propagate ANSI config via Writer #1696

subscriber: propagate ANSI config via Writer #1696

Conversation

hawkw commented Nov 3, 2021

Motivation

Solution

hawkw commented Nov 3, 2021

davidbarsky commented Nov 3, 2021

hawkw commented Nov 3, 2021

hawkw Nov 3, 2021

Choose a reason for hiding this comment

hawkw Nov 3, 2021

Choose a reason for hiding this comment

lilyball commented Nov 6, 2021

hawkw commented Nov 6, 2021

davidbarsky left a comment

Choose a reason for hiding this comment

subscriber: propagate ANSI config via `Writer` #1696

subscriber: propagate ANSI config via `Writer` #1696