benchkit: add the measurement harness as a second module - #335
Open
meling wants to merge 5 commits into
Open
Conversation
Contributor
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Go |  |
Aug 12, 2026 12:38p.m. | Review ↗ |
| Shell | |
Aug 12, 2026 12:38p.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces benchkit as a second (workspace-joined) Go module under github.com/relab/gorums/benchkit, intended to hold benchmarking/measurement harness code and its orchestration dependencies without polluting the root module.
Changes:
- Adds the
benchkitmodule togo.workand extends the rootMakefileto generate/maintain benchkit’s protobuf + Gorums artifacts. - Introduces benchkit’s measurement primitives (pacing, ticking/event stream, HDR histogram, aggregation/summarization, report encode/decode, control plane RPCs) with accompanying tests.
- Adds benchkit
.protosources underbenchkit/proto/with generated code emitted into the module root.
Reviewed changes
Copilot reviewed 49 out of 53 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Makefile | Adds benchkit proto generation targets and includes benchkit in workspace lint/modernize/deadcode flows. |
| go.work | Joins ./benchkit as a workspace module. |
| AGENTS.md | Updates repository/module layout documentation to include benchkit. |
| benchkit/go.mod | Defines github.com/relab/gorums/benchkit module and replaces gorums to the local repo root. |
| benchkit/go.sum | Adds dependency checksums for the new module. |
| benchkit/doc.go | Adds package documentation for benchkit. |
| benchkit/log.go | Adds stderr-only logging helpers for sweep-driven binaries. |
| benchkit/flags.go | Defines and registers the standard CLI contract flags and converts them into Options. |
| benchkit/flags_test.go | Tests standard flag parsing (including -call-timeout) and Options() mapping. |
| benchkit/fault.go | Adds optional fault-injection timer to simulate mid-run process exit. |
| benchkit/fault_test.go | Tests fault arming behavior and stoppability. |
| benchkit/pacer.go | Adds open-loop pacer and shared rated gate primitives. |
| benchkit/pacer_test.go | Tests pacing math, cancellation behavior, and concurrency properties. |
| benchkit/event_buffer.go | Adds a mutex-protected, nil-safe in-memory event buffer for time-series events. |
| benchkit/event_buffer_test.go | Tests event buffering, nil-safety, and offset base behavior. |
| benchkit/ticker.go | Adds background interval sampling (throughput/latency events) plus throughput CV computation. |
| benchkit/ticker_test.go | Tests nil-buffer behavior, CV arithmetic, final interval flush, and concurrency race coverage. |
| benchkit/measurement.go | Adds Measurement lifecycle wrapper to unify Stats + Ticker wiring and teardown. |
| benchkit/measurement_test.go | Regression test ensuring Abandon() stops the ticker goroutine. |
| benchkit/store.go | Implements exact and HDR-backed sample stores, plus histogram offset/merge utilities. |
| benchkit/store_test.go | Tests histogram offsetting/clamping and merging semantics. |
| benchkit/hdr.go | Implements an HDR-style bounded-memory histogram with quantiles/mean/stddev support. |
| benchkit/hdr_test.go | Tests histogram parameter validation, boundaries, accuracy, and bucket iteration. |
| benchkit/dist.go | Adds a unified latency distribution abstraction over raw samples and histogram pairs. |
| benchkit/dist_test.go | Tests distribution statistics, merge behavior, and malformed histogram handling. |
| benchkit/summary.go | Adds read-time trimming summarization over event streams with CV recomputation. |
| benchkit/summary_test.go | Tests trimming behavior across measurement modes and stats modes. |
| benchkit/timeseries.go | Renders event streams into CSV outputs for plotting (throughput/latency/saturation). |
| benchkit/timeseries_test.go | Tests CSV rendering, trimming, node tagging, and CSV quoting behavior. |
| benchkit/table.go | Adds tabular result rendering, including per-node and remote server-stats folding behavior. |
| benchkit/table_test.go | Tests alignment and guards against mutating persisted Result when folding stats. |
| benchkit/profiling.go | Adds CPU/heap/trace profiler setup with unified stop/finalization handling. |
| benchkit/profiling_test.go | Tests profiler artifacts, disablement, and cleanup on partial setup failure. |
| benchkit/runconfig.go | Adds helpers for extracting/applying sweep dimensions to/from RunConfig. |
| benchkit/runconfig_test.go | Tests dimensions round-trip and fallback fill behavior. |
| benchkit/report.go | Adds result file framing (magic header) + report encode/decode + comparison output. |
| benchkit/report_test.go | Tests report round-trips, decode rejection, comparisons, and config mismatch warnings. |
| benchkit/aggregate.go | Aggregates per-server results into a cluster-wide result (samples or histogram merge). |
| benchkit/aggregate_test.go | Tests aggregation semantics for samples, server stats ordering, and HDR merges. |
| benchkit/clocksync.go | Adds NTP-style offset estimation, latency correction (samples/hist), and offset logging. |
| benchkit/clocksync_test.go | Tests offset math, correction semantics, and unconditional stderr logging. |
| benchkit/control_server.go | Implements the Control RPC server using Stats and Done tracking. |
| benchkit/control_server_test.go | Tests control Start/Stop stats mode behavior and Done tracking mechanics. |
| benchkit/proto/benchmark/benchmark.proto | Defines the workload service proto used by benchkit-driven benchmarks. |
| benchkit/proto/benchkit/benchkit.proto | Defines benchkit’s core schema (Result/Report/events/plot-data) and run config metadata. |
| benchkit/proto/benchkit/control.proto | Defines benchkit’s control-plane service schema (Start/Stop/ClockSync/Done). |
| benchkit/control.pb.go | Generated Go code for benchkit/control.proto. |
| benchkit/control_gorums.pb.go | Generated Gorums bindings for the Control service. |
Files not reviewed (3)
- benchkit/benchkit.pb.go: Generated file
- benchkit/control.pb.go: Generated file
- benchkit/control_gorums.pb.go: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+87
to
+90
| // The index-map cut is exact only when each op produced one in-order sample. | ||
| if clientMeasured && exact && cutOps > 0 && int(cutOps) <= len(latencies) { | ||
| latencies = latencies[cutOps:] | ||
| } |
Comment on lines
+283
to
+294
| for _, task := range []struct { | ||
| plotter Plotter | ||
| filename string | ||
| }{ | ||
| {tp, group.Benchmark + "_throughput.csv"}, | ||
| {lp, group.Benchmark + "_latency.csv"}, | ||
| {sc, group.Benchmark + "_saturation.csv"}, | ||
| } { | ||
| if err := renderTimeSeries(task.plotter, filepath.Join(outDir, task.filename)); err != nil { | ||
| return nil, err | ||
| } | ||
| } |
Comment on lines
+54
to
+59
| func (c *Control) ArmDone(total int) <-chan struct{} { | ||
| c.doneSeen = make([]atomic.Bool, total+1) // index 0 unused; IDs are 1..total | ||
| c.doneLeft.Store(int32(total)) | ||
| c.doneCh = make(chan struct{}) | ||
| return c.doneCh | ||
| } |
meling
force-pushed
the
benchkit/harness
branch
from
August 12, 2026 11:42
9482cd9 to
8543952
Compare
benchkit is the measurement layer the benchmarking tools are built on: the run harness and its control plane, the pacer that shapes offered load, HDR histograms and the summary and distribution statistics over them, the event buffer and time-series layer, the result store, and the plot-data schema. It is a separate module, github.com/relab/gorums/benchkit, joined to the workspace by go.work. The dependency edge runs one way — benchkit imports gorums, never the reverse — so the root go.mod stays free of the orchestration dependencies that only a benchmarking tool needs. Its go.mod carries a relative replace for gorums, so go mod tidy resolves without the workspace, and the target is inside this repository so a fresh clone is never broken. The .proto sources live under benchkit/proto rather than beside the generated code, which keeps the import paths protoc records as benchkit/*.proto and benchmark/*.proto. Generated code is written back to the module root, so those rules cannot use the Makefile's pattern rules and are spelled out.
halfSec is a time.Duration, so a Sec suffix both restates the type and misstates the value if it ever changes. sampleInterval says what the argument means to emitThroughput.
ArmDone(0) returned an open channel that nothing could ever close: Done closes doneCh only when a signal drives doneLeft to zero, and with no peers armed every sender ID falls outside doneSeen, so Done returns early. A single-node run waiting for peers blocked until its grace deadline. A negative total also panicked in make. Close the channel up front and clamp the slice length.
The guard converted cutOps to int before comparing it against the sample count, so on a platform with a 32-bit int a large value wraps negative, passes the bound, and then panics slicing the latencies. Compare in uint64.
CSV filenames were built by concatenating the benchmark name, then joined with outDir, so a benchmark name holding a path separator wrote outside the directory or failed on a missing parent. Reduce the name to its final path element for the filename; the caller still gets the unmodified name back, since the report keys its figures by it.
meling
force-pushed
the
benchkit/harness
branch
from
August 12, 2026 12:38
8543952 to
d20a8f7
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds
benchkit, the measurement layer the benchmarking tools are built on, as a second module.It provides the run harness and its control plane, the pacer that shapes offered load, HDR histograms with the summary and distribution statistics over them, the event buffer and time-series layer, the result store, and the plot-data schema.
Why a separate module.
github.com/relab/gorums/benchkitis joined to the workspace bygo.work, and the dependency edge runs one way — benchkit imports gorums, never the reverse. That keeps the rootgo.modfree of the orchestration dependencies only a benchmarking tool needs (iago,sftp,pprof,golang.org/x/crypto), so they are not downloaded by everyone who imports gorums. Itsgo.modcarries a relativereplacefor gorums, sogo mod tidyresolves without the workspace, and the target is inside this repository so a fresh clone is never broken.The
.protosources live underbenchkit/protorather than beside the generated code, which keeps the import paths protoc records asbenchkit/*.protoandbenchmark/*.proto. Generated code is written back to the module root, so those rules cannot use the Makefile's pattern rules and are spelled out.This layout is byte-identical to what a later extraction to a separate
github.com/relab/benchkitrepository would need, so that extraction becomes a subdirectory filter plus an import-path rewrite.Known follow-up:
benchkit/go.modrequiresgithub.com/relab/gorums v0.11.0with a localreplace. A downstream consumer ignores a dependency-ownedreplaceand would resolve v0.11.0, which contains neitherruntime/gorumsimplnorgorumstest. Taggingbenchkit/v0.1.0therefore has to wait for a gorums release containing this stack. In-repository builds are unaffected.Verification:
go test ./... -count=1,go test ./benchkit/... -count=1,gofmt -l.