new(libsinsp): introduce proc.aargs field #2387
Conversation
Perf diff from master - unit testsHeap diff from master - unit testsHeap diff from master - scap fileBenchmarks diff from master |
| @@ -1104,16 +1149,18 @@ uint8_t* sinsp_filter_check_thread::extract_single(sinsp_evt* evt, | |||
| case TYPE_ARGS: { | |||
| m_tstr.clear(); | |||
|
|
|||
| uint32_t j; | |||
| uint32_t nargs = (uint32_t)tinfo->m_args.size(); | |||
| sinsp_threadinfo::populate_args(m_tstr, tinfo); | |||
There was a problem hiding this comment.
New helper was due now.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2387 +/- ##
=======================================
Coverage 77.18% 77.19%
=======================================
Files 231 231
Lines 30368 30401 +33
Branches 4657 4663 +6
=======================================
+ Hits 23441 23468 +27
- Misses 6927 6933 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -748,6 +762,21 @@ int32_t sinsp_filter_check_thread::extract_arg(std::string_view fldname, | |||
| } else { | |||
| throw sinsp_exception("filter syntax error: " + string(val)); | |||
| } | |||
| } else if(m_field_id == TYPE_AARGS) { | |||
| // Separate statement in order to expand future usage | |||
| // to proc.aargs[1][3]; allow for indexed ancestor args access | |||
There was a problem hiding this comment.
@leogr and @FedeDP TBD if we want to expand this in this PR or another PR, alongside deciding when (this release or the next one). Either way fine from my perspective.
One option could be to simply expand the string arg extraction logic (either using re2 or a custom approach), and assigning the second index that would access the arg of an ancestor to a new variable called m_argid2 or other naming.
WDYT?
There was a problem hiding this comment.
@leogr and @FedeDP TBD if we want to expand this in this PR or another PR,
I'd go for two separate PRs, mainly because allowing [1][3] would be a radical change in the syntax, and we need to consider any possible side effects.
alongside deciding when (this release or the next one). Either way fine from my perspective.
Considering Falco 0.41 is due by May 26th, we might be able to merge this PR by the end of the week and bring just proc.aargs[1] for this release. For anything more, we are too late, I guess.
There was a problem hiding this comment.
@leogr couldn't agree more re timing and possible side effect!
I have just rebased this PR!
e51f00d to
6bd261d
Compare
|
(rebasng the PR will fix the CI :D ) |
|
/milestone 0.21.0 |
Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>
6bd261d to
d3cbbe1
Compare
|
LGTM label has been added. Git tree hash: 967b486093f099500d02b8797462237f751d3805
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
|
@leogr while i like the feature, what about introducing an
Idea is simple: with my 2c, just an idea for eg: Falco 1.0.0, considering that i'd also drop all the Looking at the code, the only issue is that by now transformers are bound to work on the extracted values. This time, we should need to work before extracting the value, possibly on the |
The only issue is that transformers are related to a type, and we don't actually have a type for processes. So it sounds inconsistent to me, but I'm still open to this or a similar idea. That being said, I agree we should get rid of all @FedeDP wdyt? |
What type of PR is this?
/kind cleanup
/kind feature
Any specific area of the project related to this PR?
/area libsinsp
/area tests
Does this PR require a change in the driver versions?
What this PR does / why we need it:
Introduce proc.aargs field, community request, see falcosecurity/falco#3534
Which issue(s) this PR fixes:
Part 2 falcosecurity/falco#3534
TBD if we introduce nested proc.aargs lookups in this PR or a follow up PR.
CC @yg-oss
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: