From 9866af16405b4364f932690d35cfe7e13e433f48 Mon Sep 17 00:00:00 2001 From: Melissa Kilby Date: Mon, 13 Nov 2023 23:45:45 +0000 Subject: [PATCH] cleanup(libsinsp): apply reviewers suggestions re sinsp stats v2 Co-authored-by: Andrea Terzolo Signed-off-by: Melissa Kilby --- userspace/libsinsp/sinsp.cpp | 2 +- userspace/libsinsp/stats.cpp | 7 ++++-- userspace/libsinsp/stats.h | 1 + userspace/libsinsp/test/sinsp_stats.ut.cpp | 3 +++ .../libsinsp/test/sinsp_with_test_input.h | 3 --- userspace/libsinsp/threadinfo.cpp | 23 +++++++++++-------- userspace/libsinsp/threadinfo.h | 2 -- 7 files changed, 23 insertions(+), 18 deletions(-) diff --git a/userspace/libsinsp/sinsp.cpp b/userspace/libsinsp/sinsp.cpp index da24ca6255..73cc459000 100644 --- a/userspace/libsinsp/sinsp.cpp +++ b/userspace/libsinsp/sinsp.cpp @@ -43,7 +43,6 @@ limitations under the License. #include "plugin_filtercheck.h" #include "strl.h" #include "scap-int.h" -#include "stats.h" #if defined(HAS_CAPTURE) && !defined(CYGWING_AGENT) && !defined(MINIMAL_BUILD) && !defined(__EMSCRIPTEN__) #include @@ -2327,6 +2326,7 @@ void sinsp::set_sinsp_stats_v2_enabled() m_sinsp_stats_v2->m_n_failed_thread_lookups = 0; m_sinsp_stats_v2->m_n_added_threads = 0; m_sinsp_stats_v2->m_n_removed_threads = 0; + m_sinsp_stats_v2->m_n_drops_full_threadtable = 0; m_sinsp_stats_v2->m_n_missing_container_images = 0; m_sinsp_stats_v2->m_n_containers= 0; } diff --git a/userspace/libsinsp/stats.cpp b/userspace/libsinsp/stats.cpp index d6358c06d4..82e8fcc0ef 100644 --- a/userspace/libsinsp/stats.cpp +++ b/userspace/libsinsp/stats.cpp @@ -390,7 +390,10 @@ const scap_stats_v2* libsinsp::stats::get_sinsp_stats_v2(uint32_t flags, const s threadinfo_map_t* threadtable = thread_manager->get_threads(); threadtable->loop([&] (sinsp_threadinfo& tinfo) { sinsp_fdtable* fdtable = tinfo.get_fd_table(); - buffer[SINSP_STATS_V2_N_FDS].value.u64 += fdtable->size(); + if (fdtable != nullptr) + { + buffer[SINSP_STATS_V2_N_FDS].value.u64 += fdtable->size(); + } return true; }); buffer[SINSP_STATS_V2_NONCACHED_FD_LOOKUPS].value.u64 = stats_v2->m_n_noncached_fd_lookups; @@ -407,7 +410,7 @@ const scap_stats_v2* libsinsp::stats::get_sinsp_stats_v2(uint32_t flags, const s buffer[SINSP_STATS_V2_FAILED_THREAD_LOOKUPS].value.u64 = stats_v2->m_n_failed_thread_lookups; buffer[SINSP_STATS_V2_ADDED_THREADS].value.u64 = stats_v2->m_n_added_threads; buffer[SINSP_STATS_V2_REMOVED_THREADS].value.u64 = stats_v2->m_n_removed_threads; - buffer[SINSP_STATS_V2_N_DROPS_FULL_THREADTABLE].value.u32 = thread_manager->get_m_n_drops(); + buffer[SINSP_STATS_V2_N_DROPS_FULL_THREADTABLE].value.u32 = stats_v2->m_n_drops_full_threadtable; buffer[SINSP_STATS_V2_N_MISSING_CONTAINER_IMAGES].value.u32 = stats_v2->m_n_missing_container_images; buffer[SINSP_STATS_V2_N_CONTAINERS].value.u32 = stats_v2->m_n_containers; diff --git a/userspace/libsinsp/stats.h b/userspace/libsinsp/stats.h index c65d604fb3..6aea8e509e 100644 --- a/userspace/libsinsp/stats.h +++ b/userspace/libsinsp/stats.h @@ -37,6 +37,7 @@ typedef struct sinsp_stats_v2 uint64_t m_n_failed_thread_lookups; uint64_t m_n_added_threads; uint64_t m_n_removed_threads; + uint32_t m_n_drops_full_threadtable; uint32_t m_n_missing_container_images; uint32_t m_n_containers; }sinsp_stats_v2; diff --git a/userspace/libsinsp/test/sinsp_stats.ut.cpp b/userspace/libsinsp/test/sinsp_stats.ut.cpp index c229bbed0f..34e5faede9 100644 --- a/userspace/libsinsp/test/sinsp_stats.ut.cpp +++ b/userspace/libsinsp/test/sinsp_stats.ut.cpp @@ -23,6 +23,9 @@ limitations under the License. TEST_F(sinsp_with_test_input, sinsp_stats_v2_resource_utilization) { + m_inspector.set_sinsp_stats_v2_enabled(); + // Extra call to see we don't fail + m_inspector.set_sinsp_stats_v2_enabled(); // Adopted from test: TEST_F(sinsp_with_test_input, PROC_FILTER_nthreads) DEFAULT_TREE /* we call a random event to obtain an event associated with this thread info */ diff --git a/userspace/libsinsp/test/sinsp_with_test_input.h b/userspace/libsinsp/test/sinsp_with_test_input.h index a3c32ac82a..d69a6bf8e7 100644 --- a/userspace/libsinsp/test/sinsp_with_test_input.h +++ b/userspace/libsinsp/test/sinsp_with_test_input.h @@ -60,9 +60,6 @@ class sinsp_with_test_input : public ::testing::Test { void open_inspector(sinsp_mode_t mode = SINSP_MODE_TEST) { m_inspector.open_test_input(m_test_data.get(), mode); - m_inspector.set_sinsp_stats_v2_enabled(); - // Extra call to see we don;t fail - m_inspector.set_sinsp_stats_v2_enabled(); } scap_evt* add_event(uint64_t ts, uint64_t tid, ppm_event_code event_type, uint32_t n, ...) diff --git a/userspace/libsinsp/threadinfo.cpp b/userspace/libsinsp/threadinfo.cpp index a9947e83dc..f672c2410b 100644 --- a/userspace/libsinsp/threadinfo.cpp +++ b/userspace/libsinsp/threadinfo.cpp @@ -1423,7 +1423,6 @@ void sinsp_thread_manager::clear() m_thread_groups.clear(); m_last_tid = 0; m_last_flush_time_ns = 0; - m_n_drops = 0; } /* This is called on the table after the `/proc` scan */ @@ -1509,10 +1508,6 @@ std::unique_ptr sinsp_thread_manager::new_threadinfo() const */ bool sinsp_thread_manager::add_thread(sinsp_threadinfo *threadinfo, bool from_scap_proctable) { - if (m_inspector->m_sinsp_stats_v2) - { - m_inspector->m_sinsp_stats_v2->m_n_added_threads++; - } /* We have no more space */ if(m_threadtable.size() >= m_max_thread_table_size @@ -1521,13 +1516,16 @@ bool sinsp_thread_manager::add_thread(sinsp_threadinfo *threadinfo, bool from_sc #endif ) { - // rate limit messages to avoid spamming the logs - if (m_n_drops % m_max_thread_table_size == 0) + if (m_inspector->m_sinsp_stats_v2) { - g_logger.format(sinsp_logger::SEV_INFO, "Thread table full, dropping tid %lu (pid %lu, comm \"%s\")", - threadinfo->m_tid, threadinfo->m_pid, threadinfo->m_comm.c_str()); + // rate limit messages to avoid spamming the logs + if (m_inspector->m_sinsp_stats_v2->m_n_drops_full_threadtable % m_max_thread_table_size == 0) + { + g_logger.format(sinsp_logger::SEV_INFO, "Thread table full, dropping tid %lu (pid %lu, comm \"%s\")", + threadinfo->m_tid, threadinfo->m_pid, threadinfo->m_comm.c_str()); + } + m_inspector->m_sinsp_stats_v2->m_n_drops_full_threadtable++; } - m_n_drops++; return false; } @@ -1550,6 +1548,11 @@ bool sinsp_thread_manager::add_thread(sinsp_threadinfo *threadinfo, bool from_sc tinfo_shared_ptr->compute_program_hash(); m_threadtable.put(std::move(tinfo_shared_ptr)); + if (m_inspector->m_sinsp_stats_v2) + { + m_inspector->m_sinsp_stats_v2->m_n_added_threads++; + } + return true; } diff --git a/userspace/libsinsp/threadinfo.h b/userspace/libsinsp/threadinfo.h index 14a738b5c8..0df1d08200 100644 --- a/userspace/libsinsp/threadinfo.h +++ b/userspace/libsinsp/threadinfo.h @@ -794,7 +794,6 @@ class SINSP_PUBLIC sinsp_thread_manager: public libsinsp::state::table int32_t get_m_n_proc_lookups() const { return m_n_proc_lookups; } int32_t get_m_n_main_thread_lookups() const { return m_n_main_thread_lookups; } uint64_t get_m_n_proc_lookups_duration_ns() const { return m_n_proc_lookups_duration_ns; } - uint32_t get_m_n_drops() const { return m_n_drops; } void reset_thread_counters() { m_n_proc_lookups = 0; m_n_main_thread_lookups = 0; m_n_proc_lookups_duration_ns = 0; } void set_m_max_n_proc_lookups(int32_t val) { m_max_n_proc_lookups = val; } @@ -892,7 +891,6 @@ VISIBILITY_PRIVATE int64_t m_last_tid; std::weak_ptr m_last_tinfo; uint64_t m_last_flush_time_ns; - uint32_t m_n_drops; const uint32_t m_thread_table_absolute_max_size = 131072; uint32_t m_max_thread_table_size; int32_t m_n_proc_lookups = 0;