Disable ANSI for tracing-subscriber output?

[jstarks]

The default behavior of tracing-subscriber is to include color control codes in output. It does not try to detect whether the output is a terminal as part of this: tokio-rs/tracing#1160.

For test output this is pretty bad, since it means the color control codes get into logs and end up causing downstream log processing code to complain. Would it be reasonable to disable ANSI output (via SubscriberBuilder::with_ansi) in expand_tracing_init()? I'd be happy to send a patch.

[d-e-s-o]

I think it would be fine, as long as the check does not introduce another dependency. So pulling in libc for isatty is probably a no-go. But checking RUST_LOG_COLOR should be okay if that's what you had in mind.
Also, it seems as if env_logger honors RUST_LOG_STYLE already and we should make sure to create a proper facade for both supported crates.
Feel free to create a patch. I don't have many cycles to merge it right now, but it should happen in the next few weeks or so.

[jstarks]

Ah, OK, I was originally thinking we could just always disable ANSI for test output, but maybe it's useful for interactive cargo test invocations... In that case, probably keying off of RUST_LOG_STYLE would be reasonable so that it just works for both cases.

[ostollmann]

Maybe this could also support NO_COLOR.

[kingli-crypto]

I tied to implement this, seems SubscriberBuilder::with_ansi require the ansi trait.

Do you think should it be enabled by default? Or guarded by another new trait so we don't break backward compatibility

[d-e-s-o]

I tied to implement this, seems SubscriberBuilder::with_ansi require the ansi trait.

Nice!

Do you think should it be enabled by default?

By default we should keep colors, in my opinion. It's how everything else behaves by default.

I'd say if we follow the NO_COLOR "standard" then we should do it according to their playbook. Right now if I set NO_COLOR=1 I do get colors, which is very confusing conceptually and not in line with their suggested behavior. As per their documentation:

	char *no_color = getenv("NO_COLOR");
	bool color = true;

	if (no_color != NULL && no_color[0] != '\0')
		color = false;

	/* do getopt(3) and/or config-file parsing to possibly turn color back on */

So according to that we should just check for the existence of the env variable. Though I suppose special casing false and 0 to re-enable coloring (and disable in all other cases) would be fine, but I don't feel strongly.

[d-e-s-o]

Also (and I haven't tested that), as mentioned earlier currently it seems as if env_logger honors RUST_LOG_STYLE. So I think it would be better to do the same instead of introducing our own variable name.

[d-e-s-o]

NO_COLOR should now be honored properly (#50).