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(libsinsp): add function to set compiler filter and filter string #1976

Merged
merged 3 commits into from
Aug 7, 2024
Merged

fix(libsinsp): add function to set compiler filter and filter string #1976

merged 3 commits into from
Aug 7, 2024

Conversation

therealbobo
Copy link
Contributor

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:

When setting the compiled filter through set_filter, we lose the filter string information and never populate the m_filterstring. This adds a method to set them both.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

@poiana poiana requested review from FedeDP and hbrueckner July 26, 2024 14:03
@poiana poiana added the size/S label Jul 26, 2024
@therealbobo therealbobo force-pushed the add-set-filter-string branch from 21a39c2 to 23185fe Compare July 26, 2024 14:03
@therealbobo therealbobo changed the title cix(libsinsp): add function to set compiler filter and filter string fix(libsinsp): add function to set compiler filter and filter string Jul 26, 2024
@therealbobo therealbobo force-pushed the add-set-filter-string branch from 23185fe to 668213e Compare July 26, 2024 15:48
@github-actions GitHub Actions
Copy link

Perf diff from master - unit tests

     8.09%     -1.30%  [.] sinsp::next
     1.32%     -0.90%  [.] scap_next
     9.11%     +0.90%  [.] sinsp_parser::reset
     4.82%     -0.84%  [.] sinsp_parser::process_event
     2.87%     +0.81%  [.] sinsp_thread_manager::find_thread
     0.91%     +0.65%  [.] sinsp::fetch_next_event
     0.75%     +0.64%  [.] std::vector<sinsp_evt_param, std::allocator<sinsp_evt_param> >::emplace_back<sinsp_evt*, unsigned int&, char const*, unsigned long&>
     2.58%     +0.59%  [.] sinsp_thread_manager::get_thread_ref
     0.98%     +0.54%  [.] libsinsp::sinsp_suppress::process_event
     1.03%     -0.48%  [.] sinsp_parser::parse_context_switch

Perf diff from master - scap file

    20.26%     -5.70%  [.] sinsp_evt_formatter::tostring_withformat
     3.56%     +5.17%  [.] sinsp_split[abi:cxx11]
     4.05%     +2.92%  [.] sinsp::next
     8.08%     -2.07%  [.] sinsp_threadinfo::~sinsp_threadinfo
     8.09%     -2.04%  [.] sinsp_filter_check::get_transformed_field_info
     4.04%     +1.95%  [.] sinsp_filter_check::extract
     3.81%     +1.91%  [.] sinsp_filter_check::tostring
     4.06%     +1.65%  [.] sinsp_parser::reset
     4.03%     +1.57%  [.] libsinsp::runc::match_container_id
     8.10%     +1.50%  [.] sinsp_filter_check::rawval_to_string

Heap diff from master - unit tests

peak heap memory consumption: 0B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

Heap diff from master - scap file

peak heap memory consumption: -30B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

@codecov Codecov
Copy link

codecov bot commented Jul 26, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 50.99%. Comparing base (1feeb37) to head (8bc17ef).
Report is 3 commits behind head on master.

Files Patch % Lines
userspace/libsinsp/sinsp.cpp 0.00% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1976      +/-   ##
==========================================
- Coverage   50.99%   50.99%   -0.01%     
==========================================
  Files         310      310              
  Lines       39607    39608       +1     
  Branches    17533    17242     -291     
==========================================
- Hits        20199    20198       -1     
+ Misses      14336    14320      -16     
- Partials     5072     5090      +18

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@FedeDP
Copy link
Contributor

FedeDP commented Jul 31, 2024

/milestone 0.18.0

@poiana poiana added this to the 0.18.0 milestone Jul 31, 2024
@therealbobo therealbobo force-pushed the add-set-filter-string branch from 668213e to f5c20e4 Compare August 5, 2024 16:50
@poiana poiana added size/XS and removed size/S labels Aug 5, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 5, 2024

Perf diff from master - unit tests

     9.08%     +2.09%  [.] sinsp_parser::reset
     1.94%     -0.95%  [.] sinsp_evt::get_direction
     3.60%     +0.83%  [.] gzfile_read
     1.27%     -0.63%  [.] sinsp_evt::get_param
     2.05%     -0.52%  [.] libsinsp::sinsp_suppress::process_event
     7.14%     +0.42%  [.] sinsp::next
     4.99%     -0.41%  [.] sinsp_parser::process_event
     2.49%     -0.39%  [.] std::_Hashtable<long, std::pair<long const, std::shared_ptr<sinsp_threadinfo> >, std::allocator<std::pair<long const, std::shared_ptr<sinsp_threadinfo> > >, std::__detail::_Select1st, std::equal_to<long>, std::hash<long>, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<false, false, true> >::_M_find_before_node
     0.62%     -0.37%  [.] sinsp_container_manager::resolve_container
     0.44%     -0.36%  [.] scap_cgroup_resolve_v2

Perf diff from master - scap file

    29.07%    -17.32%  [.] sinsp_filter_check::extract_nocache
     9.66%     +6.73%  [.] sinsp_evt_formatter::tostring_withformat
     3.22%     +5.22%  [.] sinsp_filter_check::rawval_to_string
     6.87%     -2.69%  [.] sinsp::fetch_next_event
     6.43%     -2.40%  [.] sinsp_filter_check_thread::extract_single
     6.07%     -2.28%  [.] sinsp_split[abi:cxx11]
     9.66%     -0.84%  [.] sinsp_filter_check_event::extract_single
     6.54%     -0.55%  [.] next
     6.40%     -0.53%  [.] std::_Hashtable<long, std::pair<long const, std::shared_ptr<sinsp_threadinfo> >, std::allocator<std::pair<long const, std::shared_ptr<sinsp_threadinfo> > >, std::__detail::_Select1st, std::equal_to<long>, std::hash<long>, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<false, false, true> >::_M_find_before_node
     6.43%     -0.49%  [.] sinsp_filter_check::tostring

Heap diff from master - unit tests

peak heap memory consumption: 0B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

Heap diff from master - scap file

peak heap memory consumption: 0B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

jasondellaluce
jasondellaluce reviewed Aug 6, 2024
View reviewed changes
userspace/libsinsp/sinsp.cpp Outdated
@@ -1661,7 +1661,7 @@ void sinsp::start_dropping_mode(uint32_t sampling_ratio)
}
#endif // _WIN32

void sinsp::set_filter(std::unique_ptr<sinsp_filter> filter)
void sinsp::set_filter(std::unique_ptr<sinsp_filter> filter, const std::optional<std::string>& filterstring)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
void sinsp::set_filter(std::unique_ptr<sinsp_filter> filter, const std::optional<std::string>& filterstring)
void sinsp::set_filter(std::unique_ptr<sinsp_filter> filter, const std::string& filterstring = "")

I think the previous suggestion was referring to something like this -- otherwise it'd become a breaking change for whoever uses this method.

@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 6, 2024

Perf diff from master - unit tests

     1.95%     -1.25%  [.] sinsp_evt::get_direction
     5.01%     -1.21%  [.] sinsp_parser::process_event
     9.13%     +1.20%  [.] sinsp_parser::reset
     0.61%     +0.82%  [.] sinsp_parser::parse_context_switch
     0.89%     +0.77%  [.] libsinsp::events::is_unknown_event
     2.51%     -0.58%  [.] std::_Hashtable<long, std::pair<long const, std::shared_ptr<sinsp_threadinfo> >, std::allocator<std::pair<long const, std::shared_ptr<sinsp_threadinfo> > >, std::__detail::_Select1st, std::equal_to<long>, std::hash<long>, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<false, false, true> >::_M_find_before_node
     3.62%     +0.56%  [.] gzfile_read
     1.00%     -0.47%  [.] scap_event_has_large_payload
     0.33%     +0.46%  [.] sinsp_fdtable::find
     3.61%     +0.46%  [.] sinsp_evt::load_params

Perf diff from master - scap file

    28.28%    -16.16%  [.] sinsp_filter_check::extract_nocache
     6.26%     +6.25%  [.] sinsp::next
     9.39%     +5.41%  [.] sinsp_evt_formatter::tostring_withformat
     6.25%     -2.75%  [.] sinsp_evt::load_params
     5.90%     -2.65%  [.] sinsp_split[abi:cxx11]
     6.23%     -1.29%  [.] std::_Hashtable<long, std::pair<long const, std::shared_ptr<sinsp_threadinfo> >, std::allocator<std::pair<long const, std::shared_ptr<sinsp_threadinfo> > >, std::__detail::_Select1st, std::equal_to<long>, std::hash<long>, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<false, false, true> >::_M_find_before_node
     6.26%     -1.24%  [.] sinsp_filter_check::tostring
     3.13%     +0.92%  [.] sinsp_filter_check::rawval_to_string
     3.25%     -0.68%  [.] gzfile_read
     3.13%     -0.64%  [.] sinsp_parser::process_event

Heap diff from master - unit tests

peak heap memory consumption: 0B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

Heap diff from master - scap file

peak heap memory consumption: 0B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

@poiana poiana added the lgtm label Aug 6, 2024
@poiana
Copy link
Contributor

poiana commented Aug 6, 2024

LGTM label has been added.

Git tree hash: 7b9e038461122e9cf2988a7a017491405c64d445

Andreagit97
Andreagit97 approved these changes Aug 7, 2024
View reviewed changes
Copy link
Member

@Andreagit97 Andreagit97 left a comment

Choose a reason for hiding this comment

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

/approve

@poiana
Copy link
Contributor

poiana commented Aug 7, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, therealbobo

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@poiana poiana added the approved label Aug 7, 2024
@poiana poiana merged commit ae812ee into falcosecurity:master Aug 7, 2024
45 of 46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FedeDP FedeDP FedeDP left review comments

@jasondellaluce jasondellaluce jasondellaluce approved these changes

@Andreagit97 Andreagit97 Andreagit97 approved these changes

@hbrueckner hbrueckner Awaiting requested review from hbrueckner

Assignees

@jasondellaluce jasondellaluce

@Andreagit97 Andreagit97

Labels
approved dco-signoff: yes kind/feature New feature or request lgtm release-note-none size/XS
Projects
None yet
Milestone
0.18.0
Development

Successfully merging this pull request may close these issues.
5 participants
@therealbobo @FedeDP @poiana @jasondellaluce @Andreagit97