Doc: provide more clarify to the usage of check perf command #14111

patrocinio · 2022-06-13T21:26:59Z

This PR describes the fact that different workloads in the check perf command
are different, and the results might vary.

Related #13455

serathius

cc @spzala @ahrtr

ptabor · 2022-06-14T08:01:05Z

etcdctl/ctlv3/command/check.go

@@ -126,7 +126,7 @@ func NewCheckPerfCommand() *cobra.Command {
 	}

 	// TODO: support customized configuration
-	cmd.Flags().StringVar(&checkPerfLoad, "load", "s", "The performance check's workload model. Accepted workloads: s(small), m(medium), l(large), xl(xLarge)")
+	cmd.Flags().StringVar(&checkPerfLoad, "load", "s", "The performance check's workload model. Accepted workloads: s(small), m(medium), l(large), xl(xLarge). Different workload models use different configurations in terms of number of clients and expected throughtput.")


The load flag controls:

number of concurrent clients (50-1000)

maximal rate of issued put requests (across all clients) (150-15000)

The check to succeed expects server's avg. effective throughput to be >90% of the issued requests and latency of all the requests <0.5s.

It indeed means "expected throughput" to me. We set an expected throughput, then measure the real throughput.

For example, if the actual throughput is less than 90% of the expected throughput, then the test fails.

Probably it makes more sense to clearly document the standard on the pass of test.

throughput > 90%

s.Slowest <= 0.5

s.Stddev <= 0.1

Please update the doc etcdctl/README.md to get this documented.

check.go#L242-L254

I like the document as such as a concise doc. Adding more details in the README is also a good idea. Also, another thought is to add more doc in the command function to clarify how it works - and then just point to that in the readme. In case, in future, we add new threshold or change in the existing values then we don't have to update doc. Thanks @patrocinio

Thanks @spzala for the feedback.

add more doc in the command function to clarify how it works - and then just point to that in the readme

This might not work. When we update the source code, the link might be out of date. So the best approach would be to get all detailed included in the README directly instead of just adding a link.

@ahrtr thanks and yes I agree with you! It's more about pointing to the function, not a particular LOC. ReadMe sounds good to me.

ptabor · 2022-06-14T08:02:00Z

etcdctl/README.md

@@ -1505,6 +1505,10 @@ CHECK provides commands for checking properties of the etcd cluster.

 CHECK PERF checks the performance of the etcd cluster for 60 seconds. Running the `check perf` often can create a large keyspace history which can be auto compacted and defragmented using the `--auto-compact` and `--auto-defrag` options as described below.

+Notice that different workload models use different configurations in terms of number of clients and expected throughtput.


Please reference:

etcd/etcdctl/ctlv3/command/check.go

Lines 53 to 60 in fc69053

var checkPerfCfgMap = map[string]checkPerfCfg{

// TODO: support read limit

"s": {

limit: 150,

clients: 50,

duration: 60,

},

"m": {

ahrtr

LGTM

But I'd leave @ptabor to merge this PR, in case he has any different opinion.

ahrtr

Please see files#r896538376

spzala

Thanks @patrocinio lgtm with one inline comment.

spzala · 2022-06-14T16:58:18Z

etcdctl/ctlv3/command/check.go

@@ -126,7 +126,7 @@ func NewCheckPerfCommand() *cobra.Command {
 	}

 	// TODO: support customized configuration
-	cmd.Flags().StringVar(&checkPerfLoad, "load", "s", "The performance check's workload model. Accepted workloads: s(small), m(medium), l(large), xl(xLarge)")
+	cmd.Flags().StringVar(&checkPerfLoad, "load", "s", "The performance check's workload model. Accepted workloads: s(small), m(medium), l(large), xl(xLarge). Different workload models use different configurations in terms of number of clients and expected throughtput.")


I like the document as such as a concise doc. Adding more details in the README is also a good idea. Also, another thought is to add more doc in the command function to clarify how it works - and then just point to that in the readme. In case, in future, we add new threshold or change in the existing values then we don't have to update doc. Thanks @patrocinio

etcdctl/README.md

codecov-commenter · 2022-06-16T23:12:23Z

Codecov Report

Merging #14111 (5cc91ef) into main (fc69053) will decrease coverage by 0.23%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main   #14111      +/-   ##
==========================================
- Coverage   75.23%   75.00%   -0.24%     
==========================================
  Files         452      452              
  Lines       36781    36775       -6     
==========================================
- Hits        27674    27583      -91     
- Misses       7373     7448      +75     
- Partials     1734     1744      +10

Flag	Coverage Δ
all	`75.00% <100.00%> (-0.24%)`	⬇️

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

Impacted Files	Coverage Δ
etcdctl/ctlv3/command/check.go	`12.61% <100.00%> (ø)`
server/etcdserver/api/rafthttp/peer_status.go	`87.87% <0.00%> (-12.13%)`	⬇️
server/etcdserver/api/rafthttp/peer.go	`87.01% <0.00%> (-8.45%)`	⬇️
client/pkg/v3/tlsutil/tlsutil.go	`83.33% <0.00%> (-8.34%)`	⬇️
client/v3/namespace/watch.go	`87.87% <0.00%> (-6.07%)`	⬇️
raft/rafttest/node.go	`95.00% <0.00%> (-5.00%)`	⬇️
server/etcdserver/api/v3rpc/member.go	`93.54% <0.00%> (-3.23%)`	⬇️
server/etcdserver/cluster_util.go	`70.35% <0.00%> (-3.17%)`	⬇️
server/etcdserver/api/v3rpc/interceptor.go	`74.47% <0.00%> (-3.13%)`	⬇️
server/etcdserver/api/v3rpc/watch.go	`85.90% <0.00%> (-2.69%)`	⬇️
... and 24 more

Continue to review full report at Codecov.

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

ahrtr · 2022-06-16T23:28:09Z

Looks good to me. Please squash the commits, thx.

This PR describes the fact that different workloads in the `check perf` command are different, and the results might vary. Signed-off-by: Eduardo Patrocinio <epatro@gmail.com>

patrocinio · 2022-06-17T16:18:17Z

Hey @ahrtr I squashed the commits. Thanks!

ahrtr

LGTM

Thank you! @patrocinio

ahrtr · 2022-06-17T23:03:26Z

@patrocinio Can you please backport this PR to 3.5?

patrocinio · 2022-06-17T23:22:55Z

Hello @ahrtr Do you mean release v3.5.0 (https://github.com/etcd-io/etcd/releases/tag/v3.5.0) or v3.5.3 (https://github.com/etcd-io/etcd/releases/tag/v3.5.3) ? Thanks

ahrtr · 2022-06-18T05:02:51Z

@patrocinio Please cherry pick this PR to release-3.5.

serathius · 2022-06-20T08:51:35Z

I'm not sure if we should do a backport as we usually backport only fixes. Is there any benefit from backporting documentation update?

ahrtr · 2022-06-20T09:01:52Z

Yes, it isn't a big deal. But I'd prefer to backport it, because it's useful when users run etcdctl 3.5 and read the 3.5 doc.

serathius · 2022-06-20T09:24:39Z

I mean we have a official backport policy that lists The [backport] commits should be restricted to bug fixes and security patches.
https://github.com/etcd-io/etcd/blob/3cde98ca8cacddecba2bf6864f5f8fae0bbdab2d/Documentation/contributor-guide/release.md#patch-version-release

I would prefer to avoid breaking our own policies without having a strong incentive.

serathius · 2022-06-21T09:01:31Z

Removing backport label for now. Feel free to re-add it when you get support from other maintainers.

patrocinio force-pushed the check_perf_doc branch from 26011ae to 7526b9b Compare June 13, 2022 21:32

serathius approved these changes Jun 14, 2022

View reviewed changes

ptabor reviewed Jun 14, 2022

View reviewed changes

ahrtr approved these changes Jun 14, 2022

View reviewed changes

ahrtr requested changes Jun 14, 2022

View reviewed changes

spzala approved these changes Jun 14, 2022

View reviewed changes

ahrtr reviewed Jun 16, 2022

View reviewed changes

etcdctl/README.md Outdated Show resolved Hide resolved

patrocinio force-pushed the check_perf_doc branch from 2e6495f to 5cc91ef Compare June 16, 2022 23:16

Doc: provide more clarify to the usage of check perf command

e1e3ed1

This PR describes the fact that different workloads in the `check perf` command are different, and the results might vary. Signed-off-by: Eduardo Patrocinio <epatro@gmail.com>

patrocinio force-pushed the check_perf_doc branch from 5cc91ef to e1e3ed1 Compare June 17, 2022 16:15

ahrtr approved these changes Jun 17, 2022

View reviewed changes

ahrtr added the stage/merge-when-tests-green label Jun 17, 2022

ahrtr merged commit 43434af into etcd-io:main Jun 17, 2022

ahrtr added the backport/v3.5 label Jun 17, 2022

serathius removed the backport/v3.5 label Jun 21, 2022

serathius mentioned this pull request Jun 21, 2022

Plans for v3.5.5 release #14138

Closed

16 tasks

spzala removed the stage/merge-when-tests-green label Jun 29, 2022

serathius mentioned this pull request Sep 21, 2022

check perf: pass on (XL) but fail on (L) load #13455

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Doc: provide more clarify to the usage of check perf command #14111

Doc: provide more clarify to the usage of check perf command #14111

patrocinio commented Jun 13, 2022 •

edited

Loading

serathius left a comment

ptabor Jun 14, 2022 •

edited

Loading

ahrtr Jun 14, 2022

ahrtr Jun 14, 2022 •

edited

Loading

spzala Jun 14, 2022

ahrtr Jun 14, 2022

spzala Jun 15, 2022

ptabor Jun 14, 2022 •

edited

Loading

ahrtr left a comment

ahrtr left a comment

spzala left a comment

spzala Jun 14, 2022

codecov-commenter commented Jun 16, 2022 •

edited

Loading

ahrtr commented Jun 16, 2022

patrocinio commented Jun 17, 2022

ahrtr left a comment

ahrtr commented Jun 17, 2022

patrocinio commented Jun 17, 2022

ahrtr commented Jun 18, 2022

serathius commented Jun 20, 2022

ahrtr commented Jun 20, 2022

serathius commented Jun 20, 2022

serathius commented Jun 21, 2022 •

edited

Loading

		@@ -1505,6 +1505,10 @@ CHECK provides commands for checking properties of the etcd cluster.

		CHECK PERF checks the performance of the etcd cluster for 60 seconds. Running the `check perf` often can create a large keyspace history which can be auto compacted and defragmented using the `--auto-compact` and `--auto-defrag` options as described below.

		Notice that different workload models use different configurations in terms of number of clients and expected throughtput.

	var checkPerfCfgMap = map[string]checkPerfCfg{
	// TODO: support read limit
	"s": {
	limit: 150,
	clients: 50,
	duration: 60,
	},
	"m": {

Doc: provide more clarify to the usage of check perf command #14111

Doc: provide more clarify to the usage of check perf command #14111

Conversation

patrocinio commented Jun 13, 2022 • edited Loading

serathius left a comment

Choose a reason for hiding this comment

ptabor Jun 14, 2022 • edited Loading

Choose a reason for hiding this comment

ahrtr Jun 14, 2022

Choose a reason for hiding this comment

ahrtr Jun 14, 2022 • edited Loading

Choose a reason for hiding this comment

spzala Jun 14, 2022

Choose a reason for hiding this comment

ahrtr Jun 14, 2022

Choose a reason for hiding this comment

spzala Jun 15, 2022

Choose a reason for hiding this comment

ptabor Jun 14, 2022 • edited Loading

Choose a reason for hiding this comment

ahrtr left a comment

Choose a reason for hiding this comment

ahrtr left a comment

Choose a reason for hiding this comment

spzala left a comment

Choose a reason for hiding this comment

spzala Jun 14, 2022

Choose a reason for hiding this comment

codecov-commenter commented Jun 16, 2022 • edited Loading

Codecov Report

ahrtr commented Jun 16, 2022

patrocinio commented Jun 17, 2022

ahrtr left a comment

Choose a reason for hiding this comment

ahrtr commented Jun 17, 2022

patrocinio commented Jun 17, 2022

ahrtr commented Jun 18, 2022

serathius commented Jun 20, 2022

ahrtr commented Jun 20, 2022

serathius commented Jun 20, 2022

serathius commented Jun 21, 2022 • edited Loading

patrocinio commented Jun 13, 2022 •

edited

Loading

ptabor Jun 14, 2022 •

edited

Loading

ahrtr Jun 14, 2022 •

edited

Loading

ptabor Jun 14, 2022 •

edited

Loading

codecov-commenter commented Jun 16, 2022 •

edited

Loading

serathius commented Jun 21, 2022 •

edited

Loading