Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
144 changes: 143 additions & 1 deletion anchor/logging/src/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,10 @@ use std::fmt;
use nu_ansi_term::{Color, Style};
use tracing_core::{Event, Level, Subscriber};
use tracing_subscriber::{
field::MakeVisitor,
fmt::{
FmtContext, FormattedFields,
format::{FormatEvent, FormatFields, Writer},
format::{self, FormatEvent, FormatFields, Writer},
time::{FormatTime, SystemTime},
},
registry::LookupSpan,
Expand Down Expand Up @@ -208,3 +209,144 @@ where
writeln!(writer)
}
}

/// Field formatter for the file logging layer.
///
/// This type exists solely to give the file layer a separate
/// `FormattedFields<FileFields>` cache slot in the span extensions type-map,
/// distinct from the console layer's default `FormattedFields<DefaultFields>`.
/// Without this, the console layer (ANSI-enabled) caches span fields first,
/// and the file layer reuses those ANSI-contaminated fields.
pub struct FileFields;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent implementation. This is a textbook example of using Rust's type system to solve a caching problem. The distinct TypeId gives each layer its own FormattedFields<_> cache slot while delegating actual formatting logic to DefaultFields. Zero runtime overhead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks claude lol


impl<'writer> MakeVisitor<Writer<'writer>> for FileFields {
type Visitor = format::DefaultVisitor<'writer>;

fn make_visitor(&self, target: Writer<'writer>) -> Self::Visitor {
format::DefaultFields::new().make_visitor(target)
}
}

#[cfg(test)]
mod tests {
use std::sync::{Arc, Mutex};

use tracing_subscriber::{fmt, layer::SubscriberExt};

use super::*;

/// ANSI escape sequence prefix. Any output destined for a plain-text file
/// must never contain this byte sequence.
const ANSI_ESCAPE: &[u8] = b"\x1b[";

/// Returns true if the byte slice contains an ANSI escape sequence.
fn contains_ansi(bytes: &[u8]) -> bool {
bytes
.windows(ANSI_ESCAPE.len())
.any(|window| window == ANSI_ESCAPE)
}

/// Helper: build a shared in-memory buffer suitable as a `MakeWriter`.
fn shared_buffer() -> Arc<Mutex<Vec<u8>>> {
Arc::new(Mutex::new(Vec::new()))
}

/// Verifies that the `FileFields` fix prevents ANSI escape sequences from
/// leaking into file layer output when two layers share a registry.
///
/// Setup mirrors production: a console layer (ANSI-enabled, `DefaultFields`)
/// and a file layer (ANSI-disabled, `FileFields`). Both are attached to the
/// same `Registry`. Because `FileFields` has a distinct `TypeId` from
/// `DefaultFields`, each layer gets its own `FormattedFields<_>` cache slot
/// in the span extensions. The file layer therefore never reads the
/// console layer's ANSI-contaminated cached fields.
///
Comment on lines +254 to +263

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Outstanding test documentation. These doc comments explain the production scenario, the fix mechanism, and the expected behavior. This will be invaluable for future maintainers who wonder "why does FileFields exist?"

/// This test PASSES with the fix (`FileFields`) in place.
#[test]
fn test_file_fields_no_ansi_leak_in_span_fields() {
// Arrange
let console_buf = shared_buffer();
let file_buf = shared_buffer();

let console_writer = console_buf.clone();
let file_writer = file_buf.clone();

// Console layer: ANSI enabled, uses default DefaultFields (implicit).
let console_layer =
fmt::layer()
.with_ansi(true)
.with_writer(move || -> Box<dyn std::io::Write> {
Box::new(MockWriter(console_writer.clone()))
});

// File layer: ANSI disabled, uses FileFields for a distinct cache slot.
let file_layer = fmt::layer()
.with_ansi(false)
.fmt_fields(FileFields)
.with_writer(move || -> Box<dyn std::io::Write> {
Box::new(MockWriter(file_writer.clone()))
});

let subscriber = tracing_subscriber::registry()
.with(console_layer)
.with(file_layer);

// Act
tracing::subscriber::with_default(subscriber, || {
let span = tracing::info_span!("test_span", some_field = "hello_world");
let _guard = span.enter();
tracing::info!("message inside span");
});

// Assert -- file buffer must not contain ANSI escape sequences
let file_output = file_buf.lock().unwrap();
let file_str = String::from_utf8_lossy(&file_output);

assert!(
!file_output.is_empty(),
"File layer should have captured output"
);
assert!(
!contains_ansi(&file_output),
"File layer output must not contain ANSI escape sequences.\n\
File output was:\n{file_str}"
);

// The file output should contain the span field value in plain text
assert!(
file_str.contains("some_field"),
"File layer output should contain the span field name 'some_field'.\n\
File output was:\n{file_str}"
);

// Assert -- console buffer SHOULD contain ANSI (confirms both layers work)
let console_output = console_buf.lock().unwrap();
assert!(
!console_output.is_empty(),
"Console layer should have captured output"
);
assert!(
contains_ansi(&console_output),
"Console layer output should contain ANSI escape sequences, \
confirming the console layer is ANSI-enabled."
);
}
Comment on lines +266 to +333

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well-structured regression test. This test follows AAA pattern perfectly:

  • Arrange: Sets up production-like scenario with two layers
  • Act: Logs with span fields
  • Assert: Verifies file has no ANSI, console has ANSI

The assertion messages are particularly good - they include the actual output for debugging. The negative assertion (console SHOULD have ANSI) is a smart addition that verifies both layers are working.


// ==================== Test helpers ====================

/// A writer that appends to a shared `Vec<u8>` buffer.
/// Implements `std::io::Write` so it can be used with `fmt::layer().with_writer()`.
struct MockWriter(Arc<Mutex<Vec<u8>>>);

impl std::io::Write for MockWriter {
fn write(&mut self, buf: &[u8]) -> std::io::Result<usize> {
let mut lock = self.0.lock().unwrap();
lock.extend_from_slice(buf);
Ok(buf.len())
}

fn flush(&mut self) -> std::io::Result<()> {
Ok(())
}
}
}
2 changes: 1 addition & 1 deletion anchor/logging/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,4 @@ pub mod utils;
pub use count_layer::CountLayer;
pub use logging::*;
pub mod format;
pub use format::AnchorFormatter;
pub use format::{AnchorFormatter, FileFields};
3 changes: 2 additions & 1 deletion anchor/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use global_config::{GlobalConfig, GlobalFlags};
use keygen::Keygen;
use keysplit::Keysplit;
use logging::{
AnchorFormatter, CountLayer, FileLoggingFlags, create_libp2p_discv5_tracing_layer,
AnchorFormatter, CountLayer, FileFields, FileLoggingFlags, create_libp2p_discv5_tracing_layer,
init_file_logging, utils::build_workspace_filter,
};
use task_executor::ShutdownReason;
Expand Down Expand Up @@ -282,6 +282,7 @@ pub fn enable_logging(
logging_layers.push(
fmt::layer()
.event_format(anchor_formatter_log)
.fmt_fields(FileFields)
.with_writer(file_logging_layer.non_blocking_writer)
.with_ansi(file_logging_flags.logfile_color)
.with_filter(
Expand Down
Loading