ocurrent / current-bench

Experimental benchmarking infrastructure using OCurrent pipelines
Apache License 2.0
34 stars 17 forks source link

Allow specifying if a metric is ascending or descending #263

Closed punchagan closed 2 years ago

punchagan commented 2 years ago

The ascending field in the metric metadata is used to indicate whether an increase in the value is good/bad. When the metric is ascending and the value increases, or the metric is descending and the value decreases, the delta value is displayed in green. If not, the delta is displayed in red.

image

art-w commented 2 years ago

As usual this is excellent! I'm just not sure about the word ascending in the json (but I don't have a great suggestion either to evoke higher-is-better/lower-is-better)

The feature could be displayed even more prominently in the frontend! (but this can be done in follow up PRs) For example, it would be nice to have the info near the graphs of the "good" direction to facilitate reading? (as an arrow or icon if space is an issue?)

Another random idea: On mixed metrics (many lower and higher is better graphs), perhaps it is hard to switch between the modes of reading (a bit like a survey where some questions are randomly negated and you can't just assume that "yes" is the good answer). We could choose that "lower-is-always-better" and reverse the sign of the higher-is-better graphs so that reading all graphs becomes "are all the metrics visually decreasing?" (... this is probably a bit surprising and would require some works on the graphs axis to explain!)

punchagan commented 2 years ago

As usual this is excellent! I'm just not sure about the word ascending in the json (but I don't have a great suggestion either to evoke higher-is-better/lower-is-better)

Thanks for the review, @art-w. :) @gs0510 would you have any thoughts on using ascending to indicate higher-is-better metrics? I stole it from this M&E framework I used in the past.

The feature could be displayed even more prominently in the frontend! (but this can be done in follow up PRs) For example, it would be nice to have the info near the graphs of the "good" direction to facilitate reading? (as an arrow or icon if space is an issue?)

I agree on making the feature more prominent in the UI. And I punted on it, for further discussion or ideas on this PR, or for future PRs. I'll wait to see if anyone else has any ideas for this too. /cc @gs0510

gs0510 commented 2 years ago

I agree that ascending (=true) is a bit confusing for higher-is-better. Can we just call it higher-is-better (it doesn't sound as standard as ascending though).

Making it more prominent on the UI sounds good! :)) (I don't know if you want specific ideas, but starting with an arrow next to the graph sounds good :) )

art-w commented 2 years ago

Perhaps the bool is causing most of the confusion? Would a string be better?

"ascending": true, vs "direction": "ascending", (or "trend" for the key?... or "higher-is-better" and "lower-is-better" for the value)

gs0510 commented 2 years ago

Ah yes, the bool is the cause of the confusion (always 😁), a string type would definitely be better :))

punchagan commented 2 years ago

image

I've added the color indicator to the subtitle on the graphs, and also added some up/down arrows. The up and down arrows are tracking the actual deltas, while the color is based on the delta and whether the metric is higher/lower is better. I don't know if the arrows are confusing. Happy to take feedback on this.

Also, switched to a string parameter --"trend": "higher-is-better"

gs0510 commented 2 years ago

Ah nice work! The arrows are indeed confusing 😿 I think it's the problem of double negatives, because there's color + arrow direction + (+/-) so it's hard to figure out what it actually means. 😬

gs0510 commented 2 years ago

Feel free to merge this after rebase @punchagan :)

punchagan commented 2 years ago

This PR (and probably main before I rebased this PR against main?) had a problem with running make bench because of using spaces instead of tabs in the Makefile. I pushed a commit to main to fix this. It would be useful to continue to run make bench as part of the CI which we used to previously. @gs0510 did we remove this intentionally? Or is something wrong somewhere?