From 63aa50c3de61e66327fac092efbc13494eac39e5 Mon Sep 17 00:00:00 2001 From: Andy Lok Date: Tue, 18 Jul 2023 04:16:50 +0800 Subject: [PATCH] address comment Signed-off-by: Andy Lok --- minitrace/src/collector/global_collector.rs | 5 +-- minitrace/src/collector/mod.rs | 49 +++++++++++++++++++-- minitrace/src/future.rs | 2 +- minitrace/src/local/local_collector.rs | 11 ++--- minitrace/src/local/local_span.rs | 2 + minitrace/src/local/local_span_line.rs | 6 +++ minitrace/src/local/local_span_stack.rs | 15 ++++++- minitrace/src/span.rs | 17 +++---- 8 files changed, 83 insertions(+), 24 deletions(-) diff --git a/minitrace/src/collector/global_collector.rs b/minitrace/src/collector/global_collector.rs index b753dc85..16aeb92a 100644 --- a/minitrace/src/collector/global_collector.rs +++ b/minitrace/src/collector/global_collector.rs @@ -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> = @@ -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 { diff --git a/minitrace/src/collector/mod.rs b/minitrace/src/collector/mod.rs index 15cc4046..71407023 100644 --- a/minitrace/src/collector/mod.rs +++ b/minitrace/src/collector/mod.rs @@ -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. @@ -94,6 +95,7 @@ impl Collector { trace_id: parent.trace_id, parent_id: parent.span_id, collect_id, + is_root: true, } .into(), ) @@ -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); @@ -195,6 +196,25 @@ 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, @@ -202,6 +222,27 @@ impl Config { } } + /// 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) -> Self { Self { batch_report_max_spans, @@ -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, }]); } @@ -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, }]); } } diff --git a/minitrace/src/future.rs b/minitrace/src/future.rs index 0b09c2ff..5d812a99 100644 --- a/minitrace/src/future.rs +++ b/minitrace/src/future.rs @@ -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 //! diff --git a/minitrace/src/local/local_collector.rs b/minitrace/src/local/local_collector.rs index b80b4566..daf597c0 100644 --- a/minitrace/src/local/local_collector.rs +++ b/minitrace/src/local/local_collector.rs @@ -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::*; /// @@ -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); /// ``` @@ -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(); @@ -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(); diff --git a/minitrace/src/local/local_span.rs b/minitrace/src/local/local_span.rs index 6b537101..bb9fd6c8 100644 --- a/minitrace/src/local/local_span.rs +++ b/minitrace/src/local/local_span.rs @@ -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()); @@ -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()); diff --git a/minitrace/src/local/local_span_line.rs b/minitrace/src/local/local_span_line.rs index b85741e6..43010706 100644 --- a/minitrace/src/local/local_span_line.rs +++ b/minitrace/src/local/local_span_line.rs @@ -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() }) @@ -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)); @@ -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); @@ -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); diff --git a/minitrace/src/local/local_span_stack.rs b/minitrace/src/local/local_span_stack.rs index 0d0028d8..cb6b5676 100644 --- a/minitrace/src/local/local_span_stack.rs +++ b/minitrace/src/local/local_span_stack.rs @@ -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(); { @@ -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(); { @@ -206,6 +208,7 @@ span1 [] trace_id: TraceId(1234), parent_id: SpanId::default(), collect_id: 42, + is_root: false, } .into(), )) @@ -219,7 +222,8 @@ span1 [] CollectTokenItem { trace_id: TraceId(1235), parent_id: SpanId::default(), - collect_id: 43 + collect_id: 43, + is_root: false, } .into() )) @@ -238,7 +242,8 @@ span1 [] CollectTokenItem { trace_id: TraceId(1236), parent_id: SpanId::default(), - collect_id: 44 + collect_id: 44, + is_root: false, } .into() )) @@ -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(), &[ @@ -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(), &[ @@ -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(), &[ @@ -317,6 +325,7 @@ span1 [] trace_id: TraceId(1234), parent_id: SpanId::default(), collect_id: 42, + is_root: false, } .into(), )) @@ -340,6 +349,7 @@ span1 [] trace_id: TraceId(1234), parent_id: SpanId::default(), collect_id: 42, + is_root: false, } .into(), )) @@ -363,6 +373,7 @@ span1 [] trace_id: TraceId(1234), parent_id: SpanId::default(), collect_id: 42, + is_root: false, } .into(), )) diff --git a/minitrace/src/span.rs b/minitrace/src/span.rs index 1a1666bf..faf70c9c 100644 --- a/minitrace/src/span.rs +++ b/minitrace/src/span.rs @@ -264,17 +264,14 @@ impl SpanInner { #[inline] pub(crate) fn issue_collect_token(&self) -> impl Iterator + '_ { - 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]