Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 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
17 changes: 17 additions & 0 deletions tracing-subscriber/src/fmt/fmt_subscriber.rs
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,23 @@ where
}
}

/// Sets whether or not an event's source code filename is displayed.
pub fn with_filename(self, display_filename: bool) -> Subscriber<C, N, format::Format<L, T>, W> {
Subscriber {
fmt_event: self.fmt_event.with_filename(display_filename),
..self
}
}


/// Sets whether or not an event's source code line number is displayed.
pub fn with_line_number(self, display_line_number: bool) -> Subscriber<C, N, format::Format<L, T>, W> {
Subscriber {
fmt_event: self.fmt_event.with_line_number(display_line_number),
..self
}
}

/// Sets whether or not an event's level is displayed.
pub fn with_level(self, display_level: bool) -> Subscriber<C, N, format::Format<L, T>, W> {
Subscriber {
Expand Down
77 changes: 77 additions & 0 deletions tracing-subscriber/src/fmt/format/json.rs
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,18 @@ where
serializer.serialize_entry("target", meta.target())?;
}

if self.display_filename {
if let Some(filename) = meta.file() {
serializer.serialize_entry("filename", filename)?;
}
}

if self.display_line_number {
if let Some(line_number) = meta.line() {
serializer.serialize_entry("line_number", &line_number)?;
}
}

if self.format.display_current_span {
if let Some(ref span) = current_span {
serializer
Expand Down Expand Up @@ -483,6 +495,7 @@ mod test {
use tracing::{self, collect::with_default};

use std::fmt;
use std::path::Path;

struct MockTime;
impl FormatTime for MockTime {
Expand Down Expand Up @@ -510,6 +523,42 @@ mod test {
});
}

#[test]
fn json_filename() {
let current_path = Path::new("tracing-subscriber").join("src").join("fmt").join("format").join("json.rs");
let expected =
&format!("{}{}{}",
"{\"timestamp\":\"fake time\",\"level\":\"INFO\",\"span\":{\"answer\":42,\"name\":\"json_span\",\"number\":3},\"spans\":[{\"answer\":42,\"name\":\"json_span\",\"number\":3}],\"target\":\"tracing_subscriber::fmt::format::json::test\",\"filename\":\"",
&current_path.to_str().unwrap(),
"\",\"fields\":{\"message\":\"some json test\"}}\n");
let collector = collector()
.flatten_event(false)
.with_current_span(true)
.with_filename(true)
.with_span_list(true);
test_json(expected, collector, || {
let span = tracing::span!(tracing::Level::INFO, "json_span", answer = 42, number = 3);
let _guard = span.enter();
tracing::info!("some json test");
});
}

#[test]
fn json_line_number() {
let expected =
"{\"timestamp\":\"fake time\",\"level\":\"INFO\",\"span\":{\"answer\":42,\"name\":\"json_span\",\"number\":3},\"spans\":[{\"answer\":42,\"name\":\"json_span\",\"number\":3}],\"target\":\"tracing_subscriber::fmt::format::json::test\",\"line_number\":42,\"fields\":{\"message\":\"some json test\"}}\n";
let collector = collector()
.flatten_event(false)
.with_current_span(true)
.with_line_number(true)
.with_span_list(true);
test_json_with_line_number(expected, collector, || {
let span = tracing::span!(tracing::Level::INFO, "json_span", answer = 42, number = 3);
let _guard = span.enter();
tracing::info!("some json test");
});
}

#[test]
fn json_flattened_event() {
let expected =
Expand Down Expand Up @@ -740,4 +789,32 @@ mod test {
serde_json::from_str(actual).unwrap()
);
}

fn test_json_with_line_number<T>(
expected: &str,
builder: crate::fmt::CollectorBuilder<JsonFields, Format<Json>>,
producer: impl FnOnce() -> T,
) {
let make_writer = MockMakeWriter::default();
let collector = builder
.with_writer(make_writer.clone())
.with_timer(MockTime)
.finish();

with_default(collector, producer);

let buf = make_writer.buf();
let actual = std::str::from_utf8(&buf[..]).unwrap();
let mut expected = serde_json::from_str::<std::collections::HashMap<&str, serde_json::Value>>(expected)
.unwrap();
let expect_line_number = expected.remove("line_number").is_some();
let mut actual: std::collections::HashMap<&str, serde_json::Value> = serde_json::from_str(actual).unwrap();
let line_number = actual.remove("line_number");
if expect_line_number {
assert_eq!(line_number.map(|x|x.is_number()), Some(true));
} else {
assert!(line_number.is_none());
}
assert_eq!(actual, expected);
}
}
129 changes: 129 additions & 0 deletions tracing-subscriber/src/fmt/format/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,8 @@ pub struct Format<F = Full, T = SystemTime> {
pub(crate) display_level: bool,
pub(crate) display_thread_id: bool,
pub(crate) display_thread_name: bool,
pub(crate) display_filename: bool,
pub(crate) display_line_number: bool,
}

// === impl Writer ===
Expand Down Expand Up @@ -504,6 +506,8 @@ impl Default for Format<Full, SystemTime> {
display_level: true,
display_thread_id: false,
display_thread_name: false,
display_filename: false,
display_line_number: false,
}
}
}
Expand All @@ -522,6 +526,8 @@ impl<F, T> Format<F, T> {
display_level: self.display_level,
display_thread_id: self.display_thread_id,
display_thread_name: self.display_thread_name,
display_filename: self.display_filename,
display_line_number: self.display_line_number,
}
}

Expand Down Expand Up @@ -559,6 +565,8 @@ impl<F, T> Format<F, T> {
display_level: self.display_level,
display_thread_id: self.display_thread_id,
display_thread_name: self.display_thread_name,
display_filename: self.display_filename,
display_line_number: self.display_line_number,
}
}

Expand Down Expand Up @@ -589,6 +597,8 @@ impl<F, T> Format<F, T> {
display_level: self.display_level,
display_thread_id: self.display_thread_id,
display_thread_name: self.display_thread_name,
display_filename: self.display_filename,
display_line_number: self.display_line_number,
}
}

Expand Down Expand Up @@ -616,6 +626,8 @@ impl<F, T> Format<F, T> {
display_level: self.display_level,
display_thread_id: self.display_thread_id,
display_thread_name: self.display_thread_name,
display_filename: self.display_filename,
display_line_number: self.display_line_number,
}
}

Expand All @@ -630,6 +642,8 @@ impl<F, T> Format<F, T> {
display_level: self.display_level,
display_thread_id: self.display_thread_id,
display_thread_name: self.display_thread_name,
display_filename: self.display_filename,
display_line_number: self.display_line_number,
}
}

Expand Down Expand Up @@ -679,6 +693,24 @@ impl<F, T> Format<F, T> {
}
}

/// Sets whether or not the [filename] of the tracing logs source code is displayed
Copy link
Member

Choose a reason for hiding this comment

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

this looks like it's supposed to be a link, but it doesn't link to anything?

/// when formatting events
pub fn with_filename(self, display_filename: bool) -> Format<F, T> {
Format {
display_filename,
..self
}
}

/// Sets whether or not the [line_number] of the tracing logs is source codedisplayed
Copy link
Member

Choose a reason for hiding this comment

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

similarly, this looks like it's supposed to be a link to something?

/// when formatting events
pub fn with_line_number(self, display_line_number: bool) -> Format<F, T> {
Format {
display_line_number,
..self
}
}

fn format_level(&self, level: Level, writer: &mut Writer<'_>) -> fmt::Result
where
F: LevelNames,
Expand Down Expand Up @@ -862,6 +894,29 @@ where
)?;
}

let line_number = if self.display_line_number { meta.line() } else { None };

if self.display_filename {
if let Some(filename) = meta.file() {
write!(
writer,
"{}{}{}",
dimmed.paint(filename),
dimmed.paint(":"),
if line_number.is_some() { "" } else { " " }
)?;
}
}

if let Some(line_number) = line_number {
write!(
writer,
"{}{} ",
dimmed.paint(line_number.to_string()),
dimmed.paint(":")
)?;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should be calling to_string here; that will allocate a new String, which could have a performance impact that it would be nice to avoid. Nothing else in the formatting code currently allocates strings, so I'd really like to avoid this.

I think the to_string() call here is necessary to use Style::paint, which takes a Cow<'a, str> rather than a fmt::Debug implementation? We can avoid this by using ansi_term's prefix and suffix methods, changing this code to something like this:

Suggested change
write!(
writer,
"{}{} ",
dimmed.paint(line_number.to_string()),
dimmed.paint(":")
)?;
write!(
writer,
"{}{}:{} ",
dimmed.prefix(),
line_number,
dimmed.suffix(),
)?;

If we do that, we'll also need to add prefix() and suffix() methods to the no-op version of Style that's used when ANSI support is disabled, here:

#[cfg(not(feature = "ansi"))]
impl Style {
fn new() -> Self {
Style
}
fn paint(&self, d: impl fmt::Display) -> impl fmt::Display {
d
}
}

This would look something like the following:

#[cfg(not(feature = "ansi"))]
impl Style {
    // ...
    fn prefix(&self) -> &'static str {
        ""
    }

    fn suffix(&self) -> &'static str {
        ""
    }
}

}
Copy link
Member

Choose a reason for hiding this comment

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

As I mentioned here, I'm still unconvinced that we should display line numbers if both targets and file names are disabled...a line number like ':156' doesn't make a lot of sense if we don't know what file or module it's in. But, trying to handle that case nicely will complicate the implementation significantly.

@davidbarsky what do you think? Is it worthwhile to make sure we do the "right thing" if a user calls with_line_number(true) but never enables file names? What if they've also called .with_target(false)?

Copy link
Member

Choose a reason for hiding this comment

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

@davidbarsky what do you think? Is it worthwhile to make sure we do the "right thing" if a user calls .with_line_number(true) but never enables file names?

Just saw this. While I think that it's worth doing the right thing, I'd like to consider redesigning the configuration interface of fmt holistically. To that end, I'd rather accept this PR-as-is designed, and fix it later.

At the very least, the lone line number will confusing, but it won't cause runtime panics like other misconfigurations can.

Copy link
Member

@davidbarsky davidbarsky Dec 29, 2021

Choose a reason for hiding this comment

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

To be clear, I'm not entirely sure what the holistic redesign should look like, but naively, I might want to try bundling lines and file formatting information into a single struct where a missing target/file is very obvious to the end user.


ctx.format_fields(writer.by_ref(), event)?;
writeln!(writer)
}
Expand Down Expand Up @@ -916,6 +971,28 @@ where
)?;
}

if self.display_filename {
if let Some(filename) = meta.file() {
write!(
writer,
"{}{}",
writer.bold().paint(filename),
writer.dimmed().paint(":")
)?;
}
}

if self.display_line_number {
if let Some(line_number) = meta.line() {
write!(
writer,
"{}{}",
writer.bold().paint(line_number.to_string()),
Copy link
Member

Choose a reason for hiding this comment

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

as i mentioned above, i think the use of to_string should be avoided here.

writer.dimmed().paint(":")
)?;
}
}

ctx.format_fields(writer.by_ref(), event)?;

let dimmed = writer.dimmed();
Expand Down Expand Up @@ -1467,6 +1544,8 @@ pub(super) mod test {

use super::{FmtSpan, TimingDisplay, Writer};
use std::fmt;
use std::path::Path;
use regex::Regex;

pub(crate) struct MockTime;
impl FormatTime for MockTime {
Expand Down Expand Up @@ -1529,6 +1608,56 @@ pub(super) mod test {
run_test(subscriber, make_writer, expected);
}

#[test]
fn with_line_number_and_file_name() {
let make_writer = MockMakeWriter::default();
let subscriber = crate::fmt::Collector::builder()
.with_writer(make_writer.clone())
.with_filename(true)
.with_line_number(true)
.with_level(false)
.with_ansi(false)
.with_timer(MockTime);

let current_path = Path::new("tracing-subscriber").join("src").join("fmt").join("format").join("mod.rs");
let expected = Regex::new(&format!("^fake time tracing_subscriber::fmt::format::test: {}:[0-9]+: hello\n$", current_path.to_str().unwrap())).unwrap();
let _default = set_default(&subscriber.into());
tracing::info!("hello");
let res = make_writer.get_string();
assert!(expected.is_match(&res));
}

#[test]
fn with_line_number() {
let make_writer = MockMakeWriter::default();
let subscriber = crate::fmt::Collector::builder()
.with_writer(make_writer.clone())
.with_line_number(true)
.with_level(false)
.with_ansi(false)
.with_timer(MockTime);

let expected = Regex::new("^fake time tracing_subscriber::fmt::format::test: [0-9]+: hello\n$").unwrap();
let _default = set_default(&subscriber.into());
tracing::info!("hello");
let res = make_writer.get_string();
assert!(expected.is_match(&res));
}

#[test]
fn with_filename() {
let make_writer = MockMakeWriter::default();
let subscriber = crate::fmt::Collector::builder()
.with_writer(make_writer.clone())
.with_filename(true)
.with_level(false)
.with_ansi(false)
.with_timer(MockTime);
let current_path = Path::new("tracing-subscriber").join("src").join("fmt").join("format").join("mod.rs");
let expected = &format!("fake time tracing_subscriber::fmt::format::test: {}: hello\n", current_path.to_str().unwrap());
run_test(subscriber, make_writer, expected);
}

#[cfg(feature = "ansi")]
fn test_ansi(is_ansi: bool, expected: &str) {
let make_writer = MockMakeWriter::default();
Expand Down
25 changes: 25 additions & 0 deletions tracing-subscriber/src/fmt/format/pretty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,31 @@ where
target_style.infix(style)
)?;
}
let line_number = if self.display_line_number { meta.line() } else { None };


if self.display_filename {
if let Some(filename) = meta.file() {
write!(
writer,
"{}{}{}:{}",
style.prefix(),
filename,
style.infix(style),
if line_number.is_some() { "" } else { " " }
)?;
}
}

if let Some(line_number) = line_number {
write!(
writer,
"{}{}{}: ",
style.prefix(),
line_number,
style.infix(style)
)?;
}
let mut v = PrettyVisitor::new(writer.by_ref(), true).with_style(style);
event.record(&mut v);
v.finish()?;
Expand Down
Loading