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

Add condition to filters #1395

Merged
merged 7 commits into from
May 6, 2016
Merged

Conversation

monicasarbu
Copy link
Contributor

@monicasarbu monicasarbu commented Apr 14, 2016

This implements a task from the meta issue: #1447

This PR adds equals, contains, regexp and range conditions.

Please note that in the current implementation you cannot pass fields that contain .. The fix will come in a different PR.

@ruflin ruflin added in progress Pull request is currently in progress. libbeat labels Apr 18, 2016
@monicasarbu monicasarbu mentioned this pull request Apr 21, 2016
17 tasks
@monicasarbu monicasarbu force-pushed the add_condition_to_filters branch from 85276b1 to a670f83 Compare April 21, 2016 13:04
@monicasarbu monicasarbu changed the title Add condition to filters Add conditions to filtering rules Apr 21, 2016
@monicasarbu monicasarbu added review and removed in progress Pull request is currently in progress. labels Apr 21, 2016
@monicasarbu monicasarbu changed the title Add conditions to filtering rules Add condition to filtering rule Apr 21, 2016
@monicasarbu monicasarbu changed the title Add condition to filtering rule Add condition to filters Apr 21, 2016

switch value.(type) {
case int, int8, int16, int32, int64:
int_value := reflect.ValueOf(value).Int()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@urso Thanks! I already changed it with reflect. Please let me know if you find any other issues.

@ruflin
Copy link
Contributor

ruflin commented Apr 26, 2016

Should we also update the libbeat.yml file with these options to have at least 1 example per "type" in there?

type EqualsValue struct {
Int int
Str string
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It's kind of confusing that this one is defined here, but the RangeValue is in config.go. I'd say move one of them near the other.

The conditions can:
- check if the field is equal with a string or integer (equals)
- check if the field contains a given string (contains)
- check if the field matches a given regular expression (regexp)
- check if the field is in a given range and the supported values can be integer or float (range)
@monicasarbu monicasarbu force-pushed the add_condition_to_filters branch from b5d455b to cba9786 Compare May 3, 2016 13:53
@monicasarbu
Copy link
Contributor Author

I resolved all the comments. It's ready for the final review.


for field, value := range equals {

i, err := strconv.Atoi(value)
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I understand, the current implementation for equal supports strings and int. In case someone wants to use float it will return an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's right. For float values, there is range.

@ruflin
Copy link
Contributor

ruflin commented May 4, 2016

Do you plan to update the libbeat.yml in this PR? Would be good to have in a later PR some system tests inside the specific beats to valide this implementation. For example having filters on the global and module level in metricbeat and see if all works as expected.

@monicasarbu
Copy link
Contributor Author

The current implementation doesn't work in case the filtering rule contains a field containing a ".". I am planning to open a different PR for that.

@monicasarbu
Copy link
Contributor Author

Yes, this is just the initial PR. More small PRs will follow to this one once this is merged.

@ruflin ruflin merged commit 4125dea into elastic:master May 6, 2016
@ruflin
Copy link
Contributor

ruflin commented May 6, 2016

@monicasarbu Merged. It would be nice to have some method docs in the source code to clarify the questions we discussed above.

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.

3 participants