dragonball: add tracing feature for dragonball #7831

lisongqian · 2023-09-04T13:54:42Z

This PR add tracing feature for dragonball.

Signed-off-by: lisongqian mail@lisongqian.cn

katacontainersbot · 2023-09-04T13:58:25Z

Can one of the admins verify this patch?

ZizhengBian · 2023-09-12T16:09:11Z

src/dragonball/Cargo.toml

+tracing = "0.1.37"
+tracing-opentelemetry = "0.18.0"
+opentelemetry = { version = "0.18.0", features = [ "trace", "rt-tokio"] }
+opentelemetry-jaeger = { version = "0.17.0", features = ["rt-tokio", "hyper_collector_client", "collector_client","rt-tokio-current-thread"] }


add a space after ','

ZizhengBian · 2023-09-12T16:17:51Z

src/dragonball/src/event_manager.rs

+impl std::fmt::Debug for EventManager {
+    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+        f.debug_struct("EventManager")
+            .field("epoll_mgr", &self.epoll_mgr.type_id())


Do we need to get the type_id of epoll mgr?

Unnecessary, fixed.

ZizhengBian · 2023-09-12T16:19:35Z

src/dragonball/src/tracer.rs

+pub enum DragonballTraceError {
+    /// verify jaeger config failed.
+    #[error("config verify failed: {0}")]
+    ConfigVerifyFailed(anyhow::Error),


add #source to expand the error stack, and we shouldn't use anyhow in dragonball

ZizhengBian · 2023-09-12T16:20:53Z

src/dragonball/src/tracer.rs

+    fn eq(&self, other: &Self) -> bool {
+        if self.sid == other.sid
+            && self.jaeger_endpoint == other.jaeger_endpoint
+            && self.jaeger_user == other.jaeger_user


self.sid == other.sid && self.jaeger_endpoint == other.jaeger_endpoint && self.jaeger_user == other.jaeger_user && self.jaeger_password == other.jaeger_password

is enough

ZizhengBian · 2023-09-12T16:21:12Z

src/dragonball/src/tracer.rs

+    }
+}
+
+impl Eq for TraceConfigInfo {}


It will encounter a compile error: the trait std::cmp::Eq is not implemented for (dyn tracing::Subscriber + Send + Sync + 'static)

ZizhengBian · 2023-09-12T16:24:51Z

src/dragonball/src/tracer.rs

+                        .with_password(config.jaeger_password)
+                        .with_hyper()
+                        .install_batch(opentelemetry::runtime::Tokio)
+                        .expect("Tracer: create jaeger tracer err");


return error instead of expect

This code is running in the closure of block_on function, we can't process the error outside the closure. I add a comment here.

ZizhengBian · 2023-09-12T16:25:31Z

src/dragonball/src/tracer.rs

+
+    /// tracing feature for app.
+    pub fn setup_tracing(&mut self, config: TraceConfigInfo) -> Result<(), DragonballTraceError> {
+        if !self.enable_tracing {


if self.enable_tracing {return Ok(())}

xxx

ZizhengBian · 2023-09-12T16:25:53Z

src/dragonball/src/tracer.rs

+
+#[cfg(test)]
+mod tests {
+    use opentelemetry::global;


clear up use statement

ZizhengBian · 2023-09-12T16:26:30Z

src/dragonball/src/vmm.rs

-                        return v.stop(EXIT_CODE_OK as i32);
+                        let ret = v.stop(EXIT_CODE_OK as i32);
+                        let tracer_mutex = service.tracer();
+                        let mut tracer = tracer_mutex.lock().unwrap();


tracer_gruad

ZizhengBian · 2023-09-12T16:27:06Z

src/dragonball/src/vmm.rs

@@ -194,6 +203,17 @@ impl Vmm {
    }
 }

+impl std::fmt::Debug for Vmm {


why you need implement debug for vmm?

It's needed for macro instrument. The instrument collects the arguments of function and outputs them to tracing info.

ZizhengBian · 2023-10-22T13:45:05Z

src/dragonball/src/api/v1/vmm_action.rs

+        tracer
+            .setup_tracing(trace_info)
+            .map(|_| VmmData::Empty)
+            .map_err(|_| VmmActionError::TracingFailed)


Don't discard inner errors

ZizhengBian · 2023-10-22T13:45:16Z

src/dragonball/src/api/v1/vmm_action.rs

+        tracer
+            .end_tracing()
+            .map(|_| VmmData::Empty)
+            .map_err(|_| VmmActionError::TracingFailed)


Don't discard inner errors

ZizhengBian · 2023-10-22T13:47:19Z

src/dragonball/src/api/v1/vmm_action.rs

@@ -760,6 +817,16 @@ impl VmmService {
    }
 }

+impl std::fmt::Debug for VmmService {


Try to use this: https://mcarton.github.io/rust-derivative/latest/Debug.html

Lesson learned. I use it for TraceInfo. But I delete the Debug trait for VmmService since we don't need to output the information of VmmService in tracing.

ZizhengBian · 2023-10-22T13:47:51Z

src/dragonball/src/tracer.rs

+    }
+}
+
+impl Eq for TraceInfo {}


Use derive directly

Fixed it with derivative.

ZizhengBian · 2023-10-22T13:48:06Z

src/dragonball/src/tracer.rs

+    }
+}
+
+impl PartialEq for TraceInfo {


Why do you need to implement PartialEq

TraceInfo is used in VmmAction::SetHypervisorTracing(TraceInfo) and VmmAction implements the PartialEq. This is also the reason why I added the sid field to TraceInfo.

src/dragonball/src/tracer.rs

ZizhengBian · 2023-10-22T13:49:36Z

src/dragonball/src/tracer.rs

+
+    /// Return whether the tracing is enabled, enabled by [`setup_tracing`]
+    pub fn enabled(&self) -> bool {
+        self.tracing_on


just name this field to enabled?

src/dragonball/src/vmm.rs

ZizhengBian

LGTM

studychao · 2023-10-26T15:52:37Z

/test

studychao

I recommand that you write a document for how to use the trace functionality under /src/dragonball/docs including how to use and how runtime-rs could use the tracing data from dragonball.
You could choose to do that in another PR.

studychao

thanks, a few comments

src/dragonball/src/api/v1/vmm_action.rs

src/dragonball/src/tracer.rs

studychao · 2023-10-28T09:33:36Z

src/dragonball/src/tracer.rs

+    }
+
+    #[test]
+    fn test_tracing_in_lib() {


Is that more like a integration test instead of unit test?
Will that be too much to create a JAEGER service every time we run unit test?

Yes, this is more like an example and I have removed it. Actually, opentelemetry_jaeger just creates a request to export the tracing info to JAEGER.

src/dragonball/src/vmm.rs

studychao · 2023-10-28T09:35:28Z

Besides, did you see how runtime-rs code and have a plan that how could they use your new two actions?

lisongqian · 2023-10-28T11:08:22Z

Besides, did you see how runtime-rs code and have a plan that how could they use your new two actions?↳

Yes, I saw. I will new a PR to support dragonball tracing for runtime-rs after this PR is merged.

lisongqian · 2023-10-28T11:15:02Z

I recommand that you write a document for how to use the trace functionality under /src/dragonball/docs including how to use and how runtime-rs could use the tracing data from dragonball. You could choose to do that in another PR.↳

I will support the dargonball tracing for dbs-cli after this PR is merged. I can write a document and use the implementation of dbs-cli as an example.

This PR adds the tracing capability for dragonball and it depends on the tracing::Subscriber of the upper layer. Fixes: kata-containers#7249 Signed-off-by: Songqian Li <mail@lisongqian.cn>

studychao

LGTM, also I think that jaeger example could put into dbs-cli if that makes sense.

studychao · 2023-10-30T03:11:51Z

/test

katacontainersbot added the size/huge Largest and most complex task (probably needs breaking into small pieces) label Sep 4, 2023

studychao added the runtime-rs label Sep 5, 2023

lisongqian force-pushed the feat/dragonball_trace branch 2 times, most recently from 5bb62da to 75501b4 Compare September 5, 2023 07:49

ZizhengBian reviewed Sep 12, 2023

View reviewed changes

lisongqian force-pushed the feat/dragonball_trace branch 2 times, most recently from 2be3729 to 76717af Compare September 19, 2023 05:02

lisongqian force-pushed the feat/dragonball_trace branch from 76717af to c2970d8 Compare September 26, 2023 11:43

katacontainersbot added size/large Task of significant size and removed size/huge Largest and most complex task (probably needs breaking into small pieces) labels Sep 26, 2023

lisongqian force-pushed the feat/dragonball_trace branch from c2970d8 to 76630ee Compare October 13, 2023 05:59

katacontainersbot added size/huge Largest and most complex task (probably needs breaking into small pieces) and removed size/large Task of significant size labels Oct 13, 2023

lisongqian force-pushed the feat/dragonball_trace branch 3 times, most recently from 1b2c56a to 72141bb Compare October 17, 2023 15:37

ZizhengBian reviewed Oct 22, 2023

View reviewed changes

lisongqian force-pushed the feat/dragonball_trace branch 4 times, most recently from c9a67fc to 7e5a3a9 Compare October 25, 2023 07:48

ZizhengBian approved these changes Oct 25, 2023

View reviewed changes

quanweiZhou approved these changes Oct 27, 2023

View reviewed changes

lisongqian force-pushed the feat/dragonball_trace branch from 7e5a3a9 to 3cc00b8 Compare October 27, 2023 06:11

studychao reviewed Oct 28, 2023

View reviewed changes

dragonball: add tracing feature for dragonball

2f533c3

This PR adds the tracing capability for dragonball and it depends on the tracing::Subscriber of the upper layer. Fixes: kata-containers#7249 Signed-off-by: Songqian Li <mail@lisongqian.cn>

lisongqian force-pushed the feat/dragonball_trace branch from 3cc00b8 to 2f533c3 Compare October 28, 2023 11:53

katacontainersbot added size/large Task of significant size and removed size/huge Largest and most complex task (probably needs breaking into small pieces) labels Oct 28, 2023

lisongqian mentioned this pull request Oct 29, 2023

dragonball: support dragonball tracing for runtime-rs. #8335

Open

studychao approved these changes Oct 30, 2023

View reviewed changes

studychao added the ok-to-test label Oct 30, 2023

studychao merged commit 7d26604 into kata-containers:main Oct 30, 2023
146 of 153 checks passed

lisongqian deleted the feat/dragonball_trace branch November 7, 2023 11:11

dragonball: add tracing feature for dragonball #7831

dragonball: add tracing feature for dragonball #7831

Conversation

lisongqian commented Sep 4, 2023

katacontainersbot commented Sep 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZizhengBian left a comment

Choose a reason for hiding this comment

studychao commented Oct 26, 2023

studychao left a comment

Choose a reason for hiding this comment

studychao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

studychao commented Oct 28, 2023 • edited

lisongqian commented Oct 28, 2023

lisongqian commented Oct 28, 2023

studychao left a comment

Choose a reason for hiding this comment

studychao commented Oct 30, 2023

studychao commented Oct 28, 2023 •

edited