refactor(tracex): start applying recent code conventions (#773)

The code that is now into the tracex package was written a long
time ago, so let's start to make it more in line with the coding
style of packages that were written more recently.

I didn't apply all the changes I'd like to apply in a single diff
and for now I am committing just this diff.

Broadly, what we need to do is:

1. improve documentation

2. ~always use pointer receivers (object receives have the issue
that they are not mutable by accident meaning that you can mutate
them but their state do not change after the call returns, which
is potentially a source of bugs in case you later refactor to use
a pointer receiver, so always use pointer receivers)

3. ~always avoid embedding (let's say we want to avoid embedding
for types we define and it's instead fine to embed types that are
defined in the stdlib: if later we add a new method, we will not
see a broken build and we'll probably forget to add the new method
to all wrappers -- conversely, if we're wrapping rather than
embedding, we'll see a broken build and act accordingly)

4. prefer unit tests and group tests by type being tested rather
than using a flat structure for tests

There's a coverage slippage that I'll compensate in a follow-up diff where I'll focus on unit testing.

Reference issue: ooni/probe#2121

internal/engine/experiment/urlgetter/configurer_test.go

@@ -119,7 +119,7 @@ func TestConfigurerNewConfigurationResolverDNSOverHTTPSPowerdns(t *testing.T) {
 	if !ok {
 		t.Fatal("not the resolver we expected")
 	}
-	stxp, ok := sr.Txp.(tracex.SaverDNSTransport)
+	stxp, ok := sr.Txp.(*tracex.SaverDNSTransport)
 	if !ok {
 		t.Fatal("not the DNS transport we expected")
 	}
@@ -195,7 +195,7 @@ func TestConfigurerNewConfigurationResolverDNSOverHTTPSGoogle(t *testing.T) {
 	if !ok {
 		t.Fatal("not the resolver we expected")
 	}
-	stxp, ok := sr.Txp.(tracex.SaverDNSTransport)
+	stxp, ok := sr.Txp.(*tracex.SaverDNSTransport)
 	if !ok {
 		t.Fatal("not the DNS transport we expected")
 	}
@@ -271,7 +271,7 @@ func TestConfigurerNewConfigurationResolverDNSOverHTTPSCloudflare(t *testing.T)
 	if !ok {
 		t.Fatal("not the resolver we expected")
 	}
-	stxp, ok := sr.Txp.(tracex.SaverDNSTransport)
+	stxp, ok := sr.Txp.(*tracex.SaverDNSTransport)
 	if !ok {
 		t.Fatal("not the DNS transport we expected")
 	}
@@ -347,7 +347,7 @@ func TestConfigurerNewConfigurationResolverUDP(t *testing.T) {
 	if !ok {
 		t.Fatal("not the resolver we expected")
 	}
-	stxp, ok := sr.Txp.(tracex.SaverDNSTransport)
+	stxp, ok := sr.Txp.(*tracex.SaverDNSTransport)
 	if !ok {
 		t.Fatal("not the DNS transport we expected")
 	}

internal/engine/netx/netx.go

@@ -104,9 +104,7 @@ func NewResolver(config Config) model.Resolver {
 			Resolver: r,
 		}
 	}
-	if config.ResolveSaver != nil {
-		r = tracex.SaverResolver{Resolver: r, Saver: config.ResolveSaver}
-	}
+	r = config.ResolveSaver.WrapResolver(r) // WAI when config.ResolveSaver==nil
 	return &netxlite.ResolverIDNA{Resolver: r}
 }
 
@@ -129,23 +127,14 @@ func NewQUICDialer(config Config) model.QUICDialer {
 	if config.FullResolver == nil {
 		config.FullResolver = NewResolver(config)
 	}
-	ql := netxlite.NewQUICListener()
-	if config.ReadWriteSaver != nil {
-		ql = &tracex.QUICListenerSaver{
-			QUICListener: ql,
-			Saver:        config.ReadWriteSaver,
-		}
-	}
+	ql := config.ReadWriteSaver.WrapQUICListener(netxlite.NewQUICListener())
 	var logger model.DebugLogger = model.DiscardLogger
 	if config.Logger != nil {
 		logger = config.Logger
 	}
 	extensions := []netxlite.QUICDialerWrapper{
 		func(dialer model.QUICDialer) model.QUICDialer {
-			if config.TLSSaver != nil {
-				dialer = tracex.QUICHandshakeSaver{Saver: config.TLSSaver, QUICDialer: dialer}
-			}
-			return dialer
+			return config.TLSSaver.WrapQUICDialer(dialer) // robust to nil TLSSaver
 		},
 	}
 	return netxlite.NewQUICDialerWithResolver(ql, logger, config.FullResolver, extensions...)
@@ -161,9 +150,7 @@ func NewTLSDialer(config Config) model.TLSDialer {
 	if config.Logger != nil {
 		h = &netxlite.TLSHandshakerLogger{DebugLogger: config.Logger, TLSHandshaker: h}
 	}
-	if config.TLSSaver != nil {
-		h = tracex.SaverTLSHandshaker{TLSHandshaker: h, Saver: config.TLSSaver}
-	}
+	h = config.TLSSaver.WrapTLSHandshaker(h) // behaves with nil TLSSaver
 	if config.TLSConfig == nil {
 		config.TLSConfig = &tls.Config{NextProtos: []string{"h2", "http/1.1"}}
 	}
@@ -284,12 +271,7 @@ func NewDNSClientWithOverrides(config Config, URL, hostOverride, SNIOverride,
 		httpClient := &http.Client{Transport: NewHTTPTransport(config)}
 		var txp model.DNSTransport = netxlite.NewDNSOverHTTPSTransportWithHostOverride(
 			httpClient, URL, hostOverride)
-		if config.ResolveSaver != nil {
-			txp = tracex.SaverDNSTransport{
-				DNSTransport: txp,
-				Saver:        config.ResolveSaver,
-			}
-		}
+		txp = config.ResolveSaver.WrapDNSTransport(txp) // safe when config.ResolveSaver == nil
 		return netxlite.NewSerialResolver(txp), nil
 	case "udp":
 		dialer := NewDialer(config)
@@ -299,12 +281,7 @@ func NewDNSClientWithOverrides(config Config, URL, hostOverride, SNIOverride,
 		}
 		var txp model.DNSTransport = netxlite.NewDNSOverUDPTransport(
 			dialer, endpoint)
-		if config.ResolveSaver != nil {
-			txp = tracex.SaverDNSTransport{
-				DNSTransport: txp,
-				Saver:        config.ResolveSaver,
-			}
-		}
+		txp = config.ResolveSaver.WrapDNSTransport(txp) // safe when config.ResolveSaver == nil
 		return netxlite.NewSerialResolver(txp), nil
 	case "dot":
 		config.TLSConfig.NextProtos = []string{"dot"}
@@ -315,12 +292,7 @@ func NewDNSClientWithOverrides(config Config, URL, hostOverride, SNIOverride,
 		}
 		var txp model.DNSTransport = netxlite.NewDNSOverTLSTransport(
 			tlsDialer.DialTLSContext, endpoint)
-		if config.ResolveSaver != nil {
-			txp = tracex.SaverDNSTransport{
-				DNSTransport: txp,
-				Saver:        config.ResolveSaver,
-			}
-		}
+		txp = config.ResolveSaver.WrapDNSTransport(txp) // safe when config.ResolveSaver == nil
 		return netxlite.NewSerialResolver(txp), nil
 	case "tcp":
 		dialer := NewDialer(config)
@@ -330,12 +302,7 @@ func NewDNSClientWithOverrides(config Config, URL, hostOverride, SNIOverride,
 		}
 		var txp model.DNSTransport = netxlite.NewDNSOverTCPTransport(
 			dialer.DialContext, endpoint)
-		if config.ResolveSaver != nil {
-			txp = tracex.SaverDNSTransport{
-				DNSTransport: txp,
-				Saver:        config.ResolveSaver,
-			}
-		}
+		txp = config.ResolveSaver.WrapDNSTransport(txp) // safe when config.ResolveSaver == nil
 		return netxlite.NewSerialResolver(txp), nil
 	default:
 		return nil, errors.New("unsupported resolver scheme")

internal/engine/netx/netx_test.go

@@ -126,7 +126,7 @@ func TestNewResolverWithSaver(t *testing.T) {
 	if !ok {
 		t.Fatal("not the resolver we expected")
 	}
-	sr, ok := ir.Resolver.(tracex.SaverResolver)
+	sr, ok := ir.Resolver.(*tracex.SaverResolver)
 	if !ok {
 		t.Fatal("not the resolver we expected")
 	}
@@ -332,7 +332,7 @@ func TestNewTLSDialerWithSaver(t *testing.T) {
 	if rtd.TLSHandshaker == nil {
 		t.Fatal("invalid TLSHandshaker")
 	}
-	sth, ok := rtd.TLSHandshaker.(tracex.SaverTLSHandshaker)
+	sth, ok := rtd.TLSHandshaker.(*tracex.SaverTLSHandshaker)
 	if !ok {
 		t.Fatal("not the TLSHandshaker we expected")
 	}
@@ -633,7 +633,7 @@ func TestNewDNSClientCloudflareDoHSaver(t *testing.T) {
 	if !ok {
 		t.Fatal("not the resolver we expected")
 	}
-	txp, ok := r.Transport().(tracex.SaverDNSTransport)
+	txp, ok := r.Transport().(*tracex.SaverDNSTransport)
 	if !ok {
 		t.Fatal("not the transport we expected")
 	}
@@ -670,7 +670,7 @@ func TestNewDNSClientUDPDNSSaver(t *testing.T) {
 	if !ok {
 		t.Fatal("not the resolver we expected")
 	}
-	txp, ok := r.Transport().(tracex.SaverDNSTransport)
+	txp, ok := r.Transport().(*tracex.SaverDNSTransport)
 	if !ok {
 		t.Fatal("not the transport we expected")
 	}
@@ -711,7 +711,7 @@ func TestNewDNSClientTCPDNSSaver(t *testing.T) {
 	if !ok {
 		t.Fatal("not the resolver we expected")
 	}
-	txp, ok := r.Transport().(tracex.SaverDNSTransport)
+	txp, ok := r.Transport().(*tracex.SaverDNSTransport)
 	if !ok {
 		t.Fatal("not the transport we expected")
 	}
@@ -756,7 +756,7 @@ func TestNewDNSClientDoTDNSSaver(t *testing.T) {
 	if !ok {
 		t.Fatal("not the resolver we expected")
 	}
-	txp, ok := r.Transport().(tracex.SaverDNSTransport)
+	txp, ok := r.Transport().(*tracex.SaverDNSTransport)
 	if !ok {
 		t.Fatal("not the transport we expected")
 	}

internal/engine/netx/tracex/archival.go

@@ -15,7 +15,7 @@ import (
 	"github.com/ooni/probe-cli/v3/internal/netxlite"
 )
 
-// Compatibility types
+// Compatibility types. Most experiments still use these names.
 type (
 	ExtSpec          = model.ArchivalExtSpec
 	TCPConnectEntry  = model.ArchivalTCPConnectResult
@@ -32,7 +32,7 @@ type (
 	NetworkEvent     = model.ArchivalNetworkEvent
 )
 
-// Compatibility variables
+// Compatibility variables. Most experiments still use these names.
 var (
 	ExtDNS          = model.ArchivalExtDNS
 	ExtNetevents    = model.ArchivalExtNetevents
@@ -100,7 +100,7 @@ func NewFailedOperation(err error) *string {
 	return &s
 }
 
-func addheaders(
+func httpAddHeaders(
 	source http.Header,
 	destList *[]HTTPHeader,
 	destMap *map[string]MaybeBinaryValue,
@@ -150,14 +150,14 @@ func newRequestList(begin time.Time, events []Event) []RequestEntry {
 			entry.Request.BodyIsTruncated = ev.DataIsTruncated
 		case "http_request_metadata":
 			entry.Request.Headers = make(map[string]MaybeBinaryValue)
-			addheaders(
+			httpAddHeaders(
 				ev.HTTPHeaders, &entry.Request.HeadersList, &entry.Request.Headers)
 			entry.Request.Method = ev.HTTPMethod
 			entry.Request.URL = ev.HTTPURL
 			entry.Request.Transport = ev.Transport
 		case "http_response_metadata":
 			entry.Response.Headers = make(map[string]MaybeBinaryValue)
-			addheaders(
+			httpAddHeaders(
 				ev.HTTPHeaders, &entry.Response.HeadersList, &entry.Response.Headers)
 			entry.Response.Code = int64(ev.HTTPStatusCode)
 			entry.Response.Locations = ev.HTTPHeaders.Values("Location")
@@ -183,11 +183,11 @@ func NewDNSQueriesList(begin time.Time, events []Event) []DNSQueryEntry {
 			continue
 		}
 		for _, qtype := range []dnsQueryType{"A", "AAAA"} {
-			entry := qtype.makequeryentry(begin, ev)
+			entry := qtype.makeQueryEntry(begin, ev)
 			for _, addr := range ev.Addresses {
-				if qtype.ipoftype(addr) {
+				if qtype.ipOfType(addr) {
 					entry.Answers = append(
-						entry.Answers, qtype.makeanswerentry(addr))
+						entry.Answers, qtype.makeAnswerEntry(addr))
 				}
 			}
 			if len(entry.Answers) <= 0 && ev.Err == nil {
@@ -206,7 +206,7 @@ func NewDNSQueriesList(begin time.Time, events []Event) []DNSQueryEntry {
 	return out
 }
 
-func (qtype dnsQueryType) ipoftype(addr string) bool {
+func (qtype dnsQueryType) ipOfType(addr string) bool {
 	switch qtype {
 	case "A":
 		return !strings.Contains(addr, ":")
@@ -216,7 +216,7 @@ func (qtype dnsQueryType) ipoftype(addr string) bool {
 	return false
 }
 
-func (qtype dnsQueryType) makeanswerentry(addr string) DNSAnswerEntry {
+func (qtype dnsQueryType) makeAnswerEntry(addr string) DNSAnswerEntry {
 	answer := DNSAnswerEntry{AnswerType: string(qtype)}
 	asn, org, _ := geolocate.LookupASN(addr)
 	answer.ASN = int64(asn)
@@ -230,7 +230,7 @@ func (qtype dnsQueryType) makeanswerentry(addr string) DNSAnswerEntry {
 	return answer
 }
 
-func (qtype dnsQueryType) makequeryentry(begin time.Time, ev Event) DNSQueryEntry {
+func (qtype dnsQueryType) makeQueryEntry(begin time.Time, ev Event) DNSQueryEntry {
 	return DNSQueryEntry{
 		Engine:          ev.Proto,
 		Failure:         NewFailure(ev.Err),
@@ -315,7 +315,7 @@ func NewTLSHandshakesList(begin time.Time, events []Event) []TLSHandshake {
 			Failure:            NewFailure(ev.Err),
 			NegotiatedProtocol: ev.TLSNegotiatedProto,
 			NoTLSVerify:        ev.NoTLSVerify,
-			PeerCertificates:   makePeerCerts(ev.TLSPeerCerts),
+			PeerCertificates:   tlsMakePeerCerts(ev.TLSPeerCerts),
 			ServerName:         ev.TLSServerName,
 			T:                  ev.Time.Sub(begin).Seconds(),
 			TLSVersion:         ev.TLSVersion,
@@ -324,7 +324,7 @@ func NewTLSHandshakesList(begin time.Time, events []Event) []TLSHandshake {
 	return out
 }
 
-func makePeerCerts(in []*x509.Certificate) (out []MaybeBinaryValue) {
+func tlsMakePeerCerts(in []*x509.Certificate) (out []MaybeBinaryValue) {
 	for _, e := range in {
 		out = append(out, MaybeBinaryValue{Value: string(e.Raw)})
 	}

internal/engine/netx/tracex/archival_test.go

@@ -47,7 +47,7 @@ func TestDNSQueryIPOfType(t *testing.T) {
 		output: false,
 	}}
 	for _, exp := range expectations {
-		if exp.qtype.ipoftype(exp.ip) != exp.output {
+		if exp.qtype.ipOfType(exp.ip) != exp.output {
 			t.Fatalf("failure for %+v", exp)
 		}
 	}

internal/engine/netx/tracex/dialer.go

@@ -1,5 +1,9 @@
 package tracex
 
+//
+// TCP and connected UDP sockets
+//
+
 import (
 	"context"
 	"net"
@@ -11,7 +15,10 @@ import (
 
 // SaverDialer saves events occurring during the dial
 type SaverDialer struct {
-	model.Dialer
+	// Dialer is the underlying dialer,
+	Dialer model.Dialer
+
+	// Saver saves events.
 	Saver *Saver
 }
 
@@ -31,10 +38,17 @@ func (d *SaverDialer) DialContext(ctx context.Context, network, address string)
 	return conn, err
 }
 
+func (d *SaverDialer) CloseIdleConnections() {
+	d.Dialer.CloseIdleConnections()
+}
+
 // SaverConnDialer wraps the returned connection such that we
 // collect all the read/write events that occur.
 type SaverConnDialer struct {
-	model.Dialer
+	// Dialer is the underlying dialer
+	Dialer model.Dialer
+
+	// Saver saves events
 	Saver *Saver
 }
 
@@ -47,6 +61,10 @@ func (d *SaverConnDialer) DialContext(ctx context.Context, network, address stri
 	return &saverConn{saver: d.Saver, Conn: conn}, nil
 }
 
+func (d *SaverConnDialer) CloseIdleConnections() {
+	d.Dialer.CloseIdleConnections()
+}
+
 type saverConn struct {
 	net.Conn
 	saver *Saver

internal/engine/netx/tracex/doc.go

@@ -1,2 +1,8 @@
-// Package tracex contains code to perform measurements using tracing.
+// Package tracex performs measurements using tracing. To use tracing means
+// that we'll wrap netx data types (e.g., a Dialer) with equivalent data types
+// saving events into a Saver data struture. Then we will use the data types
+// normally (e.g., call the Dialer's DialContet method and then use the
+// resulting connection). When done, we will extract the trace containing
+// all the events that occurred from the saver and process it to determine
+// what happened during the measurement itself.
 package tracex