tools/benchmark: add --output-format flag for JSON output#21920
tools/benchmark: add --output-format flag for JSON output#21920Somesh42 wants to merge 4 commits into
Conversation
The benchmark tool currently outputs only human-readable text, making it difficult to consume results programmatically or track trends over time. Add a global --output-format flag (values: text, json). When json is selected, the report package's new FormatJSON function serialises all stats - throughput, latency histogram, percentiles, and error distribution - as indented JSON to stdout. Default behaviour is unchanged. Signed-off-by: Somesh kumar <someshkumargauda@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Somesh42 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @Somesh42. Thanks for your PR. I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
|
cc @kishen-v |
| @@ -0,0 +1,130 @@ | |||
| // Copyright 2015 The etcd Authors | |||
There was a problem hiding this comment.
Nit: across all the newly added files - tools/benchmark/cmd/output.go and pkg/report/json_test.go
| // Copyright 2015 The etcd Authors | |
| // Copyright 2026 The etcd Authors |
There was a problem hiding this comment.
Fixed in both tools/benchmark/cmd/output.go and pkg/report/json_test.go, updated to 2026
- Fix copyright year to 2026 in newly added files - Fix deadlock caused by r.Run()/r.Stats() being called after close(r.Results()); printReport now starts draining before wg.Wait() - Extend --output-format flag to all benchmark subcommands (lease, range, stm, txn_put, watch, watch_get) - Add optional prefix support to printReport for watch commands Tested locally with --total=100, both text and json output formats complete successfully with no deadlock. Signed-off-by: Somesh kumar <someshkumargauda@gmail.com>
| bc := 10 | ||
| buckets := make([]float64, bc+1) | ||
| counts := make([]int, bc+1) | ||
| bs := (s.Slowest - s.Fastest) / float64(bc) | ||
| for i := 0; i < bc; i++ { | ||
| buckets[i] = s.Fastest + bs*float64(i) | ||
| } | ||
| buckets[bc] = s.Slowest | ||
| var bi int | ||
| for i := 0; i < len(s.Lats); { | ||
| if s.Lats[i] <= buckets[bi] { | ||
| i++ | ||
| counts[bi]++ | ||
| } else if bi < len(buckets)-1 { | ||
| bi++ | ||
| } | ||
| } |
There was a problem hiding this comment.
This section is mostly same as histogram(), apart from tracking max for the bar-chart normalisation. Can we have a helper function in that case?
There was a problem hiding this comment.
@kishen-v extracted the shared bucketing logic into a new bucketLatencies() helper that both histogram() and FormatJSON() now call.
Verified with a clean test run:
`etcd % go clean -testcache && go test ./pkg/report/... -v
=== RUN TestFormatJSON_ValidJSON
--- PASS: TestFormatJSON_ValidJSON (0.00s)
=== RUN TestFormatJSON_ScalarFields
--- PASS: TestFormatJSON_ScalarFields (0.00s)
=== RUN TestFormatJSON_PercentileKeys
--- PASS: TestFormatJSON_PercentileKeys (0.00s)
=== RUN TestFormatJSON_ErrorsIncluded
--- PASS: TestFormatJSON_ErrorsIncluded (0.00s)
=== RUN TestFormatJSON_ErrorsOmittedWhenEmpty
--- PASS: TestFormatJSON_ErrorsOmittedWhenEmpty (0.00s)
=== RUN TestFormatJSON_Deterministic
--- PASS: TestFormatJSON_Deterministic (0.00s)
=== RUN TestFormatJSON_EmptyStats
--- PASS: TestFormatJSON_EmptyStats (0.00s)
=== RUN TestPercentiles
--- PASS: TestPercentiles (0.00s)
=== RUN TestReport
--- PASS: TestReport (0.00s)
=== RUN TestWeightedReport
--- PASS: TestWeightedReport (0.00s)
=== RUN TestGetTimeseries
--- PASS: TestGetTimeseries (0.00s)
PASS
ok go.etcd.io/etcd/pkg/v3/report 0.658s
| // printReport writes benchmark statistics to stdout in the format chosen by | ||
| // --output-format. All sub-commands should call this function instead of | ||
| // printing report.Run() output directly so that JSON support is automatic. | ||
| func printReport(r report.Report, prefixes ...string) func() { |
There was a problem hiding this comment.
Looks like prefixes are not actually passed anywhere. Can you please take a look?
There was a problem hiding this comment.
thanks. Confirmed all call sites use printReport(r) with no extra args, so prefixes was always "" in practice and removed the parameter entirely rather than wiring it up, since there's no current caller that needs it.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Somesh42 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
…aram Signed-off-by: Somesh kumar <someshkumargauda@gmail.com>
99ca7ea to
0b488ef
Compare
| } | ||
|
|
||
| rc := r.Run() | ||
|
|
There was a problem hiding this comment.
Nit: May need to check new line additions across tools/benchmark/cmd/watch_get.go, tools/benchmark/cmd/watch.go, tools/benchmark/cmd/txn_put.go, tools/benchmark/cmd/stm.go and tools/benchmark/cmd/range.go,tools/benchmark/cmd/lease.go and
go fmt against tools/benchmark/cmd/output.go
There was a problem hiding this comment.
okay... so ran gofmt -w across all the flagged files. Fixed some stray blank-line and import-ordering issues in output.go, plus trailing blank lines in the six caller files. Pushed in c9225bf.
| // printReport writes benchmark statistics to stdout in the format chosen by | ||
| // --output-format. All sub-commands should call this function instead of | ||
| // printing report.Run() output directly so that JSON support is automatic. | ||
| func printReport(r report.Report) func() { |
There was a problem hiding this comment.
Just a thought, if we'd want to print the report, we might as well redirect the contents to a file. The current implementation prints the logs+report together, unless we redirect the output to a file.
There was a problem hiding this comment.
Good catch, and you're right i confirmed via grep that several subcommands (range.go, txn_mixed.go, watch_latency.go, etc.) print status/progress messages via fmt.Println/Printf to stdout, same stream as the report output. That means --output-format json > file would currently pick up interleaved logs.
This predates this PR (those prints exist independent of the --output-format work), so I'd suggest scoping the stdout/stderr cleanup as a separate follow-up PR rather than folding it into this one happy to file that as a tracked issue if that works. Let me know if you'd rather I address it here instead.
Signed-off-by: Somesh kumar <someshkumargauda@gmail.com>
What this does
Adds a global
--output-formatflag (values:text|json) to thebenchmark tool. When
jsonis selected, results are serialised as indentedJSON to stdout instead of the human-readable text summary.
Why
The benchmark tool currently outputs only human-readable text, which is hard
to consume programmatically or track across runs. JSON output enables
scripting, trend tracking, and cross-version comparisons without fragile
text parsing.
Changes
pkg/report/report.go— addsFormatJSON()plusJSONReport,JSONBucket,and
JSONErrortypes. Purely additive; existing text formatting untouched.tools/benchmark/cmd/output.go(new) — registers the--output-formatflagand a shared
printReport()helper.tools/benchmark/cmd/put.go— routes output throughprintReport().pkg/report/json_test.go(new) — 8 unit tests covering JSON validity,scalar fields, percentile keys, error inclusion/omission, determinism,
and empty stats.
Example
```
$ benchmark --output-format=json put --total=10000
{
"total_secs": 1.23,
"slowest_secs": 0.0228,
"fastest_secs": 0.00036,
"average_secs": 0.00298,
"requests_per_sec": 8103.4,
"percentiles": { "p50": 0.00088, "p99": 0.018, ... },
"histogram": [ ... ],
"errors": []
}
```
Backward compatibility
Default behaviour (
--output-format=text) is byte-for-byte identical to before.Notes
This is an initial step; the
putsubcommand is wired toprintReport()asthe reference. Happy to extend the same one-line change to the remaining
subcommands (range, watch, txn-put, lease-keepalive, stm) in this PR or a
follow-up — whichever maintainers prefer.
AI assistance disclosure
I used an AI assistant (Bob) to help draft and review this change. I
reviewed all code, ran the tests and build locally, and take responsibility
for the contribution.