chore (supervisor): add global metrics

[itschaindev]

Closes #2714 and #2713

Dashboard preview:

This should also work with other Kona services.

[claude]

Claude finished @itschaindev's task —— View job

---

PR Review: Global Metrics Implementation

Reviewing PR #2738 for code quality, security, performance, and test coverage.

Todo List:

• Analyze core code changes in prometheus.rs
• Review dependency additions in Cargo files
• Check for security concerns
• Assess performance implications
• Evaluate test coverage
• Review Grafana dashboard changes
• Provide comprehensive feedback

---

🔍 Code Quality & Best Practices

Strong Points:

• Clean separation of concerns with dedicated MetricsArgs and prometheus.rs modules
• Good use of environment variables for configuration (KONA_METRICS_*)
• Comprehensive test coverage in metrics_args.rs:55-97 with unit tests for CLI parsing
• Proper error handling with Result<(), BuildError> return types
• Good documentation with rustdoc comments

Areas for Improvement:

• Thread Safety: crates/utilities/cli/src/prometheus.rs:23-28 spawns an unbounded thread without proper shutdown mechanism. Consider using a JoinHandle and graceful shutdown signal.
• Resource Management: The spawned thread runs an infinite loop with no way to stop it cleanly
• Error Handling: The collector thread ignores potential errors from collector.collect()

🐛 Potential Issues

1. Thread Lifecycle: prometheus.rs:23-28 - The background thread lacks proper lifecycle management:

   thread::spawn(move || {
       loop {
           collector.collect(); // No error handling
           sleep(Duration::from_secs(1));
       }
   }); // No handle stored for cleanup

2. Default Port Conflict: metrics_args.rs:22 defaults to port 9090, which is Prometheus's standard port. This could cause conflicts in containerized environments.

🚀 Performance Considerations

Positive:

• 1-second collection interval is reasonable for system metrics
• Uses metrics-process for efficient system metrics collection
• Minimal overhead with prometheus exporter

Concerns:

• No backpressure handling if metrics collection becomes slow
• Thread spawn occurs on every init_prometheus_server call without checking if already initialized

🔒 Security Assessment

Good Security Practices:

• Configurable bind address (defaults to 0.0.0.0 but can be restricted)
• No sensitive data exposure in metrics
• Uses standard prometheus metrics format

Minor Concerns:

• Default bind to 0.0.0.0:9090 exposes metrics to all interfaces - consider defaulting to 127.0.0.1

🧪 Test Coverage

Excellent Coverage:

• Comprehensive unit tests in metrics_args.rs:54-97
• Tests cover CLI parsing, defaults, and environment variables
• Integration with existing codebase is well-tested through supervisor CLI

Missing Tests:

• No integration tests for the actual metrics collection
• No tests for the prometheus server initialization
• Thread lifecycle behavior is untested

📊 Grafana Dashboard Review

Positive Changes:

• Adds essential system metrics: CPU usage, RAM usage, thread count, elapsed time
• Well-structured dashboard with appropriate visualizations
• Proper prometheus queries using process_* metrics
• Good visual hierarchy and layout

Technical Details:

• Uses standard process metrics (process_resident_memory_bytes, process_cpu_seconds_total, etc.)
• Proper time series queries with rate calculations
• Namespace filtering for kona-supervisor services

📈 Dependencies Assessment

New Dependencies Added:

• metrics-exporter-prometheus = "0.17.0" - Well-maintained, standard choice
• metrics-process = "2.4.0" - Lightweight system metrics collector

Assessment: Both dependencies are appropriate, well-maintained, and commonly used in the Rust ecosystem.

🎯 Overall Recommendation

✅ APPROVE with minor suggestions

This PR successfully implements global metrics with:

• Clean architecture and good separation of concerns
• Comprehensive CLI integration across supervisor and node
• Well-designed Grafana dashboard
• Good test coverage for configuration

Suggested Improvements:

1. Add graceful thread shutdown in prometheus.rs
2. Consider defaulting bind address to 127.0.0.1 for security
3. Add error handling in the metrics collection loop
4. Consider adding integration tests for metrics collection

The implementation follows Rust best practices and integrates well with the existing codebase architecture.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.1%. Comparing base (fc2a556) to head (9c1700d).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[emhane]

lgtm