-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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 fluentd input plugin #2661
Add fluentd input plugin #2661
Conversation
I did not catch this: plugins/inputs/fluentd/fluentd_test.go:126: arg pluginOutput for printf verb %s of wrong type: []fluentd.pluginData sorry |
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.
Looks good, I have just a few questions and suggestions:
plugins/inputs/fluentd/fluentd.go
Outdated
description = "Read metrics exposed by fluentd in_monitor plugin" | ||
sampleConfig = ` | ||
## This plugin only reads information exposed by fluentd using /api/plugins.json. | ||
## Tested using 'fluentd' version '0.14.9' |
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.
Move this comment about version in with the tests, so we can see the version you generated the sample data from. If you know the plugin will not work with any specific versions of fluentd, then we can put that in the README.
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.
Nothing that I've found during tests.
plugins/inputs/fluentd/README.md
Outdated
- All measurements have the following tags: | ||
- PluginID (unique plugin id) | ||
- PluginType (type of the plugin e.g. s3) | ||
- PluginCategory (plugin category e.g. output) |
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.
Telegraf normally uses snake_case for tag and field names, can you update these to use it?
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.
Will do.
Thanks!
plugins/inputs/fluentd/fluentd.go
Outdated
var ( | ||
pdPoint pluginData | ||
pdPointArray []pluginData | ||
parsed map[string]interface{} |
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.
Can you briefly explain why you decided to use an interface{} and reflection? It seems like since you have the struct defined it would be easy to Unmarshal directly into it.
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.
As I recall it properly, I've done it like this, because for some plugins we do not have fields e.g. buffer_queue_length or buffer_total_queued_size.
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.
In this case it may be better to use pointers in the struct and Unmarshal directly into it. If the field is not in the JSON it will be nil in the struct.
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.
Thanks for hint. I'm going to work on this tomorrow and ask you for review.
CI failed because of unrecognized zookeeper service:
|
plugins/inputs/fluentd/fluentd.go
Outdated
exclude = [ | ||
"monitor_agent", | ||
"dummy", | ||
] |
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.
Make sure the sample config has 2 space indention here and in the README, so that it will have the same indention as with the other plugins when you run telegraf config
.
I think if you rebase it will fix the circleci issue. |
Plugin is scraping metrics from JSON endpoint exposed by in_monitor plugin. I've used custom function to parse JSON output due to output incoherency. Please review and share your thoughts. Thanks!
- Change CamelCase tags, measurements and values names to sneak_case - Move information about used version during tests to test file
As danielnelson noticed, there is no need to use relfect and we can simplify whole parser using just Unmarshal method from json library, what I have done.
b71af40
to
24cf872
Compare
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 test issue cleared up when I ran the test again, unfortunately we have several intermittent test issues that can cause this from time to time.
One potential issue I ran into when testing was that every time you restart the fluentd daemon the plugins are given a new random plugin_id
. If you restart the server frequently this could result in high series cardinality, though I suspect normally you would restart infrequently. I also might make queries a little harder write if you want to group on plugin_id
.
I found that you can set the plugin_id in the configuration using @id
, which would preserve the id across restarts. If I am understanding this correctly, we should add a paragraph to the configuration suggesting its use. Here is an example from my config which seems to work:
<source>
@type http
@id http
port 8888
</source>
plugins/inputs/fluentd/fluentd.go
Outdated
PluginCategory string `json:"plugin_category"` | ||
RetryCount float64 `json:"retry_count"` | ||
BufferQueueLength float64 `json:"buffer_queue_length"` | ||
BufferTotalQueuedSize float64 `json:"buffer_total_queued_size"` |
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.
One difference in this revision of your plugin is that it will report 0 for these fields if they do not exist. In order to handle this you could have these be *float64
and test for nil before assigning the field.
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.
Good point. According to documentation we can setup this field for all plugins:
@id: Specify plugin id. in_monitor_agent uses this value for plugin_id field
. I'm going to add this to README.md
For second thing I will change this to pointer and add some checks around:
149
150 tmpFields["buffer_queue_length"] = p.BufferQueueLength
151 tmpFields["retry_count"] = p.RetryCount
152 tmpFields["buffer_total_queued_size"] = p.BufferTotalQueuedSize
153
Thanks!
New logic is checking if one of 3 fields (retry_count, buffer_queue_length and buffer_total_queued_size) is present. If there is at least one field, metric will be emitted and missing fields won't be added as values. In case where plugin doesn't have any of 3 fields, metric is skipped (not added to accumulator). Also unit test has been simplify.
Thank you! |
@danielnelson Thanks for help! |
Plugin is scraping metrics from JSON endpoint exposed by in_monitor plugin.
I've used custom function to parse JSON output due to output incoherency.
Please review and share your thoughts.
Thanks!
Required for all PRs: