Skip to content

Commit

Permalink
feat: introduce richer input
Browse files Browse the repository at this point in the history
Most of the existing code is designed to move around lists of
`model.OOAPIURLInfo` and measuring such URLs.

This design originally suited Web Connectivity but it's not good
enough for richer input because it does not contain options.

With this diff, we move into the direction of richer input by
replacing `model.OOAPIURLInfo` lists with lists of:

```Go
// internal/model/experiment.go

type ExperimentTarget struct {
	Category() string
	Country() string
	Input() string
}
```

where `*model.OOAPIURLInfo` implements `model.ExperimentTarget`
in a trivial way and where, additionally:

1. the `InputLoader` is modified to load `ExperimentTarget`;

2. the `Experiment` is modify to measure an `ExperimentTarget`.

In addition to applying these changes, this diff also adapts the
whole tree to use `ExperimentTarget` in all places and adds a
trivial constructor to obtain `OOAPIURLInfo` when the category
code and the country code are unknown.

With this diff merged, implementing richer input for real is a
matter of implementing the following changes:

1. the `*registry.Factory` has a new func field, defined by
each experiment, that loads a list of `ExperimentTarget`;

2. we have a library for input loading containing the same code
that we currently use for the input loader;

3. the `InputLoader` is gone and instead we use the factory (or its
`*engine.experimentBuilder` wrapper for input loading;

4. we modify the `ExperimentArgs` passed to the `ExperimentMeasurer`
to contain an additional field that is the `ExperimentTarget` we
want to measure;

5. each experiment that needs richer input type-casts from the
`ExperimentTarget` interface to the concrete type that the experiment
richer input should have and accesses any option.

Part of #1612.

This implementation strategy emerged while discussing this matter
with @ainghazal, thank you so much for that!
  • Loading branch information
bassosimone committed Jun 5, 2024
1 parent 15dac36 commit 5a98c81
Show file tree
Hide file tree
Showing 41 changed files with 259 additions and 151 deletions.
4 changes: 3 additions & 1 deletion cmd/ooniprobe/internal/nettests/dash.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package nettests

import "github.com/ooni/probe-cli/v3/internal/model"

// Dash test implementation
type Dash struct {
}
Expand All @@ -10,5 +12,5 @@ func (d Dash) Run(ctl *Controller) error {
if err != nil {
return err
}
return ctl.Run(builder, []string{""})
return ctl.Run(builder, []model.ExperimentTarget{model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("")})
}
2 changes: 1 addition & 1 deletion cmd/ooniprobe/internal/nettests/dnscheck.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import (
// DNSCheck nettest implementation.
type DNSCheck struct{}

func (n DNSCheck) lookupURLs(ctl *Controller) ([]string, error) {
func (n DNSCheck) lookupURLs(ctl *Controller) ([]model.ExperimentTarget, error) {
inputloader := &engine.InputLoader{
CheckInConfig: &model.OOAPICheckInConfig{
// not needed because we have default static input in the engine
Expand Down
4 changes: 3 additions & 1 deletion cmd/ooniprobe/internal/nettests/echcheck.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package nettests

import "github.com/ooni/probe-cli/v3/internal/model"

// ECHCheck nettest implementation.
type ECHCheck struct{}

Expand All @@ -11,5 +13,5 @@ func (n ECHCheck) Run(ctl *Controller) error {
}
// providing an input containing an empty string causes the experiment
// to recognize the empty string and use the default URL
return ctl.Run(builder, []string{""})
return ctl.Run(builder, []model.ExperimentTarget{model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("")})
}
4 changes: 3 additions & 1 deletion cmd/ooniprobe/internal/nettests/facebook_messenger.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package nettests

import "github.com/ooni/probe-cli/v3/internal/model"

// FacebookMessenger test implementation
type FacebookMessenger struct {
}
Expand All @@ -12,5 +14,5 @@ func (h FacebookMessenger) Run(ctl *Controller) error {
if err != nil {
return err
}
return ctl.Run(builder, []string{""})
return ctl.Run(builder, []model.ExperimentTarget{model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("")})
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package nettests

import "github.com/ooni/probe-cli/v3/internal/model"

// HTTPHeaderFieldManipulation test implementation
type HTTPHeaderFieldManipulation struct {
}
Expand All @@ -12,5 +14,5 @@ func (h HTTPHeaderFieldManipulation) Run(ctl *Controller) error {
if err != nil {
return err
}
return ctl.Run(builder, []string{""})
return ctl.Run(builder, []model.ExperimentTarget{model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("")})
}
4 changes: 3 additions & 1 deletion cmd/ooniprobe/internal/nettests/http_invalid_request_line.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package nettests

import "github.com/ooni/probe-cli/v3/internal/model"

// HTTPInvalidRequestLine test implementation
type HTTPInvalidRequestLine struct {
}
Expand All @@ -12,5 +14,5 @@ func (h HTTPInvalidRequestLine) Run(ctl *Controller) error {
if err != nil {
return err
}
return ctl.Run(builder, []string{""})
return ctl.Run(builder, []model.ExperimentTarget{model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("")})
}
4 changes: 3 additions & 1 deletion cmd/ooniprobe/internal/nettests/ndt.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package nettests

import "github.com/ooni/probe-cli/v3/internal/model"

// NDT test implementation. We use v7 of NDT since 2020-03-12.
type NDT struct {
}
Expand All @@ -11,5 +13,5 @@ func (n NDT) Run(ctl *Controller) error {
if err != nil {
return err
}
return ctl.Run(builder, []string{""})
return ctl.Run(builder, []model.ExperimentTarget{model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("")})
}
14 changes: 6 additions & 8 deletions cmd/ooniprobe/internal/nettests/nettests.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,24 +88,22 @@ type Controller struct {
// - on success, a list of strings containing URLs to test;
//
// - on failure, an error.
func (c *Controller) BuildAndSetInputIdxMap(testlist []model.OOAPIURLInfo) ([]string, error) {
var urls []string
func (c *Controller) BuildAndSetInputIdxMap(testlist []model.ExperimentTarget) ([]model.ExperimentTarget, error) {
urlIDMap := make(map[int64]int64)
for idx, url := range testlist {
log.Debugf("Going over URL %d", idx)
urlID, err := c.Probe.DB().CreateOrUpdateURL(
url.URL, url.CategoryCode, url.CountryCode,
url.Input(), url.Category(), url.Country(),
)
if err != nil {
log.Error("failed to add to the URL table")
return nil, err
}
log.Debugf("Mapped URL %s to idx %d and urlID %d", url.URL, idx, urlID)
log.Debugf("Mapped URL %s to idx %d and urlID %d", url.Input(), idx, urlID)
urlIDMap[int64(idx)] = urlID
urls = append(urls, url.URL)
}
c.inputIdxMap = urlIDMap
return urls, nil
return testlist, nil
}

// SetNettestIndex is used to set the current nettest index and total nettest
Expand All @@ -120,7 +118,7 @@ func (c *Controller) SetNettestIndex(i, n int) {
//
// This function will continue to run in most cases but will
// immediately halt if something's wrong with the file system.
func (c *Controller) Run(builder model.ExperimentBuilder, inputs []string) error {
func (c *Controller) Run(builder model.ExperimentBuilder, inputs []model.ExperimentTarget) error {
db := c.Probe.DB()
// This will configure the controller as handler for the callbacks
// called by ooni/probe-engine/experiment.Experiment.
Expand Down Expand Up @@ -193,7 +191,7 @@ func (c *Controller) Run(builder model.ExperimentBuilder, inputs []string) error
}
c.msmts[idx64] = msmt

if input != "" {
if input.Input() != "" {
c.OnProgress(0, fmt.Sprintf("processing input: %s", input))
}
measurement, err := exp.MeasureWithContext(context.Background(), input)
Expand Down
4 changes: 3 additions & 1 deletion cmd/ooniprobe/internal/nettests/psiphon.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package nettests

import "github.com/ooni/probe-cli/v3/internal/model"

// Psiphon test implementation
type Psiphon struct {
}
Expand All @@ -12,5 +14,5 @@ func (h Psiphon) Run(ctl *Controller) error {
if err != nil {
return err
}
return ctl.Run(builder, []string{""})
return ctl.Run(builder, []model.ExperimentTarget{model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("")})
}
5 changes: 3 additions & 2 deletions cmd/ooniprobe/internal/nettests/riseupvpn.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package nettests

import "github.com/ooni/probe-cli/v3/internal/model"

// RiseupVPN test implementation
type RiseupVPN struct {
}
Expand All @@ -12,6 +14,5 @@ func (h RiseupVPN) Run(ctl *Controller) error {
if err != nil {
return err
}

return ctl.Run(builder, []string{""})
return ctl.Run(builder, []model.ExperimentTarget{model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("")})
}
4 changes: 3 additions & 1 deletion cmd/ooniprobe/internal/nettests/signal.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package nettests

import "github.com/ooni/probe-cli/v3/internal/model"

// Signal nettest implementation.
type Signal struct{}

Expand All @@ -11,5 +13,5 @@ func (h Signal) Run(ctl *Controller) error {
if err != nil {
return err
}
return ctl.Run(builder, []string{""})
return ctl.Run(builder, []model.ExperimentTarget{model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("")})
}
2 changes: 1 addition & 1 deletion cmd/ooniprobe/internal/nettests/stunreachability.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import (
// STUNReachability nettest implementation.
type STUNReachability struct{}

func (n STUNReachability) lookupURLs(ctl *Controller) ([]string, error) {
func (n STUNReachability) lookupURLs(ctl *Controller) ([]model.ExperimentTarget, error) {
inputloader := &engine.InputLoader{
CheckInConfig: &model.OOAPICheckInConfig{
// not needed because we have default static input in the engine
Expand Down
4 changes: 3 additions & 1 deletion cmd/ooniprobe/internal/nettests/telegram.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package nettests

import "github.com/ooni/probe-cli/v3/internal/model"

// Telegram test implementation
type Telegram struct {
}
Expand All @@ -12,5 +14,5 @@ func (h Telegram) Run(ctl *Controller) error {
if err != nil {
return err
}
return ctl.Run(builder, []string{""})
return ctl.Run(builder, []model.ExperimentTarget{model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("")})
}
4 changes: 3 additions & 1 deletion cmd/ooniprobe/internal/nettests/tor.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package nettests

import "github.com/ooni/probe-cli/v3/internal/model"

// Tor test implementation
type Tor struct {
}
Expand All @@ -12,5 +14,5 @@ func (h Tor) Run(ctl *Controller) error {
if err != nil {
return err
}
return ctl.Run(builder, []string{""})
return ctl.Run(builder, []model.ExperimentTarget{model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("")})
}
4 changes: 3 additions & 1 deletion cmd/ooniprobe/internal/nettests/torsf.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package nettests

import "github.com/ooni/probe-cli/v3/internal/model"

// TorSf test implementation
type TorSf struct {
}
Expand All @@ -10,7 +12,7 @@ func (h TorSf) Run(ctl *Controller) error {
if err != nil {
return err
}
return ctl.Run(builder, []string{""})
return ctl.Run(builder, []model.ExperimentTarget{model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("")})
}

func (h TorSf) onlyBackground() {}
4 changes: 3 additions & 1 deletion cmd/ooniprobe/internal/nettests/vanillator.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package nettests

import "github.com/ooni/probe-cli/v3/internal/model"

// VanillaTor test implementation
type VanillaTor struct {
}
Expand All @@ -10,7 +12,7 @@ func (h VanillaTor) Run(ctl *Controller) error {
if err != nil {
return err
}
return ctl.Run(builder, []string{""})
return ctl.Run(builder, []model.ExperimentTarget{model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("")})
}

func (h VanillaTor) onlyBackground() {}
2 changes: 1 addition & 1 deletion cmd/ooniprobe/internal/nettests/web_connectivity.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import (
"github.com/ooni/probe-cli/v3/internal/model"
)

func (n WebConnectivity) lookupURLs(ctl *Controller, categories []string) ([]string, error) {
func (n WebConnectivity) lookupURLs(ctl *Controller, categories []string) ([]model.ExperimentTarget, error) {
inputloader := &engine.InputLoader{
CheckInConfig: &model.OOAPICheckInConfig{
// Setting Charging and OnWiFi to true causes the CheckIn
Expand Down
4 changes: 3 additions & 1 deletion cmd/ooniprobe/internal/nettests/whatsapp.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package nettests

import "github.com/ooni/probe-cli/v3/internal/model"

// WhatsApp test implementation
type WhatsApp struct {
}
Expand All @@ -12,5 +14,5 @@ func (h WhatsApp) Run(ctl *Controller) error {
if err != nil {
return err
}
return ctl.Run(builder, []string{""})
return ctl.Run(builder, []model.ExperimentTarget{model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("")})
}
7 changes: 5 additions & 2 deletions internal/engine/experiment.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ type experiment struct {
testVersion string
}

var _ model.Experiment = &experiment{}

// newExperiment creates a new [*experiment] given a [model.ExperimentMeasurer].
func newExperiment(sess *Session, measurer model.ExperimentMeasurer) *experiment {
return &experiment{
Expand Down Expand Up @@ -180,7 +182,8 @@ func (e *experiment) OpenReportContext(ctx context.Context) error {
}

// MeasureWithContext implements [model.Experiment].
func (e *experiment) MeasureWithContext(ctx context.Context, input string) (*model.Measurement, error) {
func (e *experiment) MeasureWithContext(
ctx context.Context, target model.ExperimentTarget) (*model.Measurement, error) {
// Here we ensure that we have already looked up the probe location
// information such that we correctly populate the measurement and also
// VERY IMPORTANTLY to scrub the IP address from the measurement.
Expand All @@ -204,7 +207,7 @@ func (e *experiment) MeasureWithContext(ctx context.Context, input string) (*mod
// by adding the test keys etc. Please, note that, as of 2024-06-05, we're using
// the measurement Input to provide input to an experiment. We'll probably
// change this, when we'll have finished implementing richer input.
measurement := e.newMeasurement(input)
measurement := e.newMeasurement(target.Input())

// Record when we started the experiment, to compute the runtime.
start := time.Now()
Expand Down
12 changes: 8 additions & 4 deletions internal/engine/experiment_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,8 @@ func TestSetCallbacks(t *testing.T) {
}
register := &registerCallbacksCalled{}
builder.SetCallbacks(register)
if _, err := builder.NewExperiment().MeasureWithContext(context.Background(), ""); err != nil {
target := model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("")
if _, err := builder.NewExperiment().MeasureWithContext(context.Background(), target); err != nil {
t.Fatal(err)
}
if register.onProgressCalled == false {
Expand Down Expand Up @@ -221,7 +222,8 @@ func TestMeasurementFailure(t *testing.T) {
if err := builder.SetOptionAny("ReturnError", true); err != nil {
t.Fatal(err)
}
measurement, err := builder.NewExperiment().MeasureWithContext(context.Background(), "")
target := model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("")
measurement, err := builder.NewExperiment().MeasureWithContext(context.Background(), target)
if err == nil {
t.Fatal("expected an error here")
}
Expand Down Expand Up @@ -255,7 +257,8 @@ func runexperimentflow(t *testing.T, experiment model.Experiment, input string)
if experiment.ReportID() == "" {
t.Fatal("reportID should not be empty here")
}
measurement, err := experiment.MeasureWithContext(ctx, input)
target := model.NewOOAPIURLInfoWithDefaultCategoryAndCountry(input)
measurement, err := experiment.MeasureWithContext(ctx, target)
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -413,7 +416,8 @@ func TestMeasureLookupLocationFailure(t *testing.T) {
exp := newExperiment(sess, new(antaniMeasurer))
ctx, cancel := context.WithCancel(context.Background())
cancel() // so we fail immediately
if _, err := exp.MeasureWithContext(ctx, "xx"); err == nil {
target := model.NewOOAPIURLInfoWithDefaultCategoryAndCountry("xx")
if _, err := exp.MeasureWithContext(ctx, target); err == nil {
t.Fatal("expected an error here")
}
}
Expand Down
Loading

0 comments on commit 5a98c81

Please sign in to comment.