PLAN: netx refactoring and improvements

[bassosimone]

This issue contains a plan to radically simplify and make netx code more correct (for context the most up-to date design document for the original netx is available at /legacy/netx/DESIGN.md).

The companion draft pull request #421 illustrates the principles described here in a PoC implementation. This plan was drafted in the March, 27 week but it probably still needs some hammering and thinking. The initial motivation for working on this plan was to solve #125, which proved to be more difficult than expected and which triggered design thinking about netx.

The plan has been implemented since then and #745 is the last issue describing this whole effort. In most cases (I hope) issues related to this plan have been linked to this Epic.

---

context.Context

We should make sure we use the context as little as possible. The current implementation is setting a MeasurementRoot to the context, but it is possible to perform the same measurements without. So, we should not be using the context, because that leads to better code.

DialTLSContext

Since Go 1.14, the standard library has support for net·http.Transport.DialTLSContext, which means that we can use the same TLS dialing code everywhere. Because of the above point of reducing the usage of the Context, we should also actually see to collect as much data as possible without using httptrace. Now that we can manage our TLS dial, httptrace is certainly useful to see some specific events but luckily is not anymore a hard requirement for observing TLS. See how this is implemented in ac9a8cb.

Every measurement uses a Transport

We're already doing that with netx. This has the profound implication that we can radically simplify the code. We could have a very simple transport, dialer, etc that just concerns itself with doing its job and logging. Then we can construct much more complex objects when measuring.

func Get(ctx context.Context, url string, logger Logger) (Results, error) {
  var dialer Dialer = new(net.Dialer)
  dialer = &ErrWrappingDialer{Dialer: dialer}
  dialSaver := &DialSaver{Dialer: dialer}
  dialer = &LogDialer{Dialer: dialSaver, Logger: logger}
  txp := NewHTTPTransport(dialer)
  txp = &ErrWrappingTransport{Transport: txp}
  txpSaver := &TransportSaver{Transport: txp}
  txp = LogTransport{Logger: logger, Transport: txpSaver}
  client := &http.Client{Transport: txp}
  // ...
}

The advantages of these strategy are: (1) that logging can completely be separate, hence changes in measuring code do not impact logging and viceversa; (2) that we most likely don't need to have an handler for events, because we can store in place in the specific saver. The direct implication is that we also don't need the MeasurementRoot anymore and possibly the context.

See how this is implemented in ac9a8cb.

Saving rather than emitting

From the above point it stems directly that we should be saving into specific objects rather than emitting on a handler that possibly later would do something with such events. We have already seen issues like blocked goroutines on emitting channels with the handler pattern.

Instead, why don't we simplify the code to be like above and use a Saver like:

type Saver struct {
  events []Event
  mu sync.Mutex
}

func (s *Saver) ReadEvents() []Events {
  s.mu.Lock()
  ev := s.events
  s.events = nil
  s.mu.Unlock()
  return s
}

func (s *Saver) emit(ev Event) {
  s.mu.Lock()
  s.events = append(s.events, ev)
  s.mu.Unlock()
}

With this pattern, events arriving after ReadEvents are safely queued. Because we are using a dedicated transport, very soon everything is removed. So, we are wasting little memory and we are using a pattern where a mistake is not going to cause blocked goroutines.

See how this is implemented in ac9a8cb.

Reducing data transformations

We should reorganize the data we save to be as similar as possible to OONI data. We currently have a whole package (oonidatamodel) whose job is to perform transformations from netx events to OONI data. This means that netx events are an intermediate layer of glue that is between the raw data that we collect from Go code and the data we care about. We should avoid glue.

In ac9a8cb we have tried to show how this could be done by generating an intermediate result that is very close to the Go originals but is structured already with the data structures expected by OONI.

Measuring consumed bytes

Since we want to measure consumed bytes for the session and for the experiment, and since we control the transport during an experiment, it means we mainly need to choose how to account when we're communicating with OONI services. Because OONI is using persistent connections, we don't want two distinct transports for experiment and session. A better solution is to sparingly use the context for this use case. This would be the only case in which we explicitly take advantage of the context.WithValue API in the new code. See 5dbf679.

[bassosimone]

We should evaluate whether it's time to close this Epic as part of Sprint 21.

[bassosimone]

So, I've quickly checked and it seems we can close this Epic now. I will first close all the dependent issues, which can be considered complete, because /experiment/tor is on the old netx and the old netx capabilities are good enough for what we wanna do in such experiment (old and new netx are very similar, but the newer is better engineered).

[bassosimone]

All the lingering activities linked to this plan have been vetted and closed. Created all the required follow ups.

It has been a long lasting effort but it's good that now netx is simpler than before. (I also think this was one of the first time I engaged into a non-destructive incremental refactoring, so I should really rejoice in closing this Epic.)