🧪 test(collect): make the load test assert something it can fail (PERF-FIX-04) - #297
Merged
Conversation
…F-FIX-04) `test/load` was never a load test. It called `extractor.Extract` on ONE identical object in a single-threaded loop with no API server, cluster, sinks, controller or concurrency, and asserted nothing beyond "did not return an error" — so it could not fail for being slow, allocating more, or regressing. It duplicated `BenchmarkExtract` while carrying the name `load-test:10k` and a nightly `ubuntu-latest-8-cores` job, which read as cluster-scale evidence nobody had measured. `test/load/load_test.go` was worse: it asserted env-var parsing and logged. Option (b) from the story — make it honest — plus assertions that can fail: - Delete `test/load/` and the `load` build tag (untagged code was invisible to golangci-lint, so this also widens lint coverage). - Add `TestExtractHotPathBudget` in `internal/collect`, driving the SINGLE shared `extractWorkload` that `BenchmarkExtract` also drives — one workload definition, no duplication. Test-package functions are not importable across packages, so co-location is the only way to genuinely share it. - Vary the object: a deterministic pool of 128 distinct Deployments, built outside the timed region, ~1 in 7 omitting `status.readyReplicas` so the CEL `has()` expression takes both branches. - Assert three ceilings against a recorded baseline +25%, with a vacuous-pass guard (N > 0, non-zero alloc counters) so a broken benchmark cannot report zeroes and sail through every check. Calibration, because a test that false-reds is worse than one that cannot fail. B/op and allocs/op are hardware-independent for a fixed toolchain, so they sit at baseline +25% and carry the regression signal. ns/op is not, and it is measured under two different conditions: - In the required PR gate this runs inside `go test $(go list ./...) -coverprofile`, i.e. instrumented (+9% measured) while ~40 sibling packages, several spinning envtest apiserver/etcd, contend for the runner. Two concurrent commands alone cost 2.8x on an 18-core laptop. The in-code default is therefore a deliberately absurd 50x net for order-of-magnitude regressions. - `task extract-budget` runs the test alone and uninstrumented, so it pins KOLECT_EXTRACT_MAX_NS_PER_OP=204408 (8x baseline) — ~6x tighter, and that is where the real latency gate lives. Calibrated hardware should set it to its own measured ns/op x 1.25 for the literal >25% floor. Baseline (S-LOCAL, darwin/arm64 Apple M5 Max, go1.26.5, worst of -count=5): 25551 ns/op · 32218 B/op · 452 allocs/op. Varying the pool did not move the allocation profile from the single-object measurement it replaces. Naming and CI: - `task load-test` / `task load-test:10k` -> `task extract-budget`, whose description states what is and is not exercised so `task --list` cannot mislead. - Workflow job `load-test-10k` (8-core, disabled behind `run_scale_jobs`) -> `extract-budget` on plain `ubuntu-latest`, no envtest cache, running every nightly instead of never. `scale-envtest-10k` remains the only 10k scale job. - `KOLECT_LOAD_TEST` / `KOLECT_LOAD_TEST_MAX` are gone; `hack/perf-report.sh` no longer derives a "load" scale tier from a dead env var. - LAB-DOC-03 now fails closed if a `load-test-10k` job reappears. - ADR-0603/0706/0707 (incl. the L5 tier row, mermaid node and ownership note), REQUIREMENTS NFR-PERF-1, performance.md, the load-test runbook tiers, testing.md, guidelines.md and setup.md restated: this is a micro-benchmark budget, not a scale tier.
… not enforce
Independent review, F1 (P1): seven surfaces said `task extract-budget` fails a
>25% ns/op regression. It does not. The reviewer injected a CPU-only slowdown
into `Extract` and measured throughput fall 39144 -> 11078 ops/s (-72%, 3.5x)
at UNCHANGED B/op and allocs/op. It passed both the task-pinned ceiling
(204408) and the in-code default (1596900). The claim was false on every
surface that made it.
Coordinator decision: fix the overclaiming, not the ceiling. Do NOT tighten
ns/op. The asymmetry argument stands and the reviewer independently confirmed
it — B/op is hardware-independent (arm64 32217 vs amd64 32212, unchanged under
-cover, 32980/459 under -race, and unchanged across go1.26.5 -> go1.26.6) while
ns/op had only a 4.65x margin under -race on the fastest hardware available, so
tightening would red the nightly. A flaky wall-clock gate is worse than none.
ON THE RECORD, so a later reader does not think it was dropped silently:
PERF-FIX-04's acceptance criterion "throughput regresses >25% => test FAILS" is
AMENDED to be met for the ALLOCATION dimensions only. B/op and allocs/op are a
real >25% gate that fails on any runner; the ns/op ceiling is a
catastrophic-regression net and is now described as exactly that everywhere.
This amendment is recorded in the test's own header comment, not just here.
Corrected surfaces: Taskfile.yml, hack/task/Taskfile.test.yml, ADR-0706,
docs/development/{guidelines,testing,setup}.md, docs/operator-manual/
performance.md, docs/operator-manual/load-test-runbook.md, and the
CALIBRATION / headroom / assertion comments in extractor_budget_test.go.
performance.md and setup.md had additionally contradicted themselves inside a
single section. The ns/op failure message now says what breaching it means.
Also from review:
- F5 (P3): dropped `CGO_ENABLED: "1"` from test:extract-budget — carried over
from the deleted load-test task; nothing here needs cgo. Verified green.
- F6 (P3): dropped the "+9% coverage instrumentation" claim. The reviewer
measured -cover at 24994 ns/op, marginally FASTER than uninstrumented. The
50x default absorbs runner contention, not -cover; the comment now says so.
- F7 (P3): `task bench` and hack/perf-report.sh ran the whole package, so a red
TestExtractHotPathBudget could fail a benchmark task. Both now pass
`-run='^$'`, making them pure benchmark passes as their names imply. Proved:
KOLECT_EXTRACT_MAX_BYTES_PER_OP=1 task bench is still green.
- MD046: the new performance.md admonition tripped the same indented-code-block
trap as 4ccba73; promoted to a subsection, per that fix.
F2 (nightly -race margin) and F3 (never measured on native ubuntu x86) are
deliberately NOT addressed here — the coordinator is filing them as a follow-up
to re-pin against the first real nightly `extract-budget` ns/op from CI.
Rebased onto origin/main (PERF-FIX-06 Go 1.26.6 + CHANGELOG sync), 0 conflicts.
Baseline re-measured under go1.26.6: 32218 B/op / 452 allocs/op unchanged,
ns/op 25077-26311 across -count=5, so the recorded 25551 still holds.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
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
1 participant
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.
PERF-FIX-04 — make the load test load-test something
test/load/collect_test.gocalledextractor.Extracton one identical object in a tight single-threaded loop, with no assertion beyond "did not return an error" — so it could not fail for being slow. It duplicatedBenchmarkExtractwhile carrying the nameload-test:10kand a nightly ubuntu-8-core CI job, which reads as cluster-scale evidence.The danger was never that it was fast. It was that a green nightly implied scale confidence nobody measured.
What changed
test/load/deleted entirely, along with its//go:build loadtag..golangci.yamlhas nobuild-tagskey, so that code was never linted — removing it is a net lint-coverage gain.TestExtractHotPathBudgetlives inpackage collectbesideBenchmarkExtract, both driving one sharedextractWorkload.status.readyReplicasso the CELhas()takes both branches.N>0and non-zero alloc metrics) so a failed benchmark reporting zeroes cannot silently satisfy every ceiling and recreate the original defect.load-test-10k(permanently gated behindrun_scale_jobs) replaced byextract-budget, which actually runs every nightly.scale-envtest-10k— the real scale job — is untouched.ON THE RECORD — AC1 is amended
Acceptance said ">25% throughput regression or memory ceiling exceeded ⇒ fails". Independent review proved the throughput half did not hold: an injected CPU-only regression made
Extract3.5× slower (39144 → 11078 ops/s) at unchangedB/op/allocs/opand passed both ceilings.AC1 is met for the allocation dimensions only. The wall-clock ceiling is a coarse catastrophic-regression net, not a >25% gate. This was decided, not silently dropped:
B/opandallocs/opare hardware-independent (demonstrated across two Go patch releases, two architectures,-raceand-cover), while ns/op is not — a reviewer's unloaded machine measured 1.47× the baseline with no regression present, so a 1.25× ceiling would false-red on the fastest hardware available. A flaky gate is worse than no gate.Nine surfaces that claimed a ">25% wall-clock gate" are corrected, and the counterexample is written into the test header and ADR-0706 so nobody re-derives the false claim.
Verification
task test:run,task lint,task verify,task extract-budget,task bench,task lint:shell,task lint:markdown— all exit 0. AC1 re-proven red-then-green per dimension by an independent reviewer on this exact head.Deferred deliberately: nightly
-racemargin and native-x86 calibration → follow-up story (needs CI measurements that do not exist yet).