-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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 support for converting tag or field to measurement in converter processor #7049
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few questions
|
||
```diff | ||
- apache,port=80,server=debian-stretch-apache BusyWorkers=1,BytesPerReq=0 | ||
+ apache,server=debian-stretch-apache port="80",BusyWorkers=1,BytesPerReq=0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should these be in the same order to be more clear?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'll update this, it should read better if I move the port to the end of the fields:
+ apache,server=debian-stretch-apache BusyWorkers=1,BytesPerReq=0,port="80"
Fields *Conversion `toml:"fields"` | ||
Tags *Conversion `toml:"tags"` | ||
Fields *Conversion `toml:"fields"` | ||
Log telegraf.Logger `toml:"-"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should log be private?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does needs to be exported, which is why we mark it unsettable from the toml. This is due to how we inject the Logger from outside the package (which we have never been super happy with). When adding this we also toyed around with injecting the logger on init: Init(Logger) error
but ultimately decided not to inject it there.
return metrics | ||
} | ||
} | ||
func (p *Converter) Init() error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this a contract change that we need to worry about? previously Apply would init for you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Init() function is optional on any plugin type, but will be called if defined. It makes it a little bit easier to initialize a plugin without needing the manage it and improves the plugins ability to report errors with the configuration was setup improperly.
"cpu", | ||
expected: []telegraf.Metric{ | ||
testutil.MustMetric( | ||
"/var/log/syslog", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#6970 talks about using filenames as the measurement name. I don't think they'll want to use the full path. Is there a way to get just the basename of the file input plugin's file_tag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You would need to use another processor to modify the tag value first, it ought to be possible to do this with the regex processor though. We could potentially have a filepath
processor that does common transformations, or perhaps it would fit in with the strings
processor.
closes: #6970
Required for all PRs: