feat: mvp of conditionally enabling experiments

internal/checkincache/checkincache.go

@@ -3,6 +3,7 @@ package checkincache
 
 import (
 	"encoding/json"
+	"fmt"
 	"time"
 
 	"github.com/ooni/probe-cli/v3/internal/model"
@@ -25,6 +26,9 @@ type checkInFlagsWrapper struct {
 }
 
 // Store stores the result of the latest check-in in the given key-value store.
+//
+// We store check-in feature flags in a file called checkinflags.state. These flags
+// are valid for 24 hours, after which we consider them stale.
 func Store(kvStore model.KeyValueStore, resp *model.OOAPICheckInResult) error {
 	// store the check-in flags in the key-value store
 	wrapper := &checkInFlagsWrapper{
@@ -52,3 +56,16 @@ func GetFeatureFlag(kvStore model.KeyValueStore, name string) bool {
 	}
 	return wrapper.Flags[name] // works even if map is nil
 }
+
+// ExperimentEnabledKey returns the [model.KeyValueStore] key to use to
+// know whether a disabled experiment has been enabled via check-in.
+func ExperimentEnabledKey(name string) string {
+	return fmt.Sprintf("%s_enabled", name)
+}
+
+// ExperimentEnabled returns whether a given experiment has been enabled by a previous
+// execution of check-in. Some experiments are disabled by default for different reasons
+// and we use the check-in API to control whether and when they should be enabled.
+func ExperimentEnabled(kvStore model.KeyValueStore, name string) bool {
+	return GetFeatureFlag(kvStore, ExperimentEnabledKey(name))
+}

internal/engine/experiment_integration_test.go

@@ -13,6 +13,7 @@ import (
 	"testing"
 
 	"github.com/ooni/probe-cli/v3/internal/model"
+	"github.com/ooni/probe-cli/v3/internal/registry"
 )
 
 func TestCreateAll(t *testing.T) {
@@ -21,6 +22,12 @@ func TestCreateAll(t *testing.T) {
 	}
 	sess := newSessionForTesting(t)
 	defer sess.Close()
+
+	// Since https://github.com/ooni/probe-cli/pull/1355, some experiments are disabled
+	// by default and we need an environment variable to instantiate them
+	os.Setenv(registry.OONI_FORCE_ENABLE_EXPERIMENT, "1")
+	defer os.Unsetenv(registry.OONI_FORCE_ENABLE_EXPERIMENT)
+
 	for _, name := range AllExperiments() {
 		builder, err := sess.NewExperimentBuilder(name)
 		if err != nil {

internal/engine/experimentbuilder.go

@@ -62,7 +62,7 @@ func (b *experimentBuilder) NewExperiment() model.Experiment {
 
 // newExperimentBuilder creates a new experimentBuilder instance.
 func newExperimentBuilder(session *Session, name string) (*experimentBuilder, error) {
-	factory, err := registry.NewFactory(name)
+	factory, err := registry.NewFactory(name, session.kvStore, session.logger)
 	if err != nil {
 		return nil, err
 	}

internal/engine/inputloader.go

@@ -219,6 +219,12 @@ func StaticBareInputForExperiment(name string) ([]string, error) {
 	// Implementation note: we may be called from pkg/oonimkall
 	// with a non-canonical experiment name, so we need to convert
 	// the experiment name to be canonical before proceeding.
+	//
+	// TODO(https://github.com/ooni/probe/issues/1390): serve DNSCheck
+	// inputs using richer input (aka check-in v2).
+	//
+	// TODO(https://github.com/ooni/probe/issues/2557): server STUNReachability
+	// inputs using richer input (aka check-in v2).
 	switch registry.CanonicalizeExperimentName(name) {
 	case "dnscheck":
 		return dnsCheckDefaultInput, nil

internal/engine/session.go

@@ -11,15 +11,13 @@ import (
 	"sync/atomic"
 
 	"github.com/ooni/probe-cli/v3/internal/bytecounter"
-	"github.com/ooni/probe-cli/v3/internal/checkincache"
 	"github.com/ooni/probe-cli/v3/internal/enginelocate"
 	"github.com/ooni/probe-cli/v3/internal/enginenetx"
 	"github.com/ooni/probe-cli/v3/internal/engineresolver"
 	"github.com/ooni/probe-cli/v3/internal/kvstore"
 	"github.com/ooni/probe-cli/v3/internal/model"
 	"github.com/ooni/probe-cli/v3/internal/platform"
 	"github.com/ooni/probe-cli/v3/internal/probeservices"
-	"github.com/ooni/probe-cli/v3/internal/registry"
 	"github.com/ooni/probe-cli/v3/internal/runtimex"
 	"github.com/ooni/probe-cli/v3/internal/tunnel"
 	"github.com/ooni/probe-cli/v3/internal/version"
@@ -406,16 +404,6 @@ var ErrAlreadyUsingProxy = errors.New(
 // for the experiment with the given name, or an error if
 // there's no such experiment with the given name
 func (s *Session) NewExperimentBuilder(name string) (model.ExperimentBuilder, error) {
-	name = registry.CanonicalizeExperimentName(name)
-	switch {
-	case name == "web_connectivity" && checkincache.GetFeatureFlag(s.kvStore, "webconnectivity_0.5"):
-		// use LTE rather than the normal webconnectivity when the
-		// feature flag has been set through the check-in API
-		s.Logger().Infof("using webconnectivity LTE")
-		name = "web_connectivity@v0.5"
-	default:
-		// nothing
-	}
 	eb, err := newExperimentBuilder(s, name)
 	if err != nil {
 		return nil, err

internal/registry/allexperiments.go

@@ -1,5 +1,7 @@
 package registry
 
+import "sort"
+
 // Where we register all the available experiments.
 var AllExperiments = map[string]*Factory{}
 
@@ -8,5 +10,6 @@ func ExperimentNames() (names []string) {
 	for key := range AllExperiments {
 		names = append(names, key)
 	}
+	sort.Strings(names) // sort by name to always provide predictable output
 	return
 }

internal/registry/dash.go

@@ -16,8 +16,9 @@ func init() {
 				*config.(*dash.Config),
 			)
 		},
-		config:        &dash.Config{},
-		interruptible: true,
-		inputPolicy:   model.InputNone,
+		config:           &dash.Config{},
+		enabledByDefault: true,
+		interruptible:    true,
+		inputPolicy:      model.InputNone,
 	}
 }

internal/registry/dnscheck.go

@@ -16,7 +16,8 @@ func init() {
 				*config.(*dnscheck.Config),
 			)
 		},
-		config:      &dnscheck.Config{},
-		inputPolicy: model.InputOrStaticDefault,
+		config:           &dnscheck.Config{},
+		enabledByDefault: true,
+		inputPolicy:      model.InputOrStaticDefault,
 	}
 }

internal/registry/dnsping.go

@@ -16,7 +16,8 @@ func init() {
 				*config.(*dnsping.Config),
 			)
 		},
-		config:      &dnsping.Config{},
-		inputPolicy: model.InputOrStaticDefault,
+		config:           &dnsping.Config{},
+		enabledByDefault: true,
+		inputPolicy:      model.InputOrStaticDefault,
 	}
 }

internal/registry/example.go

@@ -22,7 +22,8 @@ func init() {
 			Message:   "Good day from the example experiment!",
 			SleepTime: int64(time.Second),
 		},
-		interruptible: true,
-		inputPolicy:   model.InputNone,
+		enabledByDefault: true,
+		interruptible:    true,
+		inputPolicy:      model.InputNone,
 	}
 }

internal/registry/factory.go

@@ -7,9 +7,11 @@ package registry
 import (
 	"errors"
 	"fmt"
+	"os"
 	"reflect"
 	"strconv"
 
+	"github.com/ooni/probe-cli/v3/internal/checkincache"
 	"github.com/ooni/probe-cli/v3/internal/model"
 	"github.com/ooni/probe-cli/v3/internal/strcasex"
 )
@@ -22,6 +24,9 @@ type Factory struct {
 	// config contains the experiment's config.
 	config any
 
+	// enabledByDefault indicates whether this experiment is enabled by default.
+	enabledByDefault bool
+
 	// inputPolicy contains the experiment's InputPolicy.
 	inputPolicy model.InputPolicy
 
@@ -218,12 +223,65 @@ func CanonicalizeExperimentName(name string) string {
 // ErrNoSuchExperiment indicates a given experiment does not exist.
 var ErrNoSuchExperiment = errors.New("no such experiment")
 
+// ErrRequiresForceEnable is returned for experiments that are not enabled by default and are also
+// not enabled by the most recent check-in API call.
+var ErrRequiresForceEnable = errors.New("experiment not enabled by check-in API")
+
+const experimentDisabledByCheckInWarning = `experiment '%s' is not enabled by default and the
+most recent check-in API call did not enable this experiment as well. You can bypass this restriction
+by setting the OONI_FORCE_ENABLE_EXPERIMENT environment variable to the string "1". On Unix like
+systems, you can use 'export OONI_FORCE_ENABLE_EXPERIMENT=1' to set this environment variable.`
+
+// OONI_FORCE_ENABLE_EXPERIMENT is the name of the environment variable you should set to "1"
+// to bypass the algorithm preventing disabled by default experiments to be instantiated.
+const OONI_FORCE_ENABLE_EXPERIMENT = "OONI_FORCE_ENABLE_EXPERIMENT"
+
 // NewFactory creates a new Factory instance.
-func NewFactory(name string) (*Factory, error) {
+func NewFactory(name string, kvStore model.KeyValueStore, logger model.Logger) (*Factory, error) {
+	// Make sure we are deadling with the canonical experiment name. Historically MK used
+	// names such as WebConnectivity and we want to continue supporting this use case.
 	name = CanonicalizeExperimentName(name)
+
+	// Handle A/B testing where we dynamically choose LTE for some users. The current policy
+	// only relates to a few users to collect data.
+	//
+	// TODO(https://github.com/ooni/probe/issues/2555): perform the actual comparison
+	// and improve the LTE implementation so that we can always use it. See the actual
+	// issue test for additional details on this planned A/B test.
+	switch {
+	case name == "web_connectivity" && checkincache.GetFeatureFlag(kvStore, "webconnectivity_0.5"):
+		// use LTE rather than the normal webconnectivity when the
+		// feature flag has been set through the check-in API
+		logger.Infof("using webconnectivity LTE")
+		name = "web_connectivity@v0.5"
+
+	default:
+		// nothing
+	}
+
+	// Obtain the factory for the canonical name.
 	factory := AllExperiments[name]
 	if factory == nil {
 		return nil, fmt.Errorf("%w: %s", ErrNoSuchExperiment, name)
 	}
-	return factory, nil
+
+	// Some experiments are not enabled by default. To enable them we use
+	// the cached check-in response or an environment variable.
+	//
+	// Note: check-in flags expire after 24h.
+	//
+	// TODO(https://github.com/ooni/probe/issues/2554): we need to restructure
+	// how we run experiments to make sure check-in flags are always fresh.
+	if factory.enabledByDefault {
+		return factory, nil // enabled by default
+	}
+	if os.Getenv(OONI_FORCE_ENABLE_EXPERIMENT) == "1" {
+		return factory, nil // enabled by environment variable
+	}
+	if checkincache.ExperimentEnabled(kvStore, name) {
+		return factory, nil // enabled by check-in
+	}
+
+	logger.Warnf(experimentDisabledByCheckInWarning, name)
+	return nil, fmt.Errorf("%s: %w", name, ErrRequiresForceEnable)
 }