cleanup(libsinsp): consolidation and extension of libsinsp stats / metrics `sinsp_stats_v2` #1433

incertum · 2023-10-20T23:05:32Z

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area API-version

/area build

/area CI

/area driver-kmod

/area driver-bpf

/area driver-modern-bpf

/area libscap-engine-bpf

/area libscap-engine-gvisor

/area libscap-engine-kmod

/area libscap-engine-modern-bpf

/area libscap-engine-nodriver

/area libscap-engine-noop

/area libscap-engine-source-plugin

/area libscap-engine-savefile

/area libscap-engine-udig

/area libscap

/area libpman

/area libsinsp

/area tests

/area proposals

Does this PR require a change in the driver versions?

/version driver-API-version-major

/version driver-API-version-minor

/version driver-API-version-patch

/version driver-SCHEMA-version-major

/version driver-SCHEMA-version-minor

/version driver-SCHEMA-version-patch

What this PR does / why we need it:

See #1347: Expanding libsinsp stats will help to debug and optimize libsinsp functionality around the threadtable and other aspects in production settings.

In essence, this PR refactors / consolidates / simplifies the libsinsp stats interface and adds valuable metrics that already existed, but weren't easily accessible by clients such as Falco. Plus added overall CPU and memory usages of the host to put the agents' usage easier in relation to the current server load.

Justification for removing the old sinsp stats:

First every metric was preserved with the replacement solution, except the counters below were duplicative of the scap counters, hence we do not re-copy them in the new sinsp stats solution

m_stats->m_n_seen_evts = stats.n_evts;
m_stats->m_n_drops = stats.n_drops;
m_stats->m_n_preemptions = stats.n_preemptions;

The following 2 metrics are snapshot metrics anyways, therefore they now take place in libsinsp::stats::get_sinsp_stats_v2 just like the resource utilization snapshots.

uint64_t m_n_threads;
uint64_t m_n_fds;

As said all other counters that were on the hot path are preserved in the simple sinsp_stats_v2 struct.

Please note that GATHER_INTERNAL_STATS is currently broken in 2 places: When activated libsinsp would not compile because of missing updates in fdinfo.h and a legacy threadtable loop approach void sinsp_thread_manager::update_statistics() -> This is why I believe no one could have seriously used those stats for some time.

Additional information:

During the October core maintainer meeting we discussed, that we are all fine with having all these stats / metrics in the sinsp_stats_v2_buffer and while this PR at least removes and consolidate legacy libsinsp stats, we are all on the same page that as we further cleanup and refactor libsinsp the libsinsp stats functionality will be part of future cleanups.

Question:

Are we ok with just having all these counters just like that on the hot path or do we want to gate them? Meaning another if condition or is it cheaper to just increment or decrement the respective count real fast?

Which issue(s) this PR fixes:

#1347

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

cleanup(libsinsp): consolidation and extension of libsinsp stats / metrics `sinsp_stats_v2 `

incertum · 2023-10-20T23:26:12Z

The corresponding Falco PR is up and here is some example output of the new metrics falcosecurity/falco#2883 (comment)

incertum · 2023-10-21T23:03:23Z

Had a new idea on how to expose additional stats / metrics around the container engine cache.

Missing container images in the logs has been called out for a very very long time. While in some cases when a new container just comes up we can't do much about it, we at least want to make sure everything we can improve has been considered.

It will help to investigate falcosecurity/falco#2708 better.

FedeDP

I like this PR; generally speaking, i'd avoid the check for m_inspector != nullptr since that is already taken for granted in so many places.

Also, i'd disable stats by default and i'd add a sinsp_enable_stats to enable them, making the m_sinsp_stats_v2 field a unique_ptr that is only allocated if stats are enabled.

FedeDP · 2023-10-25T09:57:34Z

userspace/libsinsp/container.cpp

@@ -88,11 +88,24 @@ bool sinsp_container_manager::remove_inactive_containers()
 		});

 		auto containers = m_containers.lock();
+		if (m_inspector != nullptr)


I don't think these checks are necessary since eg: L69 above is already assuming that m_inspector cannot be null.

userspace/libsinsp/fdinfo.cpp

FedeDP · 2023-10-25T09:58:40Z

userspace/libsinsp/fdinfo.cpp

-#ifdef GATHER_INTERNAL_STATS
-			m_inspector->m_stats->m_n_added_fds++;
-#endif
+			if (m_inspector != nullptr)


Same as other comment, 3 lines above we are already accessing m_inspector with no checks; i'd avoid polluting the code :)

Same below.

FedeDP · 2023-10-25T09:59:21Z

userspace/libsinsp/parsers.cpp

-#ifdef GATHER_INTERNAL_STATS
-			m_inspector->m_thread_manager->m_failed_lookups->decrement();
-#endif
+			if (m_inspector != nullptr)


FedeDP · 2023-10-25T10:01:37Z

userspace/libsinsp/sinsp.h

@@ -1121,7 +1122,8 @@ VISIBILITY_PRIVATE
 	bool m_is_dumping;
 	const scap_machine_info* m_machine_info;
 	const scap_agent_info* m_agent_info;
-	scap_stats_v2 m_sinsp_stats_v2[SINSP_MAX_RESOURCE_UTILIZATION];
+	sinsp_stats_v2 m_sinsp_stats_v2;


Since stats were protected by a macro before, can we retains a similar approach by making this back a unique_ptr and only creating it if requested (ie: by a sinsp_enable_stats() API or similar that must be called before the opening of the inspector).

Maybe we don't need a unique_ptr, just ensure the struct is zero-initialized and have an option to disable the stats updates?

If you disable the stats and then read them and expect non-zero values, I'm not sure you have someone else to blame :D

This would save us lots of if m_sinsp_stats_v2 != nil checks, neat!

EDIT: on a second thought, we still have the checks, and still have to embed a "huge" struct inside the inspector + a boolean to know if stats are enabled.
I think a single unique_ptr would allow us:

to not waste space when not needed

do not use the bool at all since we already have the ptr != nil

@gnosek ^ you also onboard, if yes can go ahead and re-push later.

@FedeDP pushed a suggestion, plz let me know. It seems checking the inspector would only be needed in one place and in fact in most cases it's already de-referenced anyways before.

In addition, luckily had a pod sandbox running in the background and realized we should not be counting sandboxes as missing container images ... that's the one important metric I really would like to check on in prod, so we better get it right.

incertum · 2023-10-25T16:33:40Z

@FedeDP thanks for answering the question if we want a flag to enable everything.

Re the null checks, some unit tests create conditions where the inspector in fact would be NULL, hence follow up question:

Change unit tests?
Check if inspector is null only sometimes?
Never check if inspector is null?

Since you didn't object against removing the old implementation I assume that means it's ok?

FedeDP · 2023-10-25T16:41:31Z

thanks for answering the question if we want a flag to enable everything.

Uh i totally missed that and answered it randomly 🤣

hence follow up question:

I'd say either 1 or 3; checking if inspector is null "sometimes" makes no great sense since it has a penalty and we cannot still be sure that inspector is always not null.

Since you didn't object against removing the old implementation I assume that means it's ok?

I haven't got strong opinion, i think it is ok to remove/improve it like you did.
But i am not an user of the API :)
cc @gnosek

incertum · 2023-10-25T21:51:39Z

@FedeDP and @gnosek I added the flag in for now and kept is as simple struct as before as @gnosek suggested, but I can always change it if needed. Personally feel these stats should be as simple as possible in any regards.

Also cleaned up the git history a little bit.

Once we decide on #1433 (comment) I'll make those changes.

incertum · 2023-10-25T21:52:20Z

userspace/libsinsp/test/sinsp_with_test_input.h

@@ -59,7 +59,8 @@ class sinsp_with_test_input : public ::testing::Test {
 	sinsp m_inspector;

 	void open_inspector(scap_mode_t mode = SCAP_MODE_TEST) {
-		m_inspector.open_test_input(m_test_data.get(), mode);
+		m_inspector.open_test_input(m_test_data.get(), mode);	
+		m_inspector.set_sinsp_stats_v2_enabled(true);


Note: Setting this in the unit test only did not work.

incertum · 2023-10-29T07:51:57Z

userspace/libsinsp/sinsp.cpp

@@ -2708,6 +2660,27 @@ void sinsp::set_proc_scan_log_interval_ms(uint64_t val)
 	m_proc_scan_log_interval_ms = val;
 }

+void sinsp::set_sinsp_stats_v2_enabled()
+{
+	m_sinsp_stats_v2 = std::make_unique<sinsp_stats_v2>();


@FedeDP forgot to add checks if it's NULL in case you would call it multiple times. Will re-push, but I'll wait for your feedback first in case there is more.

incertum · 2023-10-31T21:11:18Z

@jasondellaluce is also on board with this interim refactor that will allow us to consume the new metrics.
I created #1463 to track future API refactors we already agreed on.

One more question: Do we or can we merge this and the Falco PR this week? Else I would propose the week of Nov 13th as Jason and I will be out at KubeCon NA.

This Falco PR falcosecurity/falco#2879 also needs to be merged before the corresponding Falco PR falcosecurity/falco#2883 or I consolidate those 2 PRs in one.
In any case we should update Falco so it's not incompatible anymore with current libs master.

Andreagit97

I left some comments. To be honest, I'm not fully convinced by the approach we are following with these stats, we have sinsp_stats, machine_info_stats, agent_info_stats, and resource_stats ... some in the platforms some with a shared pointer in the inspector and then populated with an external method libsinsp::stats::get_.... Probably before going on with further steps, I would like to plan a final design to understand what is the final aim because at the moment it is not clear to me

Andreagit97 · 2023-11-07T11:14:06Z

userspace/libsinsp/sinsp.cpp

+{
+	if (m_sinsp_stats_v2 == nullptr)
+	{
+		m_sinsp_stats_v2 = std::make_unique<sinsp_stats_v2>();


do we want a shared or a unique pointer? if consumers want to manage these metrics probably we want to use a shared pointer

correct needs to be a shared pointer exactly because of what you wrote here

Andreagit97 · 2023-11-07T12:14:46Z

userspace/libsinsp/stats.h

+	uint64_t m_n_failed_thread_lookups;
+	uint64_t m_n_added_threads;
+	uint64_t m_n_removed_threads;
+	uint32_t m_n_missing_container_images;


why uint32_t?

Seems large enough and uint64_t likely never needed. I doubt machines would go above 200-500 containers per host at a time anytime soon ... and let me know if we need additional comments to clarify that these container counters are not monotonic and the current view only similar to n_threads and n_fds. We could make them monotonic, but then we need to hook into the container engine differently. I know it's always a mess and believe me likely no winning ever with finding the perfect metrics.

Andreagit97 · 2023-11-07T12:18:32Z

userspace/libsinsp/sinsp.cpp

@@ -44,6 +44,7 @@ limitations under the License.
 #include "plugin_filtercheck.h"
 #include "strl.h"
 #include "scap-int.h"
+#include "stats.h"


we include sinsp.h so probably we don't need stats.h also here

Andreagit97 · 2023-11-07T12:30:54Z

userspace/libsinsp/test/sinsp_with_test_input.h

+		m_inspector.set_sinsp_stats_v2_enabled();
+		// Extra call to see we don;t fail
+		m_inspector.set_sinsp_stats_v2_enabled();


i would enable the stats directly on the tests we need, not in all tests by default

Andreagit97 · 2023-11-07T12:32:38Z

userspace/libsinsp/threadinfo.cpp

-		m_failed_lookups->increment();
-#endif
+		// Extra m_inspector nullptr check
+		if (m_inspector != nullptr && m_inspector->m_sinsp_stats_v2)


the thread manager is contained into the inspector so probably we don't need this null check

Yes the problem child condition: asan will fail if we don't check here because of TEST(sinsp_thread_manager, remove_non_existing_thread), change the unit test?

Andreagit97 · 2023-11-07T13:17:42Z

userspace/libsinsp/threadinfo.cpp

@@ -1444,14 +1444,6 @@ void sinsp_thread_manager::clear()
 	m_last_tid = 0;
 	m_last_flush_time_ns = 0;
 	m_n_drops = 0;
-


SInce we have removed all these fields at this point i would remove also m_n_drops and add it to sinsp_stats. Maybe we could use a more meaningful name like full_threadtable_drops. In this way we can handle this stat like all the others without a dedicated method get_m_n_drops

Done, but came at a small costs in terms of changing a g_logger log that now only fires if sinsp stats v2 are enabled.

Andreagit97 · 2023-11-07T13:20:09Z

userspace/libsinsp/threadinfo.cpp

+	if (m_inspector->m_sinsp_stats_v2)
+	{
+		m_inspector->m_sinsp_stats_v2->m_n_added_threads++;
+	}


I would move this to the end of the function because here it is not counting the added_threads, the table could be full. This counts the requests of adding a thread

Andreagit97 · 2023-11-07T16:18:50Z

userspace/libsinsp/stats.cpp

+		buffer[SINSP_STATS_V2_N_FDS].value.u64 = 0;
+		threadinfo_map_t* threadtable = thread_manager->get_threads();
+		threadtable->loop([&] (sinsp_threadinfo& tinfo) {
+			sinsp_fdtable* fdtable = tinfo.get_fd_table();


here we need a check sinsp_fdtable != nullptr

❤️ more segfaults is the last thing we need, which makes we wonder if we could come up with a more proper project inspection in this regard as sometimes we say we trust the pointer to be never NULL and prefer to save the overhead of checking vs sometimes we definitely need to check ...

Andreagit97 · 2023-11-13T08:48:23Z

Before going on with code changes, I have some general questions not related to the code:

I saw that in Falco (and I imagine also in other consumers) we use the get_capture_stats method to obtain the number of drops/events. On the other hand, with stats_v2 we are using an agnostic approach, where the final consumer receives a vector of metrics already populated by sinsp. My question here is, do we want scap_stats_v2 to replace the old scap_stats? If yes, how do we obtain the specific number of drops/events from this agnostic approach? Do we want to keep these specific numbers or the final goal is to expose a set of metrics with a Prometheus endpoint?
If the final goal is to expose only a Prometheus endpoint using something like https://github.com/jupp0r/prometheus-cpp, are we doing the right thing by exposing a vector of struct scap_stats_v2? At this point we could directly create Prometheus structs in sinsp and expose them to the consumers, so they can add them to the registry and expose them.

WDYT?

- Prepare m_sinsp_stats_v2 buffer to hold more stats and metrics than resource utilization metrics - Expand resource utilization metrics (e.g. include overall host CPU and memory usages) Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

- Finalize exposing valuable legacy libsinsp stats / metrics / counters related to the sinsp state tables (e.g. adding, removing, storing, failed lookup activities) to the consolidated libsinsp stats - Add counter of currently cached containers as well - Includes additional cleanups Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

- Get container cache stats via hijacking the periodic container cache flush functionality - Includes some general cleanup Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

Co-authored-by: Federico Di Pierro <nierro92@gmail.com> Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

* make m_sinsp_stats_v2 a smart pointer initialized only when enabling sinsp_stats_v2 counters * only keep m_inspector nullptr check for `sinsp_thread_manager::remove_thread(int64_t tid)` Co-authored-by: Federico Di Pierro <nierro92@gmail.com> Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

Co-authored-by: Andrea Terzolo <andreaterzolo3@gmail.com> Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

incertum · 2023-11-14T00:56:39Z

@Andreagit97 thank you, I have moved the discussion to relevant issues.

Andreagit97 · 2023-11-14T16:48:37Z

thank you, I will take a look in these days! sorry, but I'm a bit busy with k8s plugin + falco config/falco-driver-loader + various cleanup 😖 In the meanwhile if someone has other ideas please jump in 🙏

Andreagit97

/approve

poiana · 2023-11-17T13:37:58Z

LGTM label has been added.

Git tree hash: b1e038d84fffa650a145d188fae3ef2244f92de1

jasondellaluce

/approve

poiana · 2023-11-17T18:52:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, incertum, jasondellaluce

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Andreagit97,incertum,jasondellaluce]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

poiana added kind/cleanup release-note dco-signoff: yes kind/feature New feature or request area/libsinsp size/XXL approved labels Oct 20, 2023

poiana requested review from hbrueckner and leogr October 20, 2023 23:05

incertum mentioned this pull request Oct 20, 2023

update(userspace/falco): add libsinsp state metrics option falcosecurity/falco#2883

Merged

incertum force-pushed the sinsp-stats-v2-extension branch from 532e92d to 9e79c35 Compare October 21, 2023 22:58

Andreagit97 added this to the 0.14.0 milestone Oct 23, 2023

incertum force-pushed the sinsp-stats-v2-extension branch 3 times, most recently from ca550c7 to 3b2dc0b Compare October 24, 2023 16:40

FedeDP reviewed Oct 25, 2023

View reviewed changes

incertum force-pushed the sinsp-stats-v2-extension branch from 764b093 to 140f5d7 Compare October 25, 2023 21:16

incertum commented Oct 25, 2023

View reviewed changes

incertum force-pushed the sinsp-stats-v2-extension branch from 140f5d7 to c58282c Compare October 29, 2023 04:52

incertum commented Oct 29, 2023

View reviewed changes

incertum force-pushed the sinsp-stats-v2-extension branch 2 times, most recently from c2f67ce to 1d50e28 Compare October 30, 2023 23:13

incertum mentioned this pull request Oct 31, 2023

[TRACKING] Create a more coherent stats model for libs and consumer #1463

Closed

Andreagit97 reviewed Nov 7, 2023

View reviewed changes

incertum and others added 10 commits November 13, 2023 22:53

new(test): add sinsp_stats unit test

837df55

Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

feat(libsinsp): remove legacy libsinsp stats GATHER_INTERNAL_STATS

0d6d5cf

Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

new(libsinsp): expose full threadtable drops to libsinsp stats

de9863a

Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

new(libsinsp): expose crucial container cache stats

68da9ff

- Get container cache stats via hijacking the periodic container cache flush functionality - Includes some general cleanup Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

fix(cmake): adjust setup re sinsp stats

042db53

Co-authored-by: Federico Di Pierro <nierro92@gmail.com> Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

cleanup(libsinsp): don't count sanboxes as missing_container_images

a4ccc7c

Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

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

0c856e8

Co-authored-by: Andrea Terzolo <andreaterzolo3@gmail.com> Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

incertum force-pushed the sinsp-stats-v2-extension branch from 1d50e28 to 0c856e8 Compare November 13, 2023 23:59

incertum mentioned this pull request Nov 14, 2023

[DISCUSSION] deprecate / remove old internal drop stats? falcosecurity/falco#2910

Open

Andreagit97 approved these changes Nov 17, 2023

View reviewed changes

poiana assigned Andreagit97 Nov 17, 2023

poiana added the lgtm label Nov 17, 2023

jasondellaluce approved these changes Nov 17, 2023

View reviewed changes

poiana assigned jasondellaluce Nov 17, 2023

poiana merged commit 9866af1 into falcosecurity:master Nov 17, 2023
26 checks passed

incertum deleted the sinsp-stats-v2-extension branch December 8, 2023 20:04

incertum mentioned this pull request Dec 16, 2023

[TRACKING] Re-audit container engines for empty container info values (Initial focus on CRI for Kubernetes) falcosecurity/falco#2708

Open

cleanup(libsinsp): consolidation and extension of libsinsp stats / metrics sinsp_stats_v2 #1433

cleanup(libsinsp): consolidation and extension of libsinsp stats / metrics sinsp_stats_v2 #1433

Conversation

incertum commented Oct 20, 2023

incertum commented Oct 20, 2023

incertum commented Oct 21, 2023

FedeDP left a 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

FedeDP Oct 26, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

incertum commented Oct 25, 2023

FedeDP commented Oct 25, 2023 • edited

incertum commented Oct 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

incertum commented Oct 31, 2023

Andreagit97 left a 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

Andreagit97 commented Nov 13, 2023

incertum commented Nov 14, 2023

Andreagit97 commented Nov 14, 2023 • edited

Andreagit97 left a comment

Choose a reason for hiding this comment

poiana commented Nov 17, 2023

jasondellaluce left a comment

Choose a reason for hiding this comment

poiana commented Nov 17, 2023

cleanup(libsinsp): consolidation and extension of libsinsp stats / metrics `sinsp_stats_v2` #1433

cleanup(libsinsp): consolidation and extension of libsinsp stats / metrics `sinsp_stats_v2` #1433

FedeDP Oct 26, 2023 •

edited

FedeDP commented Oct 25, 2023 •

edited

Andreagit97 commented Nov 14, 2023 •

edited