Merge pull request #2329 from CortexFoundation/dev

p2p: fix dial metrics not picking up the right error

Author: ucwong

p2p/metrics.go

@@ -45,11 +45,11 @@ var (
 	egressTrafficMeter  = metrics.NewRegisteredMeter("p2p/egress", nil)
 
 	// general ingress/egress connection meters
-	serveMeter          metrics.Meter = metrics.NilMeter{}
-	serveSuccessMeter   metrics.Meter = metrics.NilMeter{}
-	dialMeter           metrics.Meter = metrics.NilMeter{}
-	dialSuccessMeter    metrics.Meter = metrics.NilMeter{}
-	dialConnectionError metrics.Meter = metrics.NilMeter{}
+	serveMeter          = metrics.NewRegisteredMeter("p2p/serves", nil)
+	serveSuccessMeter   = metrics.NewRegisteredMeter("p2p/serves/success", nil)
+	dialMeter           = metrics.NewRegisteredMeter("p2p/dials", nil)
+	dialSuccessMeter    = metrics.NewRegisteredMeter("p2p/dials/success", nil)
+	dialConnectionError = metrics.NewRegisteredMeter("p2p/dials/error/connection", nil) // dial timeout; no route to host; connection refused; network is unreachable
 
 	// count peers that stayed connected for at least 1 min
 	serve1MinSuccessMeter = metrics.NewRegisteredMeter("p2p/serves/success/1min", nil)
@@ -61,49 +61,41 @@ var (
 	dialSelf                = metrics.NewRegisteredMeter("p2p/dials/error/self", nil)
 	dialUselessPeer         = metrics.NewRegisteredMeter("p2p/dials/error/useless", nil)
 	dialUnexpectedIdentity  = metrics.NewRegisteredMeter("p2p/dials/error/id/unexpected", nil)
-	dialEncHandshakeError   = metrics.NewRegisteredMeter("p2p/dials/error/rlpx/enc", nil)
-	dialProtoHandshakeError = metrics.NewRegisteredMeter("p2p/dials/error/rlpx/proto", nil)
-)
-
-func init() {
-	if !metrics.Enabled {
-		return
-	}
+	dialEncHandshakeError   = metrics.NewRegisteredMeter("p2p/dials/error/rlpx/enc", nil)   // EOF; connection reset during handshake; message too big; i/o timeout
+	dialProtoHandshakeError = metrics.NewRegisteredMeter("p2p/dials/error/rlpx/proto", nil) // EOF
 
-	activePeerGauge = metrics.NewRegisteredGauge("p2p/peers", nil)
-	activeInboundPeerGauge = metrics.NewRegisteredGauge("p2p/peers/inbound", nil)
-	activeOutboundPeerGauge = metrics.NewRegisteredGauge("p2p/peers/outbound", nil)
-	serveMeter = metrics.NewRegisteredMeter("p2p/serves", nil)
-	serveSuccessMeter = metrics.NewRegisteredMeter("p2p/serves/success", nil)
-	dialMeter = metrics.NewRegisteredMeter("p2p/dials", nil)
-	dialSuccessMeter = metrics.NewRegisteredMeter("p2p/dials/success", nil)
-	dialConnectionError = metrics.NewRegisteredMeter("p2p/dials/error/connection", nil)
-}
+	// capture the rest of errors that are not handled by the above meters
+	dialOtherError = metrics.NewRegisteredMeter("p2p/dials/error/other", nil)
+)
 
-// markDialError matches errors that occur while setting up a dial connection
-// to the corresponding meter.
+// markDialError matches errors that occur while setting up a dial connection to the
+// corresponding meter. We don't maintain meters for evert possible error, just for
+// the most interesting ones.
 func markDialError(err error) {
 	if !metrics.Enabled {
 		return
 	}
-	if err2 := errors.Unwrap(err); err2 != nil {
-		err = err2
-	}
-	switch err {
-	case DiscTooManyPeers:
+
+	var reason DiscReason
+	var handshakeErr *protoHandshakeError
+	d := errors.As(err, &reason)
+	switch {
+	case d && reason == DiscTooManyPeers:
 		dialTooManyPeers.Mark(1)
-	case DiscAlreadyConnected:
+	case d && reason == DiscAlreadyConnected:
 		dialAlreadyConnected.Mark(1)
-	case DiscSelf:
+	case d && reason == DiscSelf:
 		dialSelf.Mark(1)
-	case DiscUselessPeer:
+	case d && reason == DiscUselessPeer:
 		dialUselessPeer.Mark(1)
-	case DiscUnexpectedIdentity:
+	case d && reason == DiscUnexpectedIdentity:
 		dialUnexpectedIdentity.Mark(1)
-	case errEncHandshakeError:
-		dialEncHandshakeError.Mark(1)
-	case errProtoHandshakeError:
+	case errors.As(err, &handshakeErr):
 		dialProtoHandshakeError.Mark(1)
+	case errors.Is(err, errEncHandshakeError):
+		dialEncHandshakeError.Mark(1)
+	default:
+		dialOtherError.Mark(1)
 	}
 }
 

p2p/server.go

@@ -66,11 +66,15 @@ const (
 )
 
 var (
-	errServerStopped       = errors.New("server stopped")
-	errEncHandshakeError   = errors.New("rlpx enc error")
-	errProtoHandshakeError = errors.New("rlpx proto error")
+	errServerStopped     = errors.New("server stopped")
+	errEncHandshakeError = errors.New("rlpx enc error")
 )
 
+type protoHandshakeError struct{ err error }
+
+func (e *protoHandshakeError) Error() string { return fmt.Sprintf("rlpx proto error: %v", e.err) }
+func (e *protoHandshakeError) Unwrap() error { return e.err }
+
 // Server manages all peer connections.
 type Server struct {
 	// Config fields may not be modified while the server is running.
@@ -907,7 +911,7 @@ func (srv *Server) setupConn(c *conn, dialDest *enode.Node) error {
 	phs, err := c.doProtoHandshake(srv.ourHandshake)
 	if err != nil {
 		clog.Trace("Failed p2p handshake", "err", err)
-		return fmt.Errorf("%w: %v", errProtoHandshakeError, err)
+		return &protoHandshakeError{err: err}
 	}
 	if id := c.node.ID(); !bytes.Equal(crypto.Keccak256(phs.ID), id[:]) {
 		clog.Trace("Wrong devp2p handshake identity", "phsid", hex.EncodeToString(phs.ID))

p2p/server_test.go

@@ -410,11 +410,11 @@ func TestServerSetupConn(t *testing.T) {
 			wantCloseErr: DiscUnexpectedIdentity,
 		},
 		{
-			tt:           &setupTransport{pubkey: clientpub, protoHandshakeErr: errProtoHandshakeError},
+			tt:           &setupTransport{pubkey: clientpub, protoHandshakeErr: DiscTooManyPeers},
 			dialDest:     enode.NewV4(clientpub, nil, 0, 0),
 			flags:        dynDialedConn,
 			wantCalls:    "doEncHandshake,doProtoHandshake,close,",
-			wantCloseErr: errProtoHandshakeError,
+			wantCloseErr: DiscTooManyPeers,
 		},
 		{
 			tt:           &setupTransport{pubkey: srvpub, phs: protoHandshake{ID: crypto.FromECDSAPub(srvpub)[1:]}},