Address PR comments and add in more user feedback

core/commands/ping.go

@@ -2,7 +2,6 @@ package commands
 
 import (
 	"bytes"
-	"errors"
 	"fmt"
 	"io"
 	"time"
@@ -19,6 +18,7 @@ const kPingTimeout = 10 * time.Second
 type PingResult struct {
 	Success bool
 	Time    time.Duration
+	Text    string
 }
 
 var PingCmd = &cmds.Command{
@@ -33,7 +33,10 @@ send pings, wait for pongs, and print out round-trip latency information.
 `,
 	},
 	Arguments: []cmds.Argument{
-		cmds.StringArg("count", false, true, "Number of pings to perform"),
+		cmds.StringArg("peer ID", true, true, "ID of peer to be pinged"),
+	},
+	Options: []cmds.Option{
+		cmds.IntOption("count", "n"),
 	},
 	Marshalers: cmds.MarshalerMap{
 		cmds.Text: func(res cmds.Response) (io.Reader, error) {
@@ -49,7 +52,9 @@ send pings, wait for pongs, and print out round-trip latency information.
 				}
 
 				buf := new(bytes.Buffer)
-				if obj.Success {
+				if len(obj.Text) > 0 {
+					buf = bytes.NewBufferString(obj.Text + "\n")
+				} else if obj.Success {
 					fmt.Fprintf(buf, "Pong took %.2fms\n", obj.Time.Seconds()*1000)
 				} else {
 					fmt.Fprintf(buf, "Pong failed\n")
@@ -75,9 +80,11 @@ send pings, wait for pongs, and print out round-trip latency information.
 		}
 
 		if len(req.Arguments()) == 0 {
-			return nil, errors.New("no peer specified!")
+			return nil, cmds.ClientError("no peer specified!")
 		}
 
+		outChan := make(chan interface{}, 5)
+
 		// Set up number of pings
 		numPings := 10
 		val, found, err := req.Option("count").Int()
@@ -94,33 +101,44 @@ send pings, wait for pongs, and print out round-trip latency information.
 			return nil, err
 		}
 
-		// Make sure we can find the node in question
-		ctx, _ := context.WithTimeout(context.Background(), kPingTimeout)
-		p, err := n.Routing.FindPeer(ctx, peerID)
-		if err != nil {
-			return nil, err
-		}
-
-		outChan := make(chan interface{})
-
 		go func() {
 			defer close(outChan)
+
+			// Make sure we can find the node in question
+			outChan <- &PingResult{
+				Text: fmt.Sprintf("Looking up peer %s", peerID.Pretty()),
+			}
+			ctx, _ := context.WithTimeout(context.Background(), kPingTimeout)
+			p, err := n.Routing.FindPeer(ctx, peerID)
+			if err != nil {
+				outChan <- &PingResult{Text: "Peer lookup error!"}
+				outChan <- &PingResult{Text: err.Error()}
+				return
+			}
+			outChan <- &PingResult{
+				Text: fmt.Sprintf("Peer found, starting pings."),
+			}
+
+			var total time.Duration
 			for i := 0; i < numPings; i++ {
 				ctx, _ = context.WithTimeout(context.Background(), kPingTimeout)
-				before := time.Now()
-				err := n.Routing.Ping(ctx, p.ID)
+				took, err := n.Routing.Ping(ctx, p.ID)
 				if err != nil {
 					log.Errorf("Ping error: %s", err)
 					outChan <- &PingResult{}
 					break
 				}
-				took := time.Now().Sub(before)
 				outChan <- &PingResult{
 					Success: true,
 					Time:    took,
 				}
+				total += took
 				time.Sleep(time.Second)
 			}
+			averagems := total.Seconds() * 1000 / float64(numPings)
+			outChan <- &PingResult{
+				Text: fmt.Sprintf("Average latency: %.2fms", averagems),
+			}
 		}()
 
 		return outChan, nil

routing/dht/dht.go

@@ -103,8 +103,8 @@ func (dht *IpfsDHT) Connect(ctx context.Context, npeer peer.ID) error {
 
 	// Ping new peer to register in their routing table
 	// NOTE: this should be done better...
-	if err := dht.Ping(ctx, npeer); err != nil {
-		return fmt.Errorf("failed to ping newly connected peer: %s\n", err)
+	if _, err := dht.Ping(ctx, npeer); err != nil {
+		return fmt.Errorf("failed to ping newly connected peer: %s", err)
 	}
 	log.Event(ctx, "connect", dht.self, npeer)
 	dht.Update(ctx, npeer)
@@ -329,7 +329,7 @@ func (dht *IpfsDHT) PingRoutine(t time.Duration) {
 			peers := dht.routingTable.NearestPeers(kb.ConvertKey(u.Key(id)), 5)
 			for _, p := range peers {
 				ctx, _ := context.WithTimeout(dht.Context(), time.Second*5)
-				err := dht.Ping(ctx, p)
+				_, err := dht.Ping(ctx, p)
 				if err != nil {
 					log.Errorf("Ping error: %s", err)
 				}

routing/dht/routing.go

@@ -3,6 +3,7 @@ package dht
 import (
 	"math"
 	"sync"
+	"time"
 
 	context "github.com/jbenet/go-ipfs/Godeps/_workspace/src/code.google.com/p/go.net/context"
 
@@ -434,12 +435,14 @@ func (dht *IpfsDHT) FindPeersConnectedToPeer(ctx context.Context, id peer.ID) (<
 }
 
 // Ping a peer, log the time it took
-func (dht *IpfsDHT) Ping(ctx context.Context, p peer.ID) error {
+func (dht *IpfsDHT) Ping(ctx context.Context, p peer.ID) (time.Duration, error) {
 	// Thoughts: maybe this should accept an ID and do a peer lookup?
 	log.Debugf("ping %s start", p)
+	before := time.Now()
 
 	pmes := pb.NewMessage(pb.Message_PING, "", 0)
 	_, err := dht.sendRequest(ctx, p, pmes)
 	log.Debugf("ping %s end (err = %s)", p, err)
-	return err
+
+	return time.Now().Sub(before), err
 }

routing/mock/centralized_client.go

@@ -2,6 +2,7 @@ package mockrouting
 
 import (
 	"errors"
+	"time"
 
 	context "github.com/jbenet/go-ipfs/Godeps/_workspace/src/code.google.com/p/go.net/context"
 	ds "github.com/jbenet/go-ipfs/Godeps/_workspace/src/github.com/jbenet/go-datastore"
@@ -79,8 +80,8 @@ func (c *client) Provide(_ context.Context, key u.Key) error {
 	return c.server.Announce(info, key)
 }
 
-func (c *client) Ping(ctx context.Context, p peer.ID) error {
-	return nil
+func (c *client) Ping(ctx context.Context, p peer.ID) (time.Duration, error) {
+	return 0, nil
 }
 
 var _ routing.IpfsRouting = &client{}

routing/routing.go

@@ -3,6 +3,7 @@ package routing
 
 import (
 	"errors"
+	"time"
 
 	context "github.com/jbenet/go-ipfs/Godeps/_workspace/src/code.google.com/p/go.net/context"
 
@@ -38,5 +39,5 @@ type IpfsRouting interface {
 	FindPeer(context.Context, peer.ID) (peer.PeerInfo, error)
 
 	// Ping a peer, log the time it took
-	Ping(context.Context, peer.ID) error
+	Ping(context.Context, peer.ID) (time.Duration, error)
 }