Linting and refactoring for code health (#41)

Hide some functions and structs that were exported but not used outside the package.

Refactor interfaces to be defined where they are used instead of where they are implemented.

Add a compilation test.

Author: pboothe

ipcache/ipcache.go

@@ -9,7 +9,6 @@ import (
 	"time"
 
 	"github.com/m-lab/traceroute-caller/connection"
-	"github.com/m-lab/traceroute-caller/scamper"
 )
 
 var (
@@ -20,7 +19,14 @@ var (
 	IPCacheUpdatePeriod = flag.Duration("IPCacheUpdatePeriod", 1*time.Second, "We run the cache eviction loop with this frequency")
 )
 
-type CacheTest struct {
+// Tracer is the generic interface for all things that can perform a traceroute.
+type Tracer interface {
+	Trace(conn connection.Connection, t time.Time) string
+	CreateCacheTest(conn connection.Connection, t time.Time, cachedTest string)
+}
+
+// cachedTest is a single entry in the cache of traceroute results.
+type cachedTest struct {
 	timeStamp time.Time
 	data      string
 	done      chan struct{}
@@ -30,10 +36,10 @@ type CacheTest struct {
 // recently. We keep this list to ensure that we don't traceroute to the same
 // location repeatedly at a high frequency.
 type RecentIPCache struct {
-	cache map[string]*CacheTest
+	cache map[string]*cachedTest
 	mu    sync.RWMutex
 
-	tracer scamper.Tracer
+	tracer Tracer
 }
 
 // Trace performs a trace and adds it to the cache.
@@ -42,7 +48,7 @@ func (rc *RecentIPCache) Trace(conn connection.Connection) {
 	rc.mu.Lock()
 	_, ok := rc.cache[ip]
 	if !ok {
-		nc := &CacheTest{
+		nc := &cachedTest{
 			timeStamp: time.Now(),
 			done:      make(chan struct{}),
 		}
@@ -57,6 +63,9 @@ func (rc *RecentIPCache) Trace(conn connection.Connection) {
 	rc.tracer.CreateCacheTest(conn, time.Now(), rc.GetData(ip))
 }
 
+// GetCacheLength returns the number of items currently in the cache. The
+// primary use of this is for testing, to ensure that something was put into or
+// removed from the cache.
 func (rc *RecentIPCache) GetCacheLength() int {
 	return len(rc.cache)
 }
@@ -76,9 +85,9 @@ func (rc *RecentIPCache) GetData(ip string) string {
 
 // New creates and returns a RecentIPCache. It also starts up a background
 // goroutine that scrubs the cache.
-func New(ctx context.Context, tracer scamper.Tracer, ipCacheTimeout, ipCacheUpdatePeriod time.Duration) *RecentIPCache {
+func New(ctx context.Context, tracer Tracer, ipCacheTimeout, ipCacheUpdatePeriod time.Duration) *RecentIPCache {
 	m := &RecentIPCache{
-		cache:  make(map[string]*CacheTest),
+		cache:  make(map[string]*cachedTest),
 		tracer: tracer,
 	}
 	go func() {

scamper/scamper.go

@@ -136,6 +136,8 @@ func (d *Daemon) TraceAll(connections []connection.Connection) {
 	}
 }
 
+// Metadata is the first line of the traceroute .jsonl file.
+//
 // TODO: move this struct to ETL parser.
 type Metadata struct {
 	UUID                    string
@@ -144,9 +146,11 @@ type Metadata struct {
 	CachedUUID              string
 }
 
-// isCache indicates whether this meta line is for an orignial trace test or a cached test.
-// uuis is the original test if isCache is 1.
-func GetMetaline(conn connection.Connection, isCache int, cachedUUID string) string {
+// GetMetaline returns the what the first line of the output jsonl file should
+// be. Parameter isCache indicates whether this meta line is for an original
+// trace test or a cached test, and parameter cachedUUID is the original test if
+// isCache is 1.
+func GetMetaline(conn connection.Connection, isCache bool, cachedUUID string) string {
 	// Write the UUID as the first line of the file. If we want to add other
 	// metadata, this is the place to do it.
 	//
@@ -159,7 +163,7 @@ func GetMetaline(conn connection.Connection, isCache int, cachedUUID string) str
 	meta := Metadata{
 		UUID:                    uuid,
 		TracerouteCallerVersion: prometheusx.GitShortCommit,
-		CachedResult:            bool(isCache == 1),
+		CachedResult:            isCache,
 		CachedUUID:              cachedUUID,
 	}
 
@@ -168,7 +172,10 @@ func GetMetaline(conn connection.Connection, isCache int, cachedUUID string) str
 	return string(metaJSON) + "\n"
 }
 
-func ExtractUUID(metaline string) string {
+// extractUUID retrieves the UUID from a cached line.
+//
+// TODO: Eliminate the need to unmarshal data we marshaled in the first place.
+func extractUUID(metaline string) string {
 	var metaResult Metadata
 	err := json.Unmarshal([]byte(metaline), &metaResult)
 	if err != nil {
@@ -178,6 +185,8 @@ func ExtractUUID(metaline string) string {
 	return metaResult.UUID
 }
 
+// CreateCacheTest creates a file containing traceroute results that came from a
+// cache result, rather than performing the traceroute with scamper.
 func (d *Daemon) CreateCacheTest(conn connection.Connection, t time.Time, cachedTest string) {
 	filename := d.createTimePath(t) + d.generateFilename(conn.Cookie, t)
 	log.Println("Starting a cached trace to be put in", filename)
@@ -191,7 +200,7 @@ func (d *Daemon) CreateCacheTest(conn connection.Connection, t time.Time, cached
 	}
 
 	// Get the uuid from the first line of cachedTest
-	newTest := GetMetaline(conn, 1, ExtractUUID(cachedTest[:split])) + cachedTest[split+1:]
+	newTest := GetMetaline(conn, true, extractUUID(cachedTest[:split])) + cachedTest[split+1:]
 	rtx.PanicOnError(ioutil.WriteFile(filename, []byte(newTest), 0666), "Could not save output to file")
 }
 
@@ -200,7 +209,7 @@ func (d *Daemon) trace(conn connection.Connection, t time.Time) string {
 	log.Println("Starting a trace to be put in", filename)
 	buff := bytes.Buffer{}
 
-	_, err := buff.WriteString(GetMetaline(conn, 0, ""))
+	_, err := buff.WriteString(GetMetaline(conn, false, ""))
 	rtx.PanicOnError(err, "Could not write to buffer")
 
 	log.Printf(
@@ -219,7 +228,3 @@ func (d *Daemon) trace(conn connection.Connection, t time.Time) string {
 	return string(buff.Bytes())
 }
 
-type Tracer interface {
-	Trace(conn connection.Connection, t time.Time) string
-	CreateCacheTest(conn connection.Connection, t time.Time, cachedTest string)
-}

scamper/scamper_test.go

@@ -14,6 +14,7 @@ import (
 	"github.com/m-lab/go/prometheusx"
 	"github.com/m-lab/go/rtx"
 	"github.com/m-lab/traceroute-caller/connection"
+	"github.com/m-lab/traceroute-caller/ipcache"
 	"github.com/m-lab/uuid/prefix"
 )
 
@@ -234,12 +235,12 @@ func TestRecovery(t *testing.T) {
 }
 
 func TestExtractUUID(t *testing.T) {
-	uuid := ExtractUUID("{\"UUID\": \"ndt-plh7v_1566050090_000000000004D64D\"}")
+	uuid := extractUUID("{\"UUID\": \"ndt-plh7v_1566050090_000000000004D64D\"}")
 	if uuid != "ndt-plh7v_1566050090_000000000004D64D" {
 		t.Error("Fail to extract uuid")
 	}
 
-	failedUUID := ExtractUUID("invalid json")
+	failedUUID := extractUUID("invalid json")
 	if failedUUID != "" {
 		t.Error("Should fail to extract uuid")
 	}
@@ -253,8 +254,14 @@ func TestGetMetaline(t *testing.T) {
 		LocalPort:  345,
 		Cookie:     "abc",
 	}
-	meta := GetMetaline(conn, 1, "00EF")
+	meta := GetMetaline(conn, true, "00EF")
 	if !strings.Contains(meta, "0000000000000ABC\",\"TracerouteCallerVersion\":\"Fake Version\",\"CachedResult\":true,\"CachedUUID\":\"00EF\"") {
 		t.Error("Fail to generate meta ", meta)
 	}
 }
+
+// If this successfully compiles, then Daemon implements the Tracer interface,
+// which is what we want it to do.
+func assertDaemonIsTracer(d *Daemon) {
+	func(t ipcache.Tracer) {}(d)
+}