search

louwrentius / fio-plot

Create charts from FIO storage benchmark tool output
BSD 3-Clause "New" or "Revised" License
370 stars 87 forks source link

Allow forcing a single column layout for the labels #140

Closed nobuto-m closed 8 months ago

nobuto-m commented 9 months ago

When the number of labels is more than 3, the tables are created with two columns. The new option allows forcing a single column. I think it depends on preferences so I made it as a config option.

[w/o the option] two_columns

[w/ the option] single_column

Also, extend the table to include median.

[before] image

[after] Screenshot from 2024-02-07 00-08-31

louwrentius commented 9 months ago

Thanks a lot, this looks good. You are on a roll.

louwrentius commented 9 months ago

Hi,

The single column parameter is awesome, no comments on that. Maybe if yo can create a separate PR for just that one I'll merge it.

The percentile change, I'd like to suggest some changes.

The key issue is that originally, only the 99.99 percentile was shown. If you don't use single-column, the default causes a broken layout if you add your own custom percentile

Comparing-multiple-queue-depths_2024-02-10_125501_zE

For layout purposes, I'd like to see the 99.99% as the default as it was. However, instead of a single parameter nargs[?], we can allow nargs[+] up to maybe 3 percentiles as this doesn't hurt the layout so you are able to specify --percentile 0.01 0.5 99.99 and it will work ok.

I don't want to make the 3 percentiles fixed, like they are now as people should have the option to select just one percentile if it hurts the layout. The single column parameter you also proposed obviously could be an alternative, but the single column table eats into the space of the actual graph, so the two-column option may sometimes be a desired compromise. This was the reason why I use a two-column table in the first place, by default.

So to summarise:

It's entirely up to you if you want to make these changes.

nobuto-m commented 9 months ago

Those comments are fair. The PR had two commits (single column option, more percentiles) but now this PR only contains the single column commit.

I will rethink about the more percentiles part and create a separate PR when it's ready.