Fix nagios parser to parse all perfdata and report info message.

[sergeykhegay]

Required for all PRs:

• Signed CLA.
• Associated README.md updated. Not needed.
• Has appropriate unit tests.

Example Nagios check output:

DISK OK - free space: / 3326 MB (56%); | /=2643MB;5948;5958;0;5968
/ 15272 MB (77%);
/boot 68 MB (69%);
/home 69357 MB (27%);
/var/log 819 MB (84%); | /boot=68MB;88;93;0;98
/home=69357MB;253404;253409;0;253414
/var/log=818MB;970;975;0;980

Old behavior:

nagios_state,host=localhost state=0i 1552677172000000000
nagios,host=localhost,perfdata=/,unit=MB critical_gt=5958,critical_lt=0,max=5968,min=0,value=2643,warning_gt=5948,warning_lt=0 1552677172000000000
nagios,host=localhost,perfdata=/boot,unit=MB critical_gt=93,critical_lt=0,max=98,min=0,value=68,warning_gt=88,warning_lt=0 1552677172000000000

New behavior:

nagios,host=localhost,perfdata=/,unit=MB critical_gt=5958,critical_lt=0,max=5968,min=0,value=2643,warning_gt=5948,warning_lt=0 1552939211000000000
nagios,host=localhost,perfdata=/boot,unit=MB critical_gt=93,critical_lt=0,max=98,min=0,value=68,warning_gt=88,warning_lt=0 1552939211000000000
nagios,host=localhost,perfdata=/home,unit=MB critical_gt=253409,critical_lt=0,max=253414,min=0,value=69357,warning_gt=253404,warning_lt=0 1552939211000000000
nagios,host=localhost,perfdata=/var/log,unit=MB critical_gt=975,critical_lt=0,max=980,min=0,value=818,warning_gt=970,warning_lt=0 1552939211000000000
nagios_state,host=localhost longmsg="/ 15272 MB (77%); /boot 68 MB (69%); /home 69357 MB (27%); /var/log 819 MB (84%);",msg="DISK OK - free space: / 3326 MB (56%);",state=0i 1552939211000000000

Changes:

• Lines for perfdata in {/home, /var/log} added.
• Info message and long info message are reported for the nagios_state.

Fixes #5591.

[sergeykhegay]

@glinton, could you review please?

[sergeykhegay]

Hi @danielnelson , @glinton is there any update on this? Is it being reviewed or any ETA? Regards, Sergey.

[danielnelson]

I'll try to do a review later today

[danielnelson]

I'm not sure I like injecting the return code into the parser data, is there a reason this needs to be changed?

[sergeykhegay]

I feel like the state, service_output, long_service_output semantically belong together and should be a part of the same measurement.

The current parser interface doesn't allow to pass any extra information to the parser, whilst the service_output and the long_service_output are known during the parsing time only.

The parser will properly parse bytes for which the nagios.AppendExitCode(...) wasn't called, it will assume the default state to be 0.

[danielnelson]

It's not my primary concern but wouldn't the state be set incorrectly depending on how the output ends? Perhaps we could add the state to the telegraf.Metric after it has been returned from the parser?

[sergeykhegay]

• Because we encode the state in the exec plugin, no matter how the output ends, the parser will always properly decode the state and strip those additional bytes.

  There might be an issue, when the parser is used somewhere else. And it is my bad, because I did not properly document the behavior of the Parse method. I will add the docs, but first, could you please comment on the next point:

• Appending the error code after we return from the parser is definitely possible. But will require me to do something like this:

// To return the exit code through the error handling mechanism.
//
type exitCode int

func (c exitCode) Error() string {}

// The callee changes
//
func (c CommandRunner) Run(
	e *Exec,
	command string,
	acc telegraf.Accumulator,
) ([]byte, error) {
	<...irrelevant...>

	out = <output from the check>

	if isNagiosParser {
		exitCode, err := nagios.GetExitCode(nagiosErr)
		if err != nil {
			return nil, fmt.Errorf("exec: get exit code: %s", err)
		}
		return out.Bytes(), exitCode(exitCode)
	}

	return out.Bytes(), nil
}

// The caller changes
//
func (e *Exec) ProcessCommand(command string, acc telegraf.Accumulator, wg *sync.WaitGroup) {
	defer wg.Done()

	out, err := e.runner.Run(e, command, acc)
        var exitCode int
	if err != nil {
		if v, ok := err.(exitCode); ok {
			exitCode = int(v)
		} else {
			acc.AddError(err)
			return
		}
	}
      

	metrics, err := e.parser.Parse(out)
	if err != nil {
		acc.AddError(err)
		return
	}
	
        if _, ok := e.parser.(*nagios.NagiosParser); ok {
                // Find the metric with name == "nagios_state" and append the "state" field to it.
                // Can be a convention that it is the last metric, or iterate through all and find it.
               m := <somehow find the metric>
               m.AddField("state", exitCode)
        }
        
        for _, metric := range metrics {
		acc.AddFields(metric.Name(), metric.Fields(), metric.Tags(), metric.Time())
	}
}

• I am relying on your expertise here. Which approach seem to be better for you?

[sergeykhegay]

@danielnelson, what is your take on this ^^?

[danielnelson]

To be sure, neither solution is great, and if I could do it over then I would probably make nagios it's own plugin separate from exec and scrap the standalone parser.

Putting that aside though, since I don't think it is worth a breaking change... I would parse, then if this is the NagiosParser, get the error code and iterate over the metrics looking for the nagios_state metric using metric.Name(). You will also want to watch for the case where the state metric is not found, and add the field as a new metric.

One more thing, could you do me a favor and use this form for adding the metrics?

        for _, metric := range metrics {
		acc.AddMetric(metric)
	}

[sergeykhegay]

Hi @danielnelson, commented and made some changes. Do you think it is good to go now?

[sergeykhegay]

Hi @danielnelson, addressed your comments.

One of the test builds fails though, it seems to be something finicky with the require library. Have you seen something similar before?

There is a new version of it, I'll check if they have related changes there.

[danielnelson]

I think the tests fails because the ordering in a map in Go is unspecified, so they can't be safely compared, try using the helper functions in testutil/metric.go.

[sergeykhegay]

Fixed the tests. But this is very weird. Golang's reflect.DeepEqual should have handled this correctly. And the require library seems to use it (just skimmed the code).

Please let me know what you think of the changes, @danielnelson.

[danielnelson]

Looks great, thanks!