From 30bf24c15d22073072d9115aa4a4fa8d54687c62 Mon Sep 17 00:00:00 2001 From: decfox Date: Mon, 12 Dec 2022 13:22:56 +0530 Subject: [PATCH 1/4] refactor(ooniprobe): begin removing deprecated parts --- cmd/ooniprobe/internal/nettests/nettests.go | 71 +++++++++++++++------ 1 file changed, 50 insertions(+), 21 deletions(-) diff --git a/cmd/ooniprobe/internal/nettests/nettests.go b/cmd/ooniprobe/internal/nettests/nettests.go index bee5be39f7..a230b2814b 100644 --- a/cmd/ooniprobe/internal/nettests/nettests.go +++ b/cmd/ooniprobe/internal/nettests/nettests.go @@ -62,6 +62,15 @@ type Controller struct { // curInputIdx is the current input index curInputIdx int + + // saveToDisk indicates if we want to save the measurement to disk + saveToDisk bool + + // newSaverFn is OPTIONAL and is used for testing + newSaverFn func(model.Experiment, model.DatabaseMeasurement) (engine.Saver, error) + + // newSubmitterFn is OPTIONAL and is used for testing + newSubmitterFn func(ctx context.Context) (engine.Submitter, error) } // BuildAndSetInputIdxMap takes in input a list of URLs in the format @@ -141,17 +150,6 @@ func (c *Controller) Run(builder model.ExperimentBuilder, inputs []string) error log.Debug(color.RedString("status.queued")) log.Debug(color.RedString("status.started")) - if c.Probe.Config().Sharing.UploadResults { - if err := exp.OpenReportContext(context.Background()); err != nil { - log.Debugf( - "%s: %s", color.RedString("failure.report_create"), err.Error(), - ) - } else { - log.Debugf(color.RedString("status.report_create")) - reportID = sql.NullString{String: exp.ReportID(), Valid: true} - } - } - maxRuntime := time.Duration(c.Probe.Config().Nettests.WebsitesMaxRuntime) * time.Second if c.RunType == model.RunTypeTimed && maxRuntime > 0 { log.Debug("disabling maxRuntime when running in the background") @@ -212,12 +210,14 @@ func (c *Controller) Run(builder model.ExperimentBuilder, inputs []string) error continue } - saveToDisk := true + c.saveToDisk = true if c.Probe.Config().Sharing.UploadResults { - // Implementation note: SubmitMeasurement will fail here if we did fail - // to open the report but we still want to continue. There will be a - // bit of a spew in the logs, perhaps, but stopping seems less efficient. - if err := exp.SubmitAndUpdateMeasurementContext(context.Background(), measurement); err != nil { + ctx := context.Background() + submitter, err := c.newSubmitter(ctx) + if err != nil { + return errors.Wrap(err, "failed to initialise submitter") + } + if err := submitter.Submit(ctx, measurement); err != nil { log.Debug(color.RedString("failure.measurement_submission")) if err := db.UploadFailed(c.msmts[idx64], err.Error()); err != nil { return errors.Wrap(err, "failed to mark upload as failed") @@ -226,14 +226,18 @@ func (c *Controller) Run(builder model.ExperimentBuilder, inputs []string) error return errors.Wrap(err, "failed to mark upload as succeeded") } else { // Everything went OK, don't save to disk - saveToDisk = false + c.saveToDisk = false } } + // We only save the measurement to disk if we failed to upload the measurement - if saveToDisk { - if err := exp.SaveMeasurement(measurement, msmt.MeasurementFilePath.String); err != nil { - return errors.Wrap(err, "failed to save measurement on disk") - } + saver, err := c.newSaver(exp, *msmt) + if err != nil { + return errors.Wrap(err, "failed to initialise saver") + } + err = saver.SaveMeasurement(measurement) + if err != nil { + return errors.Wrap(err, "failed to save measurement on disk") } if err := db.Done(c.msmts[idx64]); err != nil { @@ -295,3 +299,28 @@ func (c *Controller) OnProgress(perc float64, msg string) { key := fmt.Sprintf("%T", c.nt) output.Progress(key, perc, eta, msg) } + +// newSaver creates a new engine.Saver instance. +func (c *Controller) newSaver(experiment model.Experiment, msmt model.DatabaseMeasurement) (engine.Saver, error) { + if c.newSaverFn != nil { + return c.newSaverFn(experiment, msmt) + } + return engine.NewSaver(engine.SaverConfig{ + Enabled: c.saveToDisk, + Experiment: experiment, + FilePath: msmt.MeasurementFilePath.String, + Logger: c.Session.Logger(), + }) +} + +// newSubmitter creates a new engine.Submitter instance. +func (c *Controller) newSubmitter(ctx context.Context) (engine.Submitter, error) { + if c.newSubmitterFn != nil { + return c.newSubmitterFn(ctx) + } + return engine.NewSubmitter(ctx, engine.SubmitterConfig{ + Enabled: c.Probe.Config().Sharing.UploadResults, + Session: c.Session, + Logger: c.Session.Logger(), + }) +} From d5ff4b025e0fb05e6ef57c0f2dace86aea59c430 Mon Sep 17 00:00:00 2001 From: decfox Date: Mon, 12 Dec 2022 14:04:27 +0530 Subject: [PATCH 2/4] refactor(ooniprobe): remove testing functions --- cmd/ooniprobe/internal/nettests/nettests.go | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/cmd/ooniprobe/internal/nettests/nettests.go b/cmd/ooniprobe/internal/nettests/nettests.go index a230b2814b..27bf0005b0 100644 --- a/cmd/ooniprobe/internal/nettests/nettests.go +++ b/cmd/ooniprobe/internal/nettests/nettests.go @@ -65,12 +65,6 @@ type Controller struct { // saveToDisk indicates if we want to save the measurement to disk saveToDisk bool - - // newSaverFn is OPTIONAL and is used for testing - newSaverFn func(model.Experiment, model.DatabaseMeasurement) (engine.Saver, error) - - // newSubmitterFn is OPTIONAL and is used for testing - newSubmitterFn func(ctx context.Context) (engine.Submitter, error) } // BuildAndSetInputIdxMap takes in input a list of URLs in the format @@ -302,9 +296,6 @@ func (c *Controller) OnProgress(perc float64, msg string) { // newSaver creates a new engine.Saver instance. func (c *Controller) newSaver(experiment model.Experiment, msmt model.DatabaseMeasurement) (engine.Saver, error) { - if c.newSaverFn != nil { - return c.newSaverFn(experiment, msmt) - } return engine.NewSaver(engine.SaverConfig{ Enabled: c.saveToDisk, Experiment: experiment, @@ -315,9 +306,6 @@ func (c *Controller) newSaver(experiment model.Experiment, msmt model.DatabaseMe // newSubmitter creates a new engine.Submitter instance. func (c *Controller) newSubmitter(ctx context.Context) (engine.Submitter, error) { - if c.newSubmitterFn != nil { - return c.newSubmitterFn(ctx) - } return engine.NewSubmitter(ctx, engine.SubmitterConfig{ Enabled: c.Probe.Config().Sharing.UploadResults, Session: c.Session, From 63aee49c77e56208556b1af2960c63080ba53fdb Mon Sep 17 00:00:00 2001 From: decfox Date: Mon, 12 Dec 2022 14:21:23 +0530 Subject: [PATCH 3/4] refactor(ooniprobe): remove redundant conditional --- cmd/ooniprobe/internal/nettests/nettests.go | 31 ++++++++++----------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/cmd/ooniprobe/internal/nettests/nettests.go b/cmd/ooniprobe/internal/nettests/nettests.go index 27bf0005b0..67656048fb 100644 --- a/cmd/ooniprobe/internal/nettests/nettests.go +++ b/cmd/ooniprobe/internal/nettests/nettests.go @@ -205,23 +205,22 @@ func (c *Controller) Run(builder model.ExperimentBuilder, inputs []string) error } c.saveToDisk = true - if c.Probe.Config().Sharing.UploadResults { - ctx := context.Background() - submitter, err := c.newSubmitter(ctx) - if err != nil { - return errors.Wrap(err, "failed to initialise submitter") - } - if err := submitter.Submit(ctx, measurement); err != nil { - log.Debug(color.RedString("failure.measurement_submission")) - if err := db.UploadFailed(c.msmts[idx64], err.Error()); err != nil { - return errors.Wrap(err, "failed to mark upload as failed") - } - } else if err := db.UploadSucceeded(c.msmts[idx64]); err != nil { - return errors.Wrap(err, "failed to mark upload as succeeded") - } else { - // Everything went OK, don't save to disk - c.saveToDisk = false + + ctx := context.Background() + submitter, err := c.newSubmitter(ctx) + if err != nil { + return errors.Wrap(err, "failed to initialise submitter") + } + if err := submitter.Submit(ctx, measurement); err != nil { + log.Debug(color.RedString("failure.measurement_submission")) + if err := db.UploadFailed(c.msmts[idx64], err.Error()); err != nil { + return errors.Wrap(err, "failed to mark upload as failed") } + } else if err := db.UploadSucceeded(c.msmts[idx64]); err != nil { + return errors.Wrap(err, "failed to mark upload as succeeded") + } else { + // Everything went OK, don't save to disk + c.saveToDisk = false } // We only save the measurement to disk if we failed to upload the measurement From 4f86eab2850ca5dfcc0db44b049df69a3fad21f8 Mon Sep 17 00:00:00 2001 From: decfox Date: Mon, 12 Dec 2022 15:26:48 +0530 Subject: [PATCH 4/4] refactor(ooniprobe): instantiate submitter before iterating inputs --- cmd/ooniprobe/internal/nettests/nettests.go | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/cmd/ooniprobe/internal/nettests/nettests.go b/cmd/ooniprobe/internal/nettests/nettests.go index 67656048fb..fb5f45c3c3 100644 --- a/cmd/ooniprobe/internal/nettests/nettests.go +++ b/cmd/ooniprobe/internal/nettests/nettests.go @@ -160,6 +160,13 @@ func (c *Controller) Run(builder model.ExperimentBuilder, inputs []string) error } start := time.Now() c.ntStartTime = start + + ctx := context.Background() + submitter, err := c.newSubmitter(ctx) + if err != nil { + return errors.Wrap(err, "failed to initialise submitter") + } + for idx, input := range inputs { if c.Probe.IsTerminated() { log.Info("user requested us to terminate using Ctrl-C") @@ -177,6 +184,10 @@ func (c *Controller) Run(builder model.ExperimentBuilder, inputs []string) error urlID = sql.NullInt64{Int64: c.inputIdxMap[idx64], Valid: true} } + // TODO(DecFox): we currently pass a nil reportID which should be replaced by the reportID + // we get from the submitter. However, since the reportID is generated on uploading the first + // measurement, we do not have a valid reportID to pass here. Therefore, we want to populate the + // submitter without having to upload a measurement. msmt, err := db.CreateMeasurement( reportID, exp.Name(), c.res.MeasurementDir, idx, resultID, urlID, ) @@ -206,11 +217,7 @@ func (c *Controller) Run(builder model.ExperimentBuilder, inputs []string) error c.saveToDisk = true - ctx := context.Background() - submitter, err := c.newSubmitter(ctx) - if err != nil { - return errors.Wrap(err, "failed to initialise submitter") - } + // upload measurement if err := submitter.Submit(ctx, measurement); err != nil { log.Debug(color.RedString("failure.measurement_submission")) if err := db.UploadFailed(c.msmts[idx64], err.Error()); err != nil {