Skip to content
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
Closed

Conversation

kimor79
Copy link
Contributor

@kimor79 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
Copy link

+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
Copy link
Contributor Author

kimor79 commented Oct 1, 2014

CLA submitted

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
Copy link
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.

@Dieterbe
Copy link
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.

@pauldix
Copy link
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?

case "value":
// Don't add to metric name
case "":
// Don't add to metric name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jvshahid
Copy link
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.

@kimor79
Copy link
Contributor Author

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!

@@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kimor79 any reason why you ignore the error ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants