tests: metrics: formalise the metrics reporting methods #955

grahamwhaley · 2017-06-06T18:43:14Z

The metrics report through more than one method at present (csv files, stdout). Add a more formal and flexible method for reporting metrics results so we can then migrate all the metrics over to 'one true method'.

Fixes: #954

grahamwhaley · 2017-06-06T18:44:07Z

@MarioCarrilloA Please take a look as well.
@GabyCT You may be interested, as I intend to move all the network metrics to use this interface as one of the next actions.

chavafg · 2017-06-06T19:07:33Z

qa-passed

chavafg · 2017-06-06T21:43:37Z

tests/lib/send_results.sh

+	fi
+
+	if [ -z "$IMG" ];then
+		IMG="$(readlink "$cc_img_path/clear-containers.img")"


we can use @CONTAINERS_IMG@ to prevent hardcoding the path to /usr/share/clear-containers and add this file to list of generated files in the Makefile.am

Right. Was aware of the hardcoding. I'll look at moving to a .in file. I may pick up #945 before I do that then to make development easier (I was hoping to delay that for a bit, oh well ;-)

chavafg · 2017-06-06T21:44:00Z

tests/lib/send_results.sh

+	fi
+
+	if [ -z "$KERNEL" ];then
+		KERNEL="$(readlink "$cc_img_path/vmlinux.container")"


same as above, we can use @CONTAINER_KERNEL@

Hi @chavafg , if we use @CONTAINER_KERNEL@ we are forced to build the project in order to expand that variable right?, I think the PnP tools should run independently the project is build or not, what do you think?

I think that is true @MarioCarrilloA , and I am coding a bit of 'backup' into the script right now - but, the ultimate code would locate exactly what runtime the test ran with, and then extract the info about the img file and kernel from that runtime setup. Sadly, we are some way from coding that up today - so, today we will go with @xxx@, and 'adapt'. It may be for now that the person or system who runs the tests may have to set some ENV or command line vars to point to the correct img and kernel files.

@MarioCarrilloA yes, you would need to build the project. For usability, I would say that you are right, you shouldn't need build the whole project for just running the metrics.
But right now other scripts needed here depend from building the project, so you will still need to run make metrics-tests.

See #945 the intention being that a simple make target (intermediate step) be introduced so we can just generate the pre-processed files without having to build the whole project.

jodh-intel · 2017-06-07T06:49:02Z

tests/lib/send_results.sh

@@ -0,0 +1,202 @@
+#!/bin/bash
+


This file is missing a license header.

jodh-intel · 2017-06-07T06:50:51Z

tests/lib/send_results.sh

+
+function save_to_csv()
+{
+	runtime="/usr/bin/cc-oci-runtime"


This path could be incorrect (CentOS for example).

jodh-intel · 2017-06-07T06:51:18Z

tests/lib/send_results.sh

+function save_to_csv()
+{
+	runtime="/usr/bin/cc-oci-runtime"
+	release_file="/usr/lib/os-release"


This path is Clear Linux-specific I think? That files lives in /etc/ on Ubuntu+Fedora for example.

This path works forfor Ubuntu, Fedora and Clear Linux. Seems that CentOS does not have it.

... and of course if we only check /etc/, it won't work for clr ;-( It's almost like this needs to be a getent database... ;-) So maybe special-case CentOS (as it's the oldest)?

https://www.freedesktop.org/software/systemd/man/os-release.html

jodh-intel · 2017-06-07T06:51:38Z

tests/lib/send_results.sh

+	cc_img_path="/usr/share/clear-containers"
+
+	if [ -z "$TEST_NAME" ];then
+		echo "error: test name no found"


Redirect all these error messages to stderr?

s/test name no/test name not/

In fact, if this script sourced test-common.bash, you could just call:

die "test name not found"

jodh-intel · 2017-06-07T06:54:38Z

tests/metrics/README.md

-This will run all metrics tests and generete a `results` directory.
+This will run all metrics tests and place the results in the `lib/metrics/results` directory.
+
+Each file in the `results` directory contains the result for a test as a comma separated values (CSV).


s/comma separated values/comma separated value/

jodh-intel · 2017-06-07T07:06:08Z

tests/lib/send_results.sh

+	fi
+
+	if [ -z "$UNITS" ];then
+		echo "error: units no found"


s/no found/not found/.

jodh-intel · 2017-06-07T07:08:51Z

tests/lib/send_results.sh

+
+	if [ -z "$TAG" ];then
+		if [ -f "$runtime" ];then
+			commit="$(cc-oci-runtime -v | tail -n1 | sed s/"commit: "//g)"


Do we envisage this script even moving to the tests repo? If so, it would be worth creating a variable for the runtime (so we could switch it to cc-runtime and this test would then become:

$runtime -v|grep "commit.*:"|cut -d: -f2-|tr -d ' '

(works for cc-oci-runtime and cc-runtime).

jodh-intel · 2017-06-07T07:12:23Z

tests/lib/send_results.sh

+	CSV_FILE=${RESULT_DIR}/$(echo ${TEST_NAME} | sed 's/ /-/g').csv
+
+	# Are we dry-running?
+	if [ -z "$SEND" ];then


I like the idea of having a dry-run mode, but I'm not really clear what it buys us here. I think it would just perform the checks? It might be worth having it "almost" run the code below by showing would run '...' using an echo $cmd and if not in dry-run mode, running eval $cmd instead.

jodh-intel · 2017-06-07T07:14:52Z

tests/lib/send_results.sh

+			echo "Timestamp,Group,Name,Args,Result,Units,System,SystemVersion,Platform,Image,Kernel,Commit" > ${CSV_FILE}
+		fi
+
+		s1=$(echo "$timestamp,$GROUP,$TEST_NAME,$ARGS,$RESULT,$UNITS,$SYSTEM,$SYS_VERSION,$platform,$IMG,$KERNEL,$commit")


Strictly, the code should handle commas in these variables - either error out or quote the values. A simple check might be to echo $1 and count the commas to ensure we have the expected number of fields.

jodh-intel · 2017-06-07T07:15:14Z

tests/lib/send_results.sh

+	 This tool will save results to csv
+	 files.
+   Options:
+	 -h   Help page


It would be good if this list of options was sorted alphabetically 😄

jodh-intel · 2017-06-07T07:17:50Z

Oh - could you add that Fixes: #954 into one of the commits? That will soon be required (once #952 lands).

grahamwhaley · 2017-06-07T09:17:38Z

@jodh-intel thanks for the thorough review - the script was pulled in from another project. I did some mods and cleanups, but looks like there is a bunch more to do to meet our requirements :-) Let me get on that and send a new version soon.
Yeah, I mean to add the 'fixes'... thanks for picking that up as well.

grahamwhaley · 2017-06-07T10:04:08Z

Jodh - oh, iswym with wanting the fixes in the commit, not the merge/PR message. Hmm, I can - but, personally it feels a bit 'wrong', on a couple of fronts:

In a multi-commit series, the Fix is really spread across all of them, so you would have to put the fixes comment on the last commit. It feels 'better' in/on the PR/merge message to me
I don't particularly like putting github specific info into the code commits themselves - that sort of ties our git repo directly to github - which, sure, we may never have an intention of moving anywhere else - but, it just feels wrong to me.

Just noting like. I'll follow suit and add the fixes at the next update.

jodh-intel · 2017-06-07T10:27:12Z

@grahamwhaley - I can understand that, but -- I think -- for most the workflow is easier if you do have a "Fixes:" comment in one of the commits since you don't need to adjust the PR text and the wokflow becomes minimally:

push branch
click to raise a PR
have github auto-close the issue for you when the PR is merged.

I take the point about a stack of commits, but I'd tend to mark those that contribute to fixing an issue as "partially fixes #XXX". Again, I understand not wanting to encode github specifics in the branch, but as shown above, it is useful to use a "Fixes:" comment to make life easier for developers. I think it's not unreasonable to have close ties between the version control system and change control systems. The final point is that if we only put the fixes on the PR itself, checkcommits can't perform the fixes check so wouldn't add value there.

Flipping it round, I'd say we want to minimise the amount of extra detail we add to a PR description since that isn't stored as history in the git branch.

grahamwhaley · 2017-06-08T13:59:34Z

@jodh-intel @chavafg all feedback should have been addressed in the new version.

grahamwhaley · 2017-06-08T14:00:02Z

and then I spot I've acquired a Makefile.am conflict... so, expect a rebase and another push...

chavafg · 2017-06-08T14:23:33Z

qa-passed

chavafg · 2017-06-08T14:47:16Z

qa-passed

iphutch · 2017-06-08T18:26:58Z

Looks good from a docs perspective. Awaiting Makefile.am conflict resolution to approve.

jodh-intel · 2017-06-09T07:39:56Z

tests/lib/send_results.sh.in

+		# This is somewhat imperfect. Ideally we'd have knowledge passed
+		# in from the test itself about which runtime it used so we could
+		# be certain to have the correct runtime and extract the correct
+		# commit id


Worth capturing in an issue?

Indeed. Added #973

jodh-intel · 2017-06-12T09:23:15Z

lgtm

grahamwhaley · 2017-06-12T09:37:33Z

@chavafg any chance you can re-review so we can move this on? thanks.

chavafg · 2017-06-12T13:31:35Z

tests/lib/test-common.bash.in

+		die "save_results() requires 4 parameters, not $#"
+	fi
+
+	$LIB_DIR/send_results.sh -n "$1" -a "$2" -r "$3" -u "$4"


I tested the PR and when calling this line I got:

/root/go/src/github.com/01org/cc-oci-runtime/tests/metrics/density/../../lib/test-common.bash: line 80: /root/go/src/github.com/01org/cc-oci-runtime/tests/metrics/density/../../lib/send_results.sh: Permission denied

It can be fixed with:

bash $LIB_DIR/send_results.sh -n "$1" -a "$2" -r "$3" -u "$4"

ah, indeed, good spot. A side effect of the .in file processing. My local version got chmod'd during test, and then that 'stuck'. thx. will fix and update.

iphutch

lgtm

Add a new function (save_results) for the metrics tests to migrate to use, which then calls out to a script to do the any actual processing and saving of the results. This is the first step on formalising, documenting and unifying the results reporting method across all the metrics tests. Signed-off-by: Graham Whaley <[email protected]>

Add the send_results.sh.in script that saves the metrics results into csv files under the tests/metrics/results dir. Also add it to the generated files list so it can be pre-processed into a runnable script. Signed-off-by: Graham Whaley <[email protected]>

Now we have some new metrics reporting infrastructure, update the README to expand on that, enabling further use, enhancement and modifications. Fixes: intel#954 Signed-off-by: Graham Whaley <[email protected]>

Move from using the old 'cvs file' reporting functions to the new save_results call. Signed-off-by: Graham Whaley <[email protected]>

grahamwhaley · 2017-06-16T16:05:17Z

Updated @chavafg @jodh-intel.

added the bash invocation of the sub script
Also added translation of '/' chars to '-'s in the result file name generation (I realised we are quite likely to hit this when some tests have arguments such as '/bin/true'
Fixed a single trailing whitespace typo in the README.

jodh-intel · 2017-06-16T16:18:03Z

lgtm

chavafg · 2017-06-16T16:43:26Z

qa-passed

chavafg · 2017-06-16T16:52:52Z

lgtm

grahamwhaley added the in progress label Jun 6, 2017

grahamwhaley requested review from iphutch and chavafg June 6, 2017 18:46

chavafg reviewed Jun 6, 2017

View reviewed changes

jodh-intel reviewed Jun 7, 2017

View reviewed changes

grahamwhaley force-pushed the 20170605_metrics_rewrite_csv branch from d402eee to 74161ae Compare June 8, 2017 13:58

grahamwhaley force-pushed the 20170605_metrics_rewrite_csv branch from 74161ae to 60d8b0e Compare June 8, 2017 14:11

jodh-intel approved these changes Jun 9, 2017

View reviewed changes

chavafg reviewed Jun 12, 2017

View reviewed changes

iphutch approved these changes Jun 15, 2017

View reviewed changes

grahamwhaley added the do-not-merge label Jun 16, 2017

Graham Whaley added 4 commits June 16, 2017 16:16

tests: metrics: Update README with details of new infra

35de59e

Now we have some new metrics reporting infrastructure, update the README to expand on that, enabling further use, enhancement and modifications. Fixes: intel#954 Signed-off-by: Graham Whaley <[email protected]>

tests: metrics: density: Use the new metrics reporting function

5b83071

Move from using the old 'cvs file' reporting functions to the new save_results call. Signed-off-by: Graham Whaley <[email protected]>

grahamwhaley force-pushed the 20170605_metrics_rewrite_csv branch from 60d8b0e to 5b83071 Compare June 16, 2017 16:02

grahamwhaley removed the do-not-merge label Jun 16, 2017

chavafg merged commit 7b3a743 into intel:master Jun 16, 2017

tests: metrics: formalise the metrics reporting methods #955

tests: metrics: formalise the metrics reporting methods #955

Conversation

grahamwhaley commented Jun 6, 2017

grahamwhaley commented Jun 6, 2017

chavafg commented Jun 6, 2017 • edited by dlespiau Loading

chavafg Jun 6, 2017 • edited Loading

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

jodh-intel commented Jun 7, 2017

grahamwhaley commented Jun 7, 2017

grahamwhaley commented Jun 7, 2017

jodh-intel commented Jun 7, 2017

grahamwhaley commented Jun 8, 2017

grahamwhaley commented Jun 8, 2017

chavafg commented Jun 8, 2017

chavafg commented Jun 8, 2017

iphutch commented Jun 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodh-intel commented Jun 12, 2017

grahamwhaley commented Jun 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iphutch left a comment

Choose a reason for hiding this comment

grahamwhaley commented Jun 16, 2017

jodh-intel commented Jun 16, 2017

chavafg commented Jun 16, 2017

chavafg commented Jun 16, 2017 • edited by dlespiau Loading

chavafg commented Jun 6, 2017 •

edited by dlespiau

Loading

chavafg Jun 6, 2017 •

edited

Loading

chavafg commented Jun 16, 2017 •

edited by dlespiau

Loading