Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support custom timer via FormatTime #49

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 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
1 change: 1 addition & 0 deletions examples/basic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ fn main() {
.with_writer(std::io::stdout)
.with_indent_lines(true)
.with_indent_amount(2)
.with_timer(tracing_subscriber::fmt::time::SystemTime)
.with_thread_names(true)
.with_thread_ids(true)
.with_verbose_exit(true)
Expand Down
32 changes: 28 additions & 4 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use tracing_core::{
#[cfg(feature = "tracing-log")]
use tracing_log::NormalizeEvent;
use tracing_subscriber::{
fmt::MakeWriter,
fmt::{time::FormatTime, MakeWriter},
layer::{Context, Layer},
registry::{self, LookupSpan},
};
Expand All @@ -43,11 +43,13 @@ impl Visit for Data {
}
}
#[derive(Debug)]
pub struct HierarchicalLayer<W = fn() -> io::Stderr>
pub struct HierarchicalLayer<W = fn() -> io::Stderr, FT = ()>
where
W: for<'writer> MakeWriter<'writer> + 'static,
FT: FormatTime,
{
make_writer: W,
timer: Option<FT>,
bufs: Mutex<Buffers>,
config: Config,
}
Expand All @@ -68,15 +70,17 @@ impl HierarchicalLayer<fn() -> io::Stderr> {
};
Self {
make_writer: io::stderr,
timer: None,
bufs: Mutex::new(Buffers::new()),
config,
}
}
}

impl<W> HierarchicalLayer<W>
impl<W, FT> HierarchicalLayer<W, FT>
where
W: for<'writer> MakeWriter<'writer> + 'static,
FT: FormatTime,
{
/// Enables terminal colors, boldness and italics.
pub fn with_ansi(self, ansi: bool) -> Self {
Expand All @@ -86,12 +90,13 @@ where
}
}

pub fn with_writer<W2>(self, make_writer: W2) -> HierarchicalLayer<W2>
pub fn with_writer<W2>(self, make_writer: W2) -> HierarchicalLayer<W2, FT>
where
W2: for<'writer> MakeWriter<'writer>,
{
HierarchicalLayer {
make_writer,
timer: self.timer,
config: self.config,
bufs: self.bufs,
}
Expand All @@ -113,6 +118,16 @@ where
}
}

/// Specifies how to measure and format time at which event has occurred.
pub fn with_timer<FT2: FormatTime>(self, timer: FT2) -> HierarchicalLayer<W, FT2> {
HierarchicalLayer {
make_writer: self.make_writer,
timer: Some(timer),
config: self.config,
bufs: self.bufs,
}
}

/// Whether to render the event and span targets. Usually targets are the module path to the
/// event/span macro invocation.
pub fn with_targets(self, targets: bool) -> Self {
Expand Down Expand Up @@ -334,6 +349,15 @@ where
};
if let Some(start) = start {
let elapsed = start.elapsed();

if let Some(timer) = self.timer.as_ref() {
timer
// TODO(TmLev): Wants `Writer`, have `event_buf`.
.format_time()
.expect("Unable to write time to buffer");
Comment on lines +349 to +353
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know how to actually call format_time() with event_buf since it requires &mut Writer which cannot be instantiated because of the private constructors:
https://github.com/tokio-rs/tracing/blob/8030eb9dfb56201b1b0843f6d6105eb89e3660ed/tracing-subscriber/src/fmt/format/mod.rs#L421-L431

Copy link
Owner

@davidbarsky davidbarsky Jul 16, 2022

Choose a reason for hiding this comment

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

Whoops, that's a bit of an issue. If the lack of a timer blocking you, let's not wait on tracing-subscriber. We can add a dependency on time directly and provide a few out-of-the-box formatters. I'm not sure if we should have our own FormatTime trait similar to that of tracing-subscrber's, but if we do, then I think we can probably tweak it so that it works well with tracing-tree (e.g., you can specify what writer you want to use here).

Getting tracing-tree onto tracing-subscriber's FormatEvent/FormatTime infrastructure might be a bigger lift that I don't want to commit to spending time on now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opened an issue about this in tracing repo: tokio-rs/tracing#2223. I suggest we wait for an answer from Eliza a little. If she agrees to make constructor public, then problem is solved (hopefully). If not, we can always mimic FormatTime for tracing-tree.

Copy link
Owner

Choose a reason for hiding this comment

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

I'm aware of the issue you opened; check who the co-author of tracing-subscriber is :).

In any case, I think that a proper solution for this issue would be build atop of tokio-rs/tracing#2132 (but that issue is slightly orthogonal to this), but I'm pretty sure that we don't want to make a decision about the API in tracing-subscriber just yet. If we do, it'll require some larger changes across tracing-subscriber, and one of the best ways to determine what that API should be is to vendor the FormatTime trait in tracing-tree and experiment here. It has the nice side-effect of unblocking you as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, thanks for elaborating. I'll try to sketch an API similar to FormatTime from tracing-subscriber.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I hide FormatTime under the feature flag? Doing so complicates things with generic parameters and where clauses.

Copy link
Owner

Choose a reason for hiding this comment

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

it can be hidden behind a feature flag, but i wouldn't be too worried, honestly

write!(&mut event_buf, " ").expect("Unable to write to buffer");
}

write!(
&mut event_buf,
"{timestamp}{unit} ",
Expand Down