Add input plugin for monit #6850

SirishaGopigiri · 2020-01-03T06:12:06Z

closes #230

Required for all PRs:

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

danielnelson · 2020-01-06T22:38:44Z

plugins/inputs/monit/monit.go

+	Address           string
+	BasicAuthUsername string
+	BasicAuthPassword string
+	parser            parsers.Parser


Remove parser, unused.

Done. Please check.

danielnelson · 2020-01-06T22:40:11Z

plugins/inputs/monit/monit.go

+  basic_auth_username = ""
+  basic_auth_password = ""


Call these basic_username and basic_password, or just username and password. This is to keep in line with existing plugins.

Done. Please check.

danielnelson · 2020-01-06T22:41:38Z

plugins/inputs/monit/monit.go

+func (m *Monit) SetParser(parser parsers.Parser) {
+	m.parser = parser
+}


Remove, unused.

Done. Please check.

danielnelson · 2020-01-06T22:42:43Z

plugins/inputs/monit/monit.go

+	Address           string
+	BasicAuthUsername string
+	BasicAuthPassword string


On all the exported fields, add a struct tag for the toml:

Address string `toml:"address"`

Done. Please check.

danielnelson · 2020-01-06T22:45:28Z

plugins/inputs/monit/monit.go

+	decoder := xml.NewDecoder(resp.Body)
+	decoder.CharsetReader = charset.NewReaderLabel
+	if err := decoder.Decode(&status); err != nil {
+		return fmt.Errorf("Cannot parse input with error: %v\n", err)


Adjust error string to match Go style guideline, the format string should be something like:

"error parsing input: %v"

https://github.com/golang/go/wiki/CodeReviewComments#error-strings

Done. Please check.

danielnelson · 2020-01-06T23:35:58Z

plugins/inputs/monit/monit.go

+		} else if service.Type == "7" {
+			fields["last_started_time"] = service.Program.Started * 1000
+			fields["program_status"] = service.Program.Status
+			fields["output"] = service.Program.Output


Is this the program output? If so we should probably skip this item due to potential length. If we would like to keep it then we need to convert/verify that the output is UTF-8 encoded and that the output isn't too large.

Done. Please check

danielnelson · 2020-01-06T23:38:59Z

plugins/inputs/monit/monit.go

+			fields["mem_kb_total"] = service.Memory.KilobyteTotal
+			fields["mem_percent"] = service.Memory.Percent
+			fields["mem_percent_total"] = service.Memory.PercentTotal
+			fields["service_uptime"] = service.Uptime


Make time units consistent with other time fields.

Done. Please check

danielnelson · 2020-01-06T23:41:07Z

plugins/inputs/monit/monit.go

+func monitoringMode(s Service) string {
+	switch s.MonitorMode {
+	case 0:
+		return "Monitoring in Active mode"


I suggest we make these strings consistent with the Monit web interface, in mine:

Monitoring status: Monitored Monitoring mode: active

We should try to be consistent like this in any string conversions.

Done. Please check

danielnelson · 2020-01-06T23:43:15Z

plugins/inputs/monit/monit.go

+		fields["status"] = serviceStatus(service)
+		fields["monitoring_status_code"] = service.MonitoringStatus
+		fields["monitoring_status"] = monitoringStatus(service)
+		fields["monitoring_mode_status"] = monitoringMode(service)


monitoring_mode to match web interface

Done. Please check

danielnelson · 2020-01-06T23:43:49Z

plugins/inputs/monit/monit.go

+		fields["status_code"] = service.Status
+		fields["status"] = serviceStatus(service)
+		fields["monitoring_status_code"] = service.MonitoringStatus
+		fields["monitoring_status"] = monitoringStatus(service)


Let's make status, monitoring status and monitoring mode as tags.

Done. Please check

Aligning code to telegraf standards and added some more testcases Co-authored-by: sirishagopigiri <[email protected]> Co-authored-by: susancoombs <[email protected]>

…iri/telegraf into feature/monitplugin

Co-authored-by: SirishaGopigiri <[email protected]> Co-authored-by: susancoombs <[email protected]>

plugins/inputs/monit/monit.go

danielnelson

Here are a few more ideas, mostly around naming, that I think we should do.

danielnelson · 2020-01-16T22:52:45Z

plugins/inputs/monit/monit_test.go

+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+


When setting up the Monit HTTPD server, you can use the ALLOW option to limit who can connect to the server. Let's add a test if possible and make sure we provide a good error message should the plugin hit this case.

https://mmonit.com/monit/documentation/monit.html#TCP-PORT

danielnelson · 2020-01-17T00:53:08Z

plugins/inputs/monit/monit.go

+	} else if s.Link.Duplex == 0 {
+		return "Simplex Mode"
+	} else {
+		return "Unknown Mode"


Have the strings be: "duplex", "simplex", "unknown"

danielnelson · 2020-01-17T00:53:38Z

plugins/inputs/monit/monit.go

+	case 1:
+		return "Monitoring mode:  passive"
+	}
+	return "Monitoring mode: unknown"


Have the strings be "active", "passive", "unknown". This is consistent with the terminology of Monit without being too repetitive with the tag key.

danielnelson · 2020-01-17T00:54:05Z

plugins/inputs/monit/monit.go

+	case 4:
+		return "Monitoring status:  Waiting"
+	}
+	return "Monitoring status:  Not monitored"


"monitored", "initializing", "waiting", "not_monitored"

danielnelson · 2020-01-17T01:04:17Z

plugins/inputs/monit/monit.go

+func serviceStatus(s Service) string {
+	var status string
+
+	if s.MonitoringStatus == 0 || s.MonitoringStatus == 2 {


To avoid combining the meaning of multiple fields, we should just directly report from the status. So this function could return "running" or "failure".

plugins/inputs/monit/monit.go

danielnelson · 2020-01-17T01:15:01Z

plugins/inputs/monit/monit.go

+		tags := map[string]string{"address": m.Address,
+			"version":       status.Server.Version,
+			"hostname":      status.Server.LocalHostname,
+			"platform_name": status.Platform.Name}


Remove the address tag, and rename the hostname tag to "source". This will be consistent with #4413.

danielnelson · 2020-01-17T02:10:52Z

plugins/inputs/monit/monit.go

+				fields["swap_percent"] = service.System.Swap.Percent
+				acc.AddFields("monit_system", fields, tags)
+			} else if service.Type == fifo {
+				fields["permissions"] = service.Mode


Call this mode

danielnelson · 2020-01-17T02:11:38Z

plugins/inputs/monit/monit.go

+				fields["permissions"] = service.Mode
+				acc.AddFields("monit_fifo", fields, tags)
+			} else if service.Type == program {
+				fields["last_started_time"] = service.Program.Started * 10000000


Call this field program_started

danielnelson · 2020-01-17T02:11:48Z

plugins/inputs/monit/monit.go

+				acc.AddFields("monit_directory", fields, tags)
+			} else if service.Type == file {
+				fields["size"] = service.Size
+				fields["permissions"] = service.Mode


Call this field mode

Co-authored-by: SirishaGopigiri <[email protected]> Co-authored-by: susancoombs <[email protected]>

reimda · 2020-01-22T21:48:05Z

plugins/inputs/monit/README.md

+- monitoring_status
+- monitoring_mode
+
+### Measurements & Fields:


L31-33 looks like leftover boilerplate that needs to be deleted. Measurements and Fields are covered below

reimda · 2020-01-22T21:49:15Z

plugins/inputs/monit/README.md

+- monitoring_mode_code 
+
+### Measurement & Fields:
+Fields for Monit service type Filesystem:


Filesystem shouldn't be capitalized if other service types aren't

reimda · 2020-01-22T23:33:50Z

plugins/inputs/monit/monit.go

+	Timeout internal.Duration `toml:"timeout"`
+}
+
+type HTTPClient interface {


I like that you mock http responses, but there's a simpler way to do it that is built in to http.Client. Before calling your http.Client's .Do(), you set its Transport to a custom http.RoundTripper.

I'd prefer to remove this HTTPClient interface and use a custom RoundTripper because it requires less code and because it's standard Go so it will be easier for whoever works on this code later.

https://golang.org/pkg/net/http/#RoundTripper

Thank you for the comments Reimda. I have written some sample code around RoundTripper. Could you please check?

type transportMock struct {
errorString string
}

func newTransportMock(errorString string) http.RoundTripper {
return &transportMock{
errorString: errorString,
}
}

func (t *transportMock) RoundTrip(r *http.Request) (*http.Response, error) {
return nil, errors.New(t.errorString)
}

func TestAllowHosts(t *testing.T) {

networkError := "Get http://127.0.0.1:2812/_status?format=xml: " + "read tcp 192.168.10.2:55610->127.0.0.1:2812: " + "read: connection reset by peer" r := &Monit{ Address: "http://127.0.0.1:2812", Username: "test", Password: "test", client: &http.Client{} } var acc testutil.Accumulator r.client.Transport = newTransportMock(networkError) err := r.Gather(&acc) if assert.Error(t, err) { assert.Contains(t, err.Error(), "read: connection reset by peer") }

}

That's the idea. Also change Monit.client to be a http.Client, not a *http.Client, so you don't have to explicitly initialize it. I'd probably make type transportMock an empty struct and put the networkError string right in the RoundTrip func. Then you don't need newTransportMock either.

Looks like we already merged this PR. If you want to make a new PR to change this, feel free to add me as a reviewer.

thanks

Thank you for the comments. I have raised a new PR. Could you please review?
#6955

SirishaGopigiri added 5 commits January 3, 2020 11:33

Adding Monit Input Plugin

9cc6292

Adding Monit Input Plugin

72b77e1

Adding Monit Input Plugin

31d44de

Fix fmt for Monit Input Plugin

b899d1f

Fix struct ppid for Monit Input Plugin

55adda4

sjwang90 requested review from reimda and danielnelson January 6, 2020 17:16

danielnelson suggested changes Jan 6, 2020

View reviewed changes

danielnelson added the new plugin label Jan 6, 2020

danielnelson changed the title ~~Feature/monitplugin~~ Add input plugin for monit Jan 6, 2020

danielnelson mentioned this pull request Jan 7, 2020

Monit Input Plugin #2028

Closed

3 tasks

SirishaGopigiri changed the title ~~Add input plugin for monit~~ [WIP] Add input plugin for monit Jan 7, 2020

Sirisha Gopigiri and others added 4 commits January 10, 2020 12:39

Monit plugin changes

ddcb71d

Aligning code to telegraf standards and added some more testcases Co-authored-by: sirishagopigiri <[email protected]> Co-authored-by: susancoombs <[email protected]>

Monit plugin changes

7d2c080

Aligning code to telegraf standards and added some more testcases Co-authored-by: sirishagopigiri <[email protected]> Co-authored-by: susancoombs <[email protected]>

Merge branch 'feature/monitplugin' of https://github.com/SirishaGopig…

296164e

…iri/telegraf into feature/monitplugin

Fix timestamp issues

36fdc3a

Co-authored-by: SirishaGopigiri <[email protected]> Co-authored-by: susancoombs <[email protected]>

SirishaGopigiri commented Jan 10, 2020

View reviewed changes

plugins/inputs/monit/monit.go Show resolved Hide resolved

sjwang90 added this to the 1.14.0 milestone Jan 13, 2020

sjwang90 requested a review from danielnelson January 16, 2020 17:42

danielnelson reviewed Jan 17, 2020

View reviewed changes

Added Testcases and changes as per comments

2b01b4d

Co-authored-by: SirishaGopigiri <[email protected]> Co-authored-by: susancoombs <[email protected]>

SirishaGopigiri changed the title ~~[WIP] Add input plugin for monit~~ Add input plugin for monit Jan 22, 2020

SirishaGopigiri requested a review from danielnelson January 22, 2020 05:06

danielnelson approved these changes Jan 22, 2020

View reviewed changes

danielnelson merged commit 9fd400c into influxdata:master Jan 22, 2020

reimda suggested changes Jan 22, 2020

View reviewed changes

athoune pushed a commit to bearstech/telegraf that referenced this pull request Apr 17, 2020

Add input plugin for monit (influxdata#6850)

91a6b2d

idohalevi pushed a commit to idohalevi/telegraf that referenced this pull request Sep 29, 2020

Add input plugin for monit (influxdata#6850)

db04927

arstercz pushed a commit to arstercz/telegraf that referenced this pull request Mar 5, 2023

Add input plugin for monit (influxdata#6850)

15b6c56

Add input plugin for monit #6850

Add input plugin for monit #6850

Conversation

SirishaGopigiri commented Jan 3, 2020 • edited by danielnelson Loading

Required for all PRs:

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

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 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

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

SirishaGopigiri Jan 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SirishaGopigiri commented Jan 3, 2020 •

edited by danielnelson

Loading

SirishaGopigiri Jan 23, 2020 •

edited

Loading