Add Condition to check if a list of fields exists #6653
Conversation
sriranganathan
commented
Mar 24, 2018
sriranganathan
commented
- Create condition to check whether the give list of fields exist in the event
- Add Tests for the created condition
- Add Documentation for the created condition
- closes - Filter conditions when field is present or not present. #6285
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
libbeat/processors/condition.go
Outdated
| for _, field := range c.hasfields.fields { | ||
| _, err := event.GetValue(field) | ||
| if err != nil { | ||
| logp.Info("Field %v not present in the event", field) |
There was a problem hiding this comment.
I think it must be close to exists code, like in checkEquals, in case if there is no value, just return false.
There is no info logging at all, is it make sense to introduce it only to this case?
There was a problem hiding this comment.
Thanks for the comments @ewgRa . I have made the changes.
libbeat/processors/condition.go
Outdated
| @@ -32,6 +32,9 @@ type Condition struct { | |||
| name string | |||
| filters map[string]match.Matcher | |||
| } | |||
| hasfields struct { | |||
There was a problem hiding this comment.
-
Maybe better just have hasfields []string?
-
And maybe rename it to requiredFields? In this case would be more natural "requiredFields []string"
There was a problem hiding this comment.
requiredFields is indeed an elegant name, but I feel it doesn't go that well with when and other conditions like not. I think we should keep this discussion open and try to find a better name for the condition.
|
@sriranganathan I leave my 5cents, just consider it as a suggestion, since I not a part of Elastica team. |
libbeat/processors/condition_test.go
Outdated
| @@ -145,6 +145,18 @@ func TestCondition(t *testing.T) { | |||
| }, | |||
| result: true, | |||
| }, | |||
| { | |||
| config: ConditionConfig{ | |||
| HasFields: []string{"proc.cmdline"}, | |||
There was a problem hiding this comment.
Could you please add some tests with several fields here? To check all of them should match
There was a problem hiding this comment.
@exekias Added a passing and a failing test with several fields.
| [[condition-has_fields]] | ||
| ===== `has_fields` | ||
|
|
||
| The `has_fields` condition checks if the given fields exist in the |
There was a problem hiding this comment.
I think this would be more clear if it said "checks if all the given fields exist".
There was a problem hiding this comment.
Thanks for the comment @andrewkroh . Updated the description.
sriranganathan
force-pushed
the
fix-6285
branch
from
8b1a701 to
e15039f
Compare
|
jenkins test it |
|
jenkins test it |
|
This is ready for merge, could you please add a line to the CHANGELOG.asciidoc file? |
- Create condition to check whether the give list of fields exist in the event - Add Tests for the created condition - Add Documentation for the created condition - closes - elastic#6285
- Remove unnecessary log statements - Use []string directly for fields instead of wrapping it in a struct
- Add test cases with multiple fields - Improve the description of the condition in the documentation
sriranganathan
force-pushed
the
fix-6285
branch
from
e15039f to
bbc37d1
Compare
|
@exekias added a changelog entry. |
|
jenkins, test it |
|
Thank you @sriranganathan for this awesome contribution!! 🎉 |
