fix(engine): deprecate EngineMetrics.Throughput, which always reads 0 (celeris#653) - #695
Conversation
… (celeris#653) EngineMetrics.Throughput was exported and documented as "the recent requests-per-second rate", and no engine ever assigned it: std, epoll and io_uring never wrote it, and adaptive's aggregator summed the two sub-engines' zeros. A field that always reads 0 cannot be told apart from a measured rate of zero. Removing it breaks the exported struct, so that is v2.0.0's (celeris#651). v1.6.0 deprecates it: the doc now says it always reads 0 and why, and points at RequestCount sampled over the caller's own interval. staticcheck's SA1019 then flags every reader outside the package. Adaptive's pass-through of the zeros is removed, so nothing in the tree touches the field, and the reflective aggregation test (celeris#627) exempts it by name. Tests: - TestThroughputIsDeprecatedAsAlwaysZero reads the field's doc with go/parser. On 9f4d89b it FAILS: the doc is "Throughput is the recent requests-per-second rate." with no Deprecated paragraph. - TestNothingSetsThroughput walks every non-test Go file of the repository and fails on any composite-literal key or selector named Throughput, so the "always reads 0" claim and the code cannot drift apart. On 9f4d89b it FAILS on adaptive/engine.go:1018. - Mutants, each killed: the Deprecated paragraph removed; adaptive forwarding the field again (which golangci-lint also reports as SA1019); std setting it.
…leris#653) TestAsyncPromotedConnRequestCount's comment named EngineMetrics.Throughput among the values that went flat with RequestCount. Throughput was never derived from anything; it is always 0. What does derive from RequestCount is the adaptive controller's ThroughputRPS (telemetry.go) and BytesPerReq.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: goceleris/celeris/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 SummarySummary by CodeRabbit
Walkthrough
ChangesThroughput field deprecation
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~12 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The deprecation change has no identified merge-blocking issue and is ready to merge after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The metric remains available, and the reviewed production changes do not alter request handling or add an attacker-accessible path. Consumers will need to migrate from the deprecated field before its planned removal. Downstream adoption is not fully established. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Summary
engine.EngineMetrics.Throughputis exported and documented as "the recent requests-per-second rate", but no engine has ever assigned it, so it always reads 0. That zero looks exactly like a measured rate of zero.Removing the field is a breaking change and belongs to v2.0.0 (#651). This PR deprecates it in v1.6.0, as the plan's D6 recommends.
Fixes #653. That is the v1.6.0 decision #653 was filed for; the removal is v2.0.0's (#651).
Changes
engine/engine.go: aDeprecated:paragraph on the field. It says:RequestCount, sampled twice over the caller's own interval, insteadadaptive/engine.go: removed theThroughput: pm.Throughput + sm.Throughputpass-through (0 + 0). Nothing in the tree touches the field any more.adaptive/close_accounting_test.go: the reflective adaptive: Metrics() silently drops Workers, AcceptCount, CloseCount, BytesRead, BytesWritten and RecvResumeWhileCancelPending #627 aggregation test exemptsThroughputby name, with the reason.engine/epoll/loop.go: one comment no longer names the field.engine/epoll/async_reqcount_test.go:144(review round 2): the comment no longer listsThroughputamong the values derived fromRequestCount. What is derived from it is the adaptive controller'sThroughputRPS(adaptive/telemetry.go:85) andBytesPerReq.Docs examples that print it: goceleris/docs (
engines.md,performance.md), in a separate docs PR.Consumers. probatorium reads the field at
validation/refapp/internal/debugvars/debugvars.go:510. It is unchanged here, per the lane rules; itsEngineKeysNotParsedentry already names #653. Its next celeris bump will get an SA1019 finding wherever golangci-lint covers that module.Test Plan
Evidence:
evidence/celeris-673-679-653-424/lane-20260926/653/(per-finding index for round 2:ROUND2.md).TestThroughputIsDeprecatedAsAlwaysZeroparses the field's doc withgo/parser.TestNothingSetsThroughputwalks every non-test.gofile in the repository and fails on any composite-literal key or selector namedThroughput..claude/worktreescopies are not scanned.On 9f4d89b: both FAIL, with the test file as pushed (
base-9f4d89b-pushedtext.log, frombase-pushedtext.sh).engine/throughput_deprecated_test.go(sha25671bd4786…, unchanged since 38d2c4d) is copied onto a detached 9f4d89b worktree, whole.MANIFEST.txtrecords forbase-9f4d89b.log:go test -v -run 'TestThroughputIsDeprecatedAsAlwaysZero|TestNothingSetsThroughput' ./engine/.throughput_deprecated_test.go:34: the doc isThroughput is the recent requests-per-second rate.and has no Deprecated paragraph.:107: 3 sites touch the field, all onadaptive/engine.go:1018.:105came from an earlier revision.Fixed, at 3477bbe:
./enginenatively with-v: 17 PASS, 0 FAIL, 0 SKIP.go vet ./engine/... ./adaptive/...passes for linux/amd64 and linux/arm64 (round2-vet-and-engine-v.log)../engine/... ./adaptive/...: 0 issues, natively (round2-golangci-native.log) and with GOOS=linux (round2-golangci-linux.log).Mutants, each killed. Re-run on 3477bbe (
round2-mutants/); every tree is restored bycpfrom a saved copy.TestThroughputIsDeprecatedAsAlwaysZeroTestNothingSetsThroughput; golangci-lint also reports SA1019 atadaptive/engine.go:1018EngineMetrics.Throughput)TestNothingSetsThroughput./adaptiveis Linux-only and is judged by this PR's CI.--- PASS/FAIL/SKIP: Testlines), includingTestMetricsCarriesEveryFieldReflectively(ci-36254874202-adaptive.log).ci-36254874202-lint.log).Review notes (round 2)
TestNothingSetsThroughputcould fail on an unrelated future field namedThroughput.Release notes