feat(tracing): add minitrace to RisingLight

[WindowsXp-Beta]

close #368
Signed-off-by: Xinpeng Wei xinpeng@singularity-data.com

[skyzh]

After some offline discussions, I think it's better to define a trace_executor(stream: impl Stream) -> impl Stream function to trace the runtime status of each executor.

[arkbriar]

I think current implementation is too intrusive in that it changes the signature of the entire executor builder. IMO it's better to hide the trace works outside the core executors and do it in a unified way (not each time we add a new executor), which can be achieved by proxying the visitor. Also we can attach any common modifiers here according to the build options, such as cancellable (since it's only necessary for those io aware executors, I don't recommend to do that).

diff --git a/src/executor/mod.rs b/src/executor/mod.rs
index cf93b53..e5040c3 100644
--- a/src/executor/mod.rs
+++ b/src/executor/mod.rs
@@ -207,6 +207,11 @@ impl ExecutorExt for BoxedExecutor {
 }

 impl PlanVisitor<BoxedExecutor> for ExecutorBuilder {
+    fn visit(&mut self, plan: PlanRef) -> Option<BoxedExecutor> {
+        println!("before");
+        // `trace` is a modifier here that do the trace.
+        self.visit_inner(plan).trace()
+    }
+
     fn visit_dummy(&mut self, _plan: &Dummy) -> Option<BoxedExecutor> {
         Some(DummyScanExecutor.execute())
     }
diff --git a/src/optimizer/plan_visitor.rs b/src/optimizer/plan_visitor.rs
index 6f1d82d..ac173f7 100644
--- a/src/optimizer/plan_visitor.rs
+++ b/src/optimizer/plan_visitor.rs
@@ -11,7 +11,11 @@ macro_rules! def_visitor {
     /// The visitor for plan nodes. visit all children and return the ret value of the left most child.
     pub trait PlanVisitor<R> {
     paste! {
-      fn visit(&mut self, plan: PlanRef) -> Option<R>{
+      fn visit(&mut self, plan: PlanRef) -> Option<R> {
+        self.visit_inner(plan)
+      }
+
+      fn visit_inner(&mut self, plan: PlanRef) -> Option<R> {
         match plan.node_type() {
         $(
           crate::optimizer::plan_nodes::PlanNodeType::$node => self.[<visit_ $node:snake>](plan.downcast_ref::<$node>().unwrap()),

BTW, minitrace is not suitable for tracing under such execution model as it does not support the async enter of the span tree unless we modify the executor's interface.

[skyzh]

BTW, minitrace is not suitable for tracing under such execution model as it does not support the async enter of the span tree unless we modify the executor's interface.

I think it can. We propose to add a trace_stream function, which traces every .next call of the executor. minitrace supports async interface, see https://docs.rs/minitrace/latest/minitrace/future/index.html for more information.

[arkbriar]

I think it can. We propose to add a trace_stream function, which traces every .next call of the executor. minitrace supports async interface, see https://docs.rs/minitrace/latest/minitrace/future/index.html for more information.

I see.

[skyzh]

Rest LGTM. Any ideas from cc @zhongzc @breeswish? Thanks!

[skyzh]

Should use .await instead of block_on.

[skyzh]

Can we make it an optional feature? Either using https://doc.rust-lang.org/cargo/reference/features.html and #[cfg(feature = enable_tracing)], or use cli arg to specify whether to enable it.

[WindowsXp-Beta]

gotcha

[skyzh]

Generally LGTM, good work!