From fc9228a066dc202fb28d7b57546670265fb220e7 Mon Sep 17 00:00:00 2001 From: Bob Weinand Date: Wed, 13 Nov 2024 16:49:15 +0100 Subject: [PATCH] clippy & add tests Signed-off-by: Bob Weinand --- sidecar-ffi/src/lib.rs | 4 +- sidecar/src/service/blocking.rs | 4 +- .../debugger_diagnostics_bookkeeper.rs | 96 +++++++++++++++---- 3 files changed, 81 insertions(+), 23 deletions(-) diff --git a/sidecar-ffi/src/lib.rs b/sidecar-ffi/src/lib.rs index 0257a66f1..c5ce8c5f3 100644 --- a/sidecar-ffi/src/lib.rs +++ b/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, instance_id: &InstanceId, queue_id: QueueId, - diagnostics_payload: DebuggerPayload<'a>, + diagnostics_payload: DebuggerPayload, ) -> MaybeError { try_c!(blocking::send_debugger_diagnostics( transport, diff --git a/sidecar/src/service/blocking.rs b/sidecar/src/service/blocking.rs index 503373bc0..72caea640 100644 --- a/sidecar/src/service/blocking.rs +++ b/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(), diff --git a/sidecar/src/service/debugger_diagnostics_bookkeeper.rs b/sidecar/src/service/debugger_diagnostics_bookkeeper.rs index 84bef06dd..489e9aa54 100644 --- a/sidecar/src/service/debugger_diagnostics_bookkeeper.rs +++ b/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))); + } +}