Skip to content
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

[RFC] Add rw heatmaps line chart #19030

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

ivanvc
Copy link
Member

@ivanvc ivanvc commented Dec 7, 2024

I've been working on this on and off, and I wanted to get it moving before the end of this year.

It implements line charts for the "rw-heatmaps." I want to get some feedback on the resulting charts. We should also rename tools/rw-heatmaps to something that suits both heatmaps and line charts.

I'll squash commits as we get closer to merging this.

Supersedes #15060.

/cc @ahrtr @jmhbnz

Signed-off-by: Ivan Valdes <[email protected]>
Signed-off-by: Ivan Valdes <[email protected]>
Signed-off-by: Ivan Valdes <[email protected]>
Signed-off-by: Ivan Valdes <[email protected]>
Signed-off-by: Ivan Valdes <[email protected]>
Signed-off-by: Ivan Valdes <[email protected]>
Signed-off-by: Ivan Valdes <[email protected]>
Signed-off-by: Ivan Valdes <[email protected]>
Signed-off-by: Ivan Valdes <[email protected]>
@k8s-ci-robot
Copy link

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ivanvc

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ivanvc
Copy link
Member Author

ivanvc commented Dec 7, 2024

Given the following files:

Running it with:

go run . result-20240* -t test-line -o test-line -c line

Plots:
test-line_readwrite

Copy link

codecov bot commented Dec 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.75%. Comparing base (675c2e3) to head (aed1d42).
Report is 6 commits behind head on main.

Additional details and impacted files

see 26 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #19030      +/-   ##
==========================================
+ Coverage   68.71%   68.75%   +0.04%     
==========================================
  Files         420      420              
  Lines       35623    35623              
==========================================
+ Hits        24478    24493      +15     
+ Misses       9715     9706       -9     
+ Partials     1430     1424       -6     

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 675c2e3...aed1d42. Read the comment docs.

@ivanvc
Copy link
Member Author

ivanvc commented Dec 7, 2024

Running it for a single dataset, i.e., result-202402281701.csv

go run . result-202402281701.csv -t test-line -o test-line -c line

Generates:

test-line_readwrite

Signed-off-by: Ivan Valdes <[email protected]>
@ivanvc
Copy link
Member Author

ivanvc commented Dec 8, 2024

Another alternative is to have the value size represent the line width, and set dashes for the second dataset, which looks like this for an individual dataset:

test-line_readwrite

And like this for multiple datasets:

test-line_readwrite

@ahrtr
Copy link
Member

ahrtr commented Dec 8, 2024

Thanks @ivanvc for the progress.

As compared to my previous solution in #15060, the good side / bad side of this PR is exactly the bad side / good side of my previous solution.

This PR,

  • good side: get everything included in one jpg file. So easy to share in PR discussion.
  • bad side: the data density is too big and accordingly less clearer.

Probably we can find a compromise or combine the best aspects of both.

  • We still present different R/W ratio in different line charts. It's the same as what this PR does.
  • For each specific RW ratio,
    • There are 4 kinds of color, we can simplify it to just 2 colors; i.e. red --> write, and blue --> read. If there are two datasets, then present the second on in dot dash line, and with the same color as the first one. The key is to create a simple rule to remember:

      • red for write and blue for read, dot dash for the second dataset;
        • dataset 1 write: solid red line
        • dataset 1 read: solid blue line
        • dataset 2 write: dot dash red line
        • dataset 2 read: dot dash blue line
      • or solid line for write and dot dash line for read, and red for first data set and blue for the second dataset.
        • dataset 1 write: solid red line
        • dataset 1 read: dot dash red line
        • dataset 2 write: solid blue line
        • dataset 2 read: dot dash blue line

      This should be easy to do. But I agree that this may involve some degree of personal subjectivity

    • present the diagram(line chart) in html page instead of jpg file, so that we can highlight the data of a specific value size (or hide the data of some value sizes). For example, we still present the result of all value sizes (i.e. including 256, 512, 1024, 2048) in one line chart, but users can hide the result for value sizes 512, 1024 and 2048, so only 256 is displayed, so as to make it clearer and more readable. Note this is one of the key good sides of my previous solution. This might be a little hard based on the library this PR selects.

Signed-off-by: Ivan Valdes <[email protected]>
@ivanvc
Copy link
Member Author

ivanvc commented Dec 10, 2024

I agree that the chart is too dense. We would benefit from an interactive format (i.e., your solution/d3js, etc.). The problem with an HTML file is that it's less portable (and embeddable) than an image. If we go that route, we should host it somewhere (i.e., Netlify), but it adds to the complexity of the implementation.

Implementing your suggestions:

test-line_readwrite

or

test-line_readwrite

@ahrtr
Copy link
Member

ahrtr commented Dec 10, 2024

The problem with an HTML file is that it's less portable (and embeddable) than an image. If we go that route, we should host it somewhere (i.e., Netlify), but it adds to the complexity of the implementation.

You can export each line chart into a .PNG file, refer to #15060 (comment).

You can also create a screen shot for all line charts, see the picture in #15060 (comment).

We can also zip the html file and share it in the PR discussion.

Implementing your suggestions:

It's a little better now, but it's really hard to differentiate the lines for different value sizes.

Signed-off-by: Ivan Valdes <[email protected]>
@ahrtr
Copy link
Member

ahrtr commented Dec 13, 2024

red for write and blue for read, dot dash for the second dataset;

  • dataset 1 write: solid red line
  • dataset 1 read: solid blue line
  • dataset 2 write: dot dash red line
  • dataset 2 read: dot dash blue line

I suggest to get this merged firstly, and consider to improve it later. Please mark this PR ready for review it's done, please also provide a final image file (line charts). thx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants