search

tokio-rs / tracing

Application level tracing for Rust.
https://tracing.rs
MIT License
5.5k stars 722 forks source link

ANSI broken on Windows 10 #445

Open andrewbanchich opened 4 years ago

andrewbanchich commented 4 years ago

ansi_term requires Windows 10 programs to explicitly enable ANSI support.

Without doing this, using cargo run or cargo run --release display tracing logs correctly, but when running the binary installed with cargo install --path . it results in broken ANSI strings.

Is this something you think tracing should fix out of the box, should just be communicated in the docs?

hawkw commented 4 years ago

@andrewbanchich hmm. We're currently using the ansi_term crate for ANSI colors. My initial thought is that it should handle this behavior — maybe the best solution is a patch upstream?

andrewbanchich commented 4 years ago

@hawkw It does handle it (see the link I posted) but just not by default.

hawkw commented 4 years ago

Ah, sorry, I misinterpreted your comment (and didn't click the link), my bad. I think we probably want to do that when constructing the subscriber if the target OS is windows, then?

andrewbanchich commented 4 years ago

That makes sense to me. I'll give this a shot and have a pull request soon.

andrewbanchich commented 4 years ago

@hawkw Any thoughts on me swapping out ansi_term for colored? colored has full Windows support and doesn't look like you need to call any special functions conditionally. Plus, it looks nicer overall.

hawkw commented 4 years ago

I'm fine with switching to colored. Currently, we do use ansi-term's Style type to isolate the feature-flagging of ANSI support: https://github.com/tokio-rs/tracing/blob/8711f63cd8c135980edaca1570ce6878ffe34d7c/tracing-subscriber/src/fmt/format/mod.rs#L550-L559 with a no-op Style type when ansi-term is disabled: https://github.com/tokio-rs/tracing/blob/8711f63cd8c135980edaca1570ce6878ffe34d7c/tracing-subscriber/src/fmt/format/mod.rs#L592-L603

We can probably do a similar thing with colored's Colorize trait?

hawkw commented 4 years ago

hmm, upon taking a closer look, it looks like colored's methods always return a ColoredString struct which appears to always allocate a String, even when the input is borrowed. This also means that formatting Debug/Display happens eagerly, writing into the string, rather than lazily (allowing us to write directly into the per-thread buffer that tracing-subscriber::fmt creates for each thread that logs).

A big part of how we've optimized the performance of logging events is by avoiding additional string allocations, and writing directly to the buffer, which is reused without deallocating — in many cases, once the buffer is "warmed up" (i.e. it has previously allocated enough length to hold the line we are currently writing), logging an event should be allocation-free. It seems like switching to colored breaks this property, which might have a noticeable performance impact...

andrewhickman commented 3 years ago

termcolor has a buffer type which supports writing colored text to a buffer and clearing afterwards to preserve the allocations. Maybe that would be suitable here?

Chaoses-Ib commented 4 months ago

termcolor also supports Windows 7 and 8 according to https://github.com/rust-lang/rust/issues/55769, while ansi-term only works on Windows 10+.

Edit: anstream also has legacy Windows support, and is possibly a better choice than termcolor: https://github.com/rust-lang/cargo/issues/12627