add socket listener & writer by phemmer · Pull Request #2094 · influxdata/telegraf

phemmer · 2016-11-27T19:15:13Z

Required for all PRs:

CHANGELOG.md updated (we recommend not updating this until the PR has been approved by a maintainer)
Sign CLA (if not already signed)
README.md updated (if adding a new plugin)

This adds support for a generic socket writer & listener.
The original intent was to add support for unix domain sockets. But it was trivial to write generic plugins that can handle all protocols. Thus the functionality of the socket_listener duplicates tcp_listener and udp_listener.
However in the case of tcp_listener, there is a critical difference in that the plugin will not ever drop a metric (such as if the buffer fills up). TCP is meant to be a reliable protocol, thus it should be up to the sender whether data gets dropped.
Another slight difference in the socket_listener is that instead of having 2 layers of buffering, an application chan buffer and a socket buffer, it only has a socket buffer. Config parameters have been provided for adjusting the size of the socket buffer. The chan buffer could be added, but I couldn't see any benefit to doing so, and thought it might be less confusing having only 1 layer of buffering.

Closes #1516, #1711, #1721
Obsoletes #1526

phemmer · 2016-11-27T19:17:08Z

testutil/accumulator.go


+// Wait waits for a metric to be added to the accumulator.
+// Accumulator must already be locked.
+func (a *Accumulator) Wait() {


This was added so that tests don't have to call sleep() with some excessive value to avoid race conditions.

sparrc · 2016-12-12T08:56:51Z

plugins/inputs/socket_listener/socket_listener.go

+func (psl *packetSocketListener) listen() {
+	buf := make([]byte, 64*1024) // 64kb - maximum size of IP packet
+	for {
+		_, _, err := psl.ReadFrom(buf)


these lines should only parse what was most recently read into the buffer, like this:

+ n, _, err := psl.ReadFrom(buf) + if err != nil { + psl.AddError(err) + break + } + + metrics, err := psl.Parse(buf[0:n])

Oops :-)
Dunno how I missed that one. Thanks for the catch.

sparrc · 2016-12-12T08:57:43Z

testutil/accumulator.go


+// Wait waits for a metric to be added to the accumulator.
+// Accumulator must already be locked.
+func (a *Accumulator) Wait() {


sparrc · 2016-12-12T09:19:40Z

plugins/outputs/socket_writer/socket_writer.go

+	}
+
+	for _, m := range metrics {
+		strs, err := sw.Serialize(m)


I've changed the behavior of the Serialize functions, they now return a byte slice which includes all newlines at the end of each metric. This part of the code will need to be refactored for that, but give me a day or two because I also have a change that will create an io.Reader from a slice of metrics. Using the io.Reader will create fewer allocations and also allow using io.CopyBuffer() into sw.Conn. Using io.CopyBuffer provides for a configurable max buffer size which users of datagram protocols will likely want.

Using io.CopyBuffer provides for a configurable max buffer size which users of datagram protocols will likely want.

We would need to slap a warning on the value that setting it below 64kb (at least when using UDP) is dangerous. If the read buffer size is set less than the size of an incoming packet, the packet will be lost. The only use case I can think of for adjusting from a default 64kb would be unix domain sockets.

The max buffer size I'm talking about here is for the writer, so that it would chop up the result of Serialize() into chunks rather than sending it all as a single byte buffer (which could easily exceed 64kb with thousands of metrics).

UDP users will also probably want their packets closer to the 512 - 4096 byte range

Doh. I need to stop reviewing code within an hour of waking up.

I also have a change that will create an io.Reader from a slice of metrics

Did this get implemented? I went looking for such a thing but couldn't find anything.

phemmer · 2016-12-12T13:16:53Z

plugins/outputs/socket_writer/socket_writer.go

+	}
+
+	for _, m := range metrics {
+		strs, err := sw.Serialize(m)


Using io.CopyBuffer provides for a configurable max buffer size which users of datagram protocols will likely want.

We would need to slap a warning on the value that setting it below 64kb (at least when using UDP) is dangerous. If the read buffer size is set less than the size of an incoming packet, the packet will be lost. The only use case I can think of for adjusting from a default 64kb would be unix domain sockets.

phemmer · 2016-12-12T13:17:52Z

plugins/inputs/socket_listener/socket_listener.go

+func (psl *packetSocketListener) listen() {
+	buf := make([]byte, 64*1024) // 64kb - maximum size of IP packet
+	for {
+		_, _, err := psl.ReadFrom(buf)


Oops :-)
Dunno how I missed that one. Thanks for the catch.

phemmer · 2016-12-12T13:21:23Z

testutil/accumulator.go

 // Accumulator defines a mocked out accumulator
 type Accumulator struct {
 	sync.Mutex
+	Cond *sync.Cond


note to self: cleanup: explicit Cond name not necessary.

sparrc

see requested changes in review, otherwise I think it looks ready to merge.

thanks again @phemmer!

phemmer · 2017-01-21T22:34:29Z

plugins/inputs/socket_listener/socket_listener.go

+	scnr := bufio.NewScanner(c)
+	for scnr.Scan() {
+		bs := scnr.Bytes()
+		bs = append(bs, '\n')


This is a workaround for #2297
I would like to remove this as growing the slice can result in more allocations. Potentially big ones as these slices will be large.

phemmer · 2017-01-21T22:34:44Z

plugins/inputs/socket_listener/socket_listener.go

+		bs := buf[:n]
+		if len(bs) > 0 && bs[len(bs)-1] != '\n' {
+			bs = append(bs, '\n')
+		}


This is the other workaround for #2297

phemmer · 2017-01-21T22:38:27Z

PR updated for change in serializer behavior.
Also added workarounds for Influx parser fails to parse but returns no error #2297
metrics io.Reader not implemented as I couldn't find it.

phemmer · 2017-01-26T03:49:37Z

PR updated to remove workarounds for #2297

phemmer · 2017-01-28T04:33:07Z

Current build failure looks unrelated to changes in this PR

phemmer · 2017-02-02T15:47:18Z

rebased for merge conflict

closes influxdata#1516 closes influxdata#1711 closes influxdata#1721 closes influxdata#1526

closes #1516 closes #1711 closes #1721 closes #1526

phemmer commented Nov 27, 2016

View reviewed changes

sparrc added this to the 1.3.0 milestone Nov 29, 2016

sparrc mentioned this pull request Dec 5, 2016

New Tcp Forwarder Output plugin. #1526

Closed

sparrc reviewed Dec 12, 2016

View reviewed changes

phemmer commented Dec 12, 2016

View reviewed changes

sparrc suggested changes Jan 13, 2017

View reviewed changes

phemmer mentioned this pull request Jan 21, 2017

Influx parser fails to parse but returns no error #2297

Closed

phemmer commented Jan 21, 2017

View reviewed changes

phemmer mentioned this pull request Jan 30, 2017

[Feature request] netns support in inputs.net section #2340

Closed

sparrc approved these changes Feb 2, 2017

View reviewed changes

add socket listener & writer

bd688a3

sparrc merged commit b3537ef into influxdata:master Feb 2, 2017

mlindes pushed a commit to Comcast/telegraf that referenced this pull request Feb 6, 2017

add socket listener & writer (influxdata#2094)

5d01a0b

closes influxdata#1516 closes influxdata#1711 closes influxdata#1721 closes influxdata#1526

phemmer deleted the generic-socket branch February 8, 2017 04:30

MrMaxBuilds pushed a commit that referenced this pull request Jun 26, 2018

add socket listener & writer (#2094)

510b750

closes #1516 closes #1711 closes #1721 closes #1526

Conversation

phemmer commented Nov 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Required for all PRs:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sparrc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phemmer commented Jan 21, 2017

Uh oh!

phemmer commented Jan 26, 2017

Uh oh!

phemmer commented Jan 28, 2017

Uh oh!

phemmer commented Feb 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

phemmer commented Nov 27, 2016 •

edited

Loading