add TCP StatsD listener support #71

jwfang · 2017-07-11T08:25:55Z

support TCP StatsD, on port 9125

SuperQ · 2017-07-11T08:33:00Z

main.go

+	listenAddress          = flag.String("web.listen-address", ":9102", "The address on which to expose the web interface and generated Prometheus metrics.")
+	metricsEndpoint        = flag.String("web.telemetry-path", "/metrics", "Path under which to expose metrics.")
+	statsdListenAddress    = flag.String("statsd.listen-address", ":9125", "The UDP address on which to receive statsd metric lines.")
+	statsdTCPListenAddress = flag.String("statsd.tcp-listen-address", ":9126", "The TCP address on which to receive statsd metric lines.")


I'd rather keep this one flag, so that it listens on the same TCP/UDP port. Also it looks like you have a typo, it's listening on 9126 here.

good catch, i fixed it.

i will make this a flag.

SuperQ · 2017-07-11T08:55:09Z

Instead of making this UDP or TCP, I would just have it listen for both.

jwfang · 2017-07-11T09:00:57Z

sorry, misunderstand you.

really make more sense to listen on both.

This reverts commit 4108c77.

SuperQ · 2017-07-11T09:05:58Z

No problem, thanks for working on this feature! 😃

…s as UDP

SuperQ · 2017-07-11T12:13:37Z

main.go

-	statsdListenAddress = flag.String("statsd.listen-address", ":9125", "The UDP address on which to receive statsd metric lines.")
+	statsdListenAddress = flag.String("statsd.listen-address", ":9125", "The UDP/TCP address on which to receive statsd metric lines.")
+	statsdListenUDP     = flag.Bool("statsd.listen-udp", true, "Whether to receive UDP statsd metrics.")
+	statsdListenTCP     = flag.Bool("statsd.listen-tcp", false, "Whether to receive TCP statsd metrics.")


This can be true by default.

SuperQ · 2017-07-13T10:53:22Z

exporter.go

+	return
+}
+
+type StatsDListener struct {


Can we make this StatsDUDPListener for consistency?

i thought UDP is primary and TCP is secondary originally.

done rename.

SuperQ

LGTM

SuperQ · 2017-07-20T17:23:28Z

Ping @juliusv Any more comments?

jacksontj · 2017-07-20T17:58:21Z

exporter.go

+
+	r := bufio.NewReader(c)
+	for {
+		line, isPrefix, err := r.ReadLine()


ReadBytes (https://golang.org/pkg/bufio/#Reader.ReadBytes) would be a better method to use-- from the docs: ReadLine is a low-level line-reading primitive. Most callers should use ReadBytes('\n') or ReadString('\n') instead or use a Scanner.

ReadBytes will read till delimiter or eof, so i think ReadLine is more safe for bogus input with really long line, or no line delimiter at all. :)

Fair enough, LGTM then :) Lets merge it!

jacksontj · 2017-07-20T18:02:40Z

exporter.go

+		if err != nil {
+			log.Fatalf("AcceptTCP failed: %v", err)
+		}
+		go l.handleConn(c, e)


Before calling handleconn (or immediately in it) -- we should probably call SetReadBuffer -- to be consistent with UDP

UDP needs a 64K buffer, since it will recv all data in one packet.
but for TCP, i guess the underlying bufio is manage the buffer for us.

SuperQ · 2017-07-22T20:08:43Z

@juliusv care to take one last look?

grobie

It'd be great to have some tests for this new feature.

grobie · 2017-07-22T20:26:44Z

exporter.go

-		if line == "" {
-			continue
-		}
+func lineToEvents(line string) (events Events) {


Please remove the named return variable here as it adds stutter to the function signature and use explicit returns below. It's not a go idiom to use naked returns in such long functions.

Named returned values should only be used on public funcs and methods
when it contributes to the documentation.

https://go-review.googlesource.com/c/20024/

grobie · 2017-07-22T20:32:08Z

exporter.go

+		line, isPrefix, err := r.ReadLine()
+		if err != nil {
+			if err != io.EOF {
+				log.Errorf("Read %s failed: %v", c.RemoteAddr(), err)


It'd be cool to have metrics for these errors so that operators can set up alerts on misbehaving clients.

grobie · 2017-07-22T20:33:01Z

main.go

@@ -122,34 +138,53 @@ func main() {
 		os.Exit(0)
 	}

+	if !*statsdListenUDP && !*statsdListenTCP {
+		log.Fatalln("At least one of UDP/TCP listeners should be specified.")


Sounds like a must then.

grobie · 2017-07-22T20:37:58Z

main.go

@@ -34,7 +34,9 @@ func init() {
 var (
 	listenAddress       = flag.String("web.listen-address", ":9102", "The address on which to expose the web interface and generated Prometheus metrics.")
 	metricsEndpoint     = flag.String("web.telemetry-path", "/metrics", "Path under which to expose metrics.")
-	statsdListenAddress = flag.String("statsd.listen-address", ":9125", "The UDP address on which to receive statsd metric lines.")
+	statsdListenAddress = flag.String("statsd.listen-address", ":9125", "The UDP/TCP address on which to receive statsd metric lines.")
+	statsdListenUDP     = flag.Bool("statsd.listen-udp", true, "Whether to receive UDP statsd metrics.")


Instead of boolean flags and introducing a dependency between flags, it'd be simpler to just have to flags with the addresses for each protocol, like -statsd.listen-address-udp=":9125" -statsd.listen-address-tcp=":9125". An empty value signals disables the server on that protocol.

doing it like what you mentioned have two conerns for me:

a bit uncomfortable for setting something to "" to disable it; and since we're default both UDP/TCP to on, use have to explicitly disable it;

the flags changes will break the user interface, from statsd.listen-address to statsd.listen-udp.

i have no strong feeling for 1; and for 2, if that's OK, i will do the flags changes.

EDIT: i think we can also support statsd.listen-address as statsd.listen-udp.

grobie · 2017-07-22T20:39:46Z

main.go

 	log.Infoln("Starting StatsD -> Prometheus Exporter", version.Info())
 	log.Infoln("Build context", version.BuildContext())
-	log.Infoln("Accepting StatsD Traffic on", *statsdListenAddress)
+	log.Infof("Accepting StatsD Traffic on %s, UDP %v, TCP %v", *statsdListenAddress, *statsdListenUDP, *statsdListenTCP)


That's not longer printing the address it's listening on, but true/false, but would be fixed with the comment above.

…o listen-udp

jwfang · 2017-07-24T06:34:52Z

@grobie thanks for your reivew, addressed most of your comments except for the test for TCPListener.

can't find some easy way to mock the TCPConnection. maybe we can make a
func (l *StatsDTCPListener) handleConn(r *io.Reader, e chan<- Events)
but that made the stats/logging a bit awkward. :(

grobie · 2017-07-24T15:25:03Z

can't find some easy way to mock the TCPConnection

I'd say, then don't mock for now and just start the server, send it a statsd line via TCP and check that it was ingested properly (by looking at the exported metrics).

jwfang · 2017-07-26T10:49:19Z

@grobie add test for TCP listener in the same place as UDP except benchmarks.

grobie · 2017-08-01T10:21:14Z

Thanks a lot for your contribution @jwfang!

add TCP StatsD listener support

d035f1a

SuperQ reviewed Jul 11, 2017

View reviewed changes

add listen-tcp flag to control UDP/TCP mode on same port

4108c77

minor tune

a43249b

Revert "add listen-tcp flag to control UDP/TCP mode on same port"

a58114a

This reverts commit 4108c77.

jwfang added 2 commits July 11, 2017 17:10

statsd.listen-tcp to control whether to accept TCP on the same addres…

d9a50da

…s as UDP

statsd.listen-udp to control whether to accept UDP metrics

5d147ac

SuperQ reviewed Jul 11, 2017

View reviewed changes

statsd.listen-tcp default to true

afd1adc

SuperQ reviewed Jul 13, 2017

View reviewed changes

jwfang added 2 commits July 13, 2017 20:15

rename StatsDListener to StatsDUDPListener

3d2dbf0

rename local variables in main

2ab641b

SuperQ approved these changes Jul 13, 2017

View reviewed changes

SuperQ requested a review from juliusv July 13, 2017 12:23

SuperQ mentioned this pull request Jul 20, 2017

Add TCP Listener #73

Closed

jacksontj reviewed Jul 20, 2017

View reviewed changes

jacksontj approved these changes Jul 22, 2017

View reviewed changes

grobie reviewed Jul 22, 2017

View reviewed changes

jwfang added 3 commits July 24, 2017 10:25

fix named return and log

d8c882e

statsdListenUDP/statsdListenTCP as string, and alias listen-address t…

efd3ddb

…o listen-udp

add stats for tcp error/line_too_long

6fb3533

jwfang added 2 commits July 25, 2017 15:29

add test for TCP listener

29294ab

close use defer

8807491

grobie merged commit 07543ac into prometheus:master Aug 1, 2017

bakins mentioned this pull request Nov 14, 2017

Preparation for v0.5.0 release #108

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add TCP StatsD listener support #71

add TCP StatsD listener support #71

jwfang commented Jul 11, 2017

SuperQ Jul 11, 2017

jwfang Jul 11, 2017 •

edited

Loading

SuperQ commented Jul 11, 2017

jwfang commented Jul 11, 2017

SuperQ commented Jul 11, 2017

SuperQ Jul 11, 2017

SuperQ Jul 13, 2017

jwfang Jul 13, 2017

SuperQ left a comment

SuperQ commented Jul 20, 2017

jacksontj Jul 20, 2017

jwfang Jul 21, 2017

jacksontj Jul 22, 2017

jacksontj Jul 20, 2017

jwfang Jul 21, 2017

SuperQ commented Jul 22, 2017

grobie left a comment

grobie Jul 22, 2017

grobie Jul 22, 2017

grobie Jul 22, 2017

grobie Jul 22, 2017

jwfang Jul 24, 2017 •

edited

Loading

grobie Jul 22, 2017

jwfang commented Jul 24, 2017

grobie commented Jul 24, 2017

jwfang commented Jul 26, 2017

grobie commented Aug 1, 2017

add TCP StatsD listener support #71

add TCP StatsD listener support #71

Conversation

jwfang commented Jul 11, 2017

Choose a reason for hiding this comment

jwfang Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

SuperQ commented Jul 11, 2017

jwfang commented Jul 11, 2017

SuperQ commented Jul 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuperQ left a comment

Choose a reason for hiding this comment

SuperQ commented Jul 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuperQ commented Jul 22, 2017

grobie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwfang Jul 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwfang commented Jul 24, 2017

grobie commented Jul 24, 2017

jwfang commented Jul 26, 2017

grobie commented Aug 1, 2017

jwfang Jul 11, 2017 •

edited

Loading

jwfang Jul 24, 2017 •

edited

Loading