-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Conversation
return "Read metrics from one or more commands that can output to stdout" | ||
} | ||
|
||
func (u *UdpLineProtocol) Start(_ telegraf.Accumulator) error { |
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.
lock/unlock the start function
thanks @whatyouhide, a few things that will need to be done to complete this:
|
@sparrc I updated the PR with your suggested changes, so this is ready for review (I'll start writing tests right now). |
@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") |
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.
print the packet and the error message here:
log.Printf("Malformed packet: [%s], Error: %s", packet, err)
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.
@sparrc updated and rebased!
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 :) |
this was related to #745, which I've just closed, could you rebase your changes and re-push? |
@sparrc rebased over master. :) |
great! thanks @whatyouhide, LGTM 👍 |
@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 :). |
yes, sure, shouldn't be a problem |
- 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
@whatyouhide I've merged this into master, with a couple changes:
hope those are both OK, and thanks for the contribution! |
@sparrc of course they're OK :) thanks for merging! |
Hey @sparrc, any ETA on version with this input? |
- 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
Hey @sparrc, sorry for bothering you again! Any news on the telegraf release that would include this PR? Thanks 💟 |
next week we will do 0.11 and it will have this plugin |
Awesome, thanks so much! 😍 |
@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 :).