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

fix logic in tags/names/fields filtering #3036

Merged
merged 1 commit into from
Jul 21, 2017
Merged

fix logic in tags/names/fields filtering #3036

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
4 changes: 2 additions & 2 deletions docs/CONFIGURATION.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,8 +134,8 @@ is tested on points after they have passed the `namepass` test.
An array of glob pattern strings. Only fields whose field key matches a
pattern in this list are emitted. Not available for outputs.
* **fielddrop**:
The inverse of `fieldpass`. Fields with a field key matching one of the
patterns will be discarded from the point. Not available for outputs.
The inverse of `fieldpass`. Fields with a field key matching one of the
patterns will be discarded from the point. This is tested on points after they have passed the `fieldpass` test. Not available for outputs.
* **tagpass**:
A table mapping tag keys to arrays of glob pattern strings. Only points
that contain a tag key in the table and a tag value matching one of its
Expand Down
41 changes: 32 additions & 9 deletions internal/models/filter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,51 +132,74 @@ func (f *Filter) Apply(
return true
}

// IsActive checking if filter is active
func (f *Filter) IsActive() bool {
return f.isActive
}

// shouldNamePass returns true if the metric should pass, false if should drop
// based on the drop/pass filter parameters
func (f *Filter) shouldNamePass(key string) bool {
if f.namePass != nil {

pass := func(f *Filter) bool {
if f.namePass.Match(key) {
return true
}
return false
}

if f.nameDrop != nil {
drop := func(f *Filter) bool {
if f.nameDrop.Match(key) {
return false
}
return true
}

if f.namePass != nil && f.nameDrop != nil {
return pass(f) && drop(f)
Copy link
Contributor

Choose a reason for hiding this comment

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

I know it's an old PR, but whenever I look at this code (because the docs aren't specific enough) I find this sequence confusing. The definition of drop(f) means "the drop filter says 'pass'", which inverts the prosaic sense of the code. if pass and drop just doesn't smell good. Would it be clearer to remove the negation inside drop , and use these?

  return pass(f) && ! drop(f)
  return pass(f)
  return ! drop(f)

} else if f.namePass != nil {
return pass(f)
} else if f.nameDrop != nil {
return drop(f)
}

return true
}

// shouldFieldPass returns true if the metric should pass, false if should drop
// based on the drop/pass filter parameters
func (f *Filter) shouldFieldPass(key string) bool {
if f.fieldPass != nil {

pass := func(f *Filter) bool {
if f.fieldPass.Match(key) {
return true
}
return false
}

if f.fieldDrop != nil {
drop := func(f *Filter) bool {
if f.fieldDrop.Match(key) {
return false
}
return true
}

if f.fieldPass != nil && f.fieldDrop != nil {
return pass(f) && drop(f)
} else if f.fieldPass != nil {
return pass(f)
} else if f.fieldDrop != nil {
return drop(f)
}

return true
}

// shouldTagsPass returns true if the metric should pass, false if should drop
// based on the tagdrop/tagpass filter parameters
func (f *Filter) shouldTagsPass(tags map[string]string) bool {

tagPass := func(f *Filter) bool {
pass := func(f *Filter) bool {
for _, pat := range f.TagPass {
if pat.filter == nil {
continue
Expand All @@ -190,7 +213,7 @@ func (f *Filter) shouldTagsPass(tags map[string]string) bool {
return false
}

tagDrop := func(f *Filter) bool {
drop := func(f *Filter) bool {
for _, pat := range f.TagDrop {
if pat.filter == nil {
continue
Expand All @@ -209,11 +232,11 @@ func (f *Filter) shouldTagsPass(tags map[string]string) bool {
if f.TagPass != nil && f.TagDrop != nil {
// return true only in case when tag pass and won't be dropped (true, true).
// in case when the same tag should be passed and dropped it will be dropped (true, false).
return tagPass(f) && tagDrop(f)
return pass(f) && drop(f)
} else if f.TagPass != nil {
return tagPass(f)
return pass(f)
} else if f.TagDrop != nil {
return tagDrop(f)
return drop(f)
}

return true
Expand Down
40 changes: 40 additions & 0 deletions internal/models/filter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,46 @@ func TestFilter_FilterTagsMatches(t *testing.T) {
}, pretags)
}

// TestFilter_FilterNamePassAndDrop used for check case when
// both parameters were defined
// see: https://github.com/influxdata/telegraf/issues/2860
func TestFilter_FilterNamePassAndDrop(t *testing.T) {

inputData := []string{"name1", "name2", "name3", "name4"}
expectedResult := []bool{false, true, false, false}

f := Filter{
NamePass: []string{"name1", "name2"},
NameDrop: []string{"name1", "name3"},
}

require.NoError(t, f.Compile())

for i, name := range inputData {
assert.Equal(t, f.shouldNamePass(name), expectedResult[i])
}
}

// TestFilter_FilterFieldPassAndDrop used for check case when
// both parameters were defined
// see: https://github.com/influxdata/telegraf/issues/2860
func TestFilter_FilterFieldPassAndDrop(t *testing.T) {

inputData := []string{"field1", "field2", "field3", "field4"}
expectedResult := []bool{false, true, false, false}

f := Filter{
FieldPass: []string{"field1", "field2"},
FieldDrop: []string{"field1", "field3"},
}

require.NoError(t, f.Compile())

for i, field := range inputData {
assert.Equal(t, f.shouldFieldPass(field), expectedResult[i])
}
}

// TestFilter_FilterTagsPassAndDrop used for check case when
// both parameters were defined
// see: https://github.com/influxdata/telegraf/issues/2860
Expand Down