new(plugin_api): add plugin metrics support #1828

mrgian · 2024-05-03T08:23:31Z

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

/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:
Adds metrics support to the plugin API.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

new(plugin_api): add plugin metrics support

FedeDP · 2024-05-07T06:49:34Z

/milestone 0.17.0

userspace/libsinsp/plugin.cpp

userspace/plugin/plugin_types.h

incertum · 2024-05-09T18:46:21Z

New proposal @mrgian: A bummer that the plugin system is so restrictive, but let's focus on this PR.

Why don't we just define a vanilla struct for the plugin metrics and then return that struct via the sinsp_plugin::get_metrics() call and then create the actual metrics within the libsinsp metrics collector.

That way we don't need any of the redefinitions and it would be very similar approach to how we retrieve the libsinsp state metrics, for instance checkout the sinsp_stats_v2 struct and handling of it in the metrics libs collector.

mrgian · 2024-05-09T20:09:58Z

@incertum Keep in mind that the main point of this feature is to let plugins define their own custom metrics.
sinsp_stats_v2 consists of a fixed set of metrics.
If we do the same with plugins, how they will be able to define a new custom metric?

incertum · 2024-05-09T20:28:16Z

@incertum Keep in mind that the main point of this feature is to let plugins define their own custom metrics. sinsp_stats_v2 consists of a fixed set of metrics. If we do the same with plugins, how they will be able to define a new custom metric?

Oh I somehow thought we wanted just some fixed wrapper metrics like event counts, drops etc since that is what we exposed in Falco before.

Anyways given the plugin system is how it is, seems that we need to keep re-defining things. Just like I called out that it is not a nice approach to needing to redefine the event types when writing a plugin for the syscalls source ...
Wondering is this really the only way it can be cc @jasondellaluce? Is there no chance to have a plugin system that integrates more easily with libs back and forth?

incertum

/approve

poiana · 2024-05-14T19:43:58Z

LGTM label has been added.

Git tree hash: f1cb57941205278b29711a490fde0ac29f8dfa59

FedeDP · 2024-05-15T07:43:46Z

Windows CI is failing plus there is now a conflict :) Care to take a look? Thanks!

mrgian · 2024-05-15T08:10:00Z

@FedeDP Can you re-run the CI? The Windows build should be ok now (I hope). Thanks :)

userspace/libsinsp/test/plugins.ut.cpp

FedeDP · 2024-05-15T09:09:11Z

/cc @jasondellaluce

Signed-off-by: Gianmatteo Palmieri <[email protected]>

FedeDP · 2024-05-15T12:41:12Z

userspace/libsinsp/test/plugins.ut.cpp

@@ -890,3 +891,39 @@ TEST(sinsp_plugin, plugin_set_config)

 	libsinsp_logger()->remove_callback_log();
 }
+
+#ifdef __linux__


Why is this linux only?

libs_metrics_collector is currently compiled on linux only (see https://github.com/falcosecurity/libs/blob/master/userspace/libsinsp/metrics_collector.h#L82)

I think the reason behind is that we had linux-only metrics.
However, with this PR that's not the case anymore, so we should definitely enable metrics in other platforms too.
I'm working on that, but maybe it's better to open another PR when I'm done :)

Out of simplicity as macOS and Windows were / are not a primary metrics use case. In more detail it's because of how we compute CPU usage etc, which is Linux specific.

I believe there is no harm to refine what actually only needs to be linux. @mrgian does this help?

Hey @incertum Thank you!
I believe we just need to rearrange #ifdef __linux__ directives.
The only hiccup I had is in https://github.com/falcosecurity/libs/blob/master/userspace/libsinsp/metrics_collector.cpp#L35-L56
That uses designated initialization (explicit array indexes) which is not supported by MSVC.
However I don't think removing these explicit indexes is a big problem, the comment on top should be enough for contributors to understand that metrics_unit_name_mappings_prometheus and metrics_metric_type_name_mappings_prometheus need to stay in sync with the enums in metrics_v2.h.

Out of scope, but have we ever thought about using a client library for prometheus, like: https://github.com/jupp0r/prometheus-cpp ? @incertum

@FedeDP see the previous discussions; that was the whole point of not adding another lib as we have a more unique use case with all of our different metrics spanning multiple areas of the code base, plus that lib seems not very suitable and supported enough IMO.

Thanks Melissa! I probably lost that part :)

@mrgian mind elaborating However I don't think removing these explicit indexes is a big problem,?

mrgian@cddf7b7
👆 @incertum This is what I've done to make libs_metrics_collector compile on Windows.
Notice how I had to remove designation initializers from metrics_unit_name_mappings_prometheus, it allows to compile with MSVC and the code behavior stays the same.
However we now have to make sure metrics_unit_name_mappings_prometheus has the same ordering as the values in metrics_v2_value_unit in metrics_v2.h. This comment should be enough to make contributors aware of this.
The same applies for metrics_metric_type_name_mappings_prometheus.

I believe ok for now since we don't have much time left for the next release.

incertum

/approve

poiana · 2024-05-16T08:46:46Z

LGTM label has been added.

Git tree hash: 677159adc1a667b27f25cfc8d61164a9366f085e

FedeDP

/approve

poiana · 2024-05-16T12:30:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP, incertum, mrgian

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~~ [FedeDP,incertum]

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

poiana added do-not-merge/work-in-progress release-note kind/feature New feature or request area/libsinsp dco-signoff: yes area/tests labels May 3, 2024

poiana requested review from hbrueckner and mstemm May 3, 2024 08:23

poiana added the size/XL label May 3, 2024

poiana added this to the 0.17.0 milestone May 7, 2024

incertum reviewed May 7, 2024

View reviewed changes

userspace/libsinsp/plugin.cpp Show resolved Hide resolved

userspace/libsinsp/plugin.cpp Outdated Show resolved Hide resolved

userspace/plugin/plugin_types.h Show resolved Hide resolved

userspace/plugin/plugin_types.h Outdated Show resolved Hide resolved

mrgian force-pushed the plugin-api-metrics-finalized branch from 08274bc to caa5c41 Compare May 13, 2024 14:09

mrgian marked this pull request as ready for review May 13, 2024 14:10

poiana removed the do-not-merge/work-in-progress label May 13, 2024

poiana requested a review from incertum May 13, 2024 14:10

mrgian force-pushed the plugin-api-metrics-finalized branch from caa5c41 to 555092b Compare May 14, 2024 14:20

incertum mentioned this pull request May 14, 2024

[TRACKING] metrics framework future refactors / cleanups falcosecurity/falco#3194

Open

incertum previously approved these changes May 14, 2024

View reviewed changes

poiana assigned incertum May 14, 2024

poiana added the lgtm label May 14, 2024

poiana added the approved label May 14, 2024

mrgian dismissed incertum’s stale review via 09a4324 May 15, 2024 08:06

mrgian force-pushed the plugin-api-metrics-finalized branch from 555092b to 09a4324 Compare May 15, 2024 08:06

poiana removed the lgtm label May 15, 2024

poiana requested a review from incertum May 15, 2024 08:06

FedeDP reviewed May 15, 2024

View reviewed changes

userspace/libsinsp/test/plugins.ut.cpp Outdated Show resolved Hide resolved

poiana requested a review from jasondellaluce May 15, 2024 09:09

mrgian force-pushed the plugin-api-metrics-finalized branch from 09a4324 to 846da48 Compare May 15, 2024 09:35

mrgian added 3 commits May 15, 2024 14:34

new(plugin_api): add plugin metrics symbols and types

5be9530

Signed-off-by: Gianmatteo Palmieri <[email protected]>

new(libsinsp): plugin metric to sinsp metric conversion

c85aef5

Signed-off-by: Gianmatteo Palmieri <[email protected]>

new(tests): plugin metrics tests

2ccdd8e

Signed-off-by: Gianmatteo Palmieri <[email protected]>

mrgian force-pushed the plugin-api-metrics-finalized branch from 846da48 to 2ccdd8e Compare May 15, 2024 12:36

FedeDP reviewed May 15, 2024

View reviewed changes

incertum approved these changes May 16, 2024

View reviewed changes

poiana added the lgtm label May 16, 2024

FedeDP approved these changes May 16, 2024

View reviewed changes

poiana assigned FedeDP May 16, 2024

poiana merged commit 0c3e799 into falcosecurity:master May 16, 2024
39 of 40 checks passed

FedeDP mentioned this pull request May 29, 2024

fix(userspace/libsinsp): fixed possible buffer overflow in sinsp_plugin::get_metrics #1880

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new(plugin_api): add plugin metrics support #1828

new(plugin_api): add plugin metrics support #1828

mrgian commented May 3, 2024

FedeDP commented May 7, 2024

incertum commented May 9, 2024

mrgian commented May 9, 2024

incertum commented May 9, 2024

incertum left a comment

poiana commented May 14, 2024

FedeDP commented May 15, 2024

mrgian commented May 15, 2024

FedeDP commented May 15, 2024

FedeDP May 15, 2024

mrgian May 15, 2024

incertum May 15, 2024

mrgian May 16, 2024

FedeDP May 16, 2024

incertum May 16, 2024

FedeDP May 16, 2024

incertum May 16, 2024

mrgian May 16, 2024 •

edited

Loading

incertum May 16, 2024

incertum left a comment

poiana commented May 16, 2024

FedeDP left a comment

poiana commented May 16, 2024

new(plugin_api): add plugin metrics support #1828

new(plugin_api): add plugin metrics support #1828

Conversation

mrgian commented May 3, 2024

FedeDP commented May 7, 2024

incertum commented May 9, 2024

mrgian commented May 9, 2024

incertum commented May 9, 2024

incertum left a comment

Choose a reason for hiding this comment

poiana commented May 14, 2024

FedeDP commented May 15, 2024

mrgian commented May 15, 2024

FedeDP commented May 15, 2024

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

mrgian May 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

incertum left a comment

Choose a reason for hiding this comment

poiana commented May 16, 2024

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented May 16, 2024

mrgian May 16, 2024 •

edited

Loading