feat: add network health dashboard #66
Conversation
📝 WalkthroughWalkthroughAdds a text-based network dashboard tool and integrates it into the Makefile/build (new dashboard binary and Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI/Main
participant Parser as Flag Parser
participant ProxyFetcher as Proxy Fetcher
participant NodeFetcher as Node Fetcher
participant ProxyEndpoint as Proxy REST Endpoint
participant NodeEndpoint as Node REST Endpoint
participant Renderer as Dashboard Renderer
participant Stdout as Output
CLI->>Parser: Parse flags (-local, -proxy-base, -node-base, -proxies, -nodes)
Parser-->>CLI: Assembled proxy & node URLs
CLI->>ProxyFetcher: For each proxy URL, fetch /health
ProxyFetcher->>ProxyEndpoint: GET /health
ProxyEndpoint-->>ProxyFetcher: ProxyHealth JSON or error
ProxyFetcher-->>CLI: ProxyInfo (populated or error)
CLI->>NodeFetcher: For each node URL, fetch /health and /state
NodeFetcher->>NodeEndpoint: GET /health and GET /state
NodeEndpoint-->>NodeFetcher: NodeHealth & NodeState JSON or error
NodeFetcher-->>CLI: NodeInfo (populated or error)
alt Proxies Available
CLI->>NodeFetcher: Request /countries from proxies to enrich nodes
NodeFetcher->>ProxyEndpoint: GET /countries
ProxyEndpoint-->>NodeFetcher: NodeCountries JSON or error
NodeFetcher-->>CLI: Country data
end
CLI->>Renderer: Provide collected ProxyInfo and NodeInfo
Renderer->>Stdout: Write formatted text dashboard
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip 🧪 Unit Test Generation v2 is now available!We have significantly improved our unit test generation capabilities. To enable: Add this to your reviews:
finishing_touches:
unit_tests:
enabled: trueTry it out by using the Have feedback? Share your thoughts on our Discord thread! 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: 6
🤖 Fix all issues with AI agents
In `@Makefile`:
- Line 15: The .PHONY line currently lists $(P2P_CLIENT), $(PROXY_CLIENT),
$(KEYGEN_BINARY), and $(DASHBOARD_BINARY) which are actual binary output files
and should not be phony; remove those variables from the .PHONY declaration and
only keep truly phony targets (e.g., setup-scripts) so Make can use
timestamp-based rebuild logic for the binaries (look for the .PHONY declaration
and the variables P2P_CLIENT, PROXY_CLIENT, KEYGEN_BINARY, DASHBOARD_BINARY to
update).
- Around line 125-142: The dashboard target currently exits with code 1 on
invalid mode or missing proxy/node URLs but prints nothing; update the dashboard
recipe to emit clear stderr messages before exiting: when mode is neither
empty/"local" nor "remote" print an "invalid mode" error referencing mode, and
in the "remote" branch when args is empty print a descriptive error that
proxy-base/node-base are required (use the existing variables mode, proxy_base,
node_base, args and the $(DASHBOARD_BINARY) invocation), then exit with code 1
as before.
In `@tools/network-dashboard/main.go`:
- Around line 250-255: The port arrays used when *proxyBase and *nodeBase are
non-empty do not match the flag help text: update the ports slices so proxy
ports append :8081 and :8082 (instead of only :8080) and node ports append
:9091,:9092,:9093,:9094 (instead of a single :8081), then build URLs and call
fetchProxyInfo/fetchNodeInfo as before; specifically modify the ports variable
in the proxy block (where proxies = append(proxies, fetchProxyInfo(...))) and
the ports variable in the node block (where nodes = append(nodes,
fetchNodeInfo(...))) to match the documented port lists.
- Around line 241-282: The current loops call fetchProxyInfo and fetchNodeInfo
sequentially causing slow startup; change to concurrent fetching by launching
goroutines that call fetchProxyInfo/fetchNodeInfo, use a sync.WaitGroup to wait
for all goroutines, and send results into buffered channels (one for proxies,
one for nodes) sized to the number of endpoints, then close channels and drain
them into the proxies and nodes slices; apply this pattern in the blocks that
build proxies (the branches using local, proxyBase, proxyURLs) and nodes
(nodeBase, nodeURLs) and ensure you trim/skip empty URLs and preserve any naming
logic (fmt.Sprintf("proxy-%d", ...), fmt.Sprintf("p2pnode-%d", ...)).
- Around line 241-249: The local slices named proxyURLs and nodeURLs in main
shadow the flag pointer variables of the same names; rename the local slice
variables (for example to proxyAddrs or proxyURLsLocal and nodeAddrs or
nodeURLsLocal) and update their for-loops that call fetchProxyInfo and
fetchNodeInfo to use the new names so you no longer shadow the flag pointer
variables.
- Around line 221-226: Remove the custom min function implementation (func
min(a, b int) int) and replace its usages with the Go 1.21 builtin min; locate
the custom function named "min" and any call sites (e.g., the call that
currently invokes min) and update them to use the builtin min(a, b) instead,
then delete the redundant function definition to avoid shadowing the standard
builtin.
📜 Review details
Configuration used: Repository: getoptimum/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
.gitignoreis excluded by!.gitignoreand included by**.gitignoretools/network-dashboard/go.modis excluded by none and included by none
📒 Files selected for processing (2)
Makefiletools/network-dashboard/main.go
🧰 Additional context used
📓 Path-based instructions (3)
**/*
⚙️ 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:
Makefiletools/network-dashboard/main.go
Makefile*
⚙️ CodeRabbit configuration file
- Provide phony targets; stable reproducible builds; avoid environment-specific paths.
Files:
Makefile
**/*.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:
tools/network-dashboard/main.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:
tools/network-dashboard/main.go
🪛 checkmake (0.2.2)
Makefile
[warning] 125-125: Target body for "dashboard" exceeds allowed length of 5 (17).
(maxbodylength)
[warning] 152-152: Missing required phony target "all"
(minphony)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test Docker Setup
🔇 Additional comments (7)
Makefile (3)
28-30: LGTM!Build target is correctly structured, consistent with other binary targets in the Makefile.
48-48: LGTM!Dashboard binary correctly added to the
buildtarget dependencies.
145-145: LGTM!Clean target properly updated to remove the new dashboard binary.
tools/network-dashboard/main.go (4)
65-83: LGTM!
fetchJSONcorrectly uses a timeout, checks HTTP status, and handles errors. The 5-second timeout is reasonable for a CLI dashboard tool.
14-63: LGTM!Type definitions are clean with appropriate JSON tags. Using pointers for
HealthandStateinNodeInfo/ProxyInfocorrectly signals optional data.
120-219: LGTM!
printDashboardhandles nil/unavailable cases gracefully and produces well-formatted output. The peer truncation logic (showing first 5 with a "...and N more" message) is a nice touch.
284-288: LGTM!Good input validation with clear error message and usage hint when no endpoints are configured.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@tools/network-dashboard/main.go`:
- Around line 206-208: The loop over the map countryCount is non-deterministic;
to make output stable, collect the map keys into a slice (e.g., countries), sort
them with sort.Strings, and then iterate over the sorted slice to print
fmt.Printf("%s: %d node(s)\n", country, countryCount[country]); update the
for-range block that currently iterates directly over countryCount to use this
sorted-key approach and import "sort" if not already present.
- Around line 248-249: The loop is appending each proxy twice by calling proxies
= append(proxies, fetchProxyInfo(fmt.Sprintf("proxy-%d", i+1), url)) two times;
fix it by removing the duplicate append so each iteration only calls
fetchProxyInfo once (update the code that populates the proxies slice where
fetchProxyInfo and fmt.Sprintf("proxy-%d", i+1) are used to ensure a single
append per proxy).
- Around line 59-61: fetchJSON currently creates a new http.Client per call
which prevents connection reuse; refactor to use a shared client by moving the
client creation out of fetchJSON (either declare a package-level variable like
var httpClient = &http.Client{Timeout: 5 * time.Second} or accept an
*http.Client parameter) and update fetchJSON to use that shared client
(referencing fetchJSON and the http.Client instance) so requests can reuse
connections and improve performance.
♻️ Duplicate comments (3)
tools/network-dashboard/main.go (3)
215-220: Remove redundantminfunction—Go 1.21 builtin exists.This was flagged in a previous review. The custom implementation shadows the builtin and should be deleted.
236-243: Variable shadowing issue remains unaddressed.Local slices
proxyURLsandnodeURLsshadow the flag pointer variables declared at lines 224-225. Rename tolocalProxyURLs/localNodeURLsas suggested in the previous review.
244-267: Port configuration still inconsistent with flag documentation.Previously flagged:
-proxy-basehelp says:8081,:8082but code uses:8080.-node-basehelp says:9091,:9092,:9093,:9094but code uses:8081. Either fix the ports or update the documentation.
📜 Review details
Configuration used: Repository: getoptimum/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tools/network-dashboard/main.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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:
tools/network-dashboard/main.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:
tools/network-dashboard/main.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:
tools/network-dashboard/main.go
🔇 Additional comments (3)
tools/network-dashboard/main.go (3)
1-57: LGTM on imports and type definitions.Clean structure using only standard library. The types are well-organized with appropriate JSON tags for serialization.
79-112: LGTM on fetch functions.Error handling is correct—
Availableremainsfalseon failure, and errors are captured for display. The two-step fetch infetchNodeInfoappropriately short-circuits on first failure.
285-291: LGTM on optional country enrichment.Using only the first available proxy for node countries is a reasonable simplification. Silent error handling is acceptable here since this data is supplementary.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tools/network-dashboard/main.go`:
- Around line 292-298: The code only attempts node-countries lookup using
proxies[0] which skips country data if that proxy is down; update the logic
around nodeCountries, proxies and fetchJSON to iterate over the proxies slice,
pick the first proxy with Available==true, and attempt
fetchJSON(proxy.URL+"/api/v1/node-countries", nc) until a fetch succeeds (set
nodeCountries = nc) or all available proxies fail; ensure you reference the
proxies slice, the Available field, the fetchJSON function and the nodeCountries
variable when making the change so the first healthy proxy that returns valid
JSON is used.
- Around line 216-301: Add unit tests covering fetchJSON, fetchNodeInfo,
fetchProxyInfo, and printDashboard: create table-driven tests that use httptest
servers to return valid JSON, malformed JSON, HTTP errors, and delayed responses
to simulate timeouts (for fetchJSON and the fetch* functions), assert returned
structs and error handling, and verify that fetchProxyInfo/fetchNodeInfo
correctly set fields like URL and Available; for printDashboard, capture
stdout/stderr using a bytes.Buffer and assert output contains expected headers,
node/proxy entries, and handles empty nodeCountries or empty node/proxy slices.
Use subtests for each edge case and ensure tests cleanly shut down servers and
restore any global state/timeouts.
♻️ Duplicate comments (2)
tools/network-dashboard/main.go (2)
203-209: Make country output deterministic.Map iteration order is non-deterministic; sort keys before printing for stable output.
229-283: Avoid sequential network fetches for proxies and nodes.Current loops block per endpoint; concurrent fetches would reduce total wait time.
📜 Review details
Configuration used: Repository: getoptimum/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (3)
grpc_p2p_client/go.modis excluded by none and included by nonegrpc_proxy_client/go.modis excluded by none and included by nonekeygen/go.modis excluded by none and included by none
📒 Files selected for processing (1)
tools/network-dashboard/main.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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:
tools/network-dashboard/main.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:
tools/network-dashboard/main.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:
tools/network-dashboard/main.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test Docker Setup
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.