fix: correctly set options from richer input (#1630)

While there, update the living design document with features and fixes
implemented in this sprint.

Closes ooni/probe#2763.

docs/design/dd-008-richer-input.md

@@ -199,6 +199,7 @@ type ExperimentTarget struct {
 	Category() string  // equivalent to OOAPIURLInfo.CategoryCode
 	Country() string   // equivalent to OOAPIURLInfo.CountryCode
 	Input() string     // equivalent to OOAPIURLInfo.URL
+	String() string    // serializes to the input
 }
 
 type InputLoader = TargetLoader // we renamed to reflect the change of purpose
@@ -212,6 +213,10 @@ type Experiment interface {
 }
 ```
 
+The `String` method is used to reduce the `ExperimentTarget` to the input string, which
+allows for backwards compatibility. We can obtain a string representation of the target's
+input and use it every time where previous we used the `input` string.
+
 Note that we also renamed the `InputLoader` to `TargetLoader` to reflect the fact that
 we're not loading bare input anymore, rather we're loading richer input targets.
 
@@ -361,14 +366,90 @@ wouldn't be able to cast such an interface to the `*target` type. Therefore,
 unconditionally casting could lead to crashes when integrating new code
 and generally makes for a less robust codebase.
 
+## Implementation: add OpenVPN
+
+Pull request [#1625](https://github.com/ooni/probe-cli/pull/1625) added richer
+input support for the `openvpn` experiment. Because this experiment already
+supports richer input through the `api.dev.ooni.io` backend, we now have the
+first experiment capable of using richer input.
+
+## Implementation: fix serializing options
+
+Pull request [#1630](https://github.com/ooni/probe-cli/pull/1630) adds
+support for correctly serializing options. We extend the model of a richer
+input target to include the following function:
+
+```Go
+type ExperimentTarget struct {
+	// ...
+	Options() []string
+}
+```
+
+Then we implement `Options` for every possible experiment target. There is
+a default implementation in the `experimentconfig` package implementing the
+default semantics that was also available before:
+
+1. skip fields whose name starts with `Safe`;
+
+2. only serialize scalar values;
+
+3. do not serializes any zero value.
+
+Additionally, we now serialize the options inside the `newMeasurement`
+constructor typical of each experiment.
+
+## Implementation: improve passing options to experiments
+
+Pull request [#1629](https://github.com/ooni/probe-cli/pull/1629) modifies
+the way in which the `./internal/oonirun` package loads data for experiments
+such that, when using OONI Run v2, we load its `options` field as a
+`json.RawMessage` rather than using a `map[string]any`. This fact is
+significant because, previously, we could only unmarshal options provided
+by command line, which were always scalar. With this change, instead, we
+can keep backwards compatibility with respect to the command line but it's
+now also possible for experiments options specified via OONI Run v2 to
+provide non-scalar options.
+
+The key change to enable this is to modify a `*registry.Factory` type to add:
+
+```Go
+type Factory struct { /* ... */ }
+
+func (fx *Factory) SetOptionsJSON(value json.RawMessage) error
+```
+
+In this way, we can directly assign the raw JSON to the experiment config
+that is kept inside of the `*Factory` itself.
+
+Additionally, constructing an experiment using `*oonirun.Experiment` now
+includes two options related field:
+
+```Go
+type Experiment struct {
+	InitialOptions json.RawMessage  // new addition
+	ExtraOptions   map[string]any   // also present before
+}
+```
+
+Initialization of experiment options will work as follows:
+
+1. the per-experiment `*Factory` constructor initializes fields to their
+default value, which, in most cases, SHOULD be the zero value;
+
+2. we update the config using `InitialOptions` unless it is empty;
+
+3. we update the config using `ExtraOptions` unless it is empty.
+
+In practice, the code would always use either `InitialOptions` or
+`ExtraOptions`, but we also wanted to specify priority in case both
+of them were available.
+
 ## Next steps
 
 This is a rough sequence of next steps that we should expand as we implement
 additional bits of richer input and for which we need reference issues.
 
-*  setting `(*Measurement).Options` inside `(*engine.experiment).newMeasurement`
-rather than inside the `oonirun` package.
-
 *  fully convert `dnscheck`'s static list to live inside `dnscheck` instead of
 `targetloading` and to use the proper richer input.
 
@@ -378,15 +459,9 @@ rather than inside the `oonirun` package.
 
 	*  implement backend API for serving `stunreachability` richer input.
 
-	*  implement backend API for serving `openvpn` richer input.
-
 *  deliver feature flags using experiment-specific richer input rather
 than using the check-in API (and maybe keep the caching support?).
 
-*  deliver OONI Run v2 options as a `json.RawMessage` rather than passing
-through a `map[string]any` such that we can support non-scalar options when
-using OONI Run v2.
-
 *  try to eliminate `InputPolicy` and instead have each experiment define
 its own constructor for the proper target loader, and split the implementation
 inside of the `targetloader` package to have multiple target loaders.

internal/engine/experiment.go

@@ -46,6 +46,10 @@ func (emr *experimentMutableReport) Get() (report probeservices.ReportChannel) {
 	return
 }
 
+// TODO(bassosimone,DecFox): it would be nice if `*experiment` depended
+// on an interface rather than depending on the concrete session, because
+// that will allow us to write tests using mocks much more easily.
+
 // experiment implements [model.Experiment].
 type experiment struct {
 	byteCounter   *bytecounter.Counter
@@ -111,13 +115,10 @@ func (e *experiment) SubmitAndUpdateMeasurementContext(
 // newMeasurement creates a new measurement for this experiment with the given input.
 func (e *experiment) newMeasurement(target model.ExperimentTarget) *model.Measurement {
 	utctimenow := time.Now().UTC()
-	// TODO(bassosimone,DecFox): move here code that supports filling the options field
-	// when there is richer input, which currently is inside ./internal/oonirun.
-	//
-	// We MUST do this because the current solution only works for OONI Run and when
-	// there are command line options but does not work for API/static targets.
+
 	m := &model.Measurement{
 		DataFormatVersion:         model.OOAPIReportDefaultDataFormatVersion,
+		Options:                   target.Options(),
 		Input:                     model.MeasurementInput(target.Input()),
 		MeasurementStartTime:      utctimenow.Format(model.MeasurementDateFormat),
 		MeasurementStartTimeSaved: utctimenow,
@@ -135,6 +136,7 @@ func (e *experiment) newMeasurement(target model.ExperimentTarget) *model.Measur
 		TestStartTime:             e.testStartTime,
 		TestVersion:               e.testVersion,
 	}
+
 	m.AddAnnotation("architecture", runtime.GOARCH)
 	m.AddAnnotation("engine_name", "ooniprobe-engine")
 	m.AddAnnotation("engine_version", version.Version)
@@ -144,6 +146,7 @@ func (e *experiment) newMeasurement(target model.ExperimentTarget) *model.Measur
 	m.AddAnnotation("vcs_revision", runtimex.BuildInfo.VcsRevision)
 	m.AddAnnotation("vcs_time", runtimex.BuildInfo.VcsTime)
 	m.AddAnnotation("vcs_tool", runtimex.BuildInfo.VcsTool)
+
 	return m
 }
 

internal/engine/experiment_test.go

@@ -1,9 +1,14 @@
 package engine
 
 import (
+	"sync"
 	"testing"
+	"time"
 
+	"github.com/google/go-cmp/cmp"
+	"github.com/ooni/probe-cli/v3/internal/bytecounter"
 	"github.com/ooni/probe-cli/v3/internal/enginelocate"
+	"github.com/ooni/probe-cli/v3/internal/experiment/dnscheck"
 	"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"
@@ -105,3 +110,84 @@ func TestExperimentMeasurementSummaryKeys(t *testing.T) {
 		}
 	})
 }
+
+// This test ensures that (*experiment).newMeasurement is working as intended.
+func TestExperimentNewMeasurement(t *testing.T) {
+	// create a session for testing that does not use the network at all
+	sess := newSessionForTestingNoLookups(t)
+
+	// create a conventional time for starting the experiment
+	t0 := time.Date(2024, 6, 27, 10, 33, 0, 0, time.UTC)
+
+	// create the experiment
+	exp := &experiment{
+		byteCounter: bytecounter.New(),
+		callbacks:   model.NewPrinterCallbacks(model.DiscardLogger),
+		measurer:    &dnscheck.Measurer{},
+		mrep: &experimentMutableReport{
+			mu:     sync.Mutex{},
+			report: nil,
+		},
+		session:       sess,
+		testName:      "dnscheck",
+		testStartTime: t0.Format(model.MeasurementDateFormat),
+		testVersion:   "0.1.0",
+	}
+
+	// create the richer input target
+	target := &dnscheck.Target{
+		Config: &dnscheck.Config{
+			DefaultAddrs: "8.8.8.8 2001:4860:4860::8888",
+			HTTP3Enabled: true,
+		},
+		URL: "https://dns.google/dns-query",
+	}
+
+	// create measurement
+	meas := exp.newMeasurement(target)
+
+	// make sure the input is correctly serialized
+	t.Run("Input", func(t *testing.T) {
+		if meas.Input != "https://dns.google/dns-query" {
+			t.Fatal("unexpected meas.Input")
+		}
+	})
+
+	// make sure the options are correctly serialized
+	t.Run("Options", func(t *testing.T) {
+		expectOptions := []string{`DefaultAddrs=8.8.8.8 2001:4860:4860::8888`, `HTTP3Enabled=true`}
+		if diff := cmp.Diff(expectOptions, meas.Options); diff != "" {
+			t.Fatal(diff)
+		}
+	})
+
+	// make sure we've got the expected annotation keys
+	t.Run("Annotations", func(t *testing.T) {
+		const (
+			expected = 1 << iota
+			got
+		)
+		m := map[string]int{
+			"architecture":   expected,
+			"engine_name":    expected,
+			"engine_version": expected,
+			"go_version":     expected,
+			"platform":       expected,
+			"vcs_modified":   expected,
+			"vcs_revision":   expected,
+			"vcs_time":       expected,
+			"vcs_tool":       expected,
+		}
+		for key := range meas.Annotations {
+			m[key] |= got
+		}
+		for key, value := range m {
+			if value != expected|got {
+				t.Fatal("expected", expected|got, "for", key, "got", value)
+			}
+		}
+	})
+
+	// TODO(bassosimone,DecFox): this is the correct place where to
+	// add more tests regarding how we create measurements.
+}

internal/experiment/dnscheck/dnscheck.go

@@ -134,7 +134,7 @@ func (m *Measurer) Run(ctx context.Context, args *model.ExperimentArgs) error {
 	if !ok {
 		return ErrInvalidInputType
 	}
-	config, input := target.Options, target.URL
+	config, input := target.Config, target.URL
 	sess.Logger().Infof("dnscheck: using richer input: %+v %+v", config, input)
 
 	// 1. fill the measurement with test keys

internal/experiment/dnscheck/dnscheck_test.go

@@ -72,7 +72,7 @@ func TestDNSCheckFailsWithoutInput(t *testing.T) {
 		Session:     newsession(),
 		Target: &Target{
 			URL: "", // explicitly empty
-			Options: &Config{
+			Config: &Config{
 				Domain: "example.com",
 			},
 		},
@@ -90,8 +90,8 @@ func TestDNSCheckFailsWithInvalidURL(t *testing.T) {
 		Measurement: &model.Measurement{Input: "Not a valid URL \x7f"},
 		Session:     newsession(),
 		Target: &Target{
-			URL:     "Not a valid URL \x7f",
-			Options: &Config{},
+			URL:    "Not a valid URL \x7f",
+			Config: &Config{},
 		},
 	}
 	err := measurer.Run(context.Background(), args)
@@ -107,8 +107,8 @@ func TestDNSCheckFailsWithUnsupportedProtocol(t *testing.T) {
 		Measurement: &model.Measurement{Input: "file://1.1.1.1"},
 		Session:     newsession(),
 		Target: &Target{
-			URL:     "file://1.1.1.1",
-			Options: &Config{},
+			URL:    "file://1.1.1.1",
+			Config: &Config{},
 		},
 	}
 	err := measurer.Run(context.Background(), args)
@@ -128,7 +128,7 @@ func TestWithCancelledContext(t *testing.T) {
 		Session:     newsession(),
 		Target: &Target{
 			URL: "dot://one.one.one.one",
-			Options: &Config{
+			Config: &Config{
 				DefaultAddrs: "1.1.1.1 1.0.0.1",
 			},
 		},
@@ -191,7 +191,7 @@ func TestDNSCheckValid(t *testing.T) {
 		Session:     newsession(),
 		Target: &Target{
 			URL: "dot://one.one.one.one:853",
-			Options: &Config{
+			Config: &Config{
 				DefaultAddrs: "1.1.1.1 1.0.0.1",
 			},
 		},
@@ -239,8 +239,8 @@ func TestDNSCheckWait(t *testing.T) {
 			Measurement: &measurement,
 			Session:     newsession(),
 			Target: &Target{
-				URL:     input,
-				Options: &Config{},
+				URL:    input,
+				Config: &Config{},
 			},
 		}
 		err := measurer.Run(context.Background(), args)

internal/experiment/dnscheck/richerinput.go

@@ -3,15 +3,16 @@ package dnscheck
 import (
 	"context"
 
+	"github.com/ooni/probe-cli/v3/internal/experimentconfig"
 	"github.com/ooni/probe-cli/v3/internal/model"
 	"github.com/ooni/probe-cli/v3/internal/reflectx"
 	"github.com/ooni/probe-cli/v3/internal/targetloading"
 )
 
 // Target is a richer-input target that this experiment should measure.
 type Target struct {
-	// Options contains the configuration.
-	Options *Config
+	// Config contains the configuration.
+	Config *Config
 
 	// URL is the input URL.
 	URL string
@@ -34,6 +35,11 @@ func (t *Target) Input() string {
 	return t.URL
 }
 
+// Options implements [model.ExperimentTarget].
+func (t *Target) Options() []string {
+	return experimentconfig.DefaultOptionsSerializer(t.Config)
+}
+
 // String implements [model.ExperimentTarget].
 func (t *Target) String() string {
 	return t.URL
@@ -83,8 +89,8 @@ func (tl *targetLoader) Load(ctx context.Context) ([]model.ExperimentTarget, err
 	var targets []model.ExperimentTarget
 	for _, input := range inputs {
 		targets = append(targets, &Target{
-			Options: tl.options,
-			URL:     input,
+			Config: tl.options,
+			URL:    input,
 		})
 	}
 	return targets, nil
@@ -100,14 +106,14 @@ var defaultInput = []model.ExperimentTarget{
 	//
 	&Target{
 		URL: "https://dns.google/dns-query",
-		Options: &Config{
+		Config: &Config{
 			HTTP3Enabled: true,
 			DefaultAddrs: "8.8.8.8 8.8.4.4",
 		},
 	},
 	&Target{
 		URL: "https://dns.google/dns-query",
-		Options: &Config{
+		Config: &Config{
 			DefaultAddrs: "8.8.8.8 8.8.4.4",
 		},
 	},