cleanup(libsinsp): apply reviewers suggestions re sinsp stats v2

Co-authored-by: Andrea Terzolo <[email protected]> Signed-off-by: Melissa Kilby <[email protected]>
falcosecurity · Nov 17, 2023 · 9866af1 · 9866af1
1 parent 1182f95
commit 9866af1
Show file tree

Hide file tree

Showing 7 changed files with 23 additions and 18 deletions.
diff --git 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 <curl/curl.h>
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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_threadinfo> 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
@@ -794,7 +794,6 @@ class SINSP_PUBLIC sinsp_thread_manager: public libsinsp::state::table<int64_t>
 	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<sinsp_threadinfo> 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;