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.

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

new(libsinsp): add basename() string transformer #1943

Merged
merged 1 commit into from
Jul 2, 2024

Conversation

LucaGuerra
Copy link
Contributor

@LucaGuerra LucaGuerra commented Jul 1, 2024

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind feature

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area libsinsp

Does this PR require a change in the driver versions?

No

What this PR does / why we need it:

Introduce basename() as transformer. This allows, for instance, to write expressions like basename(proc.exepath) = cat to match against the original executable name even if it has been symlinked without knowing the full path, or any other file name based detection.

Which issue(s) this PR fixes:

Fixes #1927

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

new(libsinsp): add basename() string transformer

@LucaGuerra
Copy link
Contributor Author

/milestone 0.18.0

@poiana poiana added the size/L label Jul 1, 2024
@poiana poiana added this to the 0.18.0 milestone Jul 1, 2024
@poiana poiana added the approved label Jul 1, 2024
@poiana poiana requested review from Andreagit97 and hbrueckner July 1, 2024 13:45
@LucaGuerra LucaGuerra force-pushed the new/basename-transformer branch 2 times, most recently from ff3e980 to 4fc8899 Compare July 1, 2024 13:49
Copy link

github-actions bot commented Jul 1, 2024

Perf diff from master - unit tests

     5.51%     +3.39%  [.] sinsp::next
    11.31%     -2.21%  [.] sinsp_parser::reset
     5.30%     -1.68%  [.] next
     4.15%     -1.16%  [.] gzfile_read
     4.84%     -0.95%  [.] sinsp_parser::process_event
     3.83%     -0.76%  [.] sinsp_thread_manager::get_thread_ref
     2.17%     +0.72%  [.] std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release
     1.02%     +0.63%  [.] sinsp_parser::event_cleanup
     1.51%     -0.54%  [.] libsinsp::sinsp_suppress::process_event
     5.26%     -0.48%  [.] sinsp_evt::get_type

Perf diff from master - scap file

    20.08%     -6.53%  [.] sinsp_filter_check::extract_nocache
     3.32%     +3.22%  [.] std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char*>
    10.09%     -2.38%  [.] sinsp_filter_check_event::extract_single
     3.31%     +1.59%  [.] libsinsp::runc::match_one_container_id
     3.36%     +1.49%  [.] sinsp_filter_check::tostring
     3.38%     +1.42%  [.] libsinsp::runc::match_container_id
    16.63%     -0.99%  [.] sinsp_evt_formatter::tostring_withformat
    10.02%     -0.53%  [.] sinsp_filter_check_thread::extract_single
     6.66%     -0.45%  [.] sinsp_evt::get_type
     3.33%     -0.37%  [.] sinsp_container_manager::resolve_container

Heap diff from master - unit tests

total runtime: 0.09s.
calls to allocation functions: -3239 (-35206/s)
temporary memory allocations: -392 (-4260/s)
peak heap memory consumption: -834B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

Heap diff from master - scap file

total runtime: -0.02s.
calls to allocation functions: 0 (0/s)
temporary memory allocations: -10 (434/s)
peak heap memory consumption: -32B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

Signed-off-by: Luca Guerra <luca@guerra.sh>
@LucaGuerra LucaGuerra force-pushed the new/basename-transformer branch from 4fc8899 to 38e9b21 Compare July 1, 2024 14:21
Copy link

github-actions bot commented Jul 1, 2024

Perf diff from master - unit tests

     5.50%     +1.80%  [.] sinsp::next
     5.35%     +1.00%  [.] sinsp_thread_manager::find_thread
     5.30%     -0.95%  [.] next
    11.30%     -0.86%  [.] sinsp_parser::reset
     4.04%     -0.71%  [.] sinsp_evt::load_params
     3.82%     -0.70%  [.] sinsp_thread_manager::get_thread_ref
     5.25%     -0.62%  [.] sinsp_evt::get_type
     1.11%     -0.38%  [.] std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char*>
     1.00%     -0.38%  [.] scap_event_encode_params_v
     0.15%     +0.37%  [.] sinsp_parser::parse_clone_exit_child

Perf diff from master - scap file

    24.17%     +7.80%  [.] sinsp_filter_check::extract_nocache
    12.06%     -4.07%  [.] sinsp_filter_check_thread::extract_single
    11.45%     -2.89%  [.] sinsp_thread_manager::get_thread_ref
    20.02%     -2.62%  [.] sinsp_evt_formatter::tostring_withformat
     4.07%     +1.56%  [.] libsinsp::runc::match_container_id
    12.15%     -0.57%  [.] sinsp_filter_check_event::extract_single
     3.99%     -0.43%  [.] std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char*>
     8.04%     -0.32%  [.] sinsp_filter_check::rawval_to_string
     4.05%     -0.17%  [.] sinsp::next

Heap diff from master - unit tests

total runtime: 0.16s.
calls to allocation functions: -2588 (-15877/s)
temporary memory allocations: -127 (-779/s)
peak heap memory consumption: -834B
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: -3 (750/s)
peak heap memory consumption: -32B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

Copy link
Contributor

@incertum incertum left a comment

Choose a reason for hiding this comment

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

/approve

I like this a lot.

@darryk10 and @loresuso for few upstream rules we may wanna consider making some checks more robust using this new transformer.

@poiana
Copy link
Contributor

poiana commented Jul 2, 2024

LGTM label has been added.

Git tree hash: c650bfe49234fab8bece68f6594e8b52ec42845a

case FTR_BASENAME:
{
return string_transformer(vec, t, [](std::string_view in, storage_t& out) -> bool {
auto last_slash_pos = in.find_last_of("/");
Copy link
Contributor

Choose a reason for hiding this comment

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

Question, why can't we directly use posix basename here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we also build for Windows and MacOS, which are not guaranteed to have POSIX compatible functions. Especially on Windows a basename function would expect backslashes instead of slashes potentially making reading Linux scap files a bit of a headache.

Copy link
Contributor

@FedeDP FedeDP 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 Jul 2, 2024

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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

@poiana poiana merged commit 0ec2ad8 into falcosecurity:master Jul 2, 2024
40 of 43 checks passed
@LucaGuerra LucaGuerra deleted the new/basename-transformer branch July 2, 2024 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New transformer: basename()
4 participants