Metrics refact (#50)

* Refact methods to calculate metrics and include closed connections in final report

* Fix stdev test

* Fix formatted go and start working on final established report

* more format stuff

* Provide a max of concurrent established sessions

* Fix tests

* Fix format

* First round of PR review comments

* Make things simpler

* Simplify pingStyleReport exposition

* Improve calculate function

* Add a constructor for Group of Connections + fix output

* Add some refactors abstracting Connection a bit more

* Refact filtering functions

* pingStyleReport simplified

* Fix unreferenced function

* Make groupofoconnection and connection more desacoupled

* Claning calculateMetricsReport

* Fix issue with reporting max connections established and some simplifications

* First round of PR review

* Improve pintStyleReport method

* Introduce iota on Const definition

* Simply connection status checkers

* Refact getConnectionWentWell

* Fix max concurrent connections

* Use func argument

* update readme

README.md

@@ -91,7 +91,11 @@ Total: 4, Dialing: 1, Established: 3, Closed: 0, Error: 0, NotInitiated: 0
 Total: 4, Dialing: 0, Established: 4, Closed: 0, Error: 0, NotInitiated: 0
 --- myhttpsamplehost.com(216.58.201.131):80 tcp test statistics ---
 Total: 4, Dialing: 0, Established: 4, Closed: 0, Error: 0, NotInitiated: 0
-Response time stats for 4 established connections min/avg/max/dev = 59.842ms/70.798ms/100.766ms/17.312ms
+--- tcpgoon execution statistics ---
+Total established connections: 4
+Max concurrent established connections: 4
+Number of established connections on closure: 4
+Response time stats for 4 successful connections min/avg/max/dev = 49.892ms/50.205ms/50.74ms/319µs
 
 % echo $?
 0
@@ -107,7 +111,11 @@ Total: 4, Dialing: 0, Established: 0, Closed: 0, Error: 4, NotInitiated: 0
 Total: 4, Dialing: 0, Established: 0, Closed: 0, Error: 4, NotInitiated: 0
 --- myhttpsamplehost.com(216.58.201.131):81 tcp test statistics ---
 Total: 4, Dialing: 0, Established: 0, Closed: 0, Error: 4, NotInitiated: 0
-Time to error stats for 4 failed connections min/avg/max/dev = 1.085ms/1.107ms/1.148ms/24µs
+--- tcpgoon execution statistics ---
+Total established connections: 0
+Max concurrent established connections: 0
+Number of established connections on closure: 0
+Time to error stats for 4 failed connections min/avg/max/dev = 1ms/1.122ms/1.165ms/37µs
 
 % echo $?
 2

cmd/tcpgoon.go

@@ -97,9 +97,9 @@ func run(params tcpgoonParams) {
 	//  one side to the other.. everything in a single structure, or applying something like the builder pattern,
 	//  may help
 	connStatusCh, connStatusTracker := mtcpclient.StartBackgroundReporting(params.numberConnections, params.reportingInterval)
-	closureCh := mtcpclient.StartBackgroundClosureTrigger(connStatusTracker)
+	closureCh := mtcpclient.StartBackgroundClosureTrigger(*connStatusTracker)
 	mtcpclient.MultiTCPConnect(params.numberConnections, params.delay, params.target, params.port, connStatusCh, closureCh)
 	fmt.Fprintln(debugging.DebugOut, "Tests execution completed")
 
-	cmdutil.CloseNicely(params.targetip, params.target, params.port, connStatusTracker)
+	cmdutil.CloseNicely(params.targetip, params.target, params.port, *connStatusTracker)
 }

mtcpclient/calculate.go

@@ -16,50 +16,41 @@ type metricsCollectionStats struct {
 	numberOfConnections int
 }
 
-func (gc GroupOfConnections) calculateMetricsReport(status tcpclient.ConnectionStatus) (mr metricsCollectionStats) {
-	// TODO: There's something i don't like... initializing values in the first loop, and the standard deviation
-	//  requiring an extra pass considering all items... i'd move initialization out of the loop, and maybe iterate
-	//  over a filtered list rather than several loops over the original one, and maybe use specific generic functions...
-	//  requires further thinking in any case
-	// TODO: (2) rather than calculate a metrics report with a filter on the connection status, lets just promote splitting
-	// group of connections and running these functions over the subsets...
+func newMetricsCollectionStats() *metricsCollectionStats {
+	mr := new(metricsCollectionStats)
+	mr.avg = 0
+	mr.min = time.Duration(time.Duration(tcpclient.DefaultDialTimeoutInMs) * time.Millisecond)
+	mr.max = 0
 	mr.total = 0
+	mr.stdDev = 0
 	mr.numberOfConnections = 0
-	for _, item := range gc {
-		if item.GetConnectionStatus() == status {
-			mr.numberOfConnections++
-			if mr.total == 0 {
-				mr.total = item.GetTCPProcessingDuration()
-				mr.min = item.GetTCPProcessingDuration()
-				mr.max = item.GetTCPProcessingDuration()
-			} else {
-				mr.min = time.Duration(math.Min(float64(mr.min), float64(item.GetTCPProcessingDuration())))
-				mr.max = time.Duration(math.Max(float64(mr.max), float64(item.GetTCPProcessingDuration())))
-				mr.total += item.GetTCPProcessingDuration()
-			}
+	return mr
+}
+
+func (gc GroupOfConnections) calculateMetricsReport() (mr *metricsCollectionStats) {
+	mr = newMetricsCollectionStats()
+	if mr.numberOfConnections = len(gc.connections); mr.numberOfConnections > 0 {
+		for _, item := range gc.connections {
+			mr.min = time.Duration(math.Min(float64(mr.min), float64(item.GetTCPProcessingDuration())))
+			mr.max = time.Duration(math.Max(float64(mr.max), float64(item.GetTCPProcessingDuration())))
+			mr.total += item.GetTCPProcessingDuration()
 		}
-	}
-	if mr.numberOfConnections > 0 {
 		mr.avg = mr.total / time.Duration(mr.numberOfConnections)
+		mr.stdDev = gc.calculateStdDev(mr.avg)
 	}
-	mr.stdDev = gc.calculateStdDev(status, mr)
 	return mr
 }
 
-func (gc GroupOfConnections) calculateStdDev(status tcpclient.ConnectionStatus, mr metricsCollectionStats) time.Duration {
-	// TODO: passing the whole mr struct looks overkilling, given we only want a single value, the average, and maybe
-	//  we can actually use a version of the algorithm that calculates it (and the number of items)
-	var nItems int
+func (gc GroupOfConnections) calculateStdDev(avg time.Duration) time.Duration {
 	var sd float64
-	for _, item := range gc {
-		if item.GetConnectionStatus() == status {
-			nItems++
-			sd += math.Pow(float64(item.GetTCPProcessingDuration())-float64(mr.avg), 2)
-		}
-	}
-	if nItems == 0 {
+
+	if len(gc.connections) == 0 {
 		return 0
 	}
-	return time.Duration(math.Sqrt(sd / float64(nItems)))
 
+	for _, item := range gc.connections {
+		sd += math.Pow(float64(item.GetTCPProcessingDuration())-float64(avg), 2)
+	}
+
+	return time.Duration(math.Sqrt(sd / float64(len(gc.connections))))
 }

mtcpclient/calculate_test.go

@@ -1,21 +1,22 @@
 package mtcpclient
 
 import (
-	"github.com/dachad/tcpgoon/tcpclient"
 	"testing"
 	"time"
+
+	"github.com/dachad/tcpgoon/tcpclient"
 )
 
 func TestCalculateMetricsReport(t *testing.T) {
 	var metricsReportScenariosChecks = []struct {
 		scenarioDescription         string
-		groupOfConnectionsToReport  GroupOfConnections
+		groupOfConnectionsToReport  *GroupOfConnections
 		tcpStatusToReport           tcpclient.ConnectionStatus
 		expectedReportWithoutStdDev metricsCollectionStats
 	}{
 		{
 			scenarioDescription:        "Empty group of connections should report 0 as associated metrics",
-			groupOfConnectionsToReport: GroupOfConnections{},
+			groupOfConnectionsToReport: newGroupOfConnections(0),
 			tcpStatusToReport:          tcpclient.ConnectionEstablished,
 			expectedReportWithoutStdDev: metricsCollectionStats{
 				avg:                 0,
@@ -26,11 +27,9 @@ func TestCalculateMetricsReport(t *testing.T) {
 			},
 		},
 		{
-			scenarioDescription: "Single connection should generate a report that describes its associated metric",
-			groupOfConnectionsToReport: GroupOfConnections{
-				tcpclient.NewConnection(0, tcpclient.ConnectionEstablished, time.Duration(500)*time.Millisecond),
-			},
-			tcpStatusToReport: tcpclient.ConnectionEstablished,
+			scenarioDescription:        "Single connection should generate a report that describes its associated metric",
+			groupOfConnectionsToReport: newSampleSingleConnection(),
+			tcpStatusToReport:          tcpclient.ConnectionEstablished,
 			expectedReportWithoutStdDev: metricsCollectionStats{
 				avg:                 500 * time.Millisecond,
 				min:                 500 * time.Millisecond,
@@ -41,13 +40,9 @@ func TestCalculateMetricsReport(t *testing.T) {
 		},
 		{
 			// TODO: We will need to extend this to cover a mix connections closed + established on closure, when the code supports it
-			scenarioDescription: "Multiple connections with different statuses should generate a report that describes the metrics of the right subset",
-			groupOfConnectionsToReport: GroupOfConnections{
-				tcpclient.NewConnection(0, tcpclient.ConnectionEstablished, time.Duration(500)*time.Millisecond),
-				tcpclient.NewConnection(1, tcpclient.ConnectionError, time.Duration(1)*time.Second),
-				tcpclient.NewConnection(2, tcpclient.ConnectionError, time.Duration(3)*time.Second),
-			},
-			tcpStatusToReport: tcpclient.ConnectionError,
+			scenarioDescription:        "Multiple connections with different statuses should generate a report that describes the metrics of the right subset",
+			groupOfConnectionsToReport: newSampleMultipleConnections(),
+			tcpStatusToReport:          tcpclient.ConnectionError,
 			expectedReportWithoutStdDev: metricsCollectionStats{
 				avg:                 2 * time.Second,
 				min:                 1 * time.Second,
@@ -59,9 +54,9 @@ func TestCalculateMetricsReport(t *testing.T) {
 	}
 
 	for _, test := range metricsReportScenariosChecks {
-		resultingReport := test.groupOfConnectionsToReport.calculateMetricsReport(test.tcpStatusToReport)
-		test.expectedReportWithoutStdDev.stdDev = test.groupOfConnectionsToReport.calculateStdDev(test.tcpStatusToReport, resultingReport)
-		if resultingReport != test.expectedReportWithoutStdDev {
+		resultingReport := test.groupOfConnectionsToReport.calculateMetricsReport()
+		test.expectedReportWithoutStdDev.stdDev = test.groupOfConnectionsToReport.calculateStdDev(resultingReport.avg)
+		if resultingReport.stdDev != test.expectedReportWithoutStdDev.stdDev {
 			t.Error(test.scenarioDescription+", and its", resultingReport)
 		}
 	}
@@ -97,23 +92,32 @@ func TestCalculateStdDev(t *testing.T) {
 	}
 
 	for _, test := range stdDevScenariosChecks {
-		var gc GroupOfConnections = []tcpclient.Connection{}
+
+		var gc *GroupOfConnections
+		gc = newGroupOfConnections(0)
 
 		var sum int
+		var connectionState tcpclient.ConnectionStatus
 		for i, connectionDuration := range test.durationsInSecs {
-			gc = append(gc, tcpclient.NewConnection(i, tcpclient.ConnectionEstablished,
+			if i%2 == 0 {
+				connectionState = tcpclient.ConnectionEstablished
+			} else {
+				connectionState = tcpclient.ConnectionClosed
+			}
+			gc.connections = append(gc.connections, tcpclient.NewConnection(i, connectionState,
 				time.Duration(connectionDuration)*time.Second))
 			sum += connectionDuration
 		}
 
-		mr := metricsCollectionStats{}
+		var mr *metricsCollectionStats
+		mr = newMetricsCollectionStats()
+
 		if len(test.durationsInSecs) != 0 {
-			mr = metricsCollectionStats{
-				avg: time.Duration(sum/len(test.durationsInSecs)) * time.Second,
-			}
+			mr.avg = time.Duration(sum/len(test.durationsInSecs)) * time.Second
+			mr.numberOfConnections = len(gc.connections)
 		}
 
-		stddev := gc.calculateStdDev(tcpclient.ConnectionEstablished, mr)
+		stddev := gc.calculateStdDev(mr.avg)
 
 		if stddev != time.Duration(test.expectedStdDev)*time.Second {
 			t.Error(test.scenarioDescription+", and its", stddev)

mtcpclient/closure.go

@@ -2,13 +2,16 @@ package mtcpclient
 
 import (
 	"fmt"
-	"github.com/dachad/tcpgoon/debugging"
 	"os"
 	"os/signal"
 	"syscall"
 	"time"
+
+	"github.com/dachad/tcpgoon/debugging"
 )
 
+// StartBackgroundClosureTrigger creates proper channels to know when to close execution
+// and triggers a goroutine that monitors if the closure conditions are met
 func StartBackgroundClosureTrigger(gc GroupOfConnections) <-chan bool {
 	closureCh := make(chan bool)
 

mtcpclient/groupofconnections.go

@@ -2,15 +2,34 @@ package mtcpclient
 
 import (
 	"fmt"
+	"strconv"
+	"time"
 
 	"github.com/dachad/tcpgoon/tcpclient"
 )
 
-type GroupOfConnections []tcpclient.Connection
+// GroupOfConnections aggregates all the running connections plus some general metrics
+type GroupOfConnections struct {
+	connections []tcpclient.Connection
+	metrics     gcMetrics
+}
+
+type gcMetrics struct {
+	maxConcurrentEstablished int
+}
+
+func newGroupOfConnections(numberConnections int) *GroupOfConnections {
+	gc := new(GroupOfConnections)
+	gc.connections = make([]tcpclient.Connection, numberConnections)
+	gc.metrics = gcMetrics{
+		maxConcurrentEstablished: 0,
+	}
+	return gc
+}
 
 func (gc GroupOfConnections) String() string {
 	var nDialing, nEstablished, nClosed, nNotInitiated, nError, nTotal int = 0, 0, 0, 0, 0, 0
-	for _, item := range gc {
+	for _, item := range gc.connections {
 		switch item.GetConnectionStatus() {
 		case tcpclient.ConnectionDialing:
 			nDialing++
@@ -29,23 +48,71 @@ func (gc GroupOfConnections) String() string {
 		nTotal, nDialing, nEstablished, nClosed, nError, nNotInitiated)
 }
 
-func (gc GroupOfConnections) isIn(status tcpclient.ConnectionStatus) bool {
-	for _, item := range gc {
-		if item.GetConnectionStatus() == status {
+func (gc GroupOfConnections) containsAConnectionWithStatus(fn tcpclient.ConnectionFunc) bool {
+	for _, connection := range gc.connections {
+		if fn(connection) {
 			return true
 		}
 	}
 	return false
 }
 
+// PendingConnections retuns True if at least one connection is being processed
 func (gc GroupOfConnections) PendingConnections() bool {
-	return gc.isIn(tcpclient.ConnectionNotInitiated) || gc.isIn(tcpclient.ConnectionDialing)
+	return gc.containsAConnectionWithStatus(tcpclient.PendingToProcess)
 }
 
+// AtLeastOneConnectionInError returns True is at least one connection establishment failed
 func (gc GroupOfConnections) AtLeastOneConnectionInError() bool {
-	return gc.isIn(tcpclient.ConnectionError)
+	return gc.containsAConnectionWithStatus(tcpclient.WithError)
+}
+
+func (gc GroupOfConnections) atLeastOneConnectionOK() bool {
+	return gc.containsAConnectionWithStatus(tcpclient.WentOk)
+}
+
+const (
+	successfulExecution int = iota + 0
+	failedExecution
+)
+
+func (gc GroupOfConnections) pingStyleReport(typeOfReport int) (output string) {
+	var headerline, state string
+	switch typeOfReport {
+	case successfulExecution:
+		headerline = "Response time"
+		state = "successful"
+	case failedExecution:
+		headerline = "Time to error"
+		state = "failed"
+	}
+	output += headerline + " stats for " + strconv.Itoa(len(gc.connections)) + " " + state +
+		" connections min/avg/max/dev = " + gc.calculateMetricsReport().String()
+
+	return output
+}
+
+func (gc GroupOfConnections) getConnectionsThatWentWell(itWentWell bool) (connectionsThatWent GroupOfConnections) {
+	for _, connection := range gc.connections {
+		if tcpclient.WentOk(connection) == itWentWell {
+			connectionsThatWent.connections = append(connectionsThatWent.connections, connection)
+		}
+	}
+	return connectionsThatWent
+}
+
+func (gc GroupOfConnections) getConnectionsThatAreOk() (connectionsThatAreOk GroupOfConnections) {
+	for _, connection := range gc.connections {
+		if tcpclient.IsOk(connection) {
+			connectionsThatAreOk.connections = append(connectionsThatAreOk.connections, connection)
+		}
+	}
+	return connectionsThatAreOk
 }
 
-func (gc GroupOfConnections) AtLeastOneConnectionEstablished() bool {
-	return gc.isIn(tcpclient.ConnectionEstablished)
+func (mr *metricsCollectionStats) String() string {
+	return mr.min.Truncate(time.Microsecond).String() + "/" +
+		mr.avg.Truncate(time.Microsecond).String() + "/" +
+		mr.max.Truncate(time.Microsecond).String() + "/" +
+		mr.stdDev.Truncate(time.Microsecond).String() + "\n"
 }