feat(processors.enum): Add fields and tags config options #13824
Conversation
telegraf-tiger
bot
added
feat
|
Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
powersj
added
the
ready for final review
| if mapping.Field != "" { | ||
| fieldFilter, err := filter.NewIncludeExcludeFilter([]string{mapping.Field}, nil) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create new field filter: %w", err) | ||
| return fmt.Errorf("failed to create new field filter from field: %w", err) | ||
| } | ||
| mapper.FieldFilters[mapping.Field] = fieldFilter | ||
| } |
There was a problem hiding this comment.
What about just adding the single Field to the Fields array?
There was a problem hiding this comment.
I do agree with @Hipska. With the deprecation this would be the best way...
| Tag string | ||
| Tags []string | ||
| Field string | ||
| Fields []string |
There was a problem hiding this comment.
Can we please add add TOML annotations here and deprecate the singular options in favor of the plural ones?
| if mapping.Field != "" { | ||
| fieldFilter, err := filter.NewIncludeExcludeFilter([]string{mapping.Field}, nil) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create new field filter: %w", err) | ||
| return fmt.Errorf("failed to create new field filter from field: %w", err) | ||
| } | ||
| mapper.FieldFilters[mapping.Field] = fieldFilter | ||
| } |
There was a problem hiding this comment.
I do agree with @Hipska. With the deprecation this would be the best way...
| } | ||
| mapper.FieldFilters[mapping.Field] = fieldFilter | ||
| } | ||
| if len(mapping.Fields) != 0 { | ||
| fieldFilter, err := filter.NewIncludeExcludeFilter(mapping.Fields, nil) |
There was a problem hiding this comment.
We should use a simple filter instead of an IncludeExcludeFilter here.
| if err != nil { | ||
| return fmt.Errorf("failed to create new field filter from fields: %w", err) | ||
| } | ||
| mapper.FieldFilters[mapping.Field] = fieldFilter |
There was a problem hiding this comment.
I don't understand why you use a map for the filter... The key for the map is either empty (if no field is defined) or has the field name, i.e. the map will only contain a single entry. I think you should store the filter directly instead of using a map.
| if mapping.Field != "" { | ||
| if mapping.Field != "" || len(mapping.Fields) != 0 { | ||
| mapper.fieldMapping(metric, mapping, newFields) | ||
| } | ||
| if mapping.Tag != "" { | ||
| if mapping.Tag != "" || len(mapping.Tags) != 0 { | ||
| mapper.tagMapping(metric, mapping, newTags) | ||
| } |
There was a problem hiding this comment.
As said before, I would prefer to create a filter for tags and fields and then simply check for the filter being not-nil here. I think this will simplify the code...
fixes: #10892