etcd icon indicating copy to clipboard operation
etcd copied to clipboard

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

Open ahrtr opened this issue 3 years ago • 11 comments

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 avatar Jan 02 '23 02:01 ahrtr

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%) :arrow_down:

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%) :arrow_down:
server/etcdserver/api/v3rpc/lease.go 77.21% <0.00%> (-5.07%) :arrow_down:
server/storage/mvcc/watchable_store.go 84.42% <0.00%> (-4.35%) :arrow_down:
client/v3/concurrency/session.go 85.10% <0.00%> (-4.26%) :arrow_down:
server/etcdserver/api/v3rpc/util.go 70.96% <0.00%> (-3.23%) :arrow_down:
server/etcdserver/api/v3rpc/interceptor.go 74.47% <0.00%> (-3.13%) :arrow_down:
server/proxy/grpcproxy/watch.go 93.64% <0.00%> (-2.90%) :arrow_down:
server/etcdserver/api/v3election/election.go 66.66% <0.00%> (-2.78%) :arrow_down:
client/v3/experimental/recipes/double_barrier.go 68.83% <0.00%> (-2.60%) :arrow_down:
server/etcdserver/apply/apply_auth.go 86.00% <0.00%> (-2.00%) :arrow_down:
... and 13 more

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

codecov-commenter avatar Jan 02 '23 03:01 codecov-commenter

Rebased this PR.

ahrtr avatar Jan 10 '23 23:01 ahrtr

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.

ptabor avatar Jan 11 '23 13:01 ptabor

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 avatar Jan 12 '23 07:01 ahrtr

Screen Shot 2023-01-18 at 16 12 29

The above example is exported from @tjungblu 's benchmark result in https://github.com/etcd-io/etcd/pull/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 avatar Jan 17 '23 13:01 ahrtr

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

ahrtr avatar Jan 28 '23 21:01 ahrtr

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

ahrtr avatar Apr 13 '23 09:04 ahrtr

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.

stale[bot] avatar Aug 12 '23 06:08 stale[bot]

@ahrtr: The following test 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-verify 18bb24ea43fe7283eef466616371846852e441f8 link true /test pull-etcd-verify

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/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot avatar Jan 16 '24 20:01 k8s-ci-robot

[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 avatar Jul 23 '24 19:07 k8s-ci-robot

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 avatar Jul 24 '24 04:07 k8s-ci-robot

@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 18bb24ea43fe7283eef466616371846852e441f8 link false /test pull-etcd-robustness-amd64
pull-etcd-unit-test-amd64 18bb24ea43fe7283eef466616371846852e441f8 link true /test pull-etcd-unit-test-amd64
pull-etcd-integration-2-cpu-amd64 18bb24ea43fe7283eef466616371846852e441f8 link true /test pull-etcd-integration-2-cpu-amd64
pull-etcd-integration-4-cpu-amd64 18bb24ea43fe7283eef466616371846852e441f8 link true /test pull-etcd-integration-4-cpu-amd64
pull-etcd-integration-1-cpu-amd64 18bb24ea43fe7283eef466616371846852e441f8 link true /test pull-etcd-integration-1-cpu-amd64
pull-etcd-unit-test-386 18bb24ea43fe7283eef466616371846852e441f8 link true /test pull-etcd-unit-test-386
pull-etcd-build 18bb24ea43fe7283eef466616371846852e441f8 link true /test pull-etcd-build
pull-etcd-verify 18bb24ea43fe7283eef466616371846852e441f8 link true /test pull-etcd-verify
pull-etcd-govulncheck 18bb24ea43fe7283eef466616371846852e441f8 link true /test pull-etcd-govulncheck
pull-etcd-e2e-amd64 18bb24ea43fe7283eef466616371846852e441f8 link false /test pull-etcd-e2e-amd64
pull-etcd-unit-test-arm64 18bb24ea43fe7283eef466616371846852e441f8 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.

k8s-ci-robot avatar Aug 05 '24 22:08 k8s-ci-robot