Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

p2p: refactor connectedness logger #1001

Merged
merged 3 commits into from
Aug 21, 2022
Merged

p2p: refactor connectedness logger #1001

merged 3 commits into from
Aug 21, 2022

Conversation

corverroos
Copy link
Contributor

Refactors the p2p connectedness logging:

  • Use ping service as thing that logs whether peer X is connected or not.
  • Extract dial error reasons per address
  • Attempt to resolve "dial backoff" errors into "real" dial errors.
  • Only log when reasons change or every 10min.

category: refactor
ticket: #986

@codecov
Copy link

codecov bot commented Aug 19, 2022

Codecov Report

Merging #1001 (845b9f7) into main (a3aa492) will increase coverage by 0.35%.
The diff coverage is 31.93%.

@@            Coverage Diff             @@
##             main    #1001      +/-   ##
==========================================
+ Coverage   53.80%   54.16%   +0.35%     
==========================================
  Files         117      118       +1     
  Lines       13141    13215      +74     
==========================================
+ Hits         7071     7158      +87     
+ Misses       5044     5030      -14     
- Partials     1026     1027       +1     
Impacted Files Coverage Δ
p2p/discovery.go 32.59% <0.00%> (ø)
p2p/name.go 68.75% <ø> (ø)
p2p/ping.go 0.00% <0.00%> (ø)
p2p/errors.go 85.00% <85.00%> (ø)
p2p/sender.go 56.89% <100.00%> (+1.53%) ⬆️
p2p/gater.go 81.08% <0.00%> (+8.10%) ⬆️
core/qbft/qbft.go 81.97% <0.00%> (+10.30%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@@ -291,7 +291,7 @@ func NewDiscoveryRouter(tcpNode host.Host, udpNode *MutableUDPNode, peers []Peer
// it returns false if the peer isn't discovered.
func getDiscoveredAddress(udpNode *MutableUDPNode, p Peer) (ma.Multiaddr, bool, error) {
resolved := udpNode.Resolve(&p.Enode)
if resolved.Seq() == 0 || resolved.TCP() == 0 {
if resolved.Seq() == p.Enode.Seq() || resolved.TCP() == 0 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do not consider lock file ENRs as "discovered".

@@ -88,7 +88,11 @@ func (s *Sender) addResult(ctx context.Context, peerID peer.ID, err error) {
}
} else if failure && (len(state.buffer) == 1 || !state.failing) {
// First attempt failed or state changed to failing
log.Warn(ctx, "P2P sending failing", err, z.Str("peer", PeerName(peerID)))

if _, ok := dialErrMsgs(err); !ok { // Only log non-dial errors
Copy link
Contributor Author

Choose a reason for hiding this comment

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

dial errors exclusively logged by ping service

p2p/errors.go Outdated
"github.com/obolnetwork/charon/app/errors"
)

// hasErrDialBackoff returns true if the error is contains swarm.ErrDialBackoff.
Copy link
Contributor

@xenowits xenowits Aug 20, 2022

Choose a reason for hiding this comment

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

Suggested change
// hasErrDialBackoff returns true if the error is contains swarm.ErrDialBackoff.
// hasErrDialBackoff returns true if the error is or contains swarm.ErrDialBackoff.

@corverroos corverroos added the merge when ready Indicates bulldozer bot may merge when all checks pass label Aug 21, 2022
@obol-bulldozer obol-bulldozer bot merged commit 9b1b974 into main Aug 21, 2022
@obol-bulldozer obol-bulldozer bot deleted the corver/pinglog branch August 21, 2022 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge when ready Indicates bulldozer bot may merge when all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants