fix: support iperf3 without --json-stream#205
Merged
Conversation
📝 WalkthroughWalkthroughCentralizes iperf3 final-metrics parsing in the backend (streaming and monolithic JSON), adds streaming-mode detection and richer failure output, and updates frontend jitter handling to treat 0 as a valid value for display and averaging. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as runSingleIperfTest
participant StdoutProc as stdout processor
participant StderrProc as stderr streamer
participant Detector as supportsIperfJSONStream
participant Parser as parseIperfFinalMetrics
participant Selector as selectIperfDirectionMetrics / selectIperfDirectionJitter
Runner->>Detector: Check JSON streaming support
Runner->>StdoutProc: Start iperf3 (stdout stream)
Runner->>StderrProc: Start capturing/stashing stderr
StdoutProc->>Parser: Stream lines / full output
alt Stream contains "end" event
Parser->>Selector: Extract direction metrics & jitter from streaming end
else No streaming end event
Parser->>Parser: Parse monolithic -J JSON
Parser->>Selector: Extract direction metrics & jitter from monolithic end
end
Selector-->>Parser: Return Mbps and optional jitter
Parser-->>Runner: Return final metrics or error (includes formatted stdout/stderr)
sequenceDiagram
participant Dashboard as DashboardTab
participant Data as CollectedResults
participant Columns as columns.tsx
Data->>Dashboard: Provide results (speed, jitter which may be 0)
Dashboard->>Dashboard: calculateAverage("jitter") when jitter !== null && jitter !== undefined
Dashboard->>Columns: Send metrics
Columns->>Columns: Render jitter when !== null && !==undefined, else "—"
Columns-->>Dashboard: UI displays jitter (0 shown and averaged)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: unknown linters: 'modernize', run 'golangci-lint help linters' to see the list of supported linters Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/speedtest/iperf_test.go (1)
13-64: Prefer table-driven tests for these parsing cases.Consider consolidating the streaming, fallback, zero-jitter, and invalid-input cases into a single table-driven test to reduce duplication and make future cases easier to add.
As per coding guidelines, “Prefer table-driven tests for new backend logic in Go.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/speedtest/iperf_test.go` around lines 13 - 64, Consolidate the four separate tests into a single table-driven test that iterates over test cases (identify cases by name like "streaming JSON", "monolithic JSON", "zero jitter", "invalid input"), each case providing input string, the boolean isDownload flag, expected speed (or nil), expected jitter pointer (or nil), and whether an error is expected; inside the loop call parseIperfFinalMetrics and assert results per-case (use require.NoError/require.Error and assert.InDelta/assert.Nil as appropriate). Keep references to parseIperfFinalMetrics and the existing expectations (download vs upload) so assertions for download/upload mapping remain correct, and ensure each case has a descriptive name to make failures readable. Ensure imports and test helpers (require/assert) remain available and maintain equivalent assertions from the original TestParseIperfFinalMetrics_* tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/speedtest/iperf.go`:
- Around line 44-52: The jitter fields in iperfEndData (currently non-pointer
floats) and the helper selectIperfDirectionJitter are dropping explicit 0.0
values by returning nil for jitter <= 0; change the SumSent.JitterMs and
SumReceived.JitterMs types to *float64 so JSON-unset vs explicit 0.0 can be
distinguished, update selectIperfDirectionJitter to return *float64 (return a
pointer to 0.0 when the parsed value is exactly 0.0 and only return nil when the
JSON field is absent/unset), and adjust any call sites and tests (e.g.,
TestParseIperfFinalMetrics_ZeroJitterReturnsNilPointer) to assert pointer
semantics rather than treating 0.0 as missing.
---
Nitpick comments:
In `@internal/speedtest/iperf_test.go`:
- Around line 13-64: Consolidate the four separate tests into a single
table-driven test that iterates over test cases (identify cases by name like
"streaming JSON", "monolithic JSON", "zero jitter", "invalid input"), each case
providing input string, the boolean isDownload flag, expected speed (or nil),
expected jitter pointer (or nil), and whether an error is expected; inside the
loop call parseIperfFinalMetrics and assert results per-case (use
require.NoError/require.Error and assert.InDelta/assert.Nil as appropriate).
Keep references to parseIperfFinalMetrics and the existing expectations
(download vs upload) so assertions for download/upload mapping remain correct,
and ensure each case has a descriptive name to make failures readable. Ensure
imports and test helpers (require/assert) remain available and maintain
equivalent assertions from the original TestParseIperfFinalMetrics_* tests.
s0up4200
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/speedtest/iperf.go (1)
453-464: Consider caching the--json-streamcapability check.
supportsIperfJSONStreamrunsiperf3 --helpon every test invocation. Since the iperf3 binary doesn't change during runtime, the result could be cached to avoid repeated subprocess spawns.♻️ Optional: cache capability detection
+var ( + iperfStreamOnce sync.Once + iperfStreamSupport bool +) + func supportsIperfJSONStream(ctx context.Context) bool { + iperfStreamOnce.Do(func() { helpCtx, cancel := context.WithTimeout(ctx, 2*time.Second) defer cancel() output, err := exec.CommandContext(helpCtx, "iperf3", "--help").CombinedOutput() if err != nil { log.Debug().Err(err).Msg("Could not detect iperf3 --json-stream support; falling back to -J") - return false + iperfStreamSupport = false + return } - return strings.Contains(string(output), "--json-stream") + iperfStreamSupport = strings.Contains(string(output), "--json-stream") + }) + return iperfStreamSupport }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/speedtest/iperf.go` around lines 453 - 464, supportsIperfJSONStream currently spawns `iperf3 --help` on every call; add a package-level cached result (e.g., a bool `iperfJSONStreamSupported` with a `sync.Once` like `detectIperfJSONStreamOnce`) and change supportsIperfJSONStream to run the subprocess only once inside the once.Do initialization (preserving the existing timeout, error logging and strings.Contains check) and thereafter return the cached boolean, so repeated invocations avoid repeated exec.CommandContext calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/speedtest/iperf.go`:
- Around line 438-451: selectIperfDirectionJitter currently treats jitter == 0.0
as missing; change the function so zero is preserved: either (preferred) make
SumReceived.JitterMs and SumSent.JitterMs *float64 and return that pointer
directly from selectIperfDirectionJitter (handle nil pointers as missing), or
(minimal) keep the current float64 fields but change the nil-check from jitter
<= 0 to jitter < 0 so only negative jitter yields nil and 0.0 is returned as a
valid value.
- Around line 47-56: The JitterMs fields in the iperfEndData struct (inside
SumSent and SumReceived) must be changed from float64 to *float64 so JSON
unmarshalling can distinguish a missing jitter_ms (nil) from an explicit 0.0;
update the struct definitions for iperfEndData -> SumSent and SumReceived to use
*float64 for JitterMs and then update any consumers (notably
selectIperfDirectionJitter) to check for nil versus non-nil and handle a nil as
"missing" while treating a non-nil 0.0 as a real zero value.
---
Nitpick comments:
In `@internal/speedtest/iperf.go`:
- Around line 453-464: supportsIperfJSONStream currently spawns `iperf3 --help`
on every call; add a package-level cached result (e.g., a bool
`iperfJSONStreamSupported` with a `sync.Once` like `detectIperfJSONStreamOnce`)
and change supportsIperfJSONStream to run the subprocess only once inside the
once.Do initialization (preserving the existing timeout, error logging and
strings.Contains check) and thereafter return the cached boolean, so repeated
invocations avoid repeated exec.CommandContext calls.
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.
Root Cause
Netronome always invoked
iperf3with--json-stream. On environments like Ubuntu 22.04 with iperf3 3.9, that flag is not supported, so iperf exits with status 1. We also were not surfacing stderr, which hid the real failure reason.Summary
--json-streamsupport fromiperf3 --helpand fall back to-Jwhen unavailableendmetrics with fallback to monolithic-JJSON outputValidation
go test ./internal/speedtest/...go test ./...pnpm -C web buildCloses #156
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Documentation