format/status: split FormatStatusSummary aggregation from rendering#4679
Merged
Conversation
FormatStatusSummary was a ~600-line function (status.go:103-711) doing both the single accumulation pass over the status snapshot and the per-section rendering inline. codex-review-174 #13 (#4656) flagged it as a cold-path god-function: hard to read, and the aggregation and rendering concerns were fully interleaved. This is a pure code-motion refactor with byte-identical output. The aggregation pass is extracted into aggregateStatusSummary, which returns a statusSummaryAggregates struct holding every counter previously kept in ~90 local variables plus the two derived residuals (currentRuntimeCoSAdmissionDrops, nonAdmissionTXErrors). Each cohesive rendering block moves into a dedicated writeXxxSection helper (overview, event stream, security counters, NAT, source-NAT pools, TX+CoS summary, three-color policers, TX-path counters, slow path, worker runtime). The new helpers live in status_sections.go; FormatStatusSummary is now a short orchestrator that calls them in the original top-to-bottom order. Guardrails held exactly as required by the issue: - ONE pass over status.Bindings. aggregateStatusSummary is the only place that walks Bindings (and Queues, and CoSInterfaces); the render helpers read the returned struct and never re-iterate. No extra status fetches were introduced. - Deterministic order preserved (same section sequence, same sorts on the source-NAT pool and three-color policer tables). - Omit-zero / conditional suppression preserved verbatim (SYN-cookie, generated-reply drops/sent/budget/rate-limit, CoS block, slow-path optional lines, worker-runtime dead/idle rows). Byte-identity is proven by a new golden test: TestFormatStatusSummary Golden compares the full rendered output against testdata/status_summary.golden, which was generated from the pre-refactor code over a fixture that exercises every conditional section (wall-clock- relative fields are deliberately left zero so the golden is stable). The golden passes byte-for-byte after the split; the existing substring assertions in status_test.go also remain green. Validation: go build ./... clean, go vet clean, gofmt -w applied, go test ./pkg/dataplane/userspace/... green (16 status-summary tests including the golden gate). Closes #4656 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors the userspace dataplane FormatStatusSummary formatter by separating the single-pass snapshot aggregation from the per-section rendering, while adding a golden-output test to ensure byte-for-byte output stability.
Changes:
- Extracted the counter accumulation work into
aggregateStatusSummary(status) statusSummaryAggregates. - Moved each formatting block into focused
writeXxxSectionhelpers in a newstatus_sections.go. - Added a golden test (
TestFormatStatusSummaryGolden) and fixture output to gate output identity across refactors.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/dataplane/userspace/format/status.go | Shrinks FormatStatusSummary into an orchestrator that aggregates once and delegates rendering to section helpers. |
| pkg/dataplane/userspace/format/status_sections.go | Introduces the aggregate struct + single-pass aggregator and the extracted writeXxxSection render helpers. |
| pkg/dataplane/userspace/format/status_golden_test.go | Adds a deterministic fixture + golden comparison (with -update support) to lock output bytes. |
| pkg/dataplane/userspace/format/testdata/status_summary.golden | Golden output for full rendered status summary. |
| _Log.md | Logs the refactor per repository logging rules. |
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.
Closes #4656.
What
Pure code-motion refactor of
FormatStatusSummary(
pkg/dataplane/userspace/format/status.go), which was a ~600-linegod-function (lines 103-711) doing both the single accumulation pass over
the status snapshot and the per-section rendering inline
(codex-review-174 #13, cold-path class-A).
aggregateStatusSummary(status) statusSummaryAggregates(the struct holds every counter previously kept in ~90 locals plus the two
derived residuals).
writeXxxSectionhelper: overview, event stream, security counters, NAT, source-NAT pools,
TX+CoS summary, three-color policers, TX-path counters, slow path, worker
runtime. New helpers live in
status_sections.go.FormatStatusSummaryis now a 22-line orchestrator calling the helpers inthe original top-to-bottom order.
Guardrails (from the issue) — all preserved
aggregateStatusSummaryis the only place thatwalks
status.Bindings(andQueues, andCoSInterfaces); render helpersread the returned struct and never re-iterate. No extra status fetches.
pool and three-color policer tables.
drops/sent/budget/rate-limit, CoS block, optional slow-path lines,
worker-runtime dead/idle rows) moved verbatim.
Byte-identity gate
New
TestFormatStatusSummaryGoldencompares the full rendered output againsttestdata/status_summary.golden, generated from the pre-refactor code over afixture that exercises every conditional section (wall-clock-relative fields
left zero so the golden is stable). The golden passes byte-for-byte after the
split — proof the output text is unchanged. Existing substring assertions in
status_test.goalso stay green.Validation
go build ./...cleango vet ./pkg/dataplane/userspace/format/cleangofmt -wappliedgo test ./pkg/dataplane/userspace/...green (16 status-summary tests incl. the golden gate)🤖 Generated with Claude Code
https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi