refactor(networking): peermanager refactor and cleanups #1539
Conversation
alrevuelta
force-pushed
the
misc-refactors
branch
4 times, most recently
from
0cf7aa8
to
e9ede91
Compare
There was a problem hiding this comment.
Please check my comments 🤔
A side note:
IMO, the next thing we must do, after this PR and before evolving anything else, is to unify the "Peer" data model. We have 3/4 data models (e.g., peer multiaddress, Ethereum and Waku ENRs,
PeerId,RemotePeerId, etc.). This fragmentation makes the code complex and "Unexpected exceptions/defect" prone.
| info "Relay peer connections", | ||
| connectedPeers = numConPeers, | ||
| targetConnectedPeers = maxConnections, | ||
| notConnectedPeers = notConnectedPeers.len, | ||
| outsideBackoffPeers = outsideBackoffPeers.len |
There was a problem hiding this comment.
Is this logged periodically? If so, we must put it behind a configurable flag (like the "poor-man's metrics" log in wakunode2) or lower the log level. This kind of thing pollutes the logs, and we already have it in the metrics.
There was a problem hiding this comment.
The thing is that as it is now, I can't log some of this information in the periodic metrics startMetricsLog.
I have some plans for this as well, so will address in future Prs.
| proc protocolMatcher*(codec: string): Matcher = | ||
| ## Returns a protocol matcher function for the provided codec | ||
| proc match(proto: string): bool {.gcsafe.} = | ||
| ## Matches a proto with any postfix to the provided codec. | ||
| ## E.g. if the codec is `/vac/waku/filter/2.0.0` it matches the protos: | ||
| ## `/vac/waku/filter/2.0.0`, `/vac/waku/filter/2.0.0-beta3`, `/vac/waku/filter/2.0.0-actualnonsense` | ||
| return proto.startsWith(codec) | ||
|
|
||
| return match |
There was a problem hiding this comment.
I noticed you moved this method to the peer_manager module. Is there any reason for this method to be exposed in this module's public API?
| proc protocolMatcher*(codec: string): Matcher = | |
| ## Returns a protocol matcher function for the provided codec | |
| proc match(proto: string): bool {.gcsafe.} = | |
| ## Matches a proto with any postfix to the provided codec. | |
| ## E.g. if the codec is `/vac/waku/filter/2.0.0` it matches the protos: | |
| ## `/vac/waku/filter/2.0.0`, `/vac/waku/filter/2.0.0-beta3`, `/vac/waku/filter/2.0.0-actualnonsense` | |
| return proto.startsWith(codec) | |
| return match | |
| proc protocolMatcher(codec: string): Matcher = | |
| ## Returns a protocol matcher function for the provided codec. The protocol matcher | |
| ## searches for protocol IDs with any postfix. | |
| ## E.g. if the codec is `/vac/waku/filter/2.0.0`, it matches the protocol IDs: | |
| ## `/vac/waku/filter/2.0.0`, `/vac/waku/filter/2.0.0-beta3`, `/vac/waku/filter/2.0.0-actualnonsense` | |
| proc match(proto: string): bool {.gcsafe.} = | |
| proto.startsWith(codec) | |
| return match |
There was a problem hiding this comment.
mm good catch, will remove it
There was a problem hiding this comment.
wops, actually used by the waku_node in startRelay. adding to the todo list since reconnection logic should be in peermanager side, and waku_node should just call a high level function.
| declarePublicGauge waku_peers_errors, "Number of peer manager errors", ["type"] | ||
| declarePublicGauge waku_connected_peers, "Number of connected peers per direction: inbound|outbound", ["direction"] | ||
| declarePublicGauge waku_peer_store_size, "Number of peers managed by the peer store" | ||
| declarePublicGauge waku_service_peers, "Configured waku service peers protocol/peerId", labels = ["protocol", "peerId"] |
There was a problem hiding this comment.
Note that this description is returned by the metrics server. The lighter we make them, the better. Shorter the description of the metric, the better.
| declarePublicGauge waku_peers_errors, "Number of peer manager errors", ["type"] | |
| declarePublicGauge waku_connected_peers, "Number of connected peers per direction: inbound|outbound", ["direction"] | |
| declarePublicGauge waku_peer_store_size, "Number of peers managed by the peer store" | |
| declarePublicGauge waku_service_peers, "Configured waku service peers protocol/peerId", labels = ["protocol", "peerId"] | |
| declarePublicGauge waku_peers_errors, "Number of peer manager errors", ["type"] | |
| declarePublicGauge waku_connected_peers, "Number of connected peers per direction", ["direction"] | |
| declarePublicGauge waku_peer_store_size, "Number of peers managed by the peer store" | |
| declarePublicGauge waku_service_peers, "Number of service peers per protocol", labels = ["protocol", "peerId"] |
There was a problem hiding this comment.
sure, can simplify it a bit but note that Number of service peers per protocol is not correct since its not the number but the protocol and peerId
alrevuelta
force-pushed
the
misc-refactors
branch
from
e9ede91
to
abb6ecc
Compare
yup, added it to #1353 to track it. |
Changes:
addServicePeerwhere neededprotofromaddPeer. From now on that is populated byIdentify.TRACEadding peer to peermanager, since it was quite spammy.connectToRelayPeersinto function + loop.