Fix supervisord collector #978

SuperQ · 2018-06-18T09:15:33Z

Use github.com/mattn/go-xmlrpc that doesn't leak goroutines.
Use Prometheus best practices for uptime metric.
- Use "start time" rather than "uptime".
- Don't emit a start time if the process is down.

Closes: #859

mjtrangoni · 2018-06-18T09:24:11Z

collector/supervisord.go

+
+	res, err := xmlrpc.Call(*supervisordURL, "supervisor.getAllProcessInfo")
+	if err != nil {
+		return fmt.Errorf("unable to call supervisord: %s", err)


@SuperQ Shouldn't it be log instead of fmt?

No, we're returning an error type up the stack to the top level collector logger.

SuperQ · 2018-06-18T10:50:07Z

/cc @zerkms I did some testing of supervisord locally, but I don't have a large deployment to test this on.

juliusv · 2018-06-19T11:41:54Z

collector/supervisord.go

@@ -98,7 +93,7 @@ func (c *supervisordCollector) isRunning(state int) bool {
 }

 func (c *supervisordCollector) Update(ch chan<- prometheus.Metric) error {
-	var infos []struct {
+	var info struct {
 		Name          string `xmlrpc:"name"`


Do you still need the field annotations now that you are setting these fields manually?

We don't need them, but it helps with understanding the code.

juliusv · 2018-06-19T11:49:30Z

collector/supervisord.go

+
+	for _, p := range res.(xmlrpc.Array) {
+		for k, v := range p.(xmlrpc.Struct) {
+			switch k {


Just slightly worried here that field names might change in the future or might not always be present, and then we won't even notice, struct field values will just be subtly wrong. Could we guard against that by somehow checking that all expected fields are present? Simplest would be to have a map[string]struct{} as a set, then explicitly set an entry for a field when it has been seen, then in the end count that len(myStruct) == 9, and if not, return an error?

Yea, I'm also not so happy about this way of mapping. The original library wrote directly to xml struct so we were sure the fields were mapped.

juliusv · 2018-06-19T11:49:45Z

👍 besides comment

discordianfish · 2018-06-23T11:47:59Z

CHANGELOG.md

@@ -2,11 +2,15 @@

 **Breaking changes**

+* supvervisord collector reports "start_time_seconds" rather than "uptime"


Should we also add this to the recording rules file for backward compatibility?

discordianfish · 2018-06-23T11:52:33Z

Isn't there a working xmlrpc lib which supports type annotations to map the response directly to a struct as before? That would be better.. But LGTM either way.

In general though I feel that supervisord is actually a bit to specific to be part of the node-exporter.. So we might want to consider deprecating it eventually. But if this fixes the issues, let's go with it for now.

SuperQ · 2018-06-23T11:56:52Z

@discordianfish The original one does do correct XML to struct data mapping, but it leaks goroutines/memory.

discordianfish · 2018-08-05T12:23:22Z

@SuperQ Bump, should we merge this now?

juliusv · 2018-08-05T12:55:42Z

👍 though the sanity check in https://github.com/prometheus/node_exporter/pull/978/files#r196392075 could be added right now already

discordianfish · 2018-08-06T10:42:51Z

Yeah I'm not happy to manually unmarshal this in the first place. But since this is a bug affecting users, I'd rather merge it as-is then waiting longer. Ideally we would replace this by somehing that supports struct annotations for the parsing.. Not sure why this isn't possible here though but no time to investigate right now.

juliusv · 2018-08-06T10:55:04Z

Yeah, I'm ok with merging it, just saying nothing would prevent a len() sanity check right now.

SuperQ · 2018-08-06T12:08:43Z

Yes, I agree about the len() thing. I just haven't had time to work on it.

juliusv · 2018-08-06T12:26:23Z

Well then let's just merge it for now - however, you need to fix a conflict in CHANGELOG.md.

Use `github.com/mattn/go-xmlrpc` that doesn't leak goroutines. Signed-off-by: Ben Kochie <superq@gmail.com>

Signed-off-by: Ben Kochie <superq@gmail.com>

Use Prometheus best practices for uptime metric. * Use "start time" rather than "uptime". * Don't emit a start time if the process is down. Signed-off-by: Ben Kochie <superq@gmail.com>

Signed-off-by: Ben Kochie <superq@gmail.com>

* Replace supervisord xmlrpc library * Use `github.com/mattn/go-xmlrpc` that doesn't leak goroutines. * Fix uptime metric * Use Prometheus best practices for uptime metric. * Use "start time" rather than "uptime". * Don't emit a start time if the process is down. * Add changelog entry. * Add example compatibility rules. Signed-off-by: Ben Kochie <superq@gmail.com>

SuperQ requested a review from gouthamve June 18, 2018 09:15

mjtrangoni reviewed Jun 18, 2018

View reviewed changes

juliusv reviewed Jun 19, 2018

View reviewed changes

discordianfish reviewed Jun 23, 2018

View reviewed changes

SuperQ force-pushed the superq/supervisord branch from 71c9615 to 6b35e2e Compare July 16, 2018 14:08

SuperQ mentioned this pull request Jul 18, 2018

Panic and crash when supervisord collector enabled, yet supervisord not listening on 127.0.0.1:9001 #1007

Open

SuperQ added 5 commits August 6, 2018 14:29

Replace supervisord xmlrpc library

a488904

Use `github.com/mattn/go-xmlrpc` that doesn't leak goroutines. Signed-off-by: Ben Kochie <superq@gmail.com>

Update xmlrpc vendoring.

fa64e96

Signed-off-by: Ben Kochie <superq@gmail.com>

Fix uptime metric

ed4d2bb

Use Prometheus best practices for uptime metric. * Use "start time" rather than "uptime". * Don't emit a start time if the process is down. Signed-off-by: Ben Kochie <superq@gmail.com>

Add changelog entry.

7cd2b39

Signed-off-by: Ben Kochie <superq@gmail.com>

Add example compatibility rules.

cee6cf9

Signed-off-by: Ben Kochie <superq@gmail.com>

SuperQ force-pushed the superq/supervisord branch from 6b35e2e to cee6cf9 Compare August 6, 2018 12:30

SuperQ merged commit 5d23ad0 into master Aug 6, 2018

SuperQ deleted the superq/supervisord branch August 6, 2018 14:54

SuperQ mentioned this pull request Aug 6, 2018

after enabling supervisord collector node_exporter fails after a while #972

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix supervisord collector #978

Fix supervisord collector #978

SuperQ commented Jun 18, 2018

mjtrangoni Jun 18, 2018

SuperQ Jun 18, 2018

SuperQ commented Jun 18, 2018

juliusv Jun 19, 2018

SuperQ Jul 2, 2018

juliusv Jun 19, 2018

SuperQ Jun 19, 2018

juliusv commented Jun 19, 2018

discordianfish Jun 23, 2018

SuperQ Jul 16, 2018

discordianfish commented Jun 23, 2018

SuperQ commented Jun 23, 2018

discordianfish commented Aug 5, 2018

juliusv commented Aug 5, 2018

discordianfish commented Aug 6, 2018

juliusv commented Aug 6, 2018

SuperQ commented Aug 6, 2018

juliusv commented Aug 6, 2018

		@@ -2,11 +2,15 @@

		Breaking changes

		* supvervisord collector reports "start_time_seconds" rather than "uptime"

Fix supervisord collector #978

Fix supervisord collector #978

Conversation

SuperQ commented Jun 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuperQ commented Jun 18, 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

juliusv commented Jun 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

discordianfish commented Jun 23, 2018

SuperQ commented Jun 23, 2018

discordianfish commented Aug 5, 2018

juliusv commented Aug 5, 2018

discordianfish commented Aug 6, 2018

juliusv commented Aug 6, 2018

SuperQ commented Aug 6, 2018

juliusv commented Aug 6, 2018