Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(plugin_api): wrong metric type enums #1885

Merged
merged 3 commits into from
Jun 3, 2024
Merged

fix(plugin_api): wrong metric type enums #1885

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0bb7dc3
fix(plugin_api): wrong metrics type enums
mrgian May 30, 2024
16fbe45
fix(libsinsp): resolve get_metrics plugin symbol
mrgian May 30, 2024
e9cd426
chore(plugin_api): bump version
mrgian May 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 13 additions & 13 deletions userspace/libsinsp/metrics_collector.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,19 +457,6 @@ void libs_metrics_collector::snapshot()
return;
}

/*
* plugins metrics
*/

if(m_metrics_flags & METRICS_V2_PLUGINS)
{
for (auto& p : m_inspector->get_plugin_manager()->plugins())
{
std::vector<metrics_v2> plugin_metrics = p->get_metrics();
m_metrics.insert(m_metrics.end(), plugin_metrics.begin(), plugin_metrics.end());
}
}

/*
* libscap metrics
*/
Expand Down Expand Up @@ -783,6 +770,19 @@ void libs_metrics_collector::snapshot()
}
}
}

/*
* plugins metrics
*/

if(m_metrics_flags & METRICS_V2_PLUGINS)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this moved?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we insert plugin metrics first in the vector, they then get overwritten by scap/sinsp metrics.
I didn't look closely at it, but the cause may be: https://github.com/falcosecurity/libs/blob/master/userspace/libsinsp/metrics_collector.cpp#L487

{
for (auto& p : m_inspector->get_plugin_manager()->plugins())
{
std::vector<metrics_v2> plugin_metrics = p->get_metrics();
m_metrics.insert(m_metrics.end(), plugin_metrics.begin(), plugin_metrics.end());
}
}
}

const std::vector<metrics_v2>& libs_metrics_collector::get_metrics() const
Expand Down
2 changes: 1 addition & 1 deletion userspace/plugin/plugin_api.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ extern "C" {
//
// todo(jasondellaluce): when/if major changes to v4, check and solve all todos
#define PLUGIN_API_VERSION_MAJOR 3
#define PLUGIN_API_VERSION_MINOR 5
#define PLUGIN_API_VERSION_MINOR 6
#define PLUGIN_API_VERSION_PATCH 0

//
Expand Down
1 change: 1 addition & 0 deletions userspace/plugin/plugin_loader.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ plugin_handle_t* plugin_load(const char* path, char* err)
SYM_RESOLVE(ret, get_async_events);
SYM_RESOLVE(ret, set_async_event_handler);
SYM_RESOLVE(ret, set_config);
SYM_RESOLVE(ret, get_metrics);
return ret;
}

Expand Down
18 changes: 9 additions & 9 deletions userspace/plugin/plugin_types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,13 +298,13 @@ typedef enum ss_plugin_log_severity
// Types supported by the by the metric values
typedef enum ss_plugin_metric_value_type
{
SS_PLUGIN_METRIC_VALUE_TYPE_U32 = 1,
SS_PLUGIN_METRIC_VALUE_TYPE_S32 = 2,
SS_PLUGIN_METRIC_VALUE_TYPE_U64 = 3,
SS_PLUGIN_METRIC_VALUE_TYPE_S64 = 4,
SS_PLUGIN_METRIC_VALUE_TYPE_D = 5,
SS_PLUGIN_METRIC_VALUE_TYPE_F = 6,
SS_PLUGIN_METRIC_VALUE_TYPE_I = 7,
SS_PLUGIN_METRIC_VALUE_TYPE_U32 = 0,
SS_PLUGIN_METRIC_VALUE_TYPE_S32 = 1,
SS_PLUGIN_METRIC_VALUE_TYPE_U64 = 2,
SS_PLUGIN_METRIC_VALUE_TYPE_S64 = 3,
SS_PLUGIN_METRIC_VALUE_TYPE_D = 4,
SS_PLUGIN_METRIC_VALUE_TYPE_F = 5,
SS_PLUGIN_METRIC_VALUE_TYPE_I = 6,
Comment on lines +301 to +307
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, this is a backward incompatible change, isn't it?

So, unfortunately, I believe we have to bump the PLUGIN_API_VERSION_MAJOR number.
cc @jasondellaluce

/hold a bit for second opinions

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option would be:

Since this couldn't be used before (because of the missing registered symbol), we assume the 3.5.0 did not have this feature at all. Thus, just bumping the PLUGIN_API_VERSION_MINOR would be enough.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree @leogr we can consider this feature not yet usable in the current release.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yet another option:
Forcing the enum values in metrics_v2.h (https://github.com/falcosecurity/libs/blob/master/userspace/libscap/metrics_v2.h) to match the enums in plugin_types.h
This should work given that these enums are only casted here https://github.com/falcosecurity/libs/blob/master/userspace/libsinsp/plugin.cpp#L947-L948

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with Leo, since it was unusable in 0.17.1, we can consider a minor bump.

} ss_plugin_metric_value_type;

// Data representation of metric values
Expand All @@ -322,8 +322,8 @@ typedef union ss_plugin_metric_value
// Metric types
typedef enum ss_plugin_metric_type
{
SS_PLUGIN_METRIC_TYPE_MONOTONIC = 1,
SS_PLUGIN_METRIC_TYPE_NON_MONOTONIC = 2,
SS_PLUGIN_METRIC_TYPE_MONOTONIC = 0,
SS_PLUGIN_METRIC_TYPE_NON_MONOTONIC = 1,
} ss_plugin_metric_type;

//
Expand Down
Loading