diff --git a/CHANGELOG.md b/CHANGELOG.md index 378c736..9df635a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -474,6 +474,11 @@ `TokenRejection::UnsupportedAlgorithm`. Configs with one algorithm, or one family, behave as before. +- `init_default_logging` colours its lines only when stdout is a terminal, + following the crate's colour rule (`NO_COLOR` > `FORCE_COLOR` > terminal). + It used to write ANSI escape codes everywhere, so a service in a container + sent them verbatim to `kubectl logs` and the log store. + - `cli-framework-oidc`: single-issuer validation (`OidcValidator::new`, `oidc_validation_layer`) and the bearer-token checks of the browser layers (`oidc_browser_session_layer`, `oidc_dual_mode_layer`) now require the `iss` diff --git a/Cargo.toml b/Cargo.toml index 04493bd..6180a34 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -899,6 +899,11 @@ required-features = ["telemetry"] # Own binary for the same reason: `init_default_logging` installs a # subscriber too, and this test calls it twice to see both outcomes. +[[test]] +name = "unit_default_logging_color" +path = "tests/unit/default_logging_color.rs" +required-features = ["observability"] + [[test]] name = "unit_telemetry_logging_guard" path = "tests/unit/telemetry_logging_guard.rs" diff --git a/src/telemetry/subscriber.rs b/src/telemetry/subscriber.rs index c46f2ac..d930ae9 100644 --- a/src/telemetry/subscriber.rs +++ b/src/telemetry/subscriber.rs @@ -402,6 +402,14 @@ impl LoggingGuard { } } +/// Whether the default subscriber colours its lines. It writes to stdout, so +/// this is the crate's stdout colour rule: `NO_COLOR` > `FORCE_COLOR` > a +/// terminal. A container or a pipe gets plain lines; escape codes there end up +/// verbatim in `kubectl logs` and the log store. +fn default_logging_ansi() -> bool { + crate::cli_mode::should_color_output() +} + /// The new body of `crate::init_default_logging`: install a process-wide /// subscriber and hand back a guard. /// @@ -422,7 +430,11 @@ pub fn install_default_logging() -> LoggingGuard { let installed = tracing_subscriber::registry() .with(slot.clone()) .with(compose::filter()) - .with(tracing_subscriber::fmt::layer().with_target(true)) + .with( + tracing_subscriber::fmt::layer() + .with_target(true) + .with_ansi(default_logging_ansi()), + ) .try_init() .is_ok(); @@ -448,6 +460,7 @@ pub fn install_default_logging() -> LoggingGuard { tracing_subscriber::fmt() .with_env_filter(filter) .with_target(true) + .with_ansi(default_logging_ansi()) .init(); LoggingGuard {} diff --git a/tests/unit/default_logging_color.rs b/tests/unit/default_logging_color.rs new file mode 100644 index 0000000..b8bb10e --- /dev/null +++ b/tests/unit/default_logging_color.rs @@ -0,0 +1,64 @@ +//! `init_default_logging` colours its lines only where a person reads them. +//! +//! Installing the subscriber is a process-global one-way door, so each case +//! runs in a fresh copy of this test binary: the parent re-executes itself +//! with `CFW_LOG_COLOR_CHILD` set and stdout piped (not a terminal), and the +//! child installs the subscriber and logs one line. + +use std::process::Command; + +const CHILD: &str = "CFW_LOG_COLOR_CHILD"; +const LINE: &str = "default logging colour probe"; + +/// The child half: a no-op unless the parent started this process. +#[test] +fn child_logs_one_line() { + if std::env::var_os(CHILD).is_none() { + return; + } + let _guard = cli_framework::init_default_logging(); + tracing::info!("{LINE}"); +} + +fn child_stdout(env: &[(&str, &str)]) -> String { + let mut cmd = Command::new(std::env::current_exe().unwrap()); + cmd.args([ + "--exact", + "child_logs_one_line", + "--nocapture", + "--test-threads=1", + ]) + .env(CHILD, "1") + .env_remove("NO_COLOR") + .env_remove("FORCE_COLOR") + .env_remove("RUST_LOG"); + for (k, v) in env { + cmd.env(k, v); + } + let out = cmd.output().expect("run the child"); + assert!(out.status.success(), "child failed: {out:?}"); + let stdout = String::from_utf8(out.stdout).unwrap(); + assert!(stdout.contains(LINE), "the child logged nothing:\n{stdout}"); + stdout +} + +#[test] +fn a_pipe_gets_plain_lines() { + let stdout = child_stdout(&[]); + assert!( + !stdout.contains('\x1b'), + "escape codes in a pipe:\n{stdout:?}" + ); +} + +#[test] +fn force_color_colours_a_pipe() { + let stdout = child_stdout(&[("FORCE_COLOR", "1")]); + assert!(stdout.contains('\x1b'), "FORCE_COLOR ignored:\n{stdout:?}"); +} + +#[test] +fn no_color_wins_over_force_color() { + let stdout = child_stdout(&[("FORCE_COLOR", "1"), ("NO_COLOR", "1")]); + assert!(!stdout.contains('\x1b'), "NO_COLOR ignored:\n{stdout:?}"); +}