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

Input plugin: collectd #992

Closed
wants to merge 8 commits into
from

Conversation

Projects
None yet
6 participants
@kimor79
Contributor

kimor79 commented Sep 30, 2014

This plugin supports receiving metrics via collectd's "network" plugin. It parses the binary protocol and stores most metric data as columns. Values are all float64. It requires the types.db file to be on the influxdb system.

Feedback appreciated.

@mfournier

This comment has been minimized.

Show comment
Hide comment
@mfournier

mfournier Oct 1, 2014

+1, it would be great to have such an option available natively !

collectd/collectd#696 also discusses what would be the best way to send collectd metrics to influxdb.

What is nice with your work @kimor79 is that people running old collectd versions can just add a couple of lines to their configuration to start shipping data to influxdb. No need to patch/upgrade/rebuild collectd on those legacy systems.

+1, it would be great to have such an option available natively !

collectd/collectd#696 also discusses what would be the best way to send collectd metrics to influxdb.

What is nice with your work @kimor79 is that people running old collectd versions can just add a couple of lines to their configuration to start shipping data to influxdb. No need to patch/upgrade/rebuild collectd on those legacy systems.

@kimor79

This comment has been minimized.

Show comment
Hide comment
@kimor79

kimor79 Oct 1, 2014

Contributor

CLA submitted

Contributor

kimor79 commented Oct 1, 2014

CLA submitted

Fix time.
Since collectd time high res is stored in 2^-30 seconds, we convert to seconds
first and then convert to milliseconds. Maybe someone smarter can figure out
converting directly to milliseconds (accurately).
@jvshahid

This comment has been minimized.

Show comment
Hide comment
@jvshahid

jvshahid Oct 15, 2014

Contributor

@pauldix @Dieterbe what do you guys think of this ? I don't have any strong opinion about adding a new plugin but I'm worried about adding too many input plugins that will require maintenance and overhead.

Contributor

jvshahid commented Oct 15, 2014

@pauldix @Dieterbe what do you guys think of this ? I don't have any strong opinion about adding a new plugin but I'm worried about adding too many input plugins that will require maintenance and overhead.

@Dieterbe

This comment has been minimized.

Show comment
Hide comment
@Dieterbe

Dieterbe Oct 15, 2014

Contributor

not sure, but FWIW in one of my other OSS projects if add-on packages become too much of a burden and no one fixes it within a reasonable time we just drop it until somebody steps up and puts things in good shape. It's just a matter of being upfront and clear about it. Granted said other project is a linux distro where packages come and go all the time, so it's a thing that people get used to. This case is a bit different.

Contributor

Dieterbe commented Oct 15, 2014

not sure, but FWIW in one of my other OSS projects if add-on packages become too much of a burden and no one fixes it within a reasonable time we just drop it until somebody steps up and puts things in good shape. It's just a matter of being upfront and clear about it. Granted said other project is a linux distro where packages come and go all the time, so it's a thing that people get used to. This case is a bit different.

@pauldix

This comment has been minimized.

Show comment
Hide comment
@pauldix

pauldix Oct 17, 2014

Member

I'm also a bit hesitant because of the maintenance issue. However, if people want it we can always take it in and pull it out later if it gets into a state of disrepair?

Member

pauldix commented Oct 17, 2014

I'm also a bit hesitant because of the maintenance issue. However, if people want it we can always take it in and pull it out later if it gets into a state of disrepair?

Show outdated Hide outdated api/collectd/api.go
case "value":
// Don't add to metric name
case "":
// Don't add to metric name

This comment has been minimized.

@jvshahid

jvshahid Oct 17, 2014

Contributor

this could just be case "value", case "":

@jvshahid

jvshahid Oct 17, 2014

Contributor

this could just be case "value", case "":

Show outdated Hide outdated api/collectd/api.go
ts, _ := strconv.ParseInt(sts, 10, 64)
for _, dataSet := range packet.Values {
metricName := fmt.Sprintf("%s %s", packet.Plugin, packet.Type)

This comment has been minimized.

@jvshahid

jvshahid Oct 17, 2014

Contributor

I'm not familiar with collectd, but curious if the metric name is supposed to have spaces ?

@jvshahid

jvshahid Oct 17, 2014

Contributor

I'm not familiar with collectd, but curious if the metric name is supposed to have spaces ?

This comment has been minimized.

@kimor79

kimor79 Oct 17, 2014

Contributor

Collectd stores the name like "hostname/type-typeinstance/plugin-plugininstance" (https://collectd.org/wiki/index.php/Naming_schema). On my first attempt, influx wouldn't let me create a series with / in the name but that might have been user error. It seems like I can store it in that format now. I'll update this PR when that is ready.

I'm open to other formats for the name as well.

@kimor79

kimor79 Oct 17, 2014

Contributor

Collectd stores the name like "hostname/type-typeinstance/plugin-plugininstance" (https://collectd.org/wiki/index.php/Naming_schema). On my first attempt, influx wouldn't let me create a series with / in the name but that might have been user error. It seems like I can store it in that format now. I'll update this PR when that is ready.

I'm open to other formats for the name as well.

enabled = false
# port = 25826
# database = ""
# typesdb = "/usr/share/collectd/types.db" # The path to the collectd types.db file

This comment has been minimized.

@jvshahid

jvshahid Oct 17, 2014

Contributor

Few questions on the typesdb:

  1. What happens if it's not set, you don't seem to check it's value in server.go and looking at collectd.TypesDb() it will return an error if it can't open the file which will be an empty string in this case
  2. How do users get that file is it part of the collectd installation. If so, then people need to install collectd in order to use this plugin which is sort of weird and defeats the purpose
@jvshahid

jvshahid Oct 17, 2014

Contributor

Few questions on the typesdb:

  1. What happens if it's not set, you don't seem to check it's value in server.go and looking at collectd.TypesDb() it will return an error if it can't open the file which will be an empty string in this case
  2. How do users get that file is it part of the collectd installation. If so, then people need to install collectd in order to use this plugin which is sort of weird and defeats the purpose

This comment has been minimized.

@kimor79

kimor79 Oct 17, 2014

Contributor

In ListenAndServer() in api/collectd/api.go if typesdb can't be parsed it will return an error. But I'll add a check to server.go as well. A simple "file exists?" style check should be enough? Or should I attempt to parse it there?

Regarding how that file is obtained, yeah, I'm not sure of a way around that. That file contains required information for many metrics so I can't just not use it. Open to ideas.

@kimor79

kimor79 Oct 17, 2014

Contributor

In ListenAndServer() in api/collectd/api.go if typesdb can't be parsed it will return an error. But I'll add a check to server.go as well. A simple "file exists?" style check should be enough? Or should I attempt to parse it there?

Regarding how that file is obtained, yeah, I'm not sure of a way around that. That file contains required information for many metrics so I can't just not use it. Open to ideas.

This comment has been minimized.

@pauldix

pauldix Oct 17, 2014

Member

Is it a file that's the same on all collectd installs or is it something the user creates? Does the user have to install collectd to have this file?

@pauldix

pauldix Oct 17, 2014

Member

Is it a file that's the same on all collectd installs or is it something the user creates? Does the user have to install collectd to have this file?

This comment has been minimized.

@kimor79

kimor79 Oct 17, 2014

Contributor

collectd ships with a default file (https://github.com/collectd/collectd/blob/master/src/types.db). If the user needs additional/different types, they can modify the installed file but the recommended way is to create additional types.db files and configure collectd to read them as well (via setting in the config file).

@kimor79

kimor79 Oct 17, 2014

Contributor

collectd ships with a default file (https://github.com/collectd/collectd/blob/master/src/types.db). If the user needs additional/different types, they can modify the installed file but the recommended way is to create additional types.db files and configure collectd to read them as well (via setting in the config file).

@jvshahid

This comment has been minimized.

Show comment
Hide comment
@jvshahid

jvshahid Oct 17, 2014

Contributor

@kimor79 i think the consensus is to merge this in. I had few questions which are in the comments above. As soon as you clarify those points I'll be happy to merge the pr into master.

Contributor

jvshahid commented Oct 17, 2014

@kimor79 i think the consensus is to merge this in. I had few questions which are in the comments above. As soon as you clarify those points I'll be happy to merge the pr into master.

@kimor79

This comment has been minimized.

Show comment
Hide comment
@kimor79

kimor79 Oct 18, 2014

Contributor

Pushed a change so the series name is how collectd formats it and types.db is checked earlier. Let me know if any other changes need to be made. Thanks!

Contributor

kimor79 commented Oct 18, 2014

Pushed a change so the series name is how collectd formats it and types.db is checked earlier. Let me know if any other changes need to be made. Thanks!

Show outdated Hide outdated server/server.go
@@ -151,6 +156,28 @@ func (self *Server) ListenAndServe() error {
log.Info("Graphite input plugins is disabled")
}
if self.Config.CollectdEnabled {
// Helper function to DRY out error log message
fail_reason := func(r string) string {

This comment has been minimized.

@jvshahid

jvshahid Oct 20, 2014

Contributor

The go idiom is to use camel cased variable names instead of underscores, can you change this variable name

@jvshahid

jvshahid Oct 20, 2014

Contributor

The go idiom is to use camel cased variable names instead of underscores, can you change this variable name

This comment has been minimized.

@kimor79

kimor79 Oct 20, 2014

Contributor

Sure. That section was pretty much a copy of the graphite section (https://github.com/influxdb/influxdb/blob/eef53f722892d98fdf30f8320bbdc3c1c0ac49ab/server/server.go#L138)

@jvshahid jvshahid added the review label Oct 20, 2014

@jvshahid jvshahid closed this in 3c84264 Oct 20, 2014

@jvshahid jvshahid removed the review label Oct 20, 2014

@jvshahid jvshahid self-assigned this Oct 20, 2014

@jvshahid jvshahid added this to the 0.8.4 milestone Oct 20, 2014

uts := (packet.TimeHR >> 30) * 1000
// TimeHR is also uint64 but influx expects int64
sts := strconv.FormatUint(uts, 10)
ts, _ := strconv.ParseInt(sts, 10, 64)

This comment has been minimized.

@jvshahid

jvshahid Oct 23, 2014

Contributor

@kimor79 any reason why you ignore the error ?

@jvshahid

jvshahid Oct 23, 2014

Contributor

@kimor79 any reason why you ignore the error ?

This comment has been minimized.

@kimor79

kimor79 Oct 25, 2014

Contributor

No particular reason. I can submit another patch if needed.

@kimor79

kimor79 Oct 25, 2014

Contributor

No particular reason. I can submit another patch if needed.

This comment has been minimized.

@dgnorton

dgnorton Oct 25, 2014

Contributor

@kimor79 can you review this PR? #1054

@dgnorton

dgnorton Oct 25, 2014

Contributor

@kimor79 can you review this PR? #1054

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment