cleanup(userspace/libsinsp): improved sinsp metrics_collector to make it more future proof #1745
Conversation
|
/cc @incertum |
|
Wow @FedeDP 🤣 so fast? @federico-sysdig mind checking given #1693. |
FedeDP
force-pushed
the
cleanup/improve_sinsp_metrics_collector
branch
from
9f04660
to
eb107a3
Compare
|
Will double check/fix tests tomorrow :) |
| std::shared_ptr<const sinsp_stats_v2> sinsp_stats_v2 = m_inspector->get_sinsp_stats_v2(); | ||
|
|
||
| const std::function<metrics_v2()> sinsp_stats_v2_collectors[] = { | ||
| [SINSP_RESOURCE_UTILIZATION_CPU_PERC] = [this,&cpu_usage_perc]() { |
There was a problem hiding this comment.
This is using C99 array designated initializers - available in C++ only through a gcc extension - in a C++ context (lambdas, this, etc.). The compiler allows us to get away with this, so I can live with it and throw away PR #1693 that was supposed to overcome this stretched usage of the language.
It's just that it feels like a lot of machinery to fill an array (sinsp_stats_v2_collectors), that will be used to fill a vector (m_metrics), that is used only in tests, IIUC; at least for the moment, there's probably more use of this lying ahead.
I don't have a solution, but it seems to me there ought to be a simpler way to reach the goal. Have basic C99 designated initializers for structs been tried?
Something like this.
metrics_v2 sinsp_stats_v2_collectors[] = {
[SINSP_RESOURCE_UTILIZATION_CPU_PERC] = {
.name = "cpu_usage_perc",
.flags = METRICS_V2_RESOURCE_UTILIZATION,
.metric_type = METRIC_VALUE_METRIC_TYPE_NON_MONOTONIC_CURRENT,
.value = { .u32 = cpu_usage_perc },
.type = METRIC_VALUE_TYPE_D,
.unit = METRIC_VALUE_UNIT_PERC
},
// ...
};Perhaps it looks a tad verbose, but it is pure C, hence it can stay in a separate C file where this construct is legal; it needs no new_metric template function, no need for lambdas used as factories, and it would represent all the information on metrics in a sort of "tabular" format, as if it were a metrics "data sheet".
There was a problem hiding this comment.
It's just that it feels like a lot of machinery to fill an array (sinsp_stats_v2_collectors), that will be used to fill a vector (m_metrics), that is used only in tests, IIUC; at least for the moment, there's probably more use of this lying ahead.
It's all used in Falco, see for example this PR falcosecurity/falco#3129 where I bump libs.
I probably can't add a lot of value here, other than saying that I liked having what you @federico-sysdig described as
information on metrics in a sort of "tabular" format, as if it were a metrics "data sheet".
There was a problem hiding this comment.
Perhaps it looks a tad verbose, but it is pure C, hence it can stay in a separate C file where this construct is legal; it needs no new_metric template function, no need for lambdas used as factories, and it would represent all the information on metrics in a sort of "tabular" format, as if it were a metrics "data sheet".
While i agree this would be a simple yet descriptive table, i don't get how would one link in the proper value: in your example your are copying cpu_usage_perc by value; does it mean that you expect the table to be defined after all metrics are collected?
Or would we use pointers for the value?
There was a problem hiding this comment.
Oh, the lambda is to get a value that dynamically changed. Now I see it. What was wrong with the original implementation, then? The fact that there was no explicit link between the index that identifies the metric and its initialization in the vector?
There was a problem hiding this comment.
Oh, the lambda is to get a value that dynamically changed
Yep :)
What was wrong with the original implementation, then?
The fact that we had no guarantee that a metric whose name was declared in the initial array was then actually used and pushed in code below; now we have that guarantee.
While it might seem "minor", considering the number of metrics we already have (plus more in the future perhaps?) it gets super error prone.
There was a problem hiding this comment.
Not that I want to add complexity to the build system but I am wondering whether we should consider code generation to create this table ?
Have one file that encodes all the parameters that defines a metrics and then something parsing it and generating the adequate C code to build it.
There was a problem hiding this comment.
I think the problem would still be how to "capture" the referenced value without doing weird stuff.
Also, yep not a big fan of adding build time complexity!
There was a problem hiding this comment.
cc @jasondellaluce for more ideas :)
There was a problem hiding this comment.
Any consensus re what's an improvement vs what would be too much? Thank you!
There was a problem hiding this comment.
Kind bump re consensus :) thanks.
FedeDP
force-pushed
the
cleanup/improve_sinsp_metrics_collector
branch
from
eb107a3
to
fe93981
Compare
This test fails on every PR and on master :/ |
… it more future proof. Signed-off-by: Federico Di Pierro <[email protected]>
FedeDP
force-pushed
the
cleanup/improve_sinsp_metrics_collector
branch
from
fe93981
to
2733774
Compare
|
Rebased on top of master to fix CI. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Andreagit97, FedeDP, incertum 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:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind cleanup
Any specific area of the project related to this PR?
/area libsinsp
Does this PR require a change in the driver versions?
What this PR does / why we need it:
A bit tedious but i quite like how it looks now. Basically, refactored metrics_collector in libsinsp to enforce that every declared metric MUST be actually used.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: