Skip to content

Conversation

@mateeullahmalik
Copy link
Collaborator

No description provided.

@roomote
Copy link

roomote bot commented Dec 30, 2025

Rooviewer Clock   See task on Roo Cloud

All issues resolved. LGTM!

  • p2p/p2p_stats.go:50-57 - Error from ristretto.NewCache is discarded, which could lead to silent cache failures
Previous reviews

Mention @roomote in a comment to request specific changes to this pull request or fix all unresolved issues.

- Replace map-based Client.Stats with StatsSnapshot
- Add DHT peer/network/db snapshot helpers and typed store stats
- Update status + metrics reporting; warn when peers_count=0
roomote[bot]
roomote bot previously approved these changes Dec 30, 2025
a-ok123
a-ok123 previously approved these changes Dec 30, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors P2P statistics collection to improve performance and type safety by introducing a typed snapshot approach with caching, replacing the previous map[string]interface{} pattern. It also increases the startup delay from 30 to 300 seconds and adds a peers_count check to health reporting.

Key Changes

  • Introduced StatsSnapshot struct and p2pStatsManager with ristretto-based caching to serve stats with predictable latency
  • Replaced untyped map-based stats with typed methods (PeersSnapshot, DatabaseStats, NetworkHandleMetricsSnapshot) across the kademlia layer
  • Modified health check to require both open ports AND non-zero peer count, and increased startup delay 10x to 5 minutes

Reviewed changes

Copilot reviewed 15 out of 16 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
p2p/stats_snapshot.go New typed struct defining the P2P stats snapshot schema
p2p/p2p_stats.go New stats manager implementing caching layer with background refresh and last-known-good pattern
p2p/p2p.go Updated Stats() method signature to return typed snapshot, added stats manager initialization
p2p/client.go Updated Stats() interface to return typed snapshot
p2p/mocks/Client.go Updated mock to match new Stats() signature
p2p/kademlia/dht.go Refactored stats methods into typed snapshots (PeersSnapshot, NetworkHandleMetricsSnapshot, DatabaseStats)
p2p/kademlia/hashtable.go Added peersCount() method for fast count without allocating peer list
p2p/kademlia/store.go Introduced DatabaseStats struct for typed database statistics
p2p/kademlia/store/sqlite/sqlite.go Updated Stats() to return DatabaseStats struct instead of map
p2p/kademlia/store/mem/mem.go Updated Stats() to return DatabaseStats struct instead of map
supernode/status/service.go Simplified stats consumption using typed snapshot, moved Network population inside includeP2PMetrics conditional
supernode/supernode_metrics/monitor_service.go Added peers_count > 0 check to health validation, updated warning message
supernode/supernode_metrics/constants.go Increased DefaultStartupDelaySeconds from 30 to 300 seconds
docs/gateway.md Added documentation explaining the caching behavior and latency characteristics
README.md Updated API documentation to clarify P2P metrics gating and caching approach
.gitignore Added analysis directory to ignore list

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@mateeullahmalik mateeullahmalik dismissed stale reviews from a-ok123 and roomote[bot] via 5f4d583 December 30, 2025 21:40
@mateeullahmalik mateeullahmalik merged commit 738c4b2 into master Dec 30, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants