chore(metrics): refactor metrics v2 so it uses classes #1920

Molter73 · 2024-06-18T15:11:35Z

What type of PR is this?

This is an alternative to the original implementation. Instead of using capturing lambdas that get called back after some additional work is done, we use classes and gather data during instantiation of the object. This approach should be a lot more straight forward and it also doesn't create unneeded objects when there's no need to do so. Another benefit of this approach is that metrics are bundled together in a very specific way, rather than depending on the ordering of definitions in an enum, which is a C idiom.

/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:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:
At the time of creation, this refactoring is merely a proposal, any other changes are welcomed and the possibility of completely scrapping the PR in favor of a better alternative is also up there

Does this PR introduce a user-facing change?:

NONE

Molter73 · 2024-06-18T15:12:58Z

/cc @incertum @FedeDP
You both worked on this code, so I'd like your opinion on it 😄

incertum

Thanks Mauro!

Yes these changes work for me. We would need a Falco PR as well for the changes related to libs::metrics::libsinsp_metrics::new_metric. Would you mind opening that one as well?

@FedeDP WDYT? It also needs to be coordinated with your open PR.

incertum · 2024-06-18T16:51:21Z

userspace/libsinsp/metrics_collector.h

@@ -46,39 +48,6 @@ struct sinsp_stats_v2
 	uint32_t m_n_containers;
 };

-enum sinsp_stats_v2_resource_utilization
-{
-	SINSP_RESOURCE_UTILIZATION_CPU_PERC = 0, ///< Current CPU usage, `ps` util like calculation for the calling process (/proc/self), unit: percentage of one CPU.


Could we keep the comments somewhere? Perhaps when we emplace_back the metrics? Just so the next developer understands better where they come from, as some metrics definitely benefit from having comments.

Agree with Melissa!

Sounds good, I'll find a new home for them!

I added the comments back, but instead of doing so where we emplace the variables, I added them next to the variables that store the values. I feel this closer resembles the original positions for the comments and has the benefit that it adds doxygen documentation for those fields.
I also used member groups to prevent duplicating some comments, this one is a bit nastier and I'm not fully convinced it's the right way to go about it, let me know what you think.

incertum · 2024-06-18T16:54:46Z

Also CC @sgaist, thanks!

FedeDP · 2024-06-19T06:56:55Z

@FedeDP WDYT?

I really like it too! Thanks @Molter73 !

It also needs to be coordinated with your open PR.

Yes; i can rebase if needed! :)

FedeDP · 2024-06-19T06:59:54Z

/milestone 0.18.0

Molter73 · 2024-06-19T08:56:18Z

Yes these changes work for me. We would need a Falco PR as well for the changes related to libs::metrics::libsinsp_metrics::new_metric. Would you mind opening that one as well?

I'll get that up and running as soon as I can, it might take a minute since I've never actually compiled Falco as a whole 😅

Molter73 · 2024-06-19T09:06:06Z

Yes; i can rebase if needed! :)

Also, since I might take sometime to put together the falco counter part to this PR, feel free to merge yours and I'll rebase this one, think perf testing is more important too.

FedeDP · 2024-06-19T09:07:05Z

Let's see what get merged first ahah

Molter73 · 2024-06-24T17:01:24Z

I'm still working on the main repo counterpart for this PR, think I have a working change but have to test it still, should have it up for review soonish.

github-actions · 2024-06-24T17:02:44Z

Perf diff from master - unit tests

     3.63%     -1.34%  [.] std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release
     4.19%     +1.32%  [.] sinsp_parser::process_event
     9.18%     -1.20%  [.] sinsp_thread_manager::find_thread
     0.68%     +0.94%  [.] scap_event_decode_params
     8.19%     -0.77%  [.] sinsp::next
    12.93%     -0.77%  [.] sinsp_parser::reset
     1.02%     +0.68%  [.] sinsp_threadinfo::~sinsp_threadinfo
     0.95%     +0.56%  [.] std::_Hashtable<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, libsinsp::state::dynamic_struct::field_info>, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, libsinsp::state::dynamic_struct::field_info> >, std::__detail::_Select1st, std::equal_to<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::hash<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<true, false, true> >::_M_emplace<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, libsinsp::state::dynamic_struct::field_info> >
     0.67%     -0.50%  [.] sinsp_cgroup::lookup_cgroups
     0.88%     -0.50%  [.] sinsp_parser::parse_rw_exit

Perf diff from master - scap file

     4.44%     +7.23%  [.] sinsp_filter_check::tostring
    13.25%     -5.71%  [.] sinsp_filter_check_event::extract_single
    12.85%     -5.56%  [.] gzfile_read
    13.16%     -5.54%  [.] sinsp::next
    12.85%     -5.41%  [.] sinsp_filter_check_thread::extract_single
    12.76%     +4.63%  [.] sinsp_evt_formatter::tostring_withformat
     4.38%     +2.26%  [.] sinsp_thread_manager::get_thread_ref
    17.43%     +1.81%  [.] sinsp_filter_check::extract
     4.44%     -0.22%  [.] sinsp_parser::reset
     4.43%     -0.22%  [.] sinsp_parser::process_event

incertum · 2024-06-25T17:52:22Z

We merged Fede's PR first. @Molter73 when you get a chance could you rebase and once the Falco PR is up we can merge it quickly.

This is an alternative to the original implementation. Instead of using capturing lambdas that get called back after some additional work is done, we use classes and gather data during instantiation of the object. This approach should be a lot more straight forward and it also doesn't create unneeded objects when there's no need to do so. Signed-off-by: Mauro Ezequiel Moltrasio <mmoltras@redhat.com>

Signed-off-by: Mauro Ezequiel Moltrasio <mmoltras@redhat.com>

github-actions · 2024-06-26T14:28:28Z

Perf diff from master - unit tests

     6.32%     +1.53%  [.] sinsp::next
     3.45%     +1.08%  [.] sinsp_evt::load_params
     3.82%     -1.01%  [.] gzfile_read
     2.20%     -0.83%  [.] std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char const*>
     5.70%     -0.75%  [.] sinsp_thread_manager::find_thread
     3.71%     -0.66%  [.] sinsp_thread_manager::get_thread_ref
     0.95%     -0.53%  [.] std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_weak_release
     1.26%     -0.53%  [.] sinsp_evt::get_ts
    10.21%     +0.51%  [.] sinsp_parser::reset
     4.56%     -0.39%  [.] sinsp_parser::process_event

Perf diff from master - scap file

    30.91%     -5.81%  [.] sinsp_filter_check::extract_nocache
     4.04%     +4.35%  [.] sinsp_filter_check::rawval_to_string
     5.77%     +3.17%  [.] std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char const*>
    12.26%     +2.15%  [.] sinsp_evt_formatter::tostring_withformat
     7.45%     -1.77%  [.] sinsp_threadinfo::~sinsp_threadinfo
     6.18%     -1.12%  [.] sinsp_parser::reset
     5.77%     -1.10%  [.] libsinsp::runc::match_container_id
     6.19%     -1.09%  [.] sinsp_parser::process_event
     6.11%     -1.05%  [.] main
     3.03%     +0.57%  [.] sinsp_evt::get_param_as_str

Heap diff from master - unit tests

total runtime: 0.02s.
calls to allocation functions: -567 (-23625/s)
temporary memory allocations: -175 (-7291/s)
peak heap memory consumption: 0B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

Heap diff from master - scap file

total runtime: 0.00s.
calls to allocation functions: 0 (0/s)
temporary memory allocations: -2 (-1000/s)
peak heap memory consumption: 0B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

FedeDP

Like it!
/approve

poiana · 2024-06-26T14:56:09Z

LGTM label has been added.

Git tree hash: b530ef1f44e4c7353f654f45179681abd85410fe

Molter73 · 2024-06-26T15:10:28Z

/hold

Still need to open the main repo counterpart.

incertum

/approve

Nice job!

@Molter73 feel free to unhold once the falco PR is up. Thanks.

poiana · 2024-06-26T16:26:56Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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,Molter73,incertum]

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

This commit will be dropped once falcosecurity/libs#1920 is merged and replaced with a commit to the master branch. Signed-off-by: Mauro Ezequiel Moltrasio <mmoltras@redhat.com>

Molter73 · 2024-06-27T13:15:11Z

I've opened falcosecurity/falco#3265 as the counter part to this PR, if everything looks good over there, we can go ahead and merge this.

Molter73 · 2024-06-27T14:38:40Z

/unhold

poiana added kind/cleanup release-note-none dco-signoff: yes area/libsinsp size/XXL labels Jun 18, 2024

poiana requested review from hbrueckner and incertum June 18, 2024 15:11

poiana added the approved label Jun 18, 2024

poiana requested a review from FedeDP June 18, 2024 15:13

incertum reviewed Jun 18, 2024

View reviewed changes

poiana added this to the 0.18.0 milestone Jun 19, 2024

Molter73 added 2 commits June 26, 2024 16:22

docs(metrics): add back doxygen comments for metrics v2

8d7c712

Signed-off-by: Mauro Ezequiel Moltrasio <mmoltras@redhat.com>

Molter73 force-pushed the refactor-metrics-collector branch from ac75f33 to 8d7c712 Compare June 26, 2024 14:24

FedeDP approved these changes Jun 26, 2024

View reviewed changes

poiana assigned FedeDP Jun 26, 2024

poiana added the lgtm label Jun 26, 2024

poiana added the do-not-merge/hold label Jun 26, 2024

incertum approved these changes Jun 26, 2024

View reviewed changes

poiana assigned incertum Jun 26, 2024

Molter73 mentioned this pull request Jun 27, 2024

fix(build): metrics compilation after refactoring into classes falcosecurity/falco#3265

Closed

poiana removed the do-not-merge/hold label Jun 27, 2024

poiana merged commit ae5fbf6 into falcosecurity:master Jun 27, 2024
41 checks passed

This was referenced Jul 31, 2024

update(cmake): bump libs and driver to latest master falcosecurity/falco#3283

Merged

cleanup(metrics): remove duplicated new_metric method #1991

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(metrics): refactor metrics v2 so it uses classes #1920

chore(metrics): refactor metrics v2 so it uses classes #1920

Molter73 commented Jun 18, 2024

Molter73 commented Jun 18, 2024

incertum left a comment

incertum Jun 18, 2024 •

edited

Loading

FedeDP Jun 19, 2024

Molter73 Jun 19, 2024

Molter73 Jun 24, 2024

incertum commented Jun 18, 2024

FedeDP commented Jun 19, 2024 •

edited

Loading

FedeDP commented Jun 19, 2024

Molter73 commented Jun 19, 2024

Molter73 commented Jun 19, 2024

FedeDP commented Jun 19, 2024

Molter73 commented Jun 24, 2024

github-actions bot commented Jun 24, 2024

incertum commented Jun 25, 2024

github-actions bot commented Jun 26, 2024

FedeDP left a comment

poiana commented Jun 26, 2024

Molter73 commented Jun 26, 2024

incertum left a comment

poiana commented Jun 26, 2024

Molter73 commented Jun 27, 2024

Molter73 commented Jun 27, 2024

chore(metrics): refactor metrics v2 so it uses classes #1920

chore(metrics): refactor metrics v2 so it uses classes #1920

Conversation

Molter73 commented Jun 18, 2024

Molter73 commented Jun 18, 2024

incertum left a comment

Choose a reason for hiding this comment

incertum Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

FedeDP Jun 19, 2024

Choose a reason for hiding this comment

Molter73 Jun 19, 2024

Choose a reason for hiding this comment

Molter73 Jun 24, 2024

Choose a reason for hiding this comment

incertum commented Jun 18, 2024

FedeDP commented Jun 19, 2024 • edited Loading

FedeDP commented Jun 19, 2024

Molter73 commented Jun 19, 2024

Molter73 commented Jun 19, 2024

FedeDP commented Jun 19, 2024

Molter73 commented Jun 24, 2024

github-actions bot commented Jun 24, 2024

Perf diff from master - unit tests

Perf diff from master - scap file

incertum commented Jun 25, 2024

github-actions bot commented Jun 26, 2024

Perf diff from master - unit tests

Perf diff from master - scap file

Heap diff from master - unit tests

Heap diff from master - scap file

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Jun 26, 2024

Molter73 commented Jun 26, 2024

incertum left a comment

Choose a reason for hiding this comment

poiana commented Jun 26, 2024

Molter73 commented Jun 27, 2024

Molter73 commented Jun 27, 2024

incertum Jun 18, 2024 •

edited

Loading

FedeDP commented Jun 19, 2024 •

edited

Loading