Add hsperfdata Input Plugin #2058
Conversation
njwhite
force-pushed
the
master
branch
9 times, most recently
from
df605d5
to
6e0b2ea
Compare
For gather data from the shared memory exposed by running processes.
|
Could I get a review for this PR? Thanks - |
|
Thanks for the update @sparrc! |
njwhite
force-pushed
the
master
branch
2 times, most recently
from
7bd350a
to
f622f80
Compare
|
Hi @sparrc - are there any docs on how to make this into an "external" plugin? Or can this PR be merged as an internal plugin? Thanks - |
|
@njwhite Sorry, will have to wait awhile more before we have time to review for official inclusion. |
|
Shame, I would like to replace collectd/jmxtrans with telegraf and JVM monitoring is the only thing stopping me from migrating. |
|
@apexlir Take a look at the collectd parser used with socket_listener and the jolokia input for JVM monitoring. |
danielnelson
added
feat
|
it's been almost a year for this PR, is anyone reviewing it? |
|
We have an interest in using a non-JMX means for monitoring the JVM from Telegraf (for a JVM where we're avoiding allocation of additional memory). This looks like a great fit. Is it likely to be included in a future version of Telegraf, or should it be included via the plugin framework? |
|
Hey guys, still waiting this plugin in master. When you will merge it? |
| ### Configuration: | ||
|
|
||
| ```toml | ||
| [[inputs.hsperfdata]] |
There was a problem hiding this comment.
Make sure this matches what is in sampleConfig
| # | ||
| ## Use the value for these keys in the hsperfdata as tags, not fields. By | ||
| ## default everything is a field. | ||
| # tags = ["sun.rt.jvmVersion"] |
There was a problem hiding this comment.
It seems this same thing (tags and filter) can be accomplished with the converter processor and metric filtering
| ## Use an arbitary directory to gather perfdata. This can be useful if you | ||
| ## want data belonging to a different user. | ||
| # directory = "/tmp/hsperfdata_otheruser" | ||
| # |
There was a problem hiding this comment.
nitpick: Rather than commenting an empty line, leave it empty.
| files, err := ioutil.ReadDir(dir) | ||
| if err != nil { | ||
| // e.g. no such directory or no permissions - just don't record metrics | ||
| return retval, nil |
There was a problem hiding this comment.
Just return the error since it get's ignored in Gather (allows for skipping the rest of the logic in gather).
| func (n *Hsperfdata) Gather(acc telegraf.Accumulator) error { | ||
| files, err := n.GetFiles() | ||
| if err != nil { | ||
| // the directory doesn't exist - so there aren't any Java processes running |
There was a problem hiding this comment.
This makes sense, but seems too permissive, though returning an error may be too abrasive.
| for _, f := range files { | ||
| // the hsperfdata files are named after the pid | ||
| if _, err := strconv.Atoi(f.Name()); err == nil { | ||
| retval[filepath.Join(dir, f.Name())] = f.Name() |
There was a problem hiding this comment.
nitpick: i would have done retval[f.Name()] = join(dir,f.Name()) or simply stored a slice of join(dir, f.name()) as the pid can be derived again by using filepath.Base() in the gatherOne function.
| return false | ||
| } | ||
|
|
||
| func (n *Hsperfdata) GatherOne(acc telegraf.Accumulator, file string, pid string) error { |
There was a problem hiding this comment.
nitpick: no need to export this function
| } | ||
| unconvertedTickFields := []tickfield{} | ||
|
|
||
| filter, err := regexp.Compile(n.Filter) |
There was a problem hiding this comment.
Consider doing this as a MustCompile in the Init() error function and storing it in the Hsperfdata structure
| func (n *Hsperfdata) GetFiles() (map[string]string, error) { | ||
| dir := n.Directory | ||
| if dir == "" { | ||
| // pick a sensible default: /tmp/hsperfdata_<user> |
There was a problem hiding this comment.
I would almost say put this default directory logic into the init() function and set it to Directory in the Hsperfdata that is set.
glinton
removed
the
feat
|
Also, @njwhite please update your branch with master to remove the conflicts and refrain from force pushing your branch now that the review has been started, in order to facilitate speedy review updates. |
|
Thanks for the review @glinton - however I raised this PR 2 1/2 years ago and now no longer need it. Happy for anyone else to take ownership - |
|
Let's close the pull request for now and if someone is interested in adopting this plugin please create a new issue first. |
A new version of #2056 (as I can't re-open the PR with my new changes @sparrc?). I've re-written the binary parsing logic (it no longer uses the dependency go-hsperfdata) and so is about 200 lines longer. I've also expanded the test coverage with some interestingly-malformed hsperfdata files and other edge cases.
Required for all PRs: