clippy & add tests

Signed-off-by: Bob Weinand <[email protected]>

sidecar-ffi/src/lib.rs

@@ -702,11 +702,11 @@ pub unsafe extern "C" fn ddog_sidecar_send_debugger_datum(
 #[no_mangle]
 #[allow(clippy::missing_safety_doc)]
 #[allow(improper_ctypes_definitions)] // DebuggerPayload is just a pointer, we hide its internals
-pub unsafe extern "C" fn ddog_sidecar_send_debugger_diagnostics<'a>(
+pub unsafe extern "C" fn ddog_sidecar_send_debugger_diagnostics(
     transport: &mut Box<SidecarTransport>,
     instance_id: &InstanceId,
     queue_id: QueueId,
-    diagnostics_payload: DebuggerPayload<'a>,
+    diagnostics_payload: DebuggerPayload,
 ) -> MaybeError {
     try_c!(blocking::send_debugger_diagnostics(
         transport,

sidecar/src/service/blocking.rs

@@ -366,11 +366,11 @@ pub fn send_debugger_data_shm_vec(
 /// # Returns
 ///
 /// An `io::Result<()>` indicating the result of the operation.
-pub fn send_debugger_diagnostics<'a>(
+pub fn send_debugger_diagnostics(
     transport: &mut SidecarTransport,
     instance_id: &InstanceId,
     queue_id: QueueId,
-    diagnostics_payload: DebuggerPayload<'a>,
+    diagnostics_payload: DebuggerPayload,
 ) -> io::Result<()> {
     transport.send(SidecarInterfaceRequest::SendDebuggerDiagnostics {
         instance_id: instance_id.clone(),

sidecar/src/service/debugger_diagnostics_bookkeeper.rs

@@ -1,3 +1,5 @@
+// Copyright 2021-Present Datadog, Inc. https://www.datadoghq.com/
+// SPDX-License-Identifier: Apache-2.0
 use datadog_live_debugger::debugger_defs::{
     DebuggerData, DebuggerPayload, Diagnostics, ProbeStatus,
 };
@@ -43,7 +45,7 @@ impl DebuggerDiagnosticsBookkeeper {
                             active.active_probes.retain(|_, status| {
                                 status.last_update.elapsed() < MAX_TIME_BEFORE_REMOVAL
                             });
-                            active.active_probes.len() != 0
+                            !active.active_probes.is_empty()
                         });
                     },
                     _ = cancel.cancelled() => {
@@ -70,21 +72,21 @@ impl DebuggerDiagnosticsBookkeeper {
             }
 
             let mut buffers = self.active_by_runtime_id.lock().unwrap();
-            if let Some(buffer) = buffers.get_mut(diagnostics.runtime_id.as_ref()) {
-                if let Some(status) = buffer
-                    .active_probes
-                    .get_mut(diagnostics.runtime_id.as_ref())
-                {
-                    if !matches!(diagnostics.status, ProbeStatus::Received) {
-                        if matches!(status.status, ProbeStatus::Received)
-                            || (!matches!(diagnostics.status, ProbeStatus::Installed)
-                                && matches!(status.status, ProbeStatus::Installed))
-                        {
-                            if status.last_update.elapsed() < MAX_TIME_BEFORE_FALLBACK {
-                                send = false;
-                            }
-                        }
-                    }
+            let runtime_id = diagnostics
+                .parent_id
+                .as_ref()
+                .unwrap_or(&diagnostics.runtime_id);
+            if let Some(buffer) = buffers.get_mut(runtime_id.as_ref()) {
+                if let Some(status) = buffer.active_probes.get_mut(diagnostics.probe_id.as_ref()) {
+                    // This is a bit confusing now, but clippy requested me to collapse this:
+                    // Essentially, we shall send if the last emitted/error/installed/etc. is older
+                    // than MAX_TIME_BEFORE_FALLBACK. If it's installed, we also
+                    // send it the current status is Received.
+                    send = matches!(status.status, ProbeStatus::Received)
+                        || (!matches!(diagnostics.status, ProbeStatus::Received)
+                            && (matches!(status.status, ProbeStatus::Installed)
+                                || !matches!(diagnostics.status, ProbeStatus::Installed)))
+                        || status.last_update.elapsed() > MAX_TIME_BEFORE_FALLBACK;
                     if send {
                         status.last_update = Instant::now();
                         if status.status != diagnostics.status {
@@ -94,12 +96,12 @@ impl DebuggerDiagnosticsBookkeeper {
                         }
                     }
                 } else {
-                    insert_probe(buffer, &diagnostics);
+                    insert_probe(buffer, diagnostics);
                 }
             } else {
-                buffers.insert(diagnostics.runtime_id.to_string(), {
+                buffers.insert(runtime_id.to_string(), {
                     let mut data = DebuggerActiveData::default();
-                    insert_probe(&mut data, &diagnostics);
+                    insert_probe(&mut data, diagnostics);
                     data
                 });
             }
@@ -139,3 +141,59 @@ pub struct DebuggerDiagnosticsBookkeeperStats {
     runtime_ids: u32,
     total_probes: u32,
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use datadog_live_debugger::debugger_defs::{
+        DebuggerData, DebuggerPayload, Diagnostics, ProbeStatus,
+    };
+    use std::borrow::Cow;
+
+    fn create_payload<'a>(
+        probe_id: &'a str,
+        runtime_id: &'a str,
+        status: ProbeStatus,
+    ) -> DebuggerPayload<'a> {
+        DebuggerPayload {
+            service: Default::default(),
+            ddsource: Default::default(),
+            timestamp: 0,
+            debugger: DebuggerData::Diagnostics(Diagnostics {
+                probe_id: Cow::Borrowed(probe_id),
+                runtime_id: Cow::Borrowed(runtime_id),
+                parent_id: None,
+                probe_version: 0,
+                status,
+                exception: None,
+                details: None,
+            }),
+            message: None,
+        }
+    }
+
+    #[tokio::test]
+    async fn test_bookkeeper() {
+        let bookkeeper = DebuggerDiagnosticsBookkeeper::start();
+        assert!(bookkeeper.add_payload(&create_payload("1", "2", ProbeStatus::Received)));
+        // Second insert of same thing is rejected
+        assert!(!bookkeeper.add_payload(&create_payload("1", "2", ProbeStatus::Received)));
+        // Different thing is allowed
+        assert!(bookkeeper.add_payload(&create_payload("1", "3", ProbeStatus::Received)));
+        assert!(bookkeeper.add_payload(&create_payload("2", "2", ProbeStatus::Received)));
+
+        // We can move to installed
+        assert!(bookkeeper.add_payload(&create_payload("1", "2", ProbeStatus::Installed)));
+        // But not back
+        assert!(!bookkeeper.add_payload(&create_payload("1", "2", ProbeStatus::Received)));
+        assert!(!bookkeeper.add_payload(&create_payload("1", "2", ProbeStatus::Installed)));
+
+        // We can move to e.g. error or emitting
+        assert!(bookkeeper.add_payload(&create_payload("1", "2", ProbeStatus::Emitting)));
+        assert!(bookkeeper.add_payload(&create_payload("1", "2", ProbeStatus::Error)));
+        assert!(bookkeeper.add_payload(&create_payload("1", "2", ProbeStatus::Emitting)));
+        // But not back
+        assert!(!bookkeeper.add_payload(&create_payload("1", "2", ProbeStatus::Received)));
+        assert!(!bookkeeper.add_payload(&create_payload("1", "2", ProbeStatus::Installed)));
+    }
+}