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

subscriber: move timing into separate subscriber layer #3063

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft
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
22 changes: 22 additions & 0 deletions examples/examples/fmt-fmtspan.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
#![deny(rust_2018_idioms)]

use tracing_subscriber::{fmt::format::FmtSpan, prelude::*, subscribe::CollectExt};
#[path = "fmt/yak_shave.rs"]
mod yak_shave;

fn main() {
tracing_subscriber::registry()
.with(tracing_subscriber::fmt::timing::TimingSubscriber::default())
.with(tracing_subscriber::FmtSubscriber::new().with_span_events(FmtSpan::FULL))
Comment on lines +9 to +10
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the main part I'd want to work better / get guidance on what's best for tracing. Perhaps this is the right thing to do. Maybe moving this into its own top level module, and providing something like the following would be useful?

tracing_subscriber::timing::subscriber()

.init();

let number_of_yaks = 3;
// this creates a new event, outside of any spans.
tracing::info!(number_of_yaks, "preparing to shave yaks");

let number_shaved = yak_shave::shave_all(number_of_yaks);
tracing::info!(
all_yaks_shaved = number_shaved == number_of_yaks,
"yak shaving completed."
);
}
87 changes: 19 additions & 68 deletions tracing-subscriber/src/fmt/fmt_subscriber.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,9 @@ use crate::{
registry::{self, LookupSpan, SpanRef},
subscribe::{self, Context},
};
use format::{FmtSpan, TimingDisplay};
use format::FmtSpan;
use std::{
any::TypeId, cell::RefCell, env, fmt, io, marker::PhantomData, ops::Deref, ptr::NonNull,
time::Instant,
};
use tracing_core::{
field,
Expand Down Expand Up @@ -851,13 +850,6 @@ where
}
}

if self.fmt_span.fmt_timing
&& self.fmt_span.trace_close()
&& extensions.get_mut::<Timings>().is_none()
{
extensions.insert(Timings::new());
}

if self.fmt_span.trace_new() {
with_event_from_span!(id, span, "message" = "new", |event| {
drop(extensions);
Expand Down Expand Up @@ -887,66 +879,41 @@ where
}

fn on_enter(&self, id: &Id, ctx: Context<'_, C>) {
if self.fmt_span.trace_enter() || self.fmt_span.trace_close() && self.fmt_span.fmt_timing {
if self.fmt_span.trace_enter() {
let span = ctx.span(id).expect("Span not found, this is a bug");
let mut extensions = span.extensions_mut();
if let Some(timings) = extensions.get_mut::<Timings>() {
let now = Instant::now();
timings.idle += (now - timings.last).as_nanos() as u64;
timings.last = now;
}

if self.fmt_span.trace_enter() {
with_event_from_span!(id, span, "message" = "enter", |event| {
drop(extensions);
drop(span);
self.on_event(&event, ctx);
});
}
with_event_from_span!(id, span, "message" = "enter", |event| {
drop(span);
self.on_event(&event, ctx);
});
}
}

fn on_exit(&self, id: &Id, ctx: Context<'_, C>) {
if self.fmt_span.trace_exit() || self.fmt_span.trace_close() && self.fmt_span.fmt_timing {
if self.fmt_span.trace_exit() {
let span = ctx.span(id).expect("Span not found, this is a bug");
let mut extensions = span.extensions_mut();
if let Some(timings) = extensions.get_mut::<Timings>() {
let now = Instant::now();
timings.busy += (now - timings.last).as_nanos() as u64;
timings.last = now;
}

if self.fmt_span.trace_exit() {
with_event_from_span!(id, span, "message" = "exit", |event| {
drop(extensions);
drop(span);
self.on_event(&event, ctx);
});
}
with_event_from_span!(id, span, "message" = "exit", |event| {
drop(span);
self.on_event(&event, ctx);
});
}
}

fn on_close(&self, id: Id, ctx: Context<'_, C>) {
if self.fmt_span.trace_close() {
let span = ctx.span(&id).expect("Span not found, this is a bug");
let extensions = span.extensions();
if let Some(timing) = extensions.get::<Timings>() {
let Timings {
busy,
mut idle,
last,
} = *timing;
idle += (Instant::now() - last).as_nanos() as u64;

let t_idle = field::display(TimingDisplay(idle));
let t_busy = field::display(TimingDisplay(busy));
if let Some(timing) = extensions.get::<super::timing::Timing>() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This likely should still check whether the fmt_timing field is set. That now should control whether to display the timing, not whether to capture it.

let busy = field::debug(timing.busy());
let idle = field::debug(timing.idle());
let total = field::debug(timing.total());

with_event_from_span!(
id,
span,
"message" = "close",
"time.busy" = t_busy,
"time.idle" = t_idle,
"time.busy" = busy,
"time.idle" = idle,
"time.total" = total,
|event| {
drop(extensions);
drop(span);
Expand Down Expand Up @@ -1221,22 +1188,6 @@ where
}
}

struct Timings {
idle: u64,
busy: u64,
last: Instant,
}

impl Timings {
fn new() -> Self {
Self {
idle: 0,
busy: 0,
last: Instant::now(),
}
}
}

#[cfg(test)]
mod test {
use super::*;
Expand Down Expand Up @@ -1302,7 +1253,7 @@ mod test {
}

fn sanitize_timings(s: String) -> String {
let re = Regex::new("time\\.(idle|busy)=([0-9.]+)[mµn]s").unwrap();
let re = Regex::new("time\\.(idle|busy|total)=([0-9.]+)[mµn]s").unwrap();
re.replace_all(s.as_str(), "timing").to_string()
}

Expand Down
18 changes: 0 additions & 18 deletions tracing-subscriber/src/fmt/format/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1630,24 +1630,6 @@ impl Default for FmtSpanConfig {
}
}

pub(super) struct TimingDisplay(pub(super) u64);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Storing a Duration instead of a u54 makes this unnecessary.

impl Display for TimingDisplay {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let mut t = self.0 as f64;
for unit in ["ns", "µs", "ms", "s"].iter() {
if t < 10.0 {
return write!(f, "{:.2}{}", t, unit);
} else if t < 100.0 {
return write!(f, "{:.1}{}", t, unit);
} else if t < 1000.0 {
return write!(f, "{:.0}{}", t, unit);
}
t /= 1000.0;
}
write!(f, "{:.0}s", t * 1000.0)
}
}

#[cfg(test)]
pub(super) mod test {
use crate::fmt::{test::MockMakeWriter, time::FormatTime};
Expand Down
1 change: 1 addition & 0 deletions tracing-subscriber/src/fmt/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,7 @@ pub mod time;
#[cfg_attr(docsrs, doc(cfg(all(feature = "fmt", feature = "std"))))]
pub mod writer;
pub use fmt_subscriber::{FmtContext, FormattedFields, Subscriber};
pub mod timing;

use crate::subscribe::Subscribe as _;
use crate::{
Expand Down
155 changes: 155 additions & 0 deletions tracing-subscriber/src/fmt/timing.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,155 @@
//! A subscriber that tracks the time spent in each span.

use core::time::Duration;
use std::time::Instant;
Copy link
Contributor Author

@joshka joshka Aug 21, 2024

Choose a reason for hiding this comment

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

I wonder whether it's worth considering using quanta here instead of std::time::Instant? That would make this inherently testable without too much complication.


use tracing::Collect;
use tracing_core::span::{self, Attributes};

use crate::{registry::LookupSpan, subscribe::Context, Subscribe};

/// A subscriber that tracks the time spent in each span.
///
/// This subscriber records the time spent in each span, storing the timing data in the span's
/// extensions. The subscriber records the time spent in each span as either "idle" time, when the
/// span is not executing, or "busy" time, when the span is executing. The subscriber records the
/// time spent in each span as a [`Timing`] resource, which can be accessed by other subscribers.
#[derive(Debug, Default)]
pub struct TimingSubscriber;

/// A resource tracking the idle and busy time spent in each span.
///
/// This is used by the [`TimingSubscriber`] to track the time spent in each span.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub struct Timing {
state: State,
idle: Duration,
busy: Duration,
last: Instant,
}

impl Default for Timing {
fn default() -> Self {
Self::new()
}
}

#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)]
enum State {
/// The span is closed.
///
/// No further timing information will be recorded.
Closed,
#[default]
/// The span is currently idle.
///
/// Timing information will be recorded when the span becomes busy or closed.
Idle,
/// The span is currently busy.
///
/// Timing information will be recorded when the span becomes idle or closed.
Busy,
}

impl Timing {
/// Create a new `Timing` resource.
fn new() -> Self {
Self {
state: State::Idle,
idle: Duration::ZERO,
busy: Duration::ZERO,
last: Instant::now(),
}
}

/// Record that the span has become idle.
fn enter(&mut self) {
if self.state == State::Busy {
return;
}
let now = Instant::now();
self.idle += now.duration_since(self.last);
self.last = now;
self.state = State::Busy;
}

/// Record that the span has become busy.
fn exit(&mut self) {
if self.state == State::Idle {
return;
}
let now = Instant::now();
self.busy += now.duration_since(self.last);
self.last = now;
self.state = State::Idle;
}

/// Record that the span has been closed.
fn close(&mut self) {
match self.state {
State::Idle => self.idle += Instant::now().duration_since(self.last),
State::Busy => self.busy += Instant::now().duration_since(self.last),
State::Closed => {}
}
self.state = State::Closed;
}

/// Get the idle time spent in this span.
pub fn idle(&self) -> Duration {
self.idle
}

/// Get the busy time spent in this span.
pub fn busy(&self) -> Duration {
self.busy
}

/// Get the total time spent in this span.
pub fn total(&self) -> Duration {
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 added total as an affordance to make viewing logs easier (and not require the viewer / software where the logs end up to do math to work out the total)

self.idle + self.busy
}
}

impl<C> Subscribe<C> for TimingSubscriber
where
C: Collect + for<'a> LookupSpan<'a>,
{
/// Records that a new span has been created.
fn on_new_span(&self, _attrs: &Attributes<'_>, id: &span::Id, ctx: Context<'_, C>) {
let span = ctx.span(id).expect("span not found");
let mut extensions = span.extensions_mut();
extensions.insert(Timing::new());
}

/// Records that a span has been entered.
///
/// The subscriber records the time spent in the span as "idle" time, as the span is not
/// executing.
fn on_enter(&self, id: &span::Id, ctx: Context<'_, C>) {
let span = ctx.span(id).expect("span not found");
let mut extensions = span.extensions_mut();
let timings = extensions.get_mut::<Timing>().expect("timings not found");
timings.enter();
}

/// Records that a span has been exited.
///
/// The subscriber records the time spent in the span as "busy" time, as the span is executing.
fn on_exit(&self, id: &span::Id, ctx: Context<'_, C>) {
let span = ctx.span(id).expect("span not found");
let mut extensions = span.extensions_mut();
let timings = extensions.get_mut::<Timing>().expect("timings not found");
timings.exit();
}

/// Records that a span has been closed.
///
/// The subscriber records the time spent in the span as either "idle" time or "busy" time, as
/// the span is not executing.
fn on_close(&self, id: span::Id, ctx: Context<'_, C>) {
let span = ctx.span(&id).expect("span not found");
let mut extensions = span.extensions_mut();
let timings = extensions.get_mut::<Timing>().expect("timings not found");
timings.close();
}
}
Loading