Fix thinSnaps (#1105)

* Fix thinSnaps logic
* Guarantee FinalSnapshot matches last snapshot
* Add export for thinSnaps for testing
* Add explicit test for thinSnaps edge cases

parser/export_test.go

@@ -11,3 +11,6 @@ var InitParserVersionForTest = initParserVersion
 // InitParserGitCommitForTest allows test to rerun initParseGitCommit after initializing
 // environement variables.
 var InitParserGitCommitForTest = initParserGitCommit
+
+// ThinSnaps allows exhaustive edge case testing of thinSnaps.
+var ThinSnaps = thinSnaps

parser/tcpinfo.go

@@ -96,15 +96,19 @@ func (p *TCPInfoParser) IsParsable(testName string, data []byte) (string, bool)
 	return "", false
 }
 
+// thinSnaps collects every 10th snapshot in orig. thinSnaps guarantees that the last snapshot is preserved.
 func thinSnaps(orig []snapshot.Snapshot) []snapshot.Snapshot {
 	n := len(orig)
+	if n == 0 {
+		return orig
+	}
 	out := make([]snapshot.Snapshot, 0, 1+n/10)
-	for i := 0; i < n; i += 10 {
+	// Take first and every 10th snapshot after (exluding the final snapshot, which is always included).
+	for i := 0; i < n-1; i += 10 {
 		out = append(out, orig[i])
 	}
-	if n%10 != 0 {
-		out = append(out, orig[n-1])
-	}
+	// Always append final snapshot.
+	out = append(out, orig[n-1])
 	return out
 }
 

parser/tcpinfo_test.go

@@ -14,11 +14,13 @@ import (
 	"time"
 
 	"cloud.google.com/go/civil"
+	"github.com/go-test/deep"
 	"github.com/m-lab/etl/etl"
 	"github.com/m-lab/etl/parser"
 	"github.com/m-lab/etl/schema"
 	"github.com/m-lab/etl/storage"
 	"github.com/m-lab/etl/task"
+	"github.com/m-lab/tcp-info/snapshot"
 )
 
 func assertTCPInfoParser(in *parser.TCPInfoParser) {
@@ -169,6 +171,10 @@ func TestTCPParser(t *testing.T) {
 		if row.A.SockID.SrcIP != "195.89.146.242" && row.A.SockID.SrcIP != "2001:5012:100:24::242" {
 			t.Error("Wrong SrcIP", row.A.SockID.SrcIP)
 		}
+		// Guarantee that FinalSnapshot matches the last raw snapshot.
+		if diff := deep.Equal(row.A.FinalSnapshot, row.Raw.Snapshots[len(row.Raw.Snapshots)-1]); diff != nil {
+			t.Errorf("TestTCPParser.ParseAndInsert() FinalSnapshot and last snapshot differ: %s", strings.Join(diff, "\n"))
+		}
 	}
 
 	// This section is just for understanding how big these objects typically are, and what kind of compression
@@ -201,10 +207,6 @@ func TestTCPParser(t *testing.T) {
 	if duration > 20*time.Second {
 		t.Error("Incorrect duration calculation", duration)
 	}
-
-	if totalSnaps != 1588 {
-		t.Error("expected 1588 (thinned) snapshots, got", totalSnaps)
-	}
 }
 
 // This is a subset of TestTCPParser, but simpler, so might be useful.
@@ -295,3 +297,96 @@ func BenchmarkTCPParser(b *testing.B) {
 		}
 	}
 }
+
+func makeSnaps(n int) []snapshot.Snapshot {
+	snaps := []snapshot.Snapshot{}
+	for i := 0; i < n; i++ {
+		snaps = append(snaps, snapshot.Snapshot{
+			Observed: uint32(i), // this field is arbitrary. Just make the Snapshot distinct without populating every field.
+		})
+	}
+	return snaps
+}
+
+func Test_thinSnaps(t *testing.T) {
+	tests := []struct {
+		name string
+		orig []snapshot.Snapshot
+		want []snapshot.Snapshot
+	}{
+		{
+			name: "success-nil",
+			orig: nil,
+			want: nil,
+		},
+		{
+			name: "success-0",
+			orig: makeSnaps(0),
+			want: []snapshot.Snapshot{},
+		},
+		{
+			name: "success-1",
+			orig: makeSnaps(1),
+			want: []snapshot.Snapshot{
+				{Observed: 0},
+			},
+		},
+		{
+			name: "success-2",
+			orig: makeSnaps(2),
+			want: []snapshot.Snapshot{
+				{Observed: 0},
+				{Observed: 1},
+			},
+		},
+		{
+			name: "success-3",
+			orig: makeSnaps(3),
+			want: []snapshot.Snapshot{
+				{Observed: 0},
+				{Observed: 2},
+			},
+		},
+		{
+			name: "success-9",
+			orig: makeSnaps(9),
+			want: []snapshot.Snapshot{
+				{Observed: 0},
+				{Observed: 8},
+			},
+		},
+		{
+			name: "success-10",
+			orig: makeSnaps(10),
+			want: []snapshot.Snapshot{
+				{Observed: 0},
+				{Observed: 9},
+			},
+		},
+		{
+			name: "success-11",
+			orig: makeSnaps(11),
+			want: []snapshot.Snapshot{
+				{Observed: 0},
+				{Observed: 10},
+			},
+		},
+		{
+			name: "success-12",
+			orig: makeSnaps(12),
+			want: []snapshot.Snapshot{
+				{Observed: 0},
+				{Observed: 10},
+				{Observed: 11},
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := parser.ThinSnaps(tt.orig)
+			if diff := deep.Equal(got, tt.want); diff != nil {
+				t.Errorf("TestThinSnaps() differ = %s", strings.Join(diff, "\n"))
+			}
+		})
+	}
+}