Coalesce some statistics into a single metric

[joehandzik]

Signed-Off-By: Joe Handzik joseph.t.handzik@hpe.com

Fixes #64

[joehandzik]

I fully expect this PR to need some major cleanup before merging. It's ugly and hacky, but it does work. So, thoughts on the general concept and the result that it comes up with would be appreciated.

[roclark]

I don't see anything wrong or bad about this PR, though I think we can certainly make it more efficient. As a quick brain exercise, I created this semi-pseudo function using a lot of your code in the getJobStatsByOperation function:

func getJobStatsOperation(job string, jobID string) (metricList []lustreJobsMetric, err error) {
	opList := []string{"open", "close", "mknod", "link", "unlink", "mkdir", "rmdir", "rename", "getattr", "setattr", "getxattr", "setxattr", "statfs", "sync", "samedir_rename", "crossdir_rename", "punch", "destroy", "create", "get_info", "set_info", "quotactl"}
	for operation := range opList {
		opStat := regexCaptureString(operation+": .*", jobBlock)
		if len(opStat) < 1 {
			continue
		}
		opNumbers := regexCaptureStrings("[0-9]*.[0-9]+|[0-9]+", opStat)
		if len(opNumbers) < 1 {
			continue
		}
		result, err := strconv.ParseUint(strings.TrimSpace(opNumbers[0]), 10, 64)
		if err != nil {
			return nil, err
		}
		extraLabelValue = operation
		helpText = jobStatsHelp
		l := lustreStatsMetric{
			title:           promName,
			help:            helpText,
			value:           result,
			extraLabel:      "operation",
			extraLabelValue: extraLabelValue,
		}
		metricList = append(metricList, lustreJobsMetric{jobID, l})		
	}

    return metricList, err
}

With this, we can replace parseJobStatsText with something along the lines of:

func parseJobStatsText(jobStats string, promName string, helpText string) (metricList []lustreJobsMetric, err error) {
    jobs := regexCaptureStrings("(?ms:job_id:.*?(-|\\z))", jobStats)
    if len(jobs) < 1 {
        return nil, nil
    }

    for _, job := range jobs {
        jobID, err := getJobNum(job)
        if err != nil {
            return nil, err
        }
        if helpText == jobStatsHelp {
            jobList, err := getJobStatsOperation(job, jobID)
            if err != nil {
                return nil, err
            }
        } else {
            jobList, err := getJobStatsIO(job, jobID, promName, helpText)  // <- Note, the original getJobStatsByOperation will be renamed to getJobStatsIO
            if err != nil {
                return nil, err
            }
        }
        for _, item := range jobList {
            metricList = append(metricList, item)
        }
    }
    return metricList, nil
}

The same principles would apply for the stats and md_stats files. I think this would simplify some of the logic and improve performance while reducing redundancies.

For performance, we can just parse the file once to get all metrics instead of once per metric. As in, if we have a file with 10 operations in it, with this method, we can just parse it once and move on instead of parsing it 10 times. This probably makes more sense with the metrics being bundled together anyway.

This will also help reduce redundancies by removing all of the help and type declarations for every metric. We won't need to specify every helpText for the operations, especially since we aren't even using that text anymore.

Just a few quick thoughts of mine, let me know if you have any questions or comments on this. Or, if you think we could save something like this for another PR.

[joehandzik]

This is a WIP at the moment, unit tests are going to be broken. I'll let you know when it's ready for re-review.

[joehandzik]

@roclark When you have a chance, take a look and let me know what you think. This is fully updated and representative of what I would like to merge. Feel free to be nitpicky about names and such.

[roclark]

Just a few comments here - let me know if you have any questions on them!

[roclark]

Since you aren't using raw string literals here, don't forget the double backslash before the dot since this will match any character between two numbers (e.g. 1234a5678 will return 1234a5678 instead of 1234, 5678). The pattern will look something like this: [0-9]*\\.[0-9]+|[0-9]+.

As an aside, I will cleanup and move all of these number parsers in a future PR, so it isn't too big of a deal.

[roclark]

As per #88, the wildcard is added during initialization while the maps on lines 338, 472 and 514 add the wildcard later. Do we want to follow suit and add + " .*" to the end of line 398?

[roclark]

Also, along the lines of #88, it might be a good idea to transition to the number regex pattern we use in all other functions as opposed to matching strings, both for consistency and safety. Though, this probably belongs in another PR which I can certainly pickup in the future.

[joehandzik]

I figure we can clean the non-converged stats up later in a separate PR.

[roclark]

Could be I'm off here, but shouldn't the hasMultipleVals element be true?

[joehandzik]

You are correct. Will fix.

[roclark]

Now that we are changing how these stats are displayed, do we want to keep this md_stats as opposed to just stats? The nodeType label will be mdt so we should be able to differentiate this from other stats. Mostly putting this here as a mild discussion point, not necessarily because I have an opinion.

[joehandzik]

We'll want to change it to stats instead of md_stats, IMO.

[roclark]

Whoops, I was looking at the filename instead of the metric name. Nevermind!

[roclark]

Just curious, is operation ever used or displayed anywhere? I know you decide how to handle stats metrics based on the extraLabelValue which is empty in this case, but any reason you are setting the extraLabel?

[joehandzik]

extraLabel is what we use to figure out what the label should be called. I could hard code it I suppose, but I try to avoid that.

[roclark]

Same question here as in line 416.

[joehandzik]

@roclark This is done from my perspective. Let me know if you think any of the comments you've left here are dealbreakers.

[roclark]

Looks like we just need to remove the operation label in the stats_integration_test.go file, then everything should work fine. Once done, you good to merge?

[joehandzik]

@roclark Ha, I pushed before lunch assuming I had bigger problems. Looks like it was that simple. Merge if you're ready.