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

Reimplement the tool rw-heatmaps using golang and rename it to rw-benchmark #15060

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ahrtr
Copy link
Member

@ahrtr ahrtr commented Jan 2, 2023

Points:

  • The original plot_data.py is out of maintainance.
  • Remove the dependency on python and related python modules.
  • It isn't a best practice to compare benchmark results of two difference branches (e.g. main vs dev) in two charts. Instead, it's better to display the benchmarks to be compared in one chart.

Please see the example HTML page at rw_benchmark.html. One of the line charts is below,

etcd rw benchmark (RW Ratio 0 500000, Value Size 16)

The read QPS is displayed as blue, and write QPS is displayed as red. The data in the second CSV file is rendered as dashed line if present. Note each line in the line chart can be hidden or displayed by clicking on the related legend.

Note: although there are 2.5K+ lines of change, actually only tools/rw-benchmark/plot_data.go and the tools/rw-benchmark/README.md need to be carefully reviewed.

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

@ahrtr ahrtr force-pushed the go_chart_20230101 branch 3 times, most recently from 29c64dd to d2ec177 Compare January 2, 2023 02:35
@codecov-commenter
Copy link

codecov-commenter commented Jan 2, 2023

Codecov Report

Merging #15060 (d908fe4) into main (ff89864) will decrease coverage by 0.11%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main   #15060      +/-   ##
==========================================
- Coverage   74.68%   74.56%   -0.12%     
==========================================
  Files         415      415              
  Lines       34288    34288              
==========================================
- Hits        25609    25568      -41     
- Misses       7044     7079      +35     
- Partials     1635     1641       +6     
Flag Coverage Δ
all 74.56% <ø> (-0.12%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
client/pkg/v3/fileutil/purge.go 68.85% <0.00%> (-6.56%) ⬇️
server/etcdserver/api/v3rpc/lease.go 77.21% <0.00%> (-5.07%) ⬇️
server/storage/mvcc/watchable_store.go 84.42% <0.00%> (-4.35%) ⬇️
client/v3/concurrency/session.go 85.10% <0.00%> (-4.26%) ⬇️
server/etcdserver/api/v3rpc/util.go 70.96% <0.00%> (-3.23%) ⬇️
server/etcdserver/api/v3rpc/interceptor.go 74.47% <0.00%> (-3.13%) ⬇️
server/proxy/grpcproxy/watch.go 93.64% <0.00%> (-2.90%) ⬇️
server/etcdserver/api/v3election/election.go 66.66% <0.00%> (-2.78%) ⬇️
client/v3/experimental/recipes/double_barrier.go 68.83% <0.00%> (-2.60%) ⬇️
server/etcdserver/apply/apply_auth.go 86.00% <0.00%> (-2.00%) ⬇️
... and 13 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ahrtr ahrtr requested review from ptabor and tjungblu January 4, 2023 02:18
@ahrtr ahrtr force-pushed the go_chart_20230101 branch 5 times, most recently from 67c34c6 to d908fe4 Compare January 10, 2023 23:38
@ahrtr
Copy link
Member Author

ahrtr commented Jan 10, 2023

Rebased this PR.

@ptabor
Copy link
Contributor

ptabor commented Jan 11, 2023

I'm definitely for having this uniform in go. But I'm not convinced the new presentation form has sufficient information density.

Previous representation has 3 advantages:

  1. as a single generated picture could be easily pasted into PR discussion. Linking to HTML is difficult as you need to host them somewhere. I think it's very important from usability perspective.
    [the rest is aesthetics]
  2. it showed percentage advantage of one version of the other vs. the absolute values [at least the right chart I was looking at usually]
  3. it was more clear that we compare the 'discrete' dimensions. The current lines chart creates impression we have continuous observations in the range of 32->4096 connections. To some degree scatter plot (or at least marking the measured points would address that). But this also creates opportunity to make the charts more narrow.
  4. [here I'm less sure] Having the charts more narrow (20 pixel * 8=160pixel) would allow to place multiple side charts in a single row (Value size is currently 7, but even if it grow to 15, it would be manageable 4k pixels). Than we would encode 3 dimensions on something that can fit on a large screen instead of 'keep scrolling'.

The PR seems to be very huge due to 'incremental' commits. I think there were not many comments so squashing it should not be net negative and more convencing to read it e2e.

@ahrtr ahrtr force-pushed the go_chart_20230101 branch 3 times, most recently from 9c27b86 to 2146477 Compare January 12, 2023 07:15
@ahrtr
Copy link
Member Author

ahrtr commented Jan 12, 2023

Thanks @ptabor for the feedback. Please see my response below,

The PR seems to be very huge due to 'incremental' commits. I think there were not many comments so squashing it should not be net negative and more convencing to read it e2e.

I squashed the commits into one. Although the PR looks huge (2K+ line of change), actually only tools/rw-benchmark/plot_data.go and tools/rw-benchmark/README.md need to be carefully reviewed. The three files under directory tools/rw-benchmark/example are just example CSV files and a generated HTML page.

4. [here I'm less sure] Having the charts more narrow (20 pixel * 8=160pixel) would allow to place multiple side charts in a single row (Value size is currently 7, but even if it grow to 15, it would be manageable 4k pixels). Than we would encode 3 dimensions on something that can fit on a large screen instead of 'keep scrolling'.

I added more flags so that users can customize the charts, see usage below,

$ ./rw-benchmark  -h
rw-benchmark is a tool for visualize etcd read-write performance result.

Usage:
    rw-benchmark [options] result-file1.csv [result-file2.csv]

Additional options:
    -legend: Comma separated names of legends, such as "main,pr", defaults to "1" or "1,2" depending on the number of CSV files provided.
    -layout: The layout of the page, valid values: none, center and flex, defaults to "flex".
    -width: The width(pixel) of the each line chart, defaults to 600.
    -height: The height(pixel) of the each line chart, defaults to 300.
    -o: The HTML file name in which the benchmark data will be rendered, defaults to "rw_benchmark.html".
    -h: Print usage.

The example file tools/rw-benchmark/example/rw_benchmark.html was generated using command go run plot_data.go ./example/main.csv ./example/dev.csv, see screen shot below,

Screen Shot 2023-01-12 at 15 16 20

  • it showed percentage advantage of one version of the other vs. the absolute values [at least the right chart I was looking at usually]

  • it was more clear that we compare the 'discrete' dimensions. The current lines chart creates impression we have continuous observations in the range of 32->4096 connections. To some degree scatter plot (or at least marking the measured points would address that). But this also creates opportunity to make the charts more narrow.

Actually I think the new version (delivered by this PR) is clearer and accordingly better to me. It gets data from both CSV files rendered in one chart, previously it was rendered in two separate charts. Previously it had to be discrete because it had three dimension in one chart (e.g. X axis is connections, Y (left) is value size, Y(right) is QPS), so there is no way to display a line for each data source. In the new version, we can render both line charts and display data as table directly. Please open and explore the example HTML file tools/rw-benchmark/example/rw_benchmark.html, you will love it.

  1. as a single generated picture could be easily pasted into PR discussion. Linking to HTML is difficult as you need to host them somewhere. I think it's very important from usability perspective.
    [the rest is aesthetics]

Yes, this is partially true to me. Indeed previously only two images were generated, but the data density is too big and I don't think it's clearer. I think many people only care about the rightest chart to get the overall comparison result.

The new version (delivered in this PR) can also export images, but one image for one line chart. We can zip the HTML file and share it to github, and export a couple of images for better discussion in github. I agree this might be a little inconvinence, because users have to download the zip file and open the HTML file locally.

I'm definitely for having this uniform in go.

Yes, this is the main reason why I deliver this PR.

@ahrtr ahrtr force-pushed the go_chart_20230101 branch 2 times, most recently from 1325c6a to 3283a4c Compare January 17, 2023 12:32
@ahrtr
Copy link
Member Author

ahrtr commented Jan 17, 2023

Screen Shot 2023-01-18 at 16 12 29

The above example is exported from @tjungblu 's benchmark result in #14877. The blue lines are read QPS, while the red lines are write QPS. The solid lines are results tested against main branch, and the dash lines are tested against his PR.

Note:

  1. It's easy to hide or show each line by clicking right side legend;
  2. It's easy to export an image for each line chart from the generated HTML page;
  3. Users can also display the original data in a table by click the button on top right side of each line chart.

We can also improve the way to plot the data in separate PR.

@ahrtr
Copy link
Member Author

ahrtr commented Jan 28, 2023

I just removed the example (to be added in a separate PR) to make this PR smaller and therefore less scary.

Reasons:
1. The original plot_data.py is out of maintainance.
2. Remove the dependency on python and related python modules.
3. It isn't a best practice to compare benchmark results of two
   difference branches (e.g. main vs dev) in two charts. Instead,
   it's better to display the benchmarks to be compared in one chart.

Signed-off-by: Benjamin Wang <wachao@vmware.com>
@ahrtr
Copy link
Member Author

ahrtr commented Apr 13, 2023

Is there any interest on this new tool? cc @mitake @ptabor @serathius @spzala

@stale
Copy link

stale bot commented Aug 12, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 21 days if no further activity occurs. Thank you for your contributions.

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr

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

@k8s-ci-robot
Copy link

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot
Copy link

@ahrtr: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-etcd-robustness-amd64 18bb24e link false /test pull-etcd-robustness-amd64
pull-etcd-unit-test-amd64 18bb24e link true /test pull-etcd-unit-test-amd64
pull-etcd-integration-2-cpu-amd64 18bb24e link true /test pull-etcd-integration-2-cpu-amd64
pull-etcd-integration-4-cpu-amd64 18bb24e link true /test pull-etcd-integration-4-cpu-amd64
pull-etcd-integration-1-cpu-amd64 18bb24e link true /test pull-etcd-integration-1-cpu-amd64
pull-etcd-unit-test-386 18bb24e link true /test pull-etcd-unit-test-386
pull-etcd-build 18bb24e link true /test pull-etcd-build
pull-etcd-verify 18bb24e link true /test pull-etcd-verify
pull-etcd-govulncheck 18bb24e link true /test pull-etcd-govulncheck
pull-etcd-e2e-amd64 18bb24e link false /test pull-etcd-e2e-amd64
pull-etcd-unit-test-arm64 18bb24e link true /test pull-etcd-unit-test-arm64

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

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.

5 participants