Fix problem with metrics when ping return Destination net unreachable ( windows ) (#1561)

* Fix problem with metrics when ping return Destination net unreachable
Add test case TestUnreachablePingGather
Add percent_reply_loss
Fix some other tests

* Add errors measurment

* fir problem with ping reply "TTL expired in transit" ( use regex for more specific condition - TTL in line but it's a not valid replay )
add test case for "TTL expired in transit" - TestTTLExpiredPingGather

plugins/inputs/ping/README.md

@@ -0,0 +1,36 @@
+# Ping input plugin
+
+This input plugin will measures the round-trip
+
+## Windows:
+### Configration:
+```
+	## urls to ping
+	urls = ["www.google.com"] # required
+	
+	## number of pings to send per collection (ping -n <COUNT>)
+	count = 4 # required
+	
+	## Ping timeout, in seconds. 0 means default timeout (ping -w <TIMEOUT>)
+	Timeout = 0
+```
+### Measurements & Fields:
+- packets_transmitted ( from ping output )
+- reply_received ( increasing only on valid metric from echo replay, eg. 'Destination net unreachable' reply will increment packets_received but not reply_received )
+- packets_received ( from ping output )
+- percent_reply_loss ( compute from packets_transmitted and reply_received )
+- percent_packets_loss ( compute from packets_transmitted and packets_received )
+- errors ( when host can not be found or wrong prameters is passed to application )
+- response time
+    - average_response_ms ( compute from minimum_response_ms and maximum_response_ms )
+    - minimum_response_ms ( from ping output )
+    - maximum_response_ms ( from ping output )
+	
+### Tags:
+- server
+
+### Example Output:
+```
+* Plugin: ping, Collection 1
+ping,host=WIN-PBAPLP511R7,url=www.google.com average_response_ms=7i,maximum_response_ms=9i,minimum_response_ms=7i,packets_received=4i,packets_transmitted=4i,percent_packet_loss=0,percent_reply_loss=0,reply_received=4i 1469879119000000000
+```

plugins/inputs/ping/ping_windows.go

@@ -65,16 +65,20 @@ func hostPinger(timeout float64, args ...string) (string, error) {
 
 // processPingOutput takes in a string output from the ping command
 // based on linux implementation but using regex ( multilanguage support ) ( shouldn't affect the performance of the program )
-// It returns (<transmitted packets>, <received packets>, <average response>, <min response>, <max response>)
+// It returns (<transmitted packets>, <received reply>, <received packet>, <average response>, <min response>, <max response>)
-func processPingOutput(out string) (int, int, int, int, int, error) {
+func processPingOutput(out string) (int, int, int, int, int, int, error) {
 	// So find a line contain 3 numbers except reply lines
 	var stats, aproxs []string = nil, nil
 	err := errors.New("Fatal error processing ping output")
 	stat := regexp.MustCompile(`=\W*(\d+)\D*=\W*(\d+)\D*=\W*(\d+)`)
 	aprox := regexp.MustCompile(`=\W*(\d+)\D*ms\D*=\W*(\d+)\D*ms\D*=\W*(\d+)\D*ms`)
+	tttLine := regexp.MustCompile(`TTL=\d+`)
 	lines := strings.Split(out, "\n")
+	var receivedReply int = 0
 	for _, line := range lines {
-		if !strings.Contains(line, "TTL") {
+		if tttLine.MatchString(line) {
+			receivedReply++
+		} else {
 			if stats == nil {
 				stats = stat.FindStringSubmatch(line)
 			}
@@ -86,35 +90,35 @@ func processPingOutput(out string) (int, int, int, int, int, error) {
 
 	// stats data should contain 4 members: entireExpression + ( Send, Receive, Lost )
 	if len(stats) != 4 {
-		return 0, 0, 0, 0, 0, err
+		return 0, 0, 0, 0, 0, 0, err
 	}
 	trans, err := strconv.Atoi(stats[1])
 	if err != nil {
-		return 0, 0, 0, 0, 0, err
+		return 0, 0, 0, 0, 0, 0, err
 	}
-	rec, err := strconv.Atoi(stats[2])
+	receivedPacket, err := strconv.Atoi(stats[2])
 	if err != nil {
-		return 0, 0, 0, 0, 0, err
+		return 0, 0, 0, 0, 0, 0, err
 	}
 
 	// aproxs data should contain 4 members: entireExpression + ( min, max, avg )
 	if len(aproxs) != 4 {
-		return trans, rec, 0, 0, 0, err
+		return trans, receivedReply, receivedPacket, 0, 0, 0, err
 	}
 	min, err := strconv.Atoi(aproxs[1])
 	if err != nil {
-		return trans, rec, 0, 0, 0, err
+		return trans, receivedReply, receivedPacket, 0, 0, 0, err
 	}
 	max, err := strconv.Atoi(aproxs[2])
 	if err != nil {
-		return trans, rec, 0, 0, 0, err
+		return trans, receivedReply, receivedPacket, 0, 0, 0, err
 	}
 	avg, err := strconv.Atoi(aproxs[3])
 	if err != nil {
-		return 0, 0, 0, 0, 0, err
+		return 0, 0, 0, 0, 0, 0, err
 	}
 
-	return trans, rec, avg, min, max, err
+	return trans, receivedReply, receivedPacket, avg, min, max, err
 }
 
 func (p *Ping) timeout() float64 {
@@ -159,21 +163,30 @@ func (p *Ping) Gather(acc telegraf.Accumulator) error {
 				pendingError = errors.New(strings.TrimSpace(out) + ", " + err.Error())
 			}
 			tags := map[string]string{"url": u}
-			trans, rec, avg, min, max, err := processPingOutput(out)
+			trans, recReply, receivePacket, avg, min, max, err := processPingOutput(out)
 			if err != nil {
 				// fatal error
 				if pendingError != nil {
 					errorChannel <- pendingError
 				}
 				errorChannel <- err
+				fields := map[string]interface{}{
+					"errors": 100.0,
+				}
+
+				acc.AddFields("ping", fields, tags)
+
 				return
 			}
 			// Calculate packet loss percentage
-			loss := float64(trans-rec) / float64(trans) * 100.0
+			lossReply := float64(trans-recReply) / float64(trans) * 100.0
+			lossPackets := float64(trans-receivePacket) / float64(trans) * 100.0
 			fields := map[string]interface{}{
 				"packets_transmitted": trans,
-				"packets_received":    rec,
+				"reply_received":      recReply,
-				"percent_packet_loss": loss,
+				"packets_received":    receivePacket,
+				"percent_packet_loss": lossPackets,
+				"percent_reply_loss":  lossReply,
 			}
 			if avg > 0 {
 				fields["average_response_ms"] = avg

plugins/inputs/ping/ping_windows_test.go

@@ -38,18 +38,20 @@ Approximate round trip times in milli-seconds:
 `
 
 func TestHost(t *testing.T) {
-	trans, rec, avg, min, max, err := processPingOutput(winPLPingOutput)
+	trans, recReply, recPacket, avg, min, max, err := processPingOutput(winPLPingOutput)
 	assert.NoError(t, err)
 	assert.Equal(t, 4, trans, "4 packets were transmitted")
-	assert.Equal(t, 4, rec, "4 packets were received")
+	assert.Equal(t, 4, recReply, "4 packets were reply")
+	assert.Equal(t, 4, recPacket, "4 packets were received")
 	assert.Equal(t, 50, avg, "Average 50")
 	assert.Equal(t, 46, min, "Min 46")
 	assert.Equal(t, 57, max, "max 57")
 
-	trans, rec, avg, min, max, err = processPingOutput(winENPingOutput)
+	trans, recReply, recPacket, avg, min, max, err = processPingOutput(winENPingOutput)
 	assert.NoError(t, err)
 	assert.Equal(t, 4, trans, "4 packets were transmitted")
-	assert.Equal(t, 4, rec, "4 packets were received")
+	assert.Equal(t, 4, recReply, "4 packets were reply")
+	assert.Equal(t, 4, recPacket, "4 packets were received")
 	assert.Equal(t, 50, avg, "Average 50")
 	assert.Equal(t, 50, min, "Min 50")
 	assert.Equal(t, 52, max, "Max 52")
@@ -72,7 +74,9 @@ func TestPingGather(t *testing.T) {
 	fields := map[string]interface{}{
 		"packets_transmitted": 4,
 		"packets_received":    4,
+		"reply_received":      4,
 		"percent_packet_loss": 0.0,
+		"percent_reply_loss":  0.0,
 		"average_response_ms": 50,
 		"minimum_response_ms": 50,
 		"maximum_response_ms": 52,
@@ -113,7 +117,9 @@ func TestBadPingGather(t *testing.T) {
 	fields := map[string]interface{}{
 		"packets_transmitted": 4,
 		"packets_received":    0,
+		"reply_received":      0,
 		"percent_packet_loss": 100.0,
+		"percent_reply_loss":  100.0,
 	}
 	acc.AssertContainsTaggedFields(t, "ping", fields, tags)
 }
@@ -154,7 +160,9 @@ func TestLossyPingGather(t *testing.T) {
 	fields := map[string]interface{}{
 		"packets_transmitted": 9,
 		"packets_received":    7,
+		"reply_received":      7,
 		"percent_packet_loss": 22.22222222222222,
+		"percent_reply_loss":  22.22222222222222,
 		"average_response_ms": 115,
 		"minimum_response_ms": 114,
 		"maximum_response_ms": 119,
@@ -207,12 +215,114 @@ func TestFatalPingGather(t *testing.T) {
 	}
 
 	p.Gather(&acc)
-	assert.False(t, acc.HasMeasurement("packets_transmitted"),
+	assert.True(t, acc.HasFloatField("ping", "errors"),
+		"Fatal ping should have packet measurements")
+	assert.False(t, acc.HasIntField("ping", "packets_transmitted"),
 		"Fatal ping should not have packet measurements")
-	assert.False(t, acc.HasMeasurement("packets_received"),
+	assert.False(t, acc.HasIntField("ping", "packets_received"),
 		"Fatal ping should not have packet measurements")
-	assert.False(t, acc.HasMeasurement("percent_packet_loss"),
+	assert.False(t, acc.HasFloatField("ping", "percent_packet_loss"),
 		"Fatal ping should not have packet measurements")
-	assert.False(t, acc.HasMeasurement("average_response_ms"),
+	assert.False(t, acc.HasFloatField("ping", "percent_reply_loss"),
+		"Fatal ping should not have packet measurements")
+	assert.False(t, acc.HasIntField("ping", "average_response_ms"),
+		"Fatal ping should not have packet measurements")
+	assert.False(t, acc.HasIntField("ping", "maximum_response_ms"),
+		"Fatal ping should not have packet measurements")
+	assert.False(t, acc.HasIntField("ping", "minimum_response_ms"),
+		"Fatal ping should not have packet measurements")
+}
+
+var UnreachablePingOutput = `
+Pinging www.google.pl [8.8.8.8] with 32 bytes of data:
+Request timed out.
+Request timed out.
+Reply from 194.204.175.50: Destination net unreachable.
+Request timed out.
+
+Ping statistics for 8.8.8.8:
+    Packets: Sent = 4, Received = 1, Lost = 3 (75% loss),
+`
+
+func mockUnreachableHostPinger(timeout float64, args ...string) (string, error) {
+	return UnreachablePingOutput, errors.New("So very bad")
+}
+
+//Reply from 185.28.251.217: TTL expired in transit.
+
+// in case 'Destination net unreachable' ping app return receive packet which is not what we need
+// it's not contain valid metric so treat it as lost one
+func TestUnreachablePingGather(t *testing.T) {
+	var acc testutil.Accumulator
+	p := Ping{
+		Urls:     []string{"www.google.com"},
+		pingHost: mockUnreachableHostPinger,
+	}
+
+	p.Gather(&acc)
+
+	tags := map[string]string{"url": "www.google.com"}
+	fields := map[string]interface{}{
+		"packets_transmitted": 4,
+		"packets_received":    1,
+		"reply_received":      0,
+		"percent_packet_loss": 75.0,
+		"percent_reply_loss":  100.0,
+	}
+	acc.AssertContainsTaggedFields(t, "ping", fields, tags)
+
+	assert.False(t, acc.HasFloatField("ping", "errors"),
+		"Fatal ping should not have packet measurements")
+	assert.False(t, acc.HasIntField("ping", "average_response_ms"),
+		"Fatal ping should not have packet measurements")
+	assert.False(t, acc.HasIntField("ping", "maximum_response_ms"),
+		"Fatal ping should not have packet measurements")
+	assert.False(t, acc.HasIntField("ping", "minimum_response_ms"),
+		"Fatal ping should not have packet measurements")
+}
+
+var TTLExpiredPingOutput = `
+Pinging www.google.pl [8.8.8.8] with 32 bytes of data:
+Request timed out.
+Request timed out.
+Reply from 185.28.251.217: TTL expired in transit.
+Request timed out.
+
+Ping statistics for 8.8.8.8:
+    Packets: Sent = 4, Received = 1, Lost = 3 (75% loss),
+`
+
+func mockTTLExpiredPinger(timeout float64, args ...string) (string, error) {
+	return TTLExpiredPingOutput, errors.New("So very bad")
+}
+
+// in case 'Destination net unreachable' ping app return receive packet which is not what we need
+// it's not contain valid metric so treat it as lost one
+func TestTTLExpiredPingGather(t *testing.T) {
+	var acc testutil.Accumulator
+	p := Ping{
+		Urls:     []string{"www.google.com"},
+		pingHost: mockTTLExpiredPinger,
+	}
+
+	p.Gather(&acc)
+
+	tags := map[string]string{"url": "www.google.com"}
+	fields := map[string]interface{}{
+		"packets_transmitted": 4,
+		"packets_received":    1,
+		"reply_received":      0,
+		"percent_packet_loss": 75.0,
+		"percent_reply_loss":  100.0,
+	}
+	acc.AssertContainsTaggedFields(t, "ping", fields, tags)
+
+	assert.False(t, acc.HasFloatField("ping", "errors"),
+		"Fatal ping should not have packet measurements")
+	assert.False(t, acc.HasIntField("ping", "average_response_ms"),
+		"Fatal ping should not have packet measurements")
+	assert.False(t, acc.HasIntField("ping", "maximum_response_ms"),
+		"Fatal ping should not have packet measurements")
+	assert.False(t, acc.HasIntField("ping", "minimum_response_ms"),
 		"Fatal ping should not have packet measurements")
 }