cleanup: remove asynchronous experiments #1613
Conversation
This diff refactors *engine.experiment to make the report field goroutine safe. It also moves at the bottom of experiment.go code that was intermixed with *engine.experiment methods. Part of ooni/probe#2607
bassosimone
force-pushed
the
issue/2607b
branch
2 times, most recently
from
528e8c0 to
348c391
Compare
We originally developed asynchronous experiments as a mean to support websteps, a nettest that returned more than one measurement per invocation of its Measure method. Since then, we removed websteps. Therefore, this code is currently technically unused. Additionally, this code further complicates implementing richer input, because it is another way of performing measurements. As such, in the interest of switfly moving forward with richer input _and_ of simplifying the engine, we are now removing this unused functionality from the tree. Part of ooni/probe#2607
Conflicts: internal/engine/experiment.go
|
The coverage error is like: It seems very unrelated so I think it's okay for us to merge. |
| // TODO(bassosimone,DecFox): historically we did this only for measuring | ||
| // and not for opening a report, which probably is not correct. Because the | ||
| // function call is idempotent, call it also when opening a report? | ||
| if err := e.session.MaybeLookupLocationContext(ctx); err != nil { |
There was a problem hiding this comment.
I was wondering if MaybeLookupLocationWithContext would be more descriptive?
Re. moment to call it, a subtle concern here might be the low likelihood that there's a network change between measurement and submission.
Thinking mostly in asynchronous submission, I think it'd be healthy to a) call it when submitting; b) raise an error (or warning) if submitting a msmt for a different (CC,ASN) that the report we've opened. I can work on that as part of my future deliverables, that include deferred submission (which is an special case).
There was a problem hiding this comment.
@ainghazal wrote:
I was wondering if
MaybeLookupLocationWithContextwould be more descriptive?
Yeah, so, first of all we can gain some more characters by removing WithContext, which is an artifact from when we also had a version of the call without a context. How would you suggest to change the name?
Re. moment to call it, a subtle concern here might be the low likelihood that there's a network change between measurement and submission.
Thinking mostly in asynchronous submission, I think it'd be healthy to a) call it when submitting; b) raise an error (or warning) if submitting a msmt for a different (CC,ASN) that the report we've opened. I can work on that as part of my future deliverables, that include deferred submission (which is an special case).
As far as I know, when we're resubmitting we're using a probeservices.Submitter: https://github.com/ooni/probe-cli/blob/master/internal/probeservices/collector.go#L185. The code should open reports only based on the data stored inside the measurement, so it should support the use case of measuring, say, w/o VPN, and then submitting with VPN. It would be great to have another pair of eyes looking at the code as well as having some manual testing to ensure whether this is still true. It might also be good to have a test guaranteeing that this is the case.
There was a problem hiding this comment.
How would you suggest to change the name?
if so, perhaps just MaybeLookupLocation?
We originally developed asynchronous experiments as a mean to support websteps, a nettest that returned more than one measurement per invocation of its Measure method.
Since then, we removed websteps. Therefore, this code is currently technically unused. Additionally, this code further complicates implementing richer input, because it is another way of performing measurements.
As such, in the interest of switfly moving forward with richer input and of simplifying the engine, we are now removing this unused functionality from the tree.
While there, better the documentation of OnProgress and undeprecate functions to open/close reports since it's clear that, for now, using a submitter in cmd/ooniprobe is a bit of a stretch given our current goals. So, it feels best to avoid deprecating until we have nice plan for replacement.
To have more confidence that the job we did is ~correct, we use the following table to cross compare the operations that we previously performed for running sync experiments (i.e., all experiments) in an async way to the code we're using after this diff to run experiments. (Note that the diff itself is such that you can easily see the deleted and the added code inside of the
experiment.gofile.) In both cases, we're looking at the operations performed starting fromMeasureWithContext.Part of ooni/probe#2607
Depends on #1612