Uniform bytes measurements and accounting

[bassosimone]

In #118 we're measuring the bytes consumed by Telegram, but we're not measuring the bytes consumed by contacting OONI services. We should actually do that, because the original MK implementation was doing that, thus we need to have feature parity.

[bassosimone]

Before we can do this now, we need to finish converting the code to netx. Moving to Sprint 7.

[bassosimone]

Done in https://github.com/ooni/probe-engine/pull/418

[bassosimone]

We also need to do some work in probe-cli, reopening

[bassosimone]

I realized, in particular, that we're not correctly measuring the bytes in probe-cli. We're missing in particular the bytes used for submitting the report. As discussed with @hellais in chat, we don't want to include in this number of bytes interactions that don't pertain the experiment itself (e.g. downloading the assets). We're going to open an issue to track whether we want to report to the user the total amount of bytes consumed by using OONI (including downloading assets).

[bassosimone]

Here's the issue for tracking counting all the bytes consumed: https://github.com/ooni/probe/issues/1069

[bassosimone]

This issue ended up being much more complicated than intended. It turns out the design of netx was (1) already rather complicated and (2) did not allow easily to independently count bytes for the experiment and for the session. Because I recognized the importance of reducing the complexity of netx due to both (1) and (2), I spent time designing how to simplify netx rather than working on this issue in this sprint. As part of that, it seems I was able to improve the design significantly (https://github.com/ooni/probe-engine/issues/359) and also to solve the problem at hand (https://github.com/ooni/probe-engine/pull/421/commits/5dbf679ba0b0798148d06c7a9c52837042a7c3dc).

[bassosimone]

The detailed plan for implementing this feature in Sprint 10 is the following:

• [x] import https://github.com/ooni/probe-engine/commit/5dbf679ba0b0798148d06c7a9c52837042a7c3dc as netx/bytecounter

• [x] modify the above diff such that it has its own definition of Dialer, so that we don't need to depend on other packages

• [x] modify the way in which we construct a dialer in netx/internal/dialer such that this bytecounting dialer is always applied when we construct the dialer, so we have this functionality

• [x] move netx/internal/dialer to netx/dialer and consolidate all dialing functionality together including merging netx/bytecounter into this new package

• [x] verify that netx/dialer has 100% code coverage

• [x] modify session.go to use the context so the register the proper byte counter

• [x] modify experiment.go such that when we submit and update a measurement we use the context to also register with the proper byte counter

• [x] modify experiment.go such that it is possible to access its byte counter

• [ ] modify oonimkall/runner.go such that it uses the bytes consumed by the experiment rather than the bytes that have actually been consumed by the session

Note that, when all the above is done, measuring all bytes consumed by a probe (i.e. https://github.com/ooni/probe/issues/1069) is also mostly done. A probe that is using Go code will then have a way to obtain all the bytes consumed within a session. To enable a mobile probe to get this information we need this final step:

• [ ] modify oonimkall/runner.go such that it also includes the overall bytes consumed by the current session, which should basically to declare https://github.com/ooni/probe/issues/1069 done as far as the probe-engine part of the work is concerned

However, even after we have done that, we're still left with one more problem. We now have two conflicting ways of counting bytes, namely this context based way and experiment callbacks. Hence, to really get to the bottom of this issue we should further do the following:

• [x] make sure that ndt7 uses a dialer that enables counting bytes (this is currently tracked by https://github.com/ooni/probe-engine/issues/417, but we should probably do that as part of this issue and close https://github.com/ooni/probe-engine/issues/417 instead)

• [ ] make sure that dash is using a daler that enables counting bytes (this is currently tracked by https://github.com/ooni/probe-engine/issues/416, but we should probably do that as part of this issue and close https://github.com/ooni/probe-engine/issues/416 instead)

• [x] make sure that we pass the proper context when we run experiments

• [x] remove the byte counting callbacks from everywhere possible

I am thus going to close https://github.com/ooni/probe-engine/issues/416 and https://github.com/ooni/probe-engine/issues/417, and I am thus going to update the title of this issue accordingly.

Finally:

• [x] make sure probe-cli is using the correct byte accounting method

[bassosimone]

So, I hit another blocker: the connection to ps.ooni.io is persistent and is created by the session, therefore we cannot observe the bytes that it has exchanged when we're submitting a report using the context of the session. This means that the current solution with the byte counter does not work when there are persistent connections. I need to think about a different approach. 😞

[bassosimone]

So, while thinking about this, I'll work on https://github.com/ooni/probe-engine/issues/359. It seems I am hitting some of the problems caused by the complexity described in https://github.com/ooni/probe-engine/issues/359 anyway. 🤷‍♀

[bassosimone]

Okay, we've now reached again the point where we need to implement changes in probe-cli and in oonimkall to consider this issue complete! 🍀

[bassosimone]

So, probe-cli is very easy, this diff does the trick:

diff --git a/nettests/nettests.go b/nettests/nettests.go
index ea06ade..2231546 100644
--- a/nettests/nettests.go
+++ b/nettests/nettests.go
@@ -76,6 +76,10 @@ func (c *Controller) Run(builder *engine.ExperimentBuilder, inputs []string) err
        builder.SetCallbacks(model.ExperimentCallbacks(c))
        c.numInputs = len(inputs)
        exp := builder.NewExperiment()
+       defer func() {
+               c.res.DataUsageDown += exp.KibiBytesReceived()
+               c.res.DataUsageUp += exp.KibiBytesSent()
+       }()

        c.msmts = make(map[int64]*database.Measurement)

@@ -202,6 +206,9 @@ func (c *Controller) OnProgress(perc float64, msg string) {

 // OnDataUsage should be called when we have a data usage update.
 func (c *Controller) OnDataUsage(dloadKiB, uploadKiB float64) {
-       c.res.DataUsageDown += dloadKiB
-       c.res.DataUsageUp += uploadKiB
+       // Unused as 2020-04-05: we're now using directly the accessors
+       // provided by the experiment. This callback is going to be removed
+       // from probe-engine in May or June.
+       //
+       // TODO(bassosimone): create an issue for this?
 }

We're nearly done

[bassosimone]

This is now hopefully done!