Add input plugin for McRouter #4077

cthayer · 2018-04-25T20:59:57Z

McRouter is a smart proxy for Memcached

https://github.com/facebook/mcrouter

Code is based on the memcached input plugin

Required for all PRs:

Signed CLA.
Associated README.md updated.
Has appropriate unit tests.

danielnelson

I know this is based heavily on the memcached input, but this is one of the oldest plugins we have and it could stand to be brought up to the current style/standards. Would you be interested in doing some of the updates on this plugin and then we can bring memcached up to match afterwards?

danielnelson · 2018-04-26T01:20:00Z

plugins/inputs/mcrouter/README.md

+  # with optional port. ie localhost, 10.0.0.1:11211, etc.
+  servers = ["localhost:11211"]
+  # An array of unix mcrouter sockets to gather stats about.
+  # unix_sockets = ["/var/run/mcrouter.sock"]


Let's use a single list of this style:

servers = ["tcp://localhost:11211", "unix:///var/run/mcrouter.sock", "udp://localhost:11211"]

We don't need to add support for UDP at first, assuming it is even available with mcrouter, but this will give us a nice way to add it later.

I've updated to use a single array of servers and pull the protocol from the connection string.

(mcrouter does not support UDP connections)

danielnelson · 2018-04-26T01:24:58Z

plugins/inputs/mcrouter/mcrouter.go

+var defaultTimeout = 5 * time.Second
+
+// The list of metrics that should be sent
+var sendMetrics = []string{


I would love to have these locked to specific types, since it would be a problem if a field changed values. Maybe we can turn this into a map[string]statType where statType is an enum being int64, float64 or string? Then when we parse we try the expected value and log an error if it doesn't match.

switched to map with types and only attempt a convert for the expected type

danielnelson · 2018-04-26T01:30:04Z

plugins/inputs/mcrouter/mcrouter.go

+  # unix_sockets = ["/var/run/mcrouter.sock"]
+`
+
+var defaultTimeout = 5 * time.Second


Let's make this configurable and have it apply to all servers.

I followed the zookeeper example here

danielnelson · 2018-04-26T01:30:17Z

plugins/inputs/mcrouter/mcrouter.go

+			address = address + ":11211"
+		}
+
+		conn, err = net.DialTimeout("tcp", address, defaultTimeout)


Would be a nice touch to create a context.Context at the beginning of Gather, and use DialContext, I recently updated the zookeeper input to do something like this:

https://github.com/influxdata/telegraf/blob/master/plugins/inputs/zookeeper/zookeeper.go#L81

I think I followed the zookeeper example here

danielnelson · 2018-04-26T01:36:35Z

plugins/inputs/mcrouter/mcrouter.go

+	conn.SetDeadline(time.Now().Add(defaultTimeout))
+
+	// Read and write buffer
+	rw := bufio.NewReadWriter(bufio.NewReader(conn), bufio.NewWriter(conn))


I don't think this is necessary, just use conn.Write( and you won't need to flush.

I followed the zookeeper example and just do fmt.Fprint(conn

danielnelson · 2018-04-26T01:39:51Z

plugins/inputs/mcrouter/mcrouter.go

+	return nil
+}
+
+func parseResponse(r *bufio.Reader) (map[string]string, error) {


We should use a bufio.Scanner to read lines, the ReadLine() function can return partial lines.

Using bufio.Scanner now

danielnelson · 2018-04-26T01:42:16Z

plugins/inputs/mcrouter/mcrouter.go

+		// Read values
+		s := bytes.SplitN(line, []byte(" "), 3)
+		if len(s) != 3 || !bytes.Equal(s[0], []byte("STAT")) {
+			return values, fmt.Errorf("unexpected line in stats response: %q", line)


Return nil as the map.

Made the change. I assume that the standard is to return no stats on error rather than partial stats?

Yes, returning an error is probably best here.

cthayer · 2018-04-27T14:57:54Z

I think I could update the memcached plugin afterwards.

It seems like a breaking change for the memcached plugin to remove the UnixSockets member from the struct?

danielnelson · 2018-04-27T22:34:52Z

plugins/inputs/mcrouter/mcrouter.go

+	var protocol string
+	var dialer net.Dialer
+
+	s := strings.SplitN(address, "://", 2)


Use url.Parse here, I think it will help simplify things and prevent errors.

I updated the code to use url.Parse

I feel like it makes it a bit stricter on the server connection string format, but I think that's ok.

I make some attempt to tease partial connection strings into a proper format, but ultimately default to tcp://localhost:11211 if the server connection string does not parse properly.

danielnelson · 2018-04-27T23:00:56Z

Yeah, for memcached we will need to keep backwards compatibility. I usually try to translate the old format to the new one on the first call to Gather.

danielnelson · 2018-04-30T21:23:53Z

plugins/inputs/mcrouter/mcrouter.go

+	}
+
+	if u.Scheme != "tcp" && u.Scheme != "unix" {
+		protocol = defaultServerURL.Scheme


return error

returning error

danielnelson · 2018-04-30T21:24:28Z

plugins/inputs/mcrouter/mcrouter.go

+		address = u.Path
+	} else {
+		if u.Host == "" {
+			u.Host = defaultServerURL.Host


return error

returning error

danielnelson · 2018-04-30T21:40:18Z

plugins/inputs/mcrouter/mcrouter.go

+
+		host, port, err = net.SplitHostPort(u.Host)
+
+		if err != nil {


I did a few quick tests and unfortunately I think we will need to handle the case when either host or port are empty strings:

func Check(input string) { host, port, err := net.SplitHostPort(input) fmt.Printf("%q %q %q: %v\n", input, host, port, err) } func TestSplitHostPort(t *testing.T) { Check("localhost:8086") Check("localhost") # we should accept this and use default port Check("localhost:") # use default port Check(":8086") # use default hostname Check(":") # default host:port or reject Check("") # reject, but we already do that above }

"localhost:8086" "localhost" "8086": <nil> "localhost" "" "": address localhost: missing port in address "localhost:" "localhost" "": <nil> ":8086" "" "8086": <nil> ":" "" "": <nil> "" "" "": missing port in address

I added a test for parsing addresses and separated the address parsing into its own function.

I'm just using the url.URL functions to get the hostname and port parts and not using net.SplitHostPort any more.

McRouter is a smart proxy for Memcached https://github.com/facebook/mcrouter

danielnelson · 2018-05-01T19:06:22Z

Great job, merged for 1.7.0

cthayer · 2018-05-02T15:16:33Z

Thanks!

danielnelson reviewed Apr 26, 2018

View reviewed changes

danielnelson added the new plugin label Apr 26, 2018

cthayer force-pushed the master branch 3 times, most recently from 2801c28 to d79b823 Compare April 27, 2018 14:55

danielnelson reviewed Apr 27, 2018

View reviewed changes

cthayer force-pushed the master branch from d79b823 to eb1d538 Compare April 28, 2018 17:31

danielnelson reviewed Apr 30, 2018

View reviewed changes

Add input plugin for McRouter

4af13a4

McRouter is a smart proxy for Memcached https://github.com/facebook/mcrouter

cthayer force-pushed the master branch from eb1d538 to 4af13a4 Compare May 1, 2018 15:02

danielnelson added this to the 1.7.0 milestone May 1, 2018

danielnelson merged commit 83345ec into influxdata:master May 1, 2018

arkady-emelyanov pushed a commit to arkady-emelyanov/telegraf that referenced this pull request May 18, 2018

Add input plugin for McRouter (influxdata#4077)

083fda8

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

Add input plugin for McRouter (#4077)

9803d62

otherpirate pushed a commit to otherpirate/telegraf that referenced this pull request Mar 15, 2019

Add input plugin for McRouter (influxdata#4077)

d20e253

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add input plugin for McRouter #4077

Add input plugin for McRouter #4077

cthayer commented Apr 25, 2018

danielnelson left a comment

danielnelson Apr 26, 2018

cthayer Apr 27, 2018

danielnelson Apr 26, 2018

cthayer Apr 27, 2018

danielnelson Apr 26, 2018

cthayer Apr 27, 2018

danielnelson Apr 26, 2018

cthayer Apr 27, 2018

danielnelson Apr 26, 2018

cthayer Apr 27, 2018

danielnelson Apr 26, 2018

cthayer Apr 27, 2018

danielnelson Apr 26, 2018

cthayer Apr 27, 2018

danielnelson Apr 27, 2018

cthayer commented Apr 27, 2018

danielnelson Apr 27, 2018

cthayer Apr 28, 2018

danielnelson commented Apr 27, 2018

danielnelson Apr 30, 2018

cthayer May 1, 2018

danielnelson Apr 30, 2018

cthayer May 1, 2018

danielnelson Apr 30, 2018

cthayer May 1, 2018

danielnelson commented May 1, 2018

cthayer commented May 2, 2018

Add input plugin for McRouter #4077

Add input plugin for McRouter #4077

Conversation

cthayer commented Apr 25, 2018

Required for all PRs:

danielnelson 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

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

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

Choose a reason for hiding this comment

cthayer commented Apr 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielnelson commented Apr 27, 2018

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

Choose a reason for hiding this comment

danielnelson commented May 1, 2018

cthayer commented May 2, 2018