NATS Monitoring Input Plugin #3186

levex · 2017-08-29T18:45:04Z

Hello,

I have a NATS bus in my setup and I'm looking to monitor its state via Grafana via InfluxDB, so I wrote a very simple Telegraf input plugin that would let me get information about the bus.

I feel the work is far from complete, but putting it here in case it gets more feedback!

Thanks!

Required for all PRs:

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

levex · 2017-08-29T18:50:58Z

PR /cc @bjflanne

danielnelson

Look really good @levex, maybe we should name it just "nats" in the same way we did with rabbitmq and nsq. Here are a few other quick observations, let me know when you are ready for a final review:

danielnelson · 2017-08-29T20:52:06Z

plugins/inputs/nats_top/nats_top.go

+	resp, err := http.Get(theServer)
+	if err != nil {
+		log.Printf("E! nats-top: Failed to HTTP GET %s", theServer)
+		return nil


Here you can just return the error, Telegraf will log it, increment the internal stats regarding failed metrics, and also print the name of the plugin for you.

Cool, will fix!

danielnelson · 2017-08-29T21:04:59Z

plugins/inputs/nats_top/nats_top.go

+	return "Provides metrics about the state of a NATS server"
+}
+
+func (n *NatsTop) Start(acc telegraf.Accumulator) error {


Since there is nothing to do in the Start and Stop functions you should probably not implement them.

danielnelson · 2017-08-29T21:06:59Z

plugins/inputs/nats_top/nats_top.go

+
+var sampleConfig = `
+  ## The address of the monitoring end-point of the NATS server
+  server = "http://localhost:1337"


TLS support could be nice, with the normal HTTP options such as in the apache input.

That's a great idea, will add.

levex · 2017-08-29T21:35:38Z

@danielnelson thanks for the quick review, i'll address your comments and get back to you.

Signed-off-by: Levente Kurusa <levex@linux.com>

levex · 2017-08-31T19:58:16Z

Renamed it to nats, no other modification (I hope). I think it should work with TLS if you just provide an https address and the NATS bus is configured to use TLS. (not tested)

danielnelson · 2017-08-31T20:02:47Z

The TLS options would only be needed for client certificate verification, it's not critical that it be added though so don't worry about adding it if you don't have the setup.

levex · 2017-08-31T20:17:48Z

I think I'll add it to my TODO list for now, I agree it'd be nice to have so I'll get back to it someday.

levex · 2017-09-18T17:39:40Z

Hi @danielnelson, any updates on this one?

danielnelson · 2017-09-18T21:17:00Z

I'll try to review soon

levex · 2017-09-18T21:19:24Z

Thanks! No rush, just was confused about the TLS thing.

…

On Monday, September 18, 2017, Daniel Nelson ***@***.***> wrote: I'll try to review soon — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3186 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAz09IGVBMnHTivUrL5GVbbKAHoVHWiCks5sjt3WgaJpZM4PGYYL> .

danielnelson · 2017-09-18T21:31:02Z

Don't worry about the TLS client cert support, we can add it later on.

AlexThurston · 2017-10-13T15:54:15Z

Any word on this being merged soon? I'd like to try it out.

danielnelson · 2017-10-17T22:44:52Z

plugins/inputs/nats/nats.go

+		map[string]interface{}{
+			"in_msgs":           stats.InMsgs,
+			"out_msgs":          stats.OutMsgs,
+			"uptime":            time.Since(stats.Start).Seconds(),


Let's store this in nanoseconds

Done. I've also made this more correct by using stats.Now to calculate the uptime instead of time.Since. Then uptime is the uptime at the point the metrics were generated, not when Gather was called. This also simplifies testing.

danielnelson · 2017-10-17T22:45:43Z

plugins/inputs/nats/nats.go

+			"out_bytes":         stats.OutBytes,
+			"mem":               stats.Mem,
+			"subscriptions":     stats.Subscriptions,
+		}, nil, time.Now())


Add a tag for the server using the url in the configuration, this will allow the plugin to be used multiple times if needed without conflicts.

danielnelson · 2017-10-17T22:46:56Z

plugins/inputs/nats/nats.go

+			"connections":       stats.Connections,
+			"total_connections": stats.TotalConnections,
+			"in_bytes":          stats.InBytes,
+			"cpu_usage":         stats.CPU,


Isn't this the number of cpu's used? If so I would call it either cpu to match upstream, or cpu_count.

CPU is a float32 containing CPU utilization (Cores has the number of CPU cores). I've renamed the metric to cpu to match upstream and the mem metric (memory usage).

danielnelson · 2017-10-17T22:48:33Z

plugins/inputs/nats/nats.go

+		return err
+	}
+
+	acc.AddFields("nats",


Maybe we call the measurement nats_varz in case we later want to support the other monitoring urls.

danielnelson · 2017-10-17T22:53:14Z

plugins/inputs/nats/nats.go

+		return err
+	}
+
+	acc.AddFields("nats",


Why not add in the missing numeric values from the /vars endpoint or at least the ones that change over time (no need to add port). In particular, what about slow_consumers, routes, cores, remotes? I am guessing about what some of these mean so only the ones that seem like time series data.

Good idea: slow_consumers, routes, cores, and remotes added. None of the other fields in Varz seem relevant/interesting.

danielnelson · 2017-10-17T22:59:02Z

plugins/inputs/nats/nats.go

+	theServer := fmt.Sprintf("%s/varz", n.Server)
+
+	/* download the page we are intereted in */
+	resp, err := http.Get(theServer)


Can you add in a custom transport for this plugin? The best example to follow is the apache input. You don't have to add SSL support or user configurable timeouts, but make sure there is a timeout on the http.Client of 5 seconds.

Done (exposed the timeout as an option too)

danielnelson · 2017-10-17T23:01:29Z

Godeps

@@ -33,6 +33,7 @@ github.com/kballard/go-shellquote d8ec1a69a250a17bb0e419c386eac1f3711dc142
 github.com/matttproud/golang_protobuf_extensions c12348ce28de40eed0136aa2b644d0ee0650e56c
 github.com/miekg/dns 99f84ae56e75126dd77e5de4fae2ea034a468ca1
 github.com/naoina/go-stringutil 6b638e95a32d0c1131db0e7fe83775cbea4a0d0b
+github.com/nats-io/gnatsd 393bbb7c031433e68707c8810fda0bfcfbe6ab9b


Can you also add this to docs/LICENSE_OF_DEPENDENCIES.md?

levex · 2017-10-17T23:58:50Z

Thanks for the review @danielnelson, I aim to address your comments this week. (ideally, tomorrow)

ifrins · 2017-11-29T07:51:16Z

I'm also interested in getting NATS support for telegraf merged, in fact I had made an input plugin before finding this PR. @levex do you want some help in addressing the review? or should I submit another PR?

mjs · 2018-01-15T22:33:08Z

@levex is no longer working on this but I'm working with the same employer he was working for. I'll take over and work to get it merged. I'll respond to the outstanding items here and then propose a new PR.

mjs · 2018-01-16T02:30:39Z

#3674 is the new PR which addresses the issues raised here. This PR can be closed.

danielnelson reviewed Aug 29, 2017

View reviewed changes

levex force-pushed the nats-collector-draft branch 2 times, most recently from 57cd848 to 0c05127 Compare August 31, 2017 19:51

Add a NATS monitoring input plugin

480211f

Signed-off-by: Levente Kurusa <levex@linux.com>

levex force-pushed the nats-collector-draft branch from 0c05127 to 480211f Compare August 31, 2017 19:53

danielnelson added this to the 1.5.0 milestone Sep 8, 2017

danielnelson added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Sep 8, 2017

danielnelson added the new plugin label Sep 18, 2017

danielnelson reviewed Oct 17, 2017

View reviewed changes

danielnelson modified the milestones: 1.5.0, 1.6.0 Nov 29, 2017

mjs mentioned this pull request Jan 16, 2018

NATS Monitoring Input Plugin #3674

Merged

3 tasks

danielnelson closed this Jan 16, 2018

NATS Monitoring Input Plugin #3186

NATS Monitoring Input Plugin #3186

Conversation

levex commented Aug 29, 2017

Required for all PRs:

levex commented Aug 29, 2017

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

levex commented Aug 29, 2017

levex commented Aug 31, 2017 • edited Loading

danielnelson commented Aug 31, 2017

levex commented Aug 31, 2017

levex commented Sep 18, 2017

danielnelson commented Sep 18, 2017

levex commented Sep 18, 2017 via email

danielnelson commented Sep 18, 2017

AlexThurston commented Oct 13, 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

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

levex commented Oct 17, 2017

ifrins commented Nov 29, 2017

mjs commented Jan 15, 2018

mjs commented Jan 16, 2018

levex commented Aug 31, 2017 •

edited

Loading