refactor(all): improve SummaryKeys management (#1491)

While approaching ooni/probe#2667, I
recognized that the approach we're using to generate `SummaryKeys` is
redundant, verbose, and fragile. This diff attempts to improve our
implementation.

We define new types for generating summary keys and
`engine.MeasurementSummaryKeys` to either return the proper summary
keys, if the experiment `TestKeys` support that or return a default
value.

While there, disable the flaky `throttlingWithHTTP` QA check (see
ooni/probe#2668).

cmd/ooniprobe/internal/nettests/nettests.go

@@ -240,18 +240,9 @@ 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
-		}
+		sk := engine.MeasurementSummaryKeys(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,10 +8,10 @@ import (
 	"net/url"
 	"os"
 	"path/filepath"
-	"reflect"
 	"time"
 
 	"github.com/apex/log"
+	"github.com/ooni/probe-cli/v3/internal/engine"
 	"github.com/ooni/probe-cli/v3/internal/model"
 	"github.com/pkg/errors"
 	"github.com/upper/db/v4"
@@ -349,27 +349,25 @@ func (d *Database) CreateOrUpdateURL(urlStr string, categoryCode string, country
 	return url.ID.Int64, nil
 }
 
-// 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 updateDatabaseMeasurementWithSummaryKeys(msmt *model.DatabaseMeasurement, sk model.MeasurementSummaryKeys) error {
+	skBytes, err := json.Marshal(sk)
 	if err != nil {
 		log.WithError(err).Error("failed to serialize summary")
+		return err
 	}
-	// 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(skBytes)
+	_, isNotImplemented := sk.(*engine.ExperimentMeasurementSummaryKeysNotImplemented)
+	msmt.IsAnomaly = sql.NullBool{Bool: sk.Anomaly(), Valid: !isNotImplemented}
+	return nil
+}
+
+// AddTestKeys implements WritableDatabase.AddTestKeys
+func (d *Database) AddTestKeys(msmt *model.DatabaseMeasurement, sk model.MeasurementSummaryKeys) error {
+	if err := updateDatabaseMeasurementWithSummaryKeys(msmt, sk); err != nil {
+		// error message already printed
+		return err
 	}
-	msmt.TestKeys = string(tkBytes)
-	msmt.IsAnomaly = sql.NullBool{Bool: isAnomaly, Valid: isAnomalyValid}
-	err = d.sess.Collection("measurements").Find("measurement_id", msmt.ID).Update(msmt)
+	err := d.sess.Collection("measurements").Find("measurement_id", msmt.ID).Update(msmt)
 	if err != nil {
 		log.WithError(err).Error("failed to update measurement")
 		return errors.Wrap(err, "updating measurement")

internal/database/actions_test.go

@@ -8,7 +8,12 @@ import (
 	"os"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
+	"github.com/ooni/probe-cli/v3/internal/engine"
+	"github.com/ooni/probe-cli/v3/internal/experiment/signal"
 	"github.com/ooni/probe-cli/v3/internal/model"
+	"github.com/ooni/probe-cli/v3/internal/must"
+	"github.com/ooni/probe-cli/v3/internal/testingx"
 	"github.com/upper/db/v4"
 )
 
@@ -427,3 +432,95 @@ func TestGetMeasurementJSON(t *testing.T) {
 		t.Error("inconsistent measurement downloaded")
 	}
 }
+
+// chansummary is used to test that we handle summary serialization errors.
+type chansummary chan int
+
+var _ model.MeasurementSummaryKeys = make(chansummary)
+
+// Anomaly implements model.MeasurementSummaryKeys.
+func (chansummary) Anomaly() bool {
+	return false
+}
+
+func TestUpdateDatabaseMeasurementWithSummaryKeys(t *testing.T) {
+	t.Run("we update the .TestKeys field", func(t *testing.T) {
+		meas := &model.DatabaseMeasurement{}
+		sk := &signal.SummaryKeys{}
+		ffiller := &testingx.FakeFiller{}
+		ffiller.Fill(sk)
+
+		if err := updateDatabaseMeasurementWithSummaryKeys(meas, sk); err != nil {
+			t.Fatal(err)
+		}
+
+		if len(meas.TestKeys) <= 0 {
+			t.Fatal("no meas.TestKeys")
+		}
+
+		var got signal.SummaryKeys
+		must.UnmarshalJSON([]byte(meas.TestKeys), &got)
+		if diff := cmp.Diff(sk, &got); diff != "" {
+			t.Fatal(diff)
+		}
+	})
+
+	t.Run("we set .Anomaly.Bool to true and .Anomaly.Valid to true when needed", func(t *testing.T) {
+		meas := &model.DatabaseMeasurement{}
+		sk := &signal.SummaryKeys{
+			IsAnomaly: true,
+		}
+
+		if err := updateDatabaseMeasurementWithSummaryKeys(meas, sk); err != nil {
+			t.Fatal(err)
+		}
+
+		if meas.IsAnomaly.Bool != sk.IsAnomaly {
+			t.Fatal("unexpected value")
+		}
+		if meas.IsAnomaly.Valid != true {
+			t.Fatal("unexpected value")
+		}
+	})
+
+	t.Run("we set .Anomaly.Bool to false and .Anomaly.Valid to true when needed", func(t *testing.T) {
+		meas := &model.DatabaseMeasurement{}
+		sk := &signal.SummaryKeys{
+			IsAnomaly: false,
+		}
+
+		if err := updateDatabaseMeasurementWithSummaryKeys(meas, sk); err != nil {
+			t.Fatal(err)
+		}
+
+		if meas.IsAnomaly.Bool != sk.IsAnomaly {
+			t.Fatal("unexpected value")
+		}
+		if meas.IsAnomaly.Valid != true {
+			t.Fatal("unexpected value")
+		}
+	})
+
+	t.Run("we set .Anomaly.Valid to false when needed", func(t *testing.T) {
+		meas := &model.DatabaseMeasurement{}
+		sk := &engine.ExperimentMeasurementSummaryKeysNotImplemented{}
+
+		if err := updateDatabaseMeasurementWithSummaryKeys(meas, sk); err != nil {
+			t.Fatal(err)
+		}
+
+		if meas.IsAnomaly.Valid != false {
+			t.Fatal("unexpected value")
+		}
+	})
+
+	t.Run("we handle the case where we cannot serialize the summary", func(t *testing.T) {
+		meas := &model.DatabaseMeasurement{}
+		sk := make(chansummary)
+
+		err := updateDatabaseMeasurementWithSummaryKeys(meas, sk)
+		if err == nil || err.Error() != "json: unsupported type: database.chansummary" {
+			t.Fatal("unexpected error", err)
+		}
+	})
+}

internal/engine/experiment.go

@@ -60,9 +60,23 @@ func (e *experiment) Name() string {
 	return e.testName
 }
 
-// GetSummaryKeys implements Experiment.GetSummaryKeys.
-func (e *experiment) GetSummaryKeys(m *model.Measurement) (interface{}, error) {
-	return e.measurer.GetSummaryKeys(m)
+// 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
+}
+
+// MeasurementSummaryKeys returns the [model.MeasurementSummaryKeys] associated with a given measurement.
+func MeasurementSummaryKeys(m *model.Measurement) model.MeasurementSummaryKeys {
+	if tk, ok := m.TestKeys.(model.MeasurementSummaryKeysProvider); ok {
+		return tk.MeasurementSummaryKeys()
+	}
+	return &ExperimentMeasurementSummaryKeysNotImplemented{}
 }
 
 // ReportID implements Experiment.ReportID.

internal/engine/experiment_integration_test.go

@@ -285,8 +285,9 @@ func runexperimentflow(t *testing.T, experiment model.Experiment, input string)
 	if experiment.KibiBytesReceived() <= 0 {
 		t.Fatal("no data received?!")
 	}
-	if _, err := experiment.GetSummaryKeys(measurement); err != nil {
-		t.Fatal(err)
+	sk := MeasurementSummaryKeys(measurement)
+	if sk == nil {
+		t.Fatal("got nil summary keys")
 	}
 }
 
@@ -485,9 +486,3 @@ func (am *antaniMeasurer) ExperimentVersion() string {
 func (am *antaniMeasurer) Run(ctx context.Context, args *model.ExperimentArgs) error {
 	return nil
 }
-
-func (am *antaniMeasurer) GetSummaryKeys(m *model.Measurement) (interface{}, error) {
-	return struct {
-		Failure *string `json:"failure"`
-	}{}, nil
-}

internal/engine/experiment_test.go

@@ -4,6 +4,8 @@ import (
 	"testing"
 
 	"github.com/ooni/probe-cli/v3/internal/enginelocate"
+	"github.com/ooni/probe-cli/v3/internal/experiment/example"
+	"github.com/ooni/probe-cli/v3/internal/experiment/signal"
 	"github.com/ooni/probe-cli/v3/internal/model"
 )
 
@@ -73,3 +75,33 @@ func TestExperimentHonoursSharingDefaults(t *testing.T) {
 		})
 	}
 }
+
+func TestExperimentMeasurementSummaryKeysNotImplemented(t *testing.T) {
+	t.Run("the .Anomaly method returns false", func(t *testing.T) {
+		sk := &ExperimentMeasurementSummaryKeysNotImplemented{}
+		if sk.Anomaly() != false {
+			t.Fatal("expected false")
+		}
+	})
+}
+
+func TestExperimentMeasurementSummaryKeys(t *testing.T) {
+	t.Run("when the TestKeys implement MeasurementSummaryKeysProvider", func(t *testing.T) {
+		tk := &signal.TestKeys{}
+		meas := &model.Measurement{TestKeys: tk}
+		sk := MeasurementSummaryKeys(meas)
+		if _, good := sk.(*signal.SummaryKeys); !good {
+			t.Fatal("not the expected type")
+		}
+	})
+
+	t.Run("otherwise", func(t *testing.T) {
+		// note: example does not implement SummaryKeys
+		tk := &example.TestKeys{}
+		meas := &model.Measurement{TestKeys: tk}
+		sk := MeasurementSummaryKeys(meas)
+		if _, good := sk.(*ExperimentMeasurementSummaryKeysNotImplemented); !good {
+			t.Fatal("not the expected type")
+		}
+	})
+}

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/dash/measurer_test.go

@@ -105,22 +105,6 @@ func TestMeasureWithCancelledContext(t *testing.T) {
 	if !errors.Is(err, nil) {
 		t.Fatal("unexpected error value")
 	}
-	sk, err := m.GetSummaryKeys(measurement)
-	if err != nil {
-		t.Fatal(err)
-	}
-	if _, ok := sk.(SummaryKeys); !ok {
-		t.Fatal("invalid type for summary keys")
-	}
-}
-
-func TestSummaryKeysInvalidType(t *testing.T) {
-	measurement := new(model.Measurement)
-	m := &Measurer{}
-	_, err := m.GetSummaryKeys(measurement)
-	if err.Error() != "invalid test keys type" {
-		t.Fatal("not the error we expected")
-	}
 }
 
 func TestSummaryKeysGood(t *testing.T) {
@@ -129,12 +113,8 @@ func TestSummaryKeysGood(t *testing.T) {
 		MedianBitrate:   123,
 		MinPlayoutDelay: 12,
 	}}}
-	m := &Measurer{}
-	osk, err := m.GetSummaryKeys(measurement)
-	if err != nil {
-		t.Fatal(err)
-	}
-	sk := osk.(SummaryKeys)
+	osk := measurement.TestKeys.(*TestKeys).MeasurementSummaryKeys()
+	sk := osk.(*SummaryKeys)
 	if sk.Latency != 1234 {
 		t.Fatal("invalid latency")
 	}
@@ -147,4 +127,7 @@ func TestSummaryKeysGood(t *testing.T) {
 	if sk.IsAnomaly {
 		t.Fatal("invalid isAnomaly")
 	}
+	if sk.Anomaly() != sk.IsAnomaly {
+		t.Fatal("sk.Anomaly() does not return sk.IsAnomaly's value")
+	}
 }

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
-}