Start improving how we set summary keys

Lots of errors and bugfixes still needed but let's publish
the branch first and then continue hammering it.

See ooni/probe#2667

cmd/ooniprobe/internal/nettests/nettests.go

@@ -240,18 +240,11 @@ func (c *Controller) Run(builder model.ExperimentBuilder, inputs []string) error
 			return errors.Wrap(err, "failed to mark measurement as done")
 		}
 
-		// We're not sure whether it's enough to log the error or we should
-		// instead also mark the measurement as failed. Strictly speaking this
-		// is an inconsistency between the code that generate the measurement
-		// and the code that process the measurement. We do have some data
-		// but we're not gonna have a summary. To be reconsidered.
-		tk, err := exp.GetSummaryKeys(measurement)
-		if err != nil {
-			log.WithError(err).Error("failed to obtain testKeys")
-			continue
-		}
+		// Since 2024-02-06, the experiment GetSummaryKeys function returns a default
+		// implementation in case the experiment does not provide one.
+		sk := exp.GetSummaryKeys(measurement)
 		log.Debugf("Fetching: %d %v", idx, c.msmts[idx64])
-		if err := db.AddTestKeys(c.msmts[idx64], tk); err != nil {
+		if err := db.AddTestKeys(c.msmts[idx64], sk); err != nil {
 			return errors.Wrap(err, "failed to add test keys to summary")
 		}
 	}

internal/database/actions.go

@@ -8,7 +8,6 @@ import (
 	"net/url"
 	"os"
 	"path/filepath"
-	"reflect"
 	"time"
 
 	"github.com/apex/log"
@@ -350,25 +349,16 @@ func (d *Database) CreateOrUpdateURL(urlStr string, categoryCode string, country
 }
 
 // AddTestKeys implements WritableDatabase.AddTestKeys
-func (d *Database) AddTestKeys(msmt *model.DatabaseMeasurement, tk any) error {
-	var (
-		isAnomaly      bool
-		isAnomalyValid bool
-	)
-	tkBytes, err := json.Marshal(tk)
+func (d *Database) AddTestKeys(msmt *model.DatabaseMeasurement, sk model.MeasurementSummaryKeys) error {
+	skBytes, err := json.Marshal(sk)
 	if err != nil {
 		log.WithError(err).Error("failed to serialize summary")
 	}
 	// This is necessary so that we can extract from the the opaque testKeys just
 	// the IsAnomaly field of bool type.
 	// Maybe generics are not so bad after-all, heh golang?
-	isAnomalyValue := reflect.ValueOf(tk).FieldByName("IsAnomaly")
-	if isAnomalyValue.IsValid() && isAnomalyValue.Kind() == reflect.Bool {
-		isAnomaly = isAnomalyValue.Bool()
-		isAnomalyValid = true
-	}
-	msmt.TestKeys = string(tkBytes)
-	msmt.IsAnomaly = sql.NullBool{Bool: isAnomaly, Valid: isAnomalyValid}
+	msmt.TestKeys = string(skBytes)
+	msmt.IsAnomaly = sql.NullBool{Bool: sk.Anomaly(), Valid: true}
 	err = d.sess.Collection("measurements").Find("measurement_id", msmt.ID).Update(msmt)
 	if err != nil {
 		log.WithError(err).Error("failed to update measurement")

internal/engine/experiment.go

@@ -60,9 +60,26 @@ func (e *experiment) Name() string {
 	return e.testName
 }
 
+// experimentMeasurementSummaryKeysNotImplemented is the [model.MeasurementSummary] we use when
+// the experiment TestKeys do not provide an implementation of [model.MeasurementSummary].
+type experimentMeasurementSummaryKeysNotImplemented struct{}
+
+var _ model.MeasurementSummaryKeys = &experimentMeasurementSummaryKeysNotImplemented{}
+
+// IsAnomaly implements MeasurementSummary.
+func (*experimentMeasurementSummaryKeysNotImplemented) Anomaly() bool {
+	return false
+}
+
+// ErrInvalidTestKeysType indicates that the test keys are invalid.
+var ErrInvalidTestKeysType = errors.New("invalid test keys type")
+
 // GetSummaryKeys implements Experiment.GetSummaryKeys.
-func (e *experiment) GetSummaryKeys(m *model.Measurement) (interface{}, error) {
-	return e.measurer.GetSummaryKeys(m)
+func (e *experiment) GetSummaryKeys(m *model.Measurement) model.MeasurementSummaryKeys {
+	if tk, ok := m.TestKeys.(model.MeasurementSummaryKeysProvider); ok {
+		return tk.MeasurementSummaryKeys()
+	}
+	return &experimentMeasurementSummaryKeysNotImplemented{}
 }
 
 // ReportID implements Experiment.ReportID.

internal/experiment/dash/measurer.go

@@ -6,7 +6,6 @@ package dash
 
 import (
 	"context"
-	"errors"
 	"net/http"
 
 	"github.com/ooni/probe-cli/v3/internal/legacy/netx"
@@ -125,6 +124,8 @@ func NewExperimentMeasurer(config Config) model.ExperimentMeasurer {
 	return &Measurer{config: config}
 }
 
+var _ model.MeasurementSummaryKeysProvider = &TestKeys{}
+
 // SummaryKeys contains summary keys for this experiment.
 //
 // Note that this structure is part of the ABI contract with ooniprobe
@@ -136,15 +137,17 @@ type SummaryKeys struct {
 	IsAnomaly bool    `json:"-"`
 }
 
-// GetSummaryKeys implements model.ExperimentMeasurer.GetSummaryKeys.
-func (m Measurer) GetSummaryKeys(measurement *model.Measurement) (any, error) {
-	sk := SummaryKeys{IsAnomaly: false}
-	tk, ok := measurement.TestKeys.(*TestKeys)
-	if !ok {
-		return sk, errors.New("invalid test keys type")
+// Anomaly implements model.MeasurementSummary.
+func (sk *SummaryKeys) Anomaly() bool {
+	return sk.IsAnomaly
+}
+
+// MeasurementSummaryKeys implements model.MeasurementSummaryKeysProvider.
+func (tk *TestKeys) MeasurementSummaryKeys() model.MeasurementSummaryKeys {
+	return &SummaryKeys{
+		Latency:   tk.Simple.ConnectLatency,
+		Bitrate:   float64(tk.Simple.MedianBitrate),
+		Delay:     tk.Simple.MinPlayoutDelay,
+		IsAnomaly: false,
 	}
-	sk.Latency = tk.Simple.ConnectLatency
-	sk.Bitrate = float64(tk.Simple.MedianBitrate)
-	sk.Delay = tk.Simple.MinPlayoutDelay
-	return sk, nil
 }

internal/experiment/dnscheck/dnscheck.go

@@ -317,16 +317,3 @@ func NewExperimentMeasurer(config Config) model.ExperimentMeasurer {
 		Endpoints: nil, // disabled by default
 	}
 }
-
-// SummaryKeys contains summary keys for this experiment.
-//
-// Note that this structure is part of the ABI contract with ooniprobe
-// therefore we should be careful when changing it.
-type SummaryKeys struct {
-	IsAnomaly bool `json:"-"`
-}
-
-// GetSummaryKeys implements model.ExperimentMeasurer.GetSummaryKeys.
-func (m *Measurer) GetSummaryKeys(measurement *model.Measurement) (interface{}, error) {
-	return SummaryKeys{IsAnomaly: false}, nil
-}

internal/experiment/dnsping/dnsping.go

@@ -226,16 +226,3 @@ func stopOperationLogger(ol stoppableOperationLogger, addrs []string, err error)
 func NewExperimentMeasurer(config Config) model.ExperimentMeasurer {
 	return &Measurer{config: config}
 }
-
-// SummaryKeys contains summary keys for this experiment.
-//
-// Note that this structure is part of the ABI contract with ooniprobe
-// therefore we should be careful when changing it.
-type SummaryKeys struct {
-	IsAnomaly bool `json:"-"`
-}
-
-// GetSummaryKeys implements model.ExperimentMeasurer.GetSummaryKeys.
-func (m Measurer) GetSummaryKeys(measurement *model.Measurement) (interface{}, error) {
-	return SummaryKeys{IsAnomaly: false}, nil
-}

internal/experiment/echcheck/measure.go

@@ -133,13 +133,3 @@ func (m *Measurer) Run(
 func NewExperimentMeasurer(config Config) model.ExperimentMeasurer {
 	return &Measurer{config: config}
 }
-
-// SummaryKeys contains summary keys for this experiment.
-type SummaryKeys struct {
-	IsAnomaly bool `json:"-"`
-}
-
-// GetSummaryKeys implements model.ExperimentMeasurer.GetSummaryKeys.
-func (m *Measurer) GetSummaryKeys(measurement *model.Measurement) (interface{}, error) {
-	return SummaryKeys{IsAnomaly: false}, nil
-}

internal/experiment/example/example.go

@@ -84,16 +84,3 @@ func (m Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error {
 func NewExperimentMeasurer(config Config, testName string) model.ExperimentMeasurer {
 	return Measurer{config: config, testName: testName}
 }
-
-// SummaryKeys contains summary keys for this experiment.
-//
-// Note that this structure is part of the ABI contract with ooniprobe
-// therefore we should be careful when changing it.
-type SummaryKeys struct {
-	IsAnomaly bool `json:"-"`
-}
-
-// GetSummaryKeys implements model.ExperimentMeasurer.GetSummaryKeys.
-func (m Measurer) GetSummaryKeys(measurement *model.Measurement) (interface{}, error) {
-	return SummaryKeys{IsAnomaly: false}, nil
-}

internal/experiment/fbmessenger/fbmessenger.go

@@ -5,7 +5,6 @@ package fbmessenger
 
 import (
 	"context"
-	"errors"
 	"math/rand"
 	"time"
 
@@ -208,6 +207,8 @@ func NewExperimentMeasurer(config Config) model.ExperimentMeasurer {
 	return Measurer{Config: config}
 }
 
+var _ model.MeasurementSummaryKeysProvider = &TestKeys{}
+
 // SummaryKeys contains summary keys for this experiment.
 //
 // Note that this structure is part of the ABI contract with ooniprobe
@@ -218,17 +219,18 @@ type SummaryKeys struct {
 	IsAnomaly   bool `json:"-"`
 }
 
-// GetSummaryKeys implements model.ExperimentMeasurer.GetSummaryKeys.
-func (m Measurer) GetSummaryKeys(measurement *model.Measurement) (interface{}, error) {
-	sk := SummaryKeys{IsAnomaly: false}
-	tk, ok := measurement.TestKeys.(*TestKeys)
-	if !ok {
-		return sk, errors.New("invalid test keys type")
-	}
+// Anomaly implements model.MeasurementSummary.
+func (sk *SummaryKeys) Anomaly() bool {
+	return sk.IsAnomaly
+}
+
+// MeasurementSummaryKeys implements model.MeasurementSummaryKeysProvider.
+func (tk *TestKeys) MeasurementSummaryKeys() model.MeasurementSummaryKeys {
 	dnsBlocking := tk.FacebookDNSBlocking != nil && *tk.FacebookDNSBlocking
 	tcpBlocking := tk.FacebookTCPBlocking != nil && *tk.FacebookTCPBlocking
-	sk.DNSBlocking = dnsBlocking
-	sk.TCPBlocking = tcpBlocking
-	sk.IsAnomaly = dnsBlocking || tcpBlocking
-	return sk, nil
+	return &SummaryKeys{
+		DNSBlocking: dnsBlocking,
+		TCPBlocking: tcpBlocking,
+		IsAnomaly:   dnsBlocking || tcpBlocking,
+	}
 }

internal/experiment/hhfm/hhfm.go

@@ -340,6 +340,8 @@ func (c Conn) Write(b []byte) (int, error) {
 	return c.Conn.Write(b)
 }
 
+var _ model.MeasurementSummaryKeysProvider = &TestKeys{}
+
 // SummaryKeys contains summary keys for this experiment.
 //
 // Note that this structure is part of the ABI contract with ooniprobe
@@ -348,18 +350,19 @@ type SummaryKeys struct {
 	IsAnomaly bool `json:"-"`
 }
 
-// GetSummaryKeys implements model.ExperimentMeasurer.GetSummaryKeys.
-func (m Measurer) GetSummaryKeys(measurement *model.Measurement) (interface{}, error) {
-	sk := SummaryKeys{IsAnomaly: false}
-	tk, ok := measurement.TestKeys.(*TestKeys)
-	if !ok {
-		return sk, errors.New("invalid test keys type")
+// Anomaly implements model.MeasurementSummaryKeys.
+func (sk *SummaryKeys) Anomaly() bool {
+	return sk.IsAnomaly
+}
+
+// MeasurementSummaryKeys implements model.MeasurementSummaryKeysProvider.
+func (tk *TestKeys) MeasurementSummaryKeys() model.MeasurementSummaryKeys {
+	return &SummaryKeys{
+		IsAnomaly: (tk.Tampering.HeaderFieldName ||
+			tk.Tampering.HeaderFieldNumber ||
+			tk.Tampering.HeaderFieldValue ||
+			tk.Tampering.HeaderNameCapitalization ||
+			tk.Tampering.RequestLineCapitalization ||
+			tk.Tampering.Total),
 	}
-	sk.IsAnomaly = (tk.Tampering.HeaderFieldName ||
-		tk.Tampering.HeaderFieldNumber ||
-		tk.Tampering.HeaderFieldValue ||
-		tk.Tampering.HeaderNameCapitalization ||
-		tk.Tampering.RequestLineCapitalization ||
-		tk.Tampering.Total)
-	return sk, nil
 }

internal/experiment/hirl/hirl.go

@@ -305,6 +305,8 @@ func RunMethod(ctx context.Context, config RunMethodConfig) {
 	}
 }
 
+var _ model.MeasurementSummaryKeysProvider = &TestKeys{}
+
 // SummaryKeys contains summary keys for this experiment.
 //
 // Note that this structure is part of the ABI contract with ooniprobe
@@ -313,13 +315,12 @@ type SummaryKeys struct {
 	IsAnomaly bool `json:"-"`
 }
 
-// GetSummaryKeys implements model.ExperimentMeasurer.GetSummaryKeys.
-func (m Measurer) GetSummaryKeys(measurement *model.Measurement) (interface{}, error) {
-	sk := SummaryKeys{IsAnomaly: false}
-	tk, ok := measurement.TestKeys.(*TestKeys)
-	if !ok {
-		return sk, errors.New("invalid test keys type")
-	}
-	sk.IsAnomaly = tk.Tampering
-	return sk, nil
+// Anomaly implements model.MeasurementSummaryKeys.
+func (sk *SummaryKeys) Anomaly() bool {
+	return sk.IsAnomaly
+}
+
+// MeasurementSummaryKeys implements model.MeasurementSummaryKeysProvider.
+func (tk *TestKeys) MeasurementSummaryKeys() model.MeasurementSummaryKeys {
+	return &SummaryKeys{IsAnomaly: tk.Tampering}
 }

internal/experiment/httphostheader/httphostheader.go

@@ -77,16 +77,3 @@ func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error {
 func NewExperimentMeasurer(config Config) model.ExperimentMeasurer {
 	return &Measurer{config: config}
 }
-
-// SummaryKeys contains summary keys for this experiment.
-//
-// Note that this structure is part of the ABI contract with ooniprobe
-// therefore we should be careful when changing it.
-type SummaryKeys struct {
-	IsAnomaly bool `json:"-"`
-}
-
-// GetSummaryKeys implements model.ExperimentMeasurer.GetSummaryKeys.
-func (m Measurer) GetSummaryKeys(measurement *model.Measurement) (interface{}, error) {
-	return SummaryKeys{IsAnomaly: false}, nil
-}

internal/experiment/ndt7/ndt7.go

@@ -3,7 +3,6 @@ package ndt7
 import (
 	"context"
 	"encoding/json"
-	"errors"
 	"fmt"
 	"time"
 
@@ -264,6 +263,8 @@ func failureFromError(err error) (failure *string) {
 	return
 }
 
+var _ model.MeasurementSummaryKeysProvider = &TestKeys{}
+
 // SummaryKeys contains summary keys for this experiment.
 //
 // Note that this structure is part of the ABI contract with ooniprobe
@@ -280,20 +281,22 @@ type SummaryKeys struct {
 	IsAnomaly      bool    `json:"-"`
 }
 
-// GetSummaryKeys implements model.ExperimentMeasurer.GetSummaryKeys.
-func (m Measurer) GetSummaryKeys(measurement *model.Measurement) (interface{}, error) {
-	sk := SummaryKeys{IsAnomaly: false}
-	tk, ok := measurement.TestKeys.(*TestKeys)
-	if !ok {
-		return sk, errors.New("invalid test keys type")
+// Anomaly implements model.MeasurementSummaryKeys.
+func (sk *SummaryKeys) Anomaly() bool {
+	return sk.IsAnomaly
+}
+
+// MeasurementSummaryKeys implements model.MeasurementSummaryKeysProvider.
+func (tk *TestKeys) MeasurementSummaryKeys() model.MeasurementSummaryKeys {
+	return &SummaryKeys{
+		Upload:         tk.Summary.Upload,
+		Download:       tk.Summary.Download,
+		Ping:           tk.Summary.Ping,
+		MaxRTT:         tk.Summary.MaxRTT,
+		AvgRTT:         tk.Summary.AvgRTT,
+		MinRTT:         tk.Summary.MinRTT,
+		MSS:            float64(tk.Summary.MSS),
+		RetransmitRate: tk.Summary.RetransmitRate,
+		IsAnomaly:      false,
 	}
-	sk.Upload = tk.Summary.Upload
-	sk.Download = tk.Summary.Download
-	sk.Ping = tk.Summary.Ping
-	sk.MaxRTT = tk.Summary.MaxRTT
-	sk.AvgRTT = tk.Summary.AvgRTT
-	sk.MinRTT = tk.Summary.MinRTT
-	sk.MSS = float64(tk.Summary.MSS)
-	sk.RetransmitRate = tk.Summary.RetransmitRate
-	return sk, nil
 }