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

Enhance contains condition to work on fields that are arrays of string #2248

Merged
merged 1 commit into from
Aug 12, 2016
Merged

Enhance contains condition to work on fields that are arrays of string #2248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ https://github.com/elastic/beats/compare/v5.0.0-alpha5...master[Check the HEAD d
- Make Kafka metadata update configurable. {pull}2190[2190]
- Add kafka version setting (optional) enabling kafka broker version support. {pull}2190[2190]
- Add kafka message timestamp if at least version 0.10 is configured. {pull}2190[2190]
- Enhance contains condition to work on fields that are arrays of strings. {issue}2237[2237]

*Metricbeat*

Expand Down
26 changes: 18 additions & 8 deletions libbeat/processors/condition.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,26 +269,36 @@ func (c *Condition) checkEquals(event common.MapStr) bool {
}

func (c *Condition) checkContains(event common.MapStr) bool {

outer:
Copy link
Member

Choose a reason for hiding this comment

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

argh :-(

Copy link
Member Author

@andrewkroh andrewkroh Aug 12, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Why no love for the label? Naming?

Copy link
Member

Choose a reason for hiding this comment

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

It's not supported in my brain :-D

for field, equalValue := range c.contains {

value, err := event.GetValue(field)
if err != nil {
return false
}

sValue, err := extractString(value)
if err != nil {
logp.Warn("unexpected type %T in contains condition as it accepts only strings. ", value)
switch value.(type) {
case string:
if !strings.Contains(value.(string), equalValue) {
return false
}
case *string:
if !strings.Contains(*value.(*string), equalValue) {
return false
}
case []string:
Copy link
Contributor

Choose a reason for hiding this comment

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

@andrewkroh why not ?

found := false
for _, s := range value.([]string) {
  if strings.Contains(s, equalValue) {
   found = true
  }
}
if !found {
  return false
}

Copy link
Member Author

@andrewkroh andrewkroh Aug 16, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

They are equivalent if you add the required break statement to the snippet above. But the code in the PR is a bit more concise.

for _, s := range value.([]string) {
if strings.Contains(s, equalValue) {
continue outer
Copy link
Member

Choose a reason for hiding this comment

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

why not just return true here?

Copy link
Member Author

Choose a reason for hiding this comment

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

That would be changing the behavior of the method. This method returns true iff all the contains conditions are satisfied. Returning true would cause the method to not evaluate any remaining conditions.

When I was adding this, I initially did return true only to realize I broken a test.

Copy link
Member

Choose a reason for hiding this comment

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

Got it. Other option would be to package the second for loop into a function/method so "standard" continue could be used. I just think it makes it somehow harder to read.

}
}
return false
}
if !strings.Contains(sValue, equalValue) {
default:
logp.Warn("unexpected type %T in contains condition as it accepts only strings.", value)
return false
}
}

return true

}

func (c *Condition) checkRegexp(event common.MapStr) bool {
Expand Down
8 changes: 8 additions & 0 deletions libbeat/processors/condition_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,12 @@ func TestContainsCondition(t *testing.T) {
"proc.name": "secddd",
}},
},

ConditionConfig{
Contains: &ConditionFields{fields: map[string]interface{}{
"proc.keywords": "bar",
}},
},
}

conds := GetConditions(t, configs)
Expand All @@ -165,12 +171,14 @@ func TestContainsCondition(t *testing.T) {
"ppid": 1,
"state": "running",
"username": "monica",
"keywords": []string{"foo", "bar"},
},
"type": "process",
}

assert.True(t, conds[0].Check(event))
assert.False(t, conds[1].Check(event))
assert.True(t, conds[2].Check(event))
}

func TestRegexpCondition(t *testing.T) {
Expand Down