Skip to content
This repository has been archived by the owner on Jun 28, 2024. It is now read-only.

metrics: report: % diff row seems to calculate incorrectly #709

Closed
grahamwhaley opened this issue Sep 5, 2018 · 9 comments
Closed

metrics: report: % diff row seems to calculate incorrectly #709

grahamwhaley opened this issue Sep 5, 2018 · 9 comments
Assignees

Comments

@grahamwhaley
Copy link
Contributor

Looking at some reports, it seems the % row calculation for at least the footprint might be incorrect.

If you have a look at the metrics_report.pdf attached at kata-containers/agent#353 (comment) then I think we can see the match is wrong.

The relevant code to inspect is at:
https://github.com/kata-containers/tests/blob/master/metrics/report/report_dockerfile/memory-footprint.R#L79-L92

Note - the % calculation will always have to be a '% of one entry vs the other' - presently I believe it is meant to be 'the % difference the second set of data has relative to the first set' iyswim. We'll have to keep that in mind, and how to best present it, whilst fixing the issue.

@grahamwhaley
Copy link
Contributor Author

@GabyCT - would you like to take a look at this? (it will be a nice first entree into the report generation innards as well ;-) ). Pull me in if you need some help or input.

@GabyCT
Copy link
Contributor

GabyCT commented Sep 5, 2018

@grahamwhaley let me take a look

@GabyCT
Copy link
Contributor

GabyCT commented Sep 6, 2018

@grahamwhaley , I have a question about this, in the report I see PSS footprint and results for without_seccomp_new and with_seccomp_new for noKSM and KSM so you want to calculate the percentage difference between noKSM and KSM with without_seccomp_new? or are you looking for the percentage difference of noKSM with without_seccomp_new and with_seccomp_new?

@GabyCT
Copy link
Contributor

GabyCT commented Sep 10, 2018

@grahamwhaley , I could not generate the report generation, I got an error #733

@GabyCT
Copy link
Contributor

GabyCT commented Sep 11, 2018

@grahamwhaley ping ^^^

@grahamwhaley
Copy link
Contributor Author

Ah, forgot to reply to that bit ;-)

If we grab the example from the pdf results over on kata-containers/agent#353 as an example:

Results noKSM KSM Units
without_seccomp_new 195708.9 67022.76 Kb
with_seccomp_new 346184.31 119987.6 Kb
diff −43.47 −44.14 %

Then you should be able to see that the % diff is calculated down the columns - so is comparing noKSM against noKSM and KSM against KSM - that is, we are comparing like-for-like across the two different data captures.

What I think is broken though is how we (I ;-)) calculate the actual % value and show it. In the above example, by my calculation, that first % for noKSM would look much better if it said '176.8' for instance, which is the increase of the second run over the first. Thus, I think the output example above should look more like:

Results noKSM KSM Units
without_seccomp_new 195708.9 67022.76 Kb
with_seccomp_new 346184.31 119987.6 Kb
diff 176.8 179 %

To me that gives a much more useful view. But, wdyt @GabyCT - I'm interested in what others think on how we should try to calculate and show the results.

I think the one that really triggered this in my head was from an earlier report on that other agent PR, where the table looked like:

Results noKSM KSM Units
without_seccomp 718292.77 231183.3 Kb
with_seccomp 346181.27 116786.82 Kb
diff 107.49 97.95 %

where that diff row is really confusing. I'd expect that to say something more like 50% or -50%.
I think that is the thing where we need to make the decision - would that be '50%' or '-50%'. I think it is going to have to be '50%' (that is, the proportional % of the second value wrt the first). Otherwise we will end up with a mix of + and - % values, and then it will be unclear what that means.

Still, either of those is better than what we have today I think.
Oh, and please ensure you check the other R files as well - there are % diff calculations in other pages of the report as well, and I suspect a copy/paste has made the same mistake in all of them.

thx

@GabyCT
Copy link
Contributor

GabyCT commented Sep 12, 2018

thanks @grahamwhaley for the explanation, yes the first result (first table) is kind of confusing to me. I really like the second table for me it makes the more clear the data.

@GabyCT
Copy link
Contributor

GabyCT commented Sep 26, 2018

@grahamwhaley , in order to do the calculation I will use the percentage difference formula which is
100 * |x−y| / ((x+y)/2), thanks

GabyCT added a commit to GabyCT/tests-1 that referenced this issue Sep 26, 2018
Fix the percentage difference calculation for the metrics report generation.

Fixes kata-containers#709

Signed-off-by: Gabriela Cervantes <gabriela.cervantes.tellez@intel.com>
@GabyCT
Copy link
Contributor

GabyCT commented Sep 26, 2018

@grahamwhaley , I raised this PR please let me know what do you think #775

GabyCT added a commit to GabyCT/tests-1 that referenced this issue Sep 27, 2018
Fix the percentage difference calculation for the metrics report generation.

Fixes kata-containers#709

Signed-off-by: Gabriela Cervantes <gabriela.cervantes.tellez@intel.com>
GabyCT added a commit to GabyCT/tests-1 that referenced this issue Sep 28, 2018
Fix the percentage difference calculation for the metrics report generation.

Fixes kata-containers#709

Signed-off-by: Gabriela Cervantes <gabriela.cervantes.tellez@intel.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants