search

ocurrent / current-bench

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

Fix adjust size metrics functionality when units are not lowercase #441

Closed punchagan closed 1 year ago

punchagan commented 1 year ago

The size metric regex is case insensitive, but when looking inside the sizeUnits array, the comparison is not case-insensitive and this causes the size units adjusting code to crash when upper-case units are used. This commit fixes the bug by making the search for a benchmark's unit in the sizeUnits array to be case-insensitive.

Fixes the crash in the UI with this benchmark: https://autumn.ocamllabs.io/ocaml-multicore/eio/pull/500/base/main/benchmark/Eio?worker=fermat&image=ocaml%2Fopam%3Adebian-ocaml-5.0

ElectreAAS commented 1 year ago

Nice catch on that bug, but I don't understand anything about it. Why are units case-insensitive? Something being "13kB" or "13kb" isn't the same thing, right? Shouldn't everything be case-sensitive?

As to why that crashes, I don't understand rescript or what we're doing in that specific piece of code. I'll count myself off as a reviewer. You can either ask art-w or merge it by yourself @punchagan if you think this PR doesn't need a full knowledgable review

punchagan commented 1 year ago

Why are units case-insensitive? Something being "13kB" or "13kb" isn't the same thing, right? Shouldn't everything be case-sensitive?

That is correct. I was just trying to fix the current crash, so that Eio developers can see the stats. But, I will send a better fix with some tests addressing the questions you raised. Thanks for your review, @ElectreAAS !

punchagan commented 1 year ago

@ElectreAAS I've updated the PR to fix a number of small bugs with how we adjust the size units. I've also added a bunch of UI tests, which needed some work-arounds to get jest to work correctly with our setup.

I've noticed that you prefer to squash commits when merging, but I think it's worth keeping these commits separate, since I've fixed one issue in each commit and every commit keeps the UI functional. Let me know if you'd like me to rearrange the changes across commits differently.

ElectreAAS commented 1 year ago

Thanks for the update, this looks great! Just as a clarification, we now treat 15 mbps the same as 15 MBps right? I wanted to uselessly bicker on about milli- vs mega- and bits vs bytes (or worse, megabytes vs mebibytes), but it doesn't matter at all here, as we expect people to use one coherent set of units The commit splitup is great as-is I think

punchagan commented 1 year ago

Just as a clarification, we now treat 15 mbps the same as 15 MBps right?

Yes, we treat bits and bytes differently. 15 mB and 15 mb differently, but we don't accommodate for milli, etc. I've not seen that used with size units, usually. https://github.com/ocurrent/current-bench/blob/main/frontend/__tests__/adjust_metric_unit.res#L18-L20

These are the sizeUnits we handle, currently: ["kb", "mb", "gb", "tb", "pb", "eb", "zb", "yb"]

punchagan commented 1 year ago

Thanks for the review and merge, @ElectreAAS !