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

[Molter73]

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]

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

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.

[FedeDP]

Agree with Melissa!

[Molter73]

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

[Molter73]

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]

Also CC @sgaist, thanks!

[FedeDP]

@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]

/milestone 0.18.0

[Molter73]

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]

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]

Let's see what get merged first ahah

[Molter73]

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]

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]

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.

[github-actions]

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]

LGTM label has been added.

Git tree hash: b530ef1f44e4c7353f654f45179681abd85410fe

[Molter73]

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

[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

[Molter73]

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]

/unhold