Integrate further with ooni/probe-engine: episode two #46
Conversation
Let's start using the engine by rewriting utils/geoip.go to be just a thin wrapper around the engine functionality.
Still have some doubts with respect to the variables that are passed to MK via probe-engine. Will double check.
Conflicts: go.mod go.sum nettests/nettests.go ooni.go
bassosimone
added
enhancement
bassosimone
added this to Icebox
in DO NOT USE (was: OONI Probe Desktop)
via automation
bassosimone
moved this from Review
to In Progress
in DO NOT USE (was: OONI Probe Desktop)
Making the concept of measurement more abstract in the engine is
not feasible because, when submitting a measurement, we need to
modify it to update the report ID and the measurement ID. Therefore,
returning a serialized measurement is not a good idea. We will
keep using a model.Measurement in the engine.
Changing model.Measurement.TestKeys's type from a `interface{}`
pointing to a well defined data structure to `map[string]interface{}`
is a regression because means that we are moving from code that
has a clear and defined structure to code that is more complicated
to parse and validate. Since we're already suffering havily from
the lack of a good schema, I'm not going to make the situation
worst by worsening the engine. At least for ndt7 and psiphon, we
now have a good schema and I don't want to lose that.
However, the current code in this repository is expecting the
test keys to be a `map[string]interface{}`. This choice was
dictated by the fact that we receive a JSON from Measurement Kit
and by the fact that there's not a clear schema.
To solve this tension, in this commit I am going to write glue
adapter code that makes sure that the TestKeys of a Measurement
are converted to `map[string]interface{}`. This will be done
using a type cast where possible and JSON serialization and parsing
otherwise. In a perfect world, glue is not a good idea, but in a
real world it may actually be useful.
When all tests in the engine will have a clear Go data structure,
we'll then remove the glue and just cast to the proper data
structure from `interface{}` where required.
For reference, to unbreak it and get back the webook, go to |
|
It also seems I did broke the Linux build. Let me check this. |
|
Bug filed: measurement-kit/script-build-linux#6 |
|
Fixed in these PRs, which require some light review:
When those are merged, we can resume the work here. |
TODO list of actions to be performed before merging@hellais here's the initial list I identified based on our exchanges above. Please add to this list if you think there's something we should address here or by opening new issues.
|
…feature/episode-two * 'feature/episode-two' of github.com:ooni/probe-cli: Upgrade ooni/probe-engine
…feature/episode-two
Set of changes to unblock ooni/probe-cli#46
|
With the merging of ooni/probe-engine#37 we have enough pieces in place to move forward this PR. While the @hellais can you please take a final look to this branch? |
``` go get -u github.com/ooni/probe-engine go mod tidy ```
|
This branch looks good to me, however I think that the For me it's good to go ahead with merging this PR, but it would be good to prioritise the |
|
Adding some notes from the discussion we had privately with @bassosimone. It appears that adding the However doing that introduces problems in data processing and normalisation which we aren't really sure how to handle. In the best interest of moving forward I suggest that for the time being we add the |
* utils/geoip.go: use github.com/ooni/probe-engine
Let's start using the engine by rewriting utils/geoip.go to
be just a thin wrapper around the engine functionality.
* Ready for review
* Checkpoint: the im tests are converted
Still have some doubts with respect to the variables that
are passed to MK via probe-engine. Will double check.
* fix(i/c/r/run.go): write the correct logic
* nettests: one more comment and also fix a format string
* Tweak previous
* progress
* Fix doofus
* better comment
* XXX => actionable comment
* Add glue to simplify test keys management
Making the concept of measurement more abstract in the engine is
not feasible because, when submitting a measurement, we need to
modify it to update the report ID and the measurement ID. Therefore,
returning a serialized measurement is not a good idea. We will
keep using a model.Measurement in the engine.
Changing model.Measurement.TestKeys's type from a `interface{}`
pointing to a well defined data structure to `map[string]interface{}`
is a regression because means that we are moving from code that
has a clear and defined structure to code that is more complicated
to parse and validate. Since we're already suffering havily from
the lack of a good schema, I'm not going to make the situation
worst by worsening the engine. At least for ndt7 and psiphon, we
now have a good schema and I don't want to lose that.
However, the current code in this repository is expecting the
test keys to be a `map[string]interface{}`. This choice was
dictated by the fact that we receive a JSON from Measurement Kit
and by the fact that there's not a clear schema.
To solve this tension, in this commit I am going to write glue
adapter code that makes sure that the TestKeys of a Measurement
are converted to `map[string]interface{}`. This will be done
using a type cast where possible and JSON serialization and parsing
otherwise. In a perfect world, glue is not a good idea, but in a
real world it may actually be useful.
When all tests in the engine will have a clear Go data structure,
we'll then remove the glue and just cast to the proper data
structure from `interface{}` where required.
* nettests/performance: use probe-engine
* go.{mod,sum}: upgrade to latest probe-engine
* nettests/middlebox: use ooni/probe-engine
* Update to the latest probe-engine
* web_connectivity: rewrite to use probe-engine
* Cosmetic change suggested by @hellais
* nettests/nettests.go: remove unused code
* nettests/nettests.go: fix progress
* nettests/nettests.go: remove go-measurement-kit code
* We don't depend on go-measurement-kit anymore
* Improve non-verbose output where possible
See also: measurement-kit/measurement-kit#1856
* Make web_connectivity output pleasant
* Update to the latest probe-engine
* nettests/nettests.go: honour sharing settings
* Update to the latest probe-engine
* Use log.WithFields for probe-engine
* Update go.mod go.sum
* Revert "Update go.mod go.sum"
This reverts commit 5ecd38d.
* Revert "Revert "Update go.mod go.sum""
This reverts commit 6114b31.
* Upgrade ooni/probe-engine
* Unset GOPATH before running go build commands
* Dockefile: fix linux build by using latest
* Update to the latest ooni/probe-engine
```
go get -u github.com/ooni/probe-engine
go mod tidy
```
* Repair build
use ooni/probe-engine for contacting the bouncer, using the collector
use ooni/probe-engine for running the IM tests
DO NOT use it for other tests for now (to keep the diff small and ease comparison between tests using the ooni/probe-engine and tests using measurement-kit/go-measurement-kit)
I'd like @hellais to review. If this is fine, we can either continue the work in this branch and making all tests use ooni/probe-engine or merge and open a new PR for other tests.
Related issue #44