refactor(all): improve SummaryKeys management

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,26 +124,26 @@ 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
-// therefore we should be careful when changing it.
 type SummaryKeys struct {
 	Latency   float64 `json:"connect_latency"`
 	Bitrate   float64 `json:"median_bitrate"`
 	Delay     float64 `json:"min_playout_delay"`
 	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")
-	}
+// MeasurementSummaryKeys implements model.MeasurementSummaryKeysProvider.
+func (tk *TestKeys) MeasurementSummaryKeys() model.MeasurementSummaryKeys {
+	sk := &SummaryKeys{IsAnomaly: false}
 	sk.Latency = tk.Simple.ConnectLatency
 	sk.Bitrate = float64(tk.Simple.MedianBitrate)
 	sk.Delay = tk.Simple.MinPlayoutDelay
-	return sk, nil
+	return sk
+}
+
+// Anomaly implements model.MeasurementSummary.
+func (sk *SummaryKeys) Anomaly() bool {
+	return sk.IsAnomaly
 }

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,27 +207,27 @@ 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
-// therefore we should be careful when changing it.
 type SummaryKeys struct {
 	DNSBlocking bool `json:"facebook_dns_blocking"`
 	TCPBlocking bool `json:"facebook_tcp_blocking"`
 	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")
-	}
+// MeasurementSummaryKeys implements model.MeasurementSummaryKeysProvider.
+func (tk *TestKeys) MeasurementSummaryKeys() model.MeasurementSummaryKeys {
+	sk := &SummaryKeys{IsAnomaly: false}
 	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 sk
+}
+
+// Anomaly implements model.MeasurementSummary.
+func (sk *SummaryKeys) Anomaly() bool {
+	return sk.IsAnomaly
 }

internal/experiment/hhfm/hhfm.go

@@ -340,26 +340,26 @@ 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
-// 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) {
-	sk := SummaryKeys{IsAnomaly: false}
-	tk, ok := measurement.TestKeys.(*TestKeys)
-	if !ok {
-		return sk, errors.New("invalid test keys type")
-	}
+// MeasurementSummaryKeys implements model.MeasurementSummaryKeysProvider.
+func (tk *TestKeys) MeasurementSummaryKeys() model.MeasurementSummaryKeys {
+	sk := &SummaryKeys{IsAnomaly: false}
 	sk.IsAnomaly = (tk.Tampering.HeaderFieldName ||
 		tk.Tampering.HeaderFieldNumber ||
 		tk.Tampering.HeaderFieldValue ||
 		tk.Tampering.HeaderNameCapitalization ||
 		tk.Tampering.RequestLineCapitalization ||
 		tk.Tampering.Total)
-	return sk, nil
+	return sk
+}
+
+// Anomaly implements model.MeasurementSummaryKeys.
+func (sk *SummaryKeys) Anomaly() bool {
+	return sk.IsAnomaly
 }

internal/experiment/hirl/hirl.go

@@ -305,21 +305,21 @@ 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
-// 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) {
-	sk := SummaryKeys{IsAnomaly: false}
-	tk, ok := measurement.TestKeys.(*TestKeys)
-	if !ok {
-		return sk, errors.New("invalid test keys type")
-	}
+// MeasurementSummaryKeys implements model.MeasurementSummaryKeysProvider.
+func (tk *TestKeys) MeasurementSummaryKeys() model.MeasurementSummaryKeys {
+	sk := &SummaryKeys{IsAnomaly: false}
 	sk.IsAnomaly = tk.Tampering
-	return sk, nil
+	return sk
+}
+
+// Anomaly implements model.MeasurementSummaryKeys.
+func (sk *SummaryKeys) Anomaly() bool {
+	return sk.IsAnomaly
 }

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,10 +263,9 @@ 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
-// therefore we should be careful when changing it.
 type SummaryKeys struct {
 	Upload         float64 `json:"upload"`
 	Download       float64 `json:"download"`
@@ -280,13 +278,9 @@ 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")
-	}
+// MeasurementSummaryKeys implements model.MeasurementSummaryKeysProvider.
+func (tk *TestKeys) MeasurementSummaryKeys() model.MeasurementSummaryKeys {
+	sk := &SummaryKeys{IsAnomaly: false}
 	sk.Upload = tk.Summary.Upload
 	sk.Download = tk.Summary.Download
 	sk.Ping = tk.Summary.Ping
@@ -295,5 +289,10 @@ func (m Measurer) GetSummaryKeys(measurement *model.Measurement) (interface{}, e
 	sk.MinRTT = tk.Summary.MinRTT
 	sk.MSS = float64(tk.Summary.MSS)
 	sk.RetransmitRate = tk.Summary.RetransmitRate
-	return sk, nil
+	return sk
+}
+
+// Anomaly implements model.MeasurementSummaryKeys.
+func (sk *SummaryKeys) Anomaly() bool {
+	return sk.IsAnomaly
 }