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

Add generic UDP line-protocol service input #758

Merged
merged 3 commits into from
Mar 1, 2016
Merged

Add generic UDP line-protocol service input #758

merged 3 commits into from
Mar 1, 2016

Conversation

whatyouhide
Copy link
Contributor

@lexmag and I added the UDP line-protocol service input discussed in #481. The PR fully implements the UDP service input but is still lacking tests and a README; we're working on both but we would like some feedback on the current implementation :).

return "Read metrics from one or more commands that can output to stdout"
}

func (u *UdpLineProtocol) Start(_ telegraf.Accumulator) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

lock/unlock the start function

@sparrc
Copy link
Contributor

sparrc commented Feb 26, 2016

thanks @whatyouhide, a few things that will need to be done to complete this:

  1. Your Start function should utilize the accumulator by continuously adding points that enter, rather than filling an internal buffer and flushing on every call to Gather. See the mqtt_consumer plugin as an example of this (Gather will become a dummy function)
  2. After doing the above, there should no longer be a need for keeping an internal buffer and dropping any metrics.
  3. Your listener should not be specific to "line protocol", it should utilize the generic data format parsers (also see mqtt_consumer or exec). There is a short guide in the README explaining how to utilize the generic parsers here

@whatyouhide
Copy link
Contributor Author

@sparrc I updated the PR with your suggested changes, so this is ready for review (I'll start writing tests right now).

@whatyouhide
Copy link
Contributor Author

@sparrc I added a README and tests as well. Let me know if I can do anything else :D

if err == nil {
u.storeMetrics(metrics)
} else {
log.Printf("Malformed packet")
Copy link
Contributor

Choose a reason for hiding this comment

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

print the packet and the error message here:

log.Printf("Malformed packet: [%s], Error: %s", packet, err)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sparrc updated and rebased!

@whatyouhide
Copy link
Contributor Author

I suspect the failings on CircleCI are not related to the changes in the PR. Let me know if they are, I'll take a deeper look at them and see if I can fix them :)

@sparrc
Copy link
Contributor

sparrc commented Mar 1, 2016

this was related to #745, which I've just closed, could you rebase your changes and re-push?

@whatyouhide
Copy link
Contributor Author

@sparrc rebased over master. :)

@sparrc
Copy link
Contributor

sparrc commented Mar 1, 2016

great! thanks @whatyouhide, LGTM 👍

@whatyouhide
Copy link
Contributor Author

@sparrc would it be a problem to have a release once this gets merged? We're about to start using telegraf in my company and a package instead of building from source would be nice :).

@sparrc
Copy link
Contributor

sparrc commented Mar 1, 2016

yes, sure, shouldn't be a problem

@sparrc sparrc merged commit e1f30ae into influxdata:master Mar 1, 2016
sparrc added a commit that referenced this pull request Mar 1, 2016
- using 8092 as the default port because it's similar to the rest of
  the TICK stack (InfluxDB, for example, uses 8083, 8086, 8088, etc.).
  didn't want to use 8125 because that conflicts with statsd.

closes #758
@sparrc
Copy link
Contributor

sparrc commented Mar 1, 2016

@whatyouhide I've merged this into master, with a couple changes:

  1. Changed default port to 8092, this was to fit in more with other TICK stack things (InfluxDB uses 8090 by default for udp, Kapacitor uses 9092). I also didn't want the default to conflict with the statsd default.
  2. Added a note on OS UDP buffer sizes in the README.

hope those are both OK, and thanks for the contribution!

@whatyouhide whatyouhide deleted the udp-line-protocol branch March 1, 2016 15:10
@whatyouhide
Copy link
Contributor Author

@sparrc of course they're OK :) thanks for merging!

@lexmag
Copy link
Contributor

lexmag commented Mar 8, 2016

Hey @sparrc, any ETA on version with this input?

geodimm pushed a commit to miketonks/telegraf that referenced this pull request Mar 10, 2016
- using 8092 as the default port because it's similar to the rest of
  the TICK stack (InfluxDB, for example, uses 8083, 8086, 8088, etc.).
  didn't want to use 8125 because that conflicts with statsd.

closes influxdata#758
@whatyouhide
Copy link
Contributor Author

Hey @sparrc, sorry for bothering you again! Any news on the telegraf release that would include this PR? Thanks 💟

@sparrc
Copy link
Contributor

sparrc commented Mar 11, 2016

next week we will do 0.11 and it will have this plugin

@whatyouhide
Copy link
Contributor Author

Awesome, thanks so much! 😍

This pull request was closed.
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.

3 participants