-
Notifications
You must be signed in to change notification settings - Fork 164
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 len() filter transformer #2131
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Luca Guerra <[email protected]>
Signed-off-by: Luca Guerra <[email protected]>
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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:
Approvers can indicate their approval by writing |
Perf diff from master - unit tests
Heap diff from master - unit tests
Heap diff from master - scap file
Benchmarks diff from master
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2131 +/- ##
==========================================
+ Coverage 74.46% 74.49% +0.02%
==========================================
Files 254 254
Lines 33333 33472 +139
Branches 5707 5733 +26
==========================================
+ Hits 24821 24934 +113
- Misses 8481 8526 +45
+ Partials 31 12 -19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
switch(m_type) { | ||
case FTR_TOUPPER: { | ||
switch(t) { | ||
case PT_CHARBUF: | ||
case PT_FSPATH: | ||
case PT_FSRELPATH: | ||
// for TOUPPER, the transformed type is the same as the input type | ||
return true; | ||
return !is_list; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this potentially a breaking change? It might be that we supported tolower
(and similars) with list-type fields without noticing. Maybe I'm wrong, just checking in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense, although I never tried it. I would suggest to test and document this behavior if we want it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I have reverted this change and I'll add a couple of tests.
return false; | ||
} | ||
|
||
for(std::size_t i = 0; i < vec.size(); i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question -- is there really a scenario where the field is not a list-type and vec.size() > 1
? Should we put an assert?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I honestly do not know, but tests were using it like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you can't think of a corner case (such as plugins) I'll remove the loop as it simplifies things.
Co-authored-by: Jason Dellaluce <[email protected]> Signed-off-by: Luca Guerra <[email protected]>
/milestone 0.19.0 |
What type of PR is this?
/kind feature
Any specific area of the project related to this PR?
/area libsinsp
Does this PR require a change in the driver versions?
No
What this PR does / why we need it:
This introduces a new
len()
transformer which acts like you'd expect:len(some_list)
evaluates to the number of elements in the listlen(some_string)
evaluates to the length of the string (excluding null terminator of course)len(some_buffer)
evaluates to the number of bytes in the bufferThis is also a handy way to check if lists are empty, since we couldn't do it before, which was apparent after a discussion with @leogr and @jasondellaluce .
Which issue(s) this PR fixes:
Fixes #2127
Special notes for your reviewer:
Does this PR introduce a user-facing change?: