feat: add tracer dashboard with load generation #76
Conversation
WalkthroughAdds a new Cobra command module implementing an interactive terminal tracer dashboard (TUI) that streams Snapshot data, renders real-time gauges/charts/tables/sparklines, and supports reset/load actions, auto-publish and subscription proxying. Introduces exported data models Snapshot and MsgInfo, SSE-style streaming with reconnect/backoff, authentication-aware helpers for resolving service URL, JWT and client ID, and HTTP proxy functions to publish and subscribe. Exposes tracer-related commands and flags and includes signal-driven graceful shutdown and cleanup. (≈65 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User (keyboard)
participant CLI as CLI (Cobra cmd)
participant API as Service API
participant SSE as SSE Stream
participant TUI as TUI Renderer
User->>CLI: run "tracer dashboard --url=..."
CLI->>CLI: resolveServiceURL(), resolveJWTAndClientID()
CLI->>API: POST /api/v1/subscribe (start SSE / subscribe)
API-->>SSE: open SSE stream (events)
SSE-->>CLI: emit Snapshot JSON
CLI->>CLI: parse Snapshot -> update Snapshot & MsgInfo state
CLI->>TUI: push updated state
TUI-->>User: render gauges/bars/table/sparklines
par User actions
User->>TUI: key 'r' (reset)
TUI->>API: POST /api/v1/tracer/reset
API-->>CLI: ack -> CLI resets local state
User->>TUI: key 'p' or auto-publish trigger
CLI->>API: POST /api/v1/publish (proxyPublishRandom)
API-->>CLI: ack
end
User->>TUI: key 'q' (quit)
TUI->>CLI: signal shutdown
CLI->>SSE: close connection
CLI->>CLI: stop goroutines, restore terminal, exit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touchesImportant Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (5 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
📜 Review details
Configuration used: Repository: getoptimum/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
go.modis excluded by!go.modand included by nonego.sumis excluded by!**/*.sum,!go.sumand included by none
📒 Files selected for processing (1)
cmd/tracer.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
Review Go code for: idiomatic patterns, error handling (check all errors), concurrency safety (context propagation, goroutine cleanup), test coverage for changed logic, exported API stability, memory efficiency (avoid unnecessary allocations), and prefer standard library over third-party when reasonable.
Files:
cmd/tracer.go
🔇 Additional comments (2)
cmd/tracer.go (2)
27-50: Snapshot / MsgInfo shapes look consistent with tracer JSON usageThe data models are straightforward, fields are typed sensibly for counters/latencies, and JSON tags look coherent with expected tracer payloads.
202-225: Utility helpers (humanBytes,pct,safeDiv) are solidThese helpers are idiomatic, avoid division-by-zero, and handle large byte values correctly for the intended UI use.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
cmd/tracer.go (3)
628-644: Critical:http.NewRequestWithContexterror still ignored at line 630.Line 630 discards the error from
http.NewRequestWithContext, despite previous review comments indicating this was fixed. If URL construction fails, you'll pass a nil request toclient.Do(), causing confusing failures.Apply the fix:
- req, _ := http.NewRequestWithContext(ctx, "GET", url, nil) + req, err := http.NewRequestWithContext(ctx, "GET", url, nil) + if err != nil { + return fmt.Errorf("invalid URL: %w", err) + }Also at: cmd/tracer.go:252 (same issue in
streamSnapshots).
578-600: Critical data race: Still directly mutatinghistory.valuesin reset handler.Lines 594 and 596 directly assign to
throughputHist.valuesandlatHist.valuesunder the wrong mutex (muinstead ofhistory.mu). Despite previous review comments indicating this was fixed with areset()method (which exists at lines 209-213), the reset handler still bypasses it.This race will manifest under
-racewhenupdateUI()concurrently callshistory.push():
- Thread A:
updateUI()→throughputHist.push()lockshistory.mu, accessesvalues- Thread B: Reset handler locks
mu, directly writesthroughputHist.values = nilUse the existing
reset()method:mu.Lock() lastDelivered = 0 lastTickTime = time.Now() - throughputHist.values = nil + throughputHist.reset() thSpark.Data = throughputHist.snapshot() - latHist.values = nil + latHist.reset() latSpark.Data = latHist.snapshot() mu.Unlock()This ensures all
historymutations go through the proper lock. Based on learnings.
240-344: Critical: Still ignoringhttp.NewRequestWithContexterror at line 252.Despite earlier review feedback, line 252 still discards the error from
http.NewRequestWithContext. If the URL is malformed, this passes a nil request toclient.Do(), causing confusing "nil Request" errors that mask the real configuration problem.Apply the fix that was discussed but not implemented:
- req, _ := http.NewRequestWithContext(ctx, "GET", url, nil) + req, err := http.NewRequestWithContext(ctx, "GET", url, nil) + if err != nil { + statusCh <- fmt.Sprintf("[Config error] %v", err) + return + }This surfaces configuration errors immediately instead of misleading users with retry loops.
📜 Review details
Configuration used: Repository: getoptimum/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
go.modis excluded by!go.modand included by nonego.sumis excluded by!**/*.sum,!go.sumand included by none
📒 Files selected for processing (1)
cmd/tracer.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: When you find the same underlying issue in multiple locations (same pattern, same fix):
- Do NOT post separate comments for each occurrence.
- Post a single primary comment on the first occurrence.
- In that comment, include a short list of “Also at: file:line” references
for the other locations (e.g. 'Also at: foo.go:42, bar.go:17').
Prioritize signal over volume: one strong comment that references multiple
locations is preferred over many near-identical comments.
Files:
cmd/tracer.go
**/*.go
⚙️ CodeRabbit configuration file
Review Go code for: idiomatic patterns, error handling (check all errors), concurrency safety (context propagation, goroutine cleanup), test coverage for changed logic, exported API stability, memory efficiency (avoid unnecessary allocations), and prefer standard library over third-party when reasonable.
Files:
cmd/tracer.go
🧠 Learnings (2)
📚 Learning: 2025-10-23T18:30:48.214Z
Learnt from: CR
Repo: getoptimum/coderabbit PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-10-23T18:30:48.214Z
Learning: Applies to **/*.go : Concurrency safety in Go: avoid goroutine leaks, unbounded channels, racy access; ensure contexts are plumbed and respected
Applied to files:
cmd/tracer.go
📚 Learning: 2025-10-23T18:30:48.214Z
Learnt from: CR
Repo: getoptimum/coderabbit PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-10-23T18:30:48.214Z
Learning: Applies to **/*.go : Security considerations in Go: flag SQL/command injection, path traversal, unvalidated inputs, hardcoded credentials, insecure crypto (MD5/SHA1), and missing rate limiting on public endpoints
Applied to files:
cmd/tracer.go
🧬 Code graph analysis (1)
cmd/tracer.go (6)
internal/auth/client.go (2)
Client(15-20)NewClient(23-31)cmd/root.go (3)
IsAuthDisabled(71-73)GetAuthPath(52-54)GetClientID(76-78)internal/config/config.go (2)
LoadConfig(34-41)ClientID(20-20)internal/auth/models.go (1)
TokenClaims(32-46)internal/auth/storage.go (1)
NewStorageWithPath(25-43)internal/auth/token.go (1)
NewTokenParser(15-17)
🔇 Additional comments (6)
cmd/tracer.go (6)
1-50: LGTM! Clean imports and well-structured data models.The
SnapshotandMsgInfotypes are properly defined with JSON tags. Usingmap[string]struct{}forPeersSeenis the idiomatic Go approach for sets.
164-185: LGTM! Standard Cobra command and flag registration.The command hierarchy and flag definitions are properly structured.
187-238: LGTM! Thread-safe history implementation and solid helper functions.The
historytype properly encapsulates mutations behindsync.Mutex, and the reset method correctly prevents the data race flagged in earlier reviews. Helper functions include appropriate zero-division checks.
346-465: LGTM! Proper context propagation and goroutine lifecycle management.The UI initialization now correctly propagates errors (line 350-352), matching the fix from previous reviews. Both
streamSnapshotsand auto-publish goroutines respect context cancellation, preventing leaks.
646-698: LGTM! Clean refactoring eliminates auth logic duplication.The extraction of
loadTokenAndClaims(lines 654-673) successfully consolidates the shared authentication flow. BothresolveJWTandresolveJWTAndClientIDnow delegate to this single source of truth, addressing the earlier review feedback about DRY violations.
82-85: Remove dead code—firstresolveJWT()call is immediately overwritten.Lines 82-85 call
resolveJWT()and store the result injwtToken, but line 98 immediately overwritesjwtTokenby callingresolveJWTAndClientID(). The first call serves no purpose.Remove the dead code:
- jwtToken, err := resolveJWT() - if err != nil { - return err - } -The second call at line 98 already handles both token and clientID resolution correctly.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: Repository: getoptimum/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
cmd/tracer.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: When you find the same underlying issue in multiple locations (same pattern, same fix):
- Do NOT post separate comments for each occurrence.
- Post a single primary comment on the first occurrence.
- In that comment, include a short list of “Also at: file:line” references
for the other locations (e.g. 'Also at: foo.go:42, bar.go:17').
Prioritize signal over volume: one strong comment that references multiple
locations is preferred over many near-identical comments.
Files:
cmd/tracer.go
**/*.go
⚙️ CodeRabbit configuration file
Review Go code for: idiomatic patterns, error handling (check all errors), concurrency safety (context propagation, goroutine cleanup), test coverage for changed logic, exported API stability, memory efficiency (avoid unnecessary allocations), and prefer standard library over third-party when reasonable.
Files:
cmd/tracer.go
🧬 Code graph analysis (1)
cmd/tracer.go (6)
internal/auth/client.go (2)
Client(15-20)NewClient(23-31)cmd/root.go (3)
IsAuthDisabled(71-73)GetAuthPath(52-54)GetClientID(76-78)internal/config/config.go (2)
LoadConfig(34-41)ClientID(20-20)internal/auth/models.go (1)
TokenClaims(32-46)internal/auth/storage.go (1)
NewStorageWithPath(25-43)internal/auth/token.go (1)
NewTokenParser(15-17)
🔇 Additional comments (1)
cmd/tracer.go (1)
345-551: Well-structured concurrent dashboard implementationThe TUI event loop and concurrency model are solid:
- Context properly propagated for graceful shutdown
- Goroutines respect
ctx.Done()signals- UI state mutations correctly guarded by mutex
- Auto-publish and reset actions run asynchronously without blocking
- 2-second cooldown on reset prevents accidental double-resets
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
cmd/tracer.go (3)
83-86: Dead code: first JWT resolution is immediately overwrittenLines 83-86 fetch
jwtTokenviaresolveJWT(), but line 99 immediately overwrites it withresolveJWTAndClientID(). The first call's result is never used—this wastes an auth check.RunE: func(cmd *cobra.Command, args []string) error { baseURL := resolveServiceURL(tracerServiceURL) - jwtToken, err := resolveJWT() - if err != nil { - return err - } - if tracerTickMs <= 0 { tracerTickMs = 500 }
589-605: REGRESSION:http.NewRequestWithContexterror still ignored inresetStatsLine 591 ignores the error from
http.NewRequestWithContext. If URL construction fails,reqis nil andhttp.DefaultClient.Do(req)will panic.func resetStats(ctx context.Context, base, jwt string) error { url := fmt.Sprintf("%s/api/v1/tracer/reset", strings.TrimRight(base, "/")) - req, _ := http.NewRequestWithContext(ctx, "GET", url, nil) + req, err := http.NewRequestWithContext(ctx, "GET", url, nil) + if err != nil { + return fmt.Errorf("invalid URL: %w", err) + } if jwt != "" { req.Header.Set("Authorization", "Bearer "+jwt) }
711-717:proxySubscribemissing HTTP status code validationUnlike
proxyPublishRandom(lines 685-688),proxySubscribereturnsnilwithout checkingresp.StatusCode. If the server returns 4xx/5xx, the caller assumes success.If this is intentional best-effort behavior for load scenarios, add a comment. Otherwise, add the same status check:
resp, err := http.DefaultClient.Do(req) if err != nil { return err } defer resp.Body.Close() //nolint:errcheck + // ignore non-2xx errors intentionally; load can still proceed return nilOr for consistency:
defer resp.Body.Close() //nolint:errcheck + if resp.StatusCode/100 != 2 { + b, _ := io.ReadAll(io.LimitReader(resp.Body, 512)) + return fmt.Errorf("subscribe error: HTTP %d %s", resp.StatusCode, strings.TrimSpace(string(b))) + } return nil
📜 Review details
Configuration used: Repository: getoptimum/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
cmd/tracer.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (Custom checks)
**/*.go: Watch for goroutine leaks, unbounded channels, and racy access; ensure contexts are plumbed and respected
If code changes affect Go packages, verify that tests cover changed logic; flag risky changes lacking tests
Files:
cmd/tracer.go
⚙️ CodeRabbit configuration file
Review Go code for: idiomatic patterns, error handling (check all errors), concurrency safety (context propagation, goroutine cleanup), test coverage for changed logic, exported API stability, memory efficiency (avoid unnecessary allocations), and prefer standard library over third-party when reasonable.
Files:
cmd/tracer.go
**/*.{go,rs,js,ts,py,java,cpp,c,rb,php}
📄 CodeRabbit inference engine (Custom checks)
Flag security risks: SQL injection, command injection, path traversal, unvalidated inputs, hardcoded credentials, insecure crypto usage (MD5/SHA1), and missing rate limiting on public endpoints
Files:
cmd/tracer.go
**/*
⚙️ CodeRabbit configuration file
**/*: When you find the same underlying issue in multiple locations (same pattern, same fix):
- Do NOT post separate comments for each occurrence.
- Post a single primary comment on the first occurrence.
- In that comment, include a short list of “Also at: file:line” references
for the other locations (e.g. 'Also at: foo.go:42, bar.go:17').
Prioritize signal over volume: one strong comment that references multiple
locations is preferred over many near-identical comments.
Files:
cmd/tracer.go
🧠 Learnings (5)
📚 Learning: 2025-11-27T11:40:12.459Z
Learnt from: CR
Repo: getoptimum/optimum-gateway PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-11-27T11:40:12.459Z
Learning: Applies to **/*.go : Watch for goroutine leaks, unbounded channels, and racy access; ensure contexts are plumbed and respected
Applied to files:
cmd/tracer.go
📚 Learning: 2025-11-28T11:27:28.785Z
Learnt from: CR
Repo: getoptimum/optimum-bootstrap PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-11-28T11:27:28.785Z
Learning: Applies to **/*.go : If code changes affect Go packages, verify that tests cover changed logic and flag risky changes lacking tests
Applied to files:
cmd/tracer.go
📚 Learning: 2025-11-27T10:53:12.793Z
Learnt from: CR
Repo: getoptimum/optimum-dev-setup-guide PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-11-27T10:53:12.793Z
Learning: Applies to **/*.go : If code changes affect Go packages, verify that tests cover changed logic; flag risky changes lacking tests
Applied to files:
cmd/tracer.go
📚 Learning: 2025-11-27T11:40:12.459Z
Learnt from: CR
Repo: getoptimum/optimum-gateway PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-11-27T11:40:12.459Z
Learning: Applies to **/*_test.go : If code changes affect Go packages, verify that tests cover changed logic; flag risky changes lacking tests
Applied to files:
cmd/tracer.go
📚 Learning: 2025-11-27T11:40:12.459Z
Learnt from: CR
Repo: getoptimum/optimum-gateway PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-11-27T11:40:12.459Z
Learning: Call out any breaking changes to exported types/functions and expect a CHANGELOG entry or migration note
Applied to files:
cmd/tracer.go
🧬 Code graph analysis (1)
cmd/tracer.go (6)
internal/auth/client.go (2)
Client(15-20)NewClient(23-31)cmd/root.go (3)
IsAuthDisabled(71-73)GetAuthPath(52-54)GetClientID(76-78)internal/config/config.go (2)
LoadConfig(34-41)ClientID(20-20)internal/auth/models.go (1)
TokenClaims(32-46)internal/auth/storage.go (1)
NewStorageWithPath(25-43)internal/auth/token.go (1)
NewTokenParser(15-17)
🔇 Additional comments (6)
cmd/tracer.go (6)
147-155: Acknowledged: best-effort error handling for load generationPer previous discussion, the silent error discards here are intentional for load generation scenarios where partial failures are expected and shouldn't halt the process.
241-344: Well-structured SSE streaming with proper error handlingThe
streamSnapshotsfunction correctly:
- Handles
http.NewRequestWithContexterrors (lines 268-272)- Implements exponential backoff with a cap
- Respects context cancellation throughout
- Uses a timeout on the HTTP client
- Properly closes the output channel via
defer
661-690: Good error handling inproxyPublishRandomJSON marshal errors, request creation errors, and non-2xx status codes are all properly handled. The lack of timeout on
http.DefaultClientis a codebase-wide pattern.
636-659: Clean auth resolution implementationThe refactored
loadTokenAndClaimshelper eliminates duplication. BothresolveJWTandresolveJWTAndClientIDare concise and handle the auth-disabled case correctly.
43-51:MsgInfo.PeersSeenwon't unmarshal correctly from JSON
map[string]struct{}is unmarshaled directly from the server response at line 328. JSON doesn't naturally deserialize into this type—if the server sends"peers_seen": {"peer1": {}, "peer2": {}}, the empty struct values won't parse. This will cause the parse error at line 329 to trigger, silently failing to display peer data.Either:
- Change to
map[string]boolormap[string]anyif the server sends an object- Change to
[]stringif the server sends an array- Implement a custom
UnmarshalJSONmethodtype MsgInfo struct { Topic string `json:"topic"` Published time.Time `json:"published"` Delivered time.Time `json:"delivered"` DelaySec float64 `json:"delay_sec"` - PeersSeen map[string]struct{} `json:"peers_seen"` + PeersSeen map[string]any `json:"peers_seen"` Duplicates int `json:"duplicates"` BytesMoved uint64 `json:"bytes_moved"` }⛔ Skipped due to learnings
Learnt from: CR Repo: getoptimum/optimum-gateway PR: 0 File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0 Timestamp: 2025-11-27T11:40:12.459Z Learning: Applies to **/*.go : Watch for goroutine leaks, unbounded channels, and racy access; ensure contexts are plumbed and respectedLearnt from: CR Repo: getoptimum/mump2p-cli PR: 0 File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0 Timestamp: 2025-11-28T19:55:52.117Z Learning: Applies to **/*.go : Watch for goroutine leaks, unbounded channels, and racy access; ensure contexts are plumbed and respectedLearnt from: CR Repo: getoptimum/docs PR: 0 File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0 Timestamp: 2025-11-28T04:42:48.134Z Learning: Applies to **/*.go : Watch for goroutine leaks, unbounded channels, and racy access; ensure contexts are plumbed and respectedLearnt from: CR Repo: getoptimum/optimum-bootstrap PR: 0 File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0 Timestamp: 2025-11-28T11:27:28.785Z Learning: Applies to **/*.go : Watch for goroutine leaks, unbounded channels, and racy access; ensure contexts are plumbed and respectedLearnt from: CR Repo: getoptimum/optimum-dev-setup-guide PR: 0 File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0 Timestamp: 2025-11-27T10:53:12.793Z Learning: Applies to **/*.go : Watch for goroutine leaks, unbounded channels, and racy access; ensure contexts are plumbed and respected
188-214: Remove unusedhistorytype and methodsThe
historytype withnewHistory,push,snapshot, andresetis defined but never instantiated or used. This appears to be dead code from a previous implementation.-type history struct { - values []float64 - limit int - mu sync.Mutex -} - -func newHistory(limit int) *history { return &history{limit: limit} } -func (h *history) push(v float64) { - h.mu.Lock() - defer h.mu.Unlock() - h.values = append(h.values, v) - if len(h.values) > h.limit { - h.values = h.values[len(h.values)-h.limit:] - } -} -func (h *history) snapshot() []float64 { - h.mu.Lock() - defer h.mu.Unlock() - out := make([]float64, len(h.values)) - copy(out, h.values) - return out -} -func (h *history) reset() { - h.mu.Lock() - defer h.mu.Unlock() - h.values = nil -}Likely an incorrect or invalid review comment.
command:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.