Skip to content

Commit

Permalink
address comment
Browse files Browse the repository at this point in the history 
Signed-off-by: Andy Lok <[email protected]>
  • Loading branch information
@andylokandy
andylokandy committed Jul 17, 2023
1 parent 72c5bb3 commit 63aa50c
Show file tree
Hide file tree
Showing 8 changed files with 83 additions and 24 deletions.
5 changes: 2 additions & 3 deletions minitrace/src/collector/global_collector.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ use crate::util::spsc::Sender;
use crate::util::spsc::{self};
use crate::util::CollectToken;

const COLLECT_LOOP_INTERVAL: Duration = Duration::from_millis(10);
const COLLECT_LOOP_INTERVAL: Duration = Duration::from_millis(50);

static NEXT_COLLECT_ID: AtomicU32 = AtomicU32::new(0);
static GLOBAL_COLLECTOR: Lazy<Mutex<GlobalCollector>> =
Expand Down Expand Up @@ -249,9 +249,8 @@ impl GlobalCollector {
if collect_token.len() == 1 {
let item = collect_token[0];
if let Some((buf, span_count)) = self.active_collectors.get_mut(&item.collect_id) {
// The root span, i.e. the span whose parent id is `SpanId::default`, is intended to be kept.
if *span_count < self.config.max_spans_per_trace.unwrap_or(usize::MAX)
|| item.parent_id == SpanId::default()
|| item.is_root
{
*span_count += spans.len();
buf.push(SpanCollection::Owned {
Expand Down
49 changes: 46 additions & 3 deletions minitrace/src/collector/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ pub struct CollectTokenItem {
pub trace_id: TraceId,
pub parent_id: SpanId,
pub collect_id: u32,
pub is_root: bool,
}

/// `Collector` collects all spans associated to a root span.
Expand All @@ -94,6 +95,7 @@ impl Collector {
trace_id: parent.trace_id,
parent_id: parent.span_id,
collect_id,
is_root: true,
}
.into(),
)
Expand Down Expand Up @@ -183,7 +185,6 @@ impl Config {
///
/// ```
/// use minitrace::collector::Config;
/// use minitrace::prelude::*;
///
/// let config = Config::default().max_spans_per_trace(Some(100));
/// minitrace::set_reporter(minitrace::collector::ConsoleReporter, config);
Expand All @@ -195,13 +196,53 @@ impl Config {
}
}

/// The time duration between two batch reports.
///
/// The default value is 500 milliseconds.
///
/// A batch report will be initiated by the earliest of these events:
///
/// - When the specified time duration between two batch reports is met.
/// - When the number of spans in a batch hits its limit.
///
/// # Examples
///
/// ```
/// use std::time::Duration;
///
/// use minitrace::collector::Config;
///
/// let config = Config::default().batch_report_interval(Duration::from_secs(1));
/// minitrace::set_reporter(minitrace::collector::ConsoleReporter, config);
/// ```
pub fn batch_report_interval(self, batch_report_interval: Duration) -> Self {
Self {
batch_report_interval,
..self
}
}

/// The soft limit for the maximum number of spans in a batch report.
///
/// A batch report will be initiated by the earliest of these events:
///
/// - When the specified time duration between two batch reports is met.
/// - When the number of spans in a batch hits its limit.
///
/// # Notice
///
/// The eventually spans being reported may exceed the limit.
///
/// # Examples
///
/// ```
/// use std::time::Duration;
///
/// use minitrace::collector::Config;
///
/// let config = Config::default().batch_report_max_spans(Some(200));
/// minitrace::set_reporter(minitrace::collector::ConsoleReporter, config);
/// ```
pub fn batch_report_max_spans(self, batch_report_max_spans: Option<usize>) -> Self {
Self {
batch_report_max_spans,
Expand Down Expand Up @@ -251,7 +292,8 @@ mod tests {
assert_eq!(token.into_inner().1.as_slice(), &[CollectTokenItem {
trace_id: TraceId(12),
parent_id: SpanId::default(),
collect_id: 42
collect_id: 42,
is_root: true,
}]);
}

Expand All @@ -277,7 +319,8 @@ mod tests {
assert_eq!(token.into_inner().1.as_slice(), &[CollectTokenItem {
trace_id: TraceId(12),
parent_id: SpanId::default(),
collect_id: 42
collect_id: 42,
is_root: true,
}]);
}
}
2 changes: 1 addition & 1 deletion minitrace/src/future.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
//! Tools to trace a `Future`.
//!
//! [`FutureExt`] extends `Future` with two methods: [`in_span()`] and [`enter_on_poll()`].
//! The out-most future must use `in_span()`, otherwise, the tracing inside the future will be lost.
//! The out-most future must use `in_span()`, otherwise, the traces inside the `Future` will be lost.
//!
//! # Example
//!
Expand Down
11 changes: 6 additions & 5 deletions minitrace/src/local/local_collector.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,14 @@ use crate::util::RawSpans;
/// A collector to collect [`LocalSpan`].
///
/// [`LocalCollector`] allows to collect [`LocalSpan`] manually without a local parent. The collected [`LocalSpan`] can later be
/// mounted to a parent.
/// attached to a parent.
///
/// At most time, [`Span`] and [`LocalSpan`] are sufficient. Use [`LocalCollector`] when the span may start before the parent
/// span. Sometimes it is useful to trace the preceding task that is blocking the current request.
/// Generally, [`Span`] and [`LocalSpan`] are sufficient. However, use [`LocalCollector`] when the span might initiate before its
/// parent span. This is particularly useful for tracing prior tasks that may be obstructing the current request.
///
/// # Examples
///
/// ```
/// use futures::executor::block_on;
/// use minitrace::local::LocalCollector;
/// use minitrace::prelude::*;
///
Expand All @@ -33,7 +32,7 @@ use crate::util::RawSpans;
/// drop(span);
/// let local_spans = collector.collect();
///
/// // Mount the local spans to a parent
/// // Attach the local spans to a parent
/// let root = Span::root("root", SpanContext::new(TraceId(12), SpanId::default()));
/// root.push_child_spans(local_spans);
/// ```
Expand Down Expand Up @@ -169,6 +168,7 @@ mod tests {
trace_id: TraceId(1234),
parent_id: SpanId::default(),
collect_id: 42,
is_root: false,
};
let collector2 = LocalCollector::new(Some(token2.into()), stack.clone());
let span2 = stack.borrow_mut().enter_span("span2").unwrap();
Expand Down Expand Up @@ -207,6 +207,7 @@ span1 []
trace_id: TraceId(1234),
parent_id: SpanId::default(),
collect_id: 42,
is_root: false,
};
let collector2 = LocalCollector::new(Some(token2.into()), stack.clone());
let span2 = stack.borrow_mut().enter_span("span2").unwrap();
Expand Down
2 changes: 2 additions & 0 deletions minitrace/src/local/local_span.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ mod tests {
trace_id: TraceId(1234),
parent_id: SpanId::default(),
collect_id: 42,
is_root: false,
};
let collector = LocalCollector::new(Some(token.into()), stack.clone());

Expand Down Expand Up @@ -137,6 +138,7 @@ span1 []
trace_id: TraceId(1234),
parent_id: SpanId::default(),
collect_id: 42,
is_root: false,
};
let collector = LocalCollector::new(Some(token.into()), stack.clone());

Expand Down
6 changes: 6 additions & 0 deletions minitrace/src/local/local_span_line.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ impl SpanLine {
trace_id: item.trace_id,
parent_id: self.span_queue.current_span_id().unwrap_or(item.parent_id),
collect_id: item.collect_id,
is_root: false,
})
.collect()
})
Expand Down Expand Up @@ -133,11 +134,13 @@ span1 []
trace_id: TraceId(1234),
parent_id: SpanId::default(),
collect_id: 42,
is_root: false,
};
let token2 = CollectTokenItem {
trace_id: TraceId(1235),
parent_id: SpanId::default(),
collect_id: 43,
is_root: false,
};
let token = [token1, token2].iter().collect();
let mut span_line = SpanLine::new(16, 1, Some(token));
Expand All @@ -153,11 +156,13 @@ span1 []
trace_id: TraceId(1234),
parent_id: span_line.span_queue.current_span_id().unwrap(),
collect_id: 42,
is_root: false,
},
CollectTokenItem {
trace_id: TraceId(1235),
parent_id: span_line.span_queue.current_span_id().unwrap(),
collect_id: 43,
is_root: false,
}
]);
span_line.finish_span(span);
Expand Down Expand Up @@ -200,6 +205,7 @@ span []
trace_id: TraceId(1234),
parent_id: SpanId::default(),
collect_id: 42,
is_root: false,
};
let mut span_line1 = SpanLine::new(16, 1, Some(item.into()));
let mut span_line2 = SpanLine::new(16, 2, None);
Expand Down
15 changes: 13 additions & 2 deletions minitrace/src/local/local_span_stack.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ mod tests {
trace_id: TraceId(1234),
parent_id: SpanId::default(),
collect_id: 42,
is_root: false,
};
let span_line1 = span_stack.register_span_line(Some(token1.into())).unwrap();
{
Expand All @@ -159,6 +160,7 @@ mod tests {
trace_id: TraceId(1235),
parent_id: SpanId::default(),
collect_id: 48,
is_root: false,
};
let span_line2 = span_stack.register_span_line(Some(token2.into())).unwrap();
{
Expand Down Expand Up @@ -206,6 +208,7 @@ span1 []
trace_id: TraceId(1234),
parent_id: SpanId::default(),
collect_id: 42,
is_root: false,
}
.into(),
))
Expand All @@ -219,7 +222,8 @@ span1 []
CollectTokenItem {
trace_id: TraceId(1235),
parent_id: SpanId::default(),
collect_id: 43
collect_id: 43,
is_root: false,
}
.into()
))
Expand All @@ -238,7 +242,8 @@ span1 []
CollectTokenItem {
trace_id: TraceId(1236),
parent_id: SpanId::default(),
collect_id: 44
collect_id: 44,
is_root: false,
}
.into()
))
Expand All @@ -263,6 +268,7 @@ span1 []
trace_id: TraceId(1),
parent_id: SpanId(1),
collect_id: 1,
is_root: false,
};
let span_line1 = span_stack.register_span_line(Some(token1.into())).unwrap();
assert_eq!(span_stack.current_collect_token().unwrap().as_slice(), &[
Expand All @@ -276,6 +282,7 @@ span1 []
trace_id: TraceId(3),
parent_id: SpanId(3),
collect_id: 3,
is_root: false,
};
let span_line3 = span_stack.register_span_line(Some(token3.into())).unwrap();
assert_eq!(span_stack.current_collect_token().unwrap().as_slice(), &[
Expand All @@ -290,6 +297,7 @@ span1 []
trace_id: TraceId(4),
parent_id: SpanId(4),
collect_id: 4,
is_root: false,
};
let span_line4 = span_stack.register_span_line(Some(token4.into())).unwrap();
assert_eq!(span_stack.current_collect_token().unwrap().as_slice(), &[
Expand Down Expand Up @@ -317,6 +325,7 @@ span1 []
trace_id: TraceId(1234),
parent_id: SpanId::default(),
collect_id: 42,
is_root: false,
}
.into(),
))
Expand All @@ -340,6 +349,7 @@ span1 []
trace_id: TraceId(1234),
parent_id: SpanId::default(),
collect_id: 42,
is_root: false,
}
.into(),
))
Expand All @@ -363,6 +373,7 @@ span1 []
trace_id: TraceId(1234),
parent_id: SpanId::default(),
collect_id: 42,
is_root: false,
}
.into(),
))
Expand Down
17 changes: 7 additions & 10 deletions minitrace/src/span.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -264,17 +264,14 @@ impl SpanInner {

#[inline]
pub(crate) fn issue_collect_token(&self) -> impl Iterator<Item = CollectTokenItem> + '_ {
self.collect_token.iter().map(
move |CollectTokenItem {
trace_id,
collect_id,
..
}| CollectTokenItem {
trace_id: *trace_id,
self.collect_token
.iter()
.map(move |collect_item| CollectTokenItem {
trace_id: collect_item.trace_id,
parent_id: self.raw_span.id,
collect_id: *collect_id,
},
)
collect_id: collect_item.collect_id,
is_root: false,
})
}

#[inline]
Expand Down

0 comments on commit 63aa50c

Please sign in to comment.