-
-
Notifications
You must be signed in to change notification settings - Fork 518
Mirrored Input/Output Graph Mode #1701
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
I'm not a fan of this way of changing code. Expect for the addition of a new meter mode, I have different opinions on how the code can be done. Firstly:
We're now displaying two different kinds of data for the DiskIO meter. I suggest splitting the meter into two. The meter named For NetworkIO meter, there's no need to split. Second. I don't like the A few issues: (a) it works with My vision was the And there are two more minor things:
|
No objection on cherry-picking c7c0cda … That one can go right in AFAICS. Though I had to do some minor adjustments. |
d7120dc
to
e01e11e
Compare
Regarding the name, I'm not particular focused on the name and looking at what these kinds of graphs are commonly referred to Regarding the approach: Splitting the DiskIO meter into one for utilization and another for bandwidth might be an option; but as you see in my PR: That's just a matter of updating these meters to either support this graph style or not. The major issue I really see with #714 and thus also #1415 is the very huge upfront code change that introduces quite complex code. OTOH this PR comes with very little up-front changes and even the changes it makes are easy to understand. The time it took me to write this PR is roughly the same it took me to go through #714's dot placing function; and while I'm kinda confident with the code in this PR I won't really say the same for #714. |
@BenBE Did you consider incorporating the "dynamic scaling" part of #714? I think that part would be needed for your changes here. Specifically these:
I'm not sure if I need to file a separate PR for that. The reason is something to do with the "total" value: The DiskIO and NetworkIO meters you are working on do not have a maximum of the read and write speeds. With the above patches, you can have "dynamic total" values that work consistently across different kinds of meters. |
This extends the existing graph mode to allow for vertically mirrors graphs representing input and output rates.
This basically implements the idea from #1390 based on the #1387 that enables only specific meters to implement these additional modes.
@Explorer09 This is what I meant with code re-use … Most of the added code is from additional constants, with most of the existing code re-used with only minor changes.
NB: There's some known bugs as I needed to slightly change the memory layout for the
Meter->values
data storage for the DiskIO/NetworkIO meters, which causes issues in bar meter mode.