[forwardport] fix(netx): stop collecting HTTP performance metrics (#689)

This diff forward ports b6db4f64dc83a2a27ee3ce6bba5ac93db922832d, whose
original log message is the following:

- - -

We're now using ooni/oohttp as our HTTP library in most cases.

A limitation of this library is that net/http/httptrace does not
work very well and reliably because (1) we need to use oohttp's
version of that code and (2) we cannot observe net events.

I noticed this fact because an integration test for collecting
HTTP performance metrics was broken.

The best solution here is to remove this functionality, since
it was basically unused in the repository. Only some integration
tests inside urlgetter bothered with these metrics.

A more clinical fix would have been to use ooni/oohttp/httptrace
instead of net/http/httptrace in the stdlib, but it does not
seem to be a good idea, given that those metrics were not used.

With this diff applied, we'll further reduce the number of locally
failing integration tests to just jafar-specific tests.

This diff WILL need to be forwardported to `master`.

internal/engine/experiment/urlgetter/getter_integration_test.go

@@ -429,9 +429,6 @@ func TestGetterIntegrationHTTPS(t *testing.T) {
 		connect                  bool
 		tlsHandshakeStart        bool
 		tlsHandshakeDone         bool
-		httpWroteHeaders         bool
-		httpWroteRequest         bool
-		httpFirstResponseByte    bool
 		httpResponseMetadata     bool
 		httpResponseBodySnapshot bool
 		httpTransactionDone      bool
@@ -452,12 +449,6 @@ func TestGetterIntegrationHTTPS(t *testing.T) {
 			tlsHandshakeStart = true
 		case "tls_handshake_done":
 			tlsHandshakeDone = true
-		case "http_wrote_headers":
-			httpWroteHeaders = true
-		case "http_wrote_request":
-			httpWroteRequest = true
-		case "http_first_response_byte":
-			httpFirstResponseByte = true
 		case "http_response_metadata":
 			httpResponseMetadata = true
 		case "http_response_body_snapshot":
@@ -474,9 +465,6 @@ func TestGetterIntegrationHTTPS(t *testing.T) {
 	ok = ok && connect
 	ok = ok && tlsHandshakeStart
 	ok = ok && tlsHandshakeDone
-	ok = ok && httpWroteHeaders
-	ok = ok && httpWroteRequest
-	ok = ok && httpFirstResponseByte
 	ok = ok && httpResponseMetadata
 	ok = ok && httpResponseBodySnapshot
 	ok = ok && httpTransactionDone
@@ -570,9 +558,6 @@ func TestGetterIntegrationTLSHandshake(t *testing.T) {
 		connect                  bool
 		tlsHandshakeStart        bool
 		tlsHandshakeDone         bool
-		httpWroteHeaders         bool
-		httpWroteRequest         bool
-		httpFirstResponseByte    bool
 		httpResponseMetadata     bool
 		httpResponseBodySnapshot bool
 		httpTransactionDone      bool
@@ -593,12 +578,6 @@ func TestGetterIntegrationTLSHandshake(t *testing.T) {
 			tlsHandshakeStart = true
 		case "tls_handshake_done":
 			tlsHandshakeDone = true
-		case "http_wrote_headers":
-			httpWroteHeaders = true
-		case "http_wrote_request":
-			httpWroteRequest = true
-		case "http_first_response_byte":
-			httpFirstResponseByte = true
 		case "http_response_metadata":
 			httpResponseMetadata = true
 		case "http_response_body_snapshot":
@@ -615,9 +594,6 @@ func TestGetterIntegrationTLSHandshake(t *testing.T) {
 	ok = ok && connect
 	ok = ok && tlsHandshakeStart
 	ok = ok && tlsHandshakeDone
-	ok = ok && !httpWroteHeaders
-	ok = ok && !httpWroteRequest
-	ok = ok && !httpFirstResponseByte
 	ok = ok && !httpResponseMetadata
 	ok = ok && !httpResponseBodySnapshot
 	ok = ok && !httpTransactionDone
@@ -688,9 +664,6 @@ func TestGetterHTTPSWithTunnel(t *testing.T) {
 		connect                  bool
 		tlsHandshakeStart        bool
 		tlsHandshakeDone         bool
-		httpWroteHeaders         bool
-		httpWroteRequest         bool
-		httpFirstResponseByte    bool
 		httpResponseMetadata     bool
 		httpResponseBodySnapshot bool
 		httpTransactionDone      bool
@@ -711,12 +684,6 @@ func TestGetterHTTPSWithTunnel(t *testing.T) {
 			tlsHandshakeStart = true
 		case "tls_handshake_done":
 			tlsHandshakeDone = true
-		case "http_wrote_headers":
-			httpWroteHeaders = true
-		case "http_wrote_request":
-			httpWroteRequest = true
-		case "http_first_response_byte":
-			httpFirstResponseByte = true
 		case "http_response_metadata":
 			httpResponseMetadata = true
 		case "http_response_body_snapshot":
@@ -733,9 +700,6 @@ func TestGetterHTTPSWithTunnel(t *testing.T) {
 	ok = ok && connect
 	ok = ok && tlsHandshakeStart
 	ok = ok && tlsHandshakeDone
-	ok = ok && httpWroteHeaders
-	ok = ok && httpWroteRequest
-	ok = ok && httpFirstResponseByte
 	ok = ok && httpResponseMetadata
 	ok = ok && httpResponseBodySnapshot
 	ok = ok && httpTransactionDone

internal/engine/netx/httptransport/saver.go

@@ -5,7 +5,6 @@ import (
 	"context"
 	"io"
 	"net/http"
-	"net/http/httptrace"
 	"time"
 
 	"github.com/ooni/probe-cli/v3/internal/engine/netx/trace"
@@ -13,34 +12,6 @@ import (
 	"github.com/ooni/probe-cli/v3/internal/netxlite"
 )
 
-// SaverPerformanceHTTPTransport is a RoundTripper that saves
-// performance events occurring during the round trip
-type SaverPerformanceHTTPTransport struct {
-	model.HTTPTransport
-	Saver *trace.Saver
-}
-
-// RoundTrip implements RoundTripper.RoundTrip
-func (txp SaverPerformanceHTTPTransport) RoundTrip(req *http.Request) (*http.Response, error) {
-	tracep := httptrace.ContextClientTrace(req.Context())
-	if tracep == nil {
-		tracep = &httptrace.ClientTrace{
-			WroteHeaders: func() {
-				txp.Saver.Write(trace.Event{Name: "http_wrote_headers", Time: time.Now()})
-			},
-			WroteRequest: func(httptrace.WroteRequestInfo) {
-				txp.Saver.Write(trace.Event{Name: "http_wrote_request", Time: time.Now()})
-			},
-			GotFirstResponseByte: func() {
-				txp.Saver.Write(trace.Event{
-					Name: "http_first_response_byte", Time: time.Now()})
-			},
-		}
-		req = req.WithContext(httptrace.WithClientTrace(req.Context(), tracep))
-	}
-	return txp.HTTPTransport.RoundTrip(req)
-}
-
 // SaverMetadataHTTPTransport is a RoundTripper that saves
 // events related to HTTP request and response metadata
 type SaverMetadataHTTPTransport struct {
@@ -166,7 +137,6 @@ type saverReadCloser struct {
 	io.Reader
 }
 
-var _ model.HTTPTransport = SaverPerformanceHTTPTransport{}
 var _ model.HTTPTransport = SaverMetadataHTTPTransport{}
 var _ model.HTTPTransport = SaverBodyHTTPTransport{}
 var _ model.HTTPTransport = SaverTransactionHTTPTransport{}

internal/engine/netx/httptransport/saver_test.go

@@ -16,52 +16,6 @@ import (
 	"github.com/ooni/probe-cli/v3/internal/netxlite"
 )
 
-func TestSaverPerformanceNoMultipleEvents(t *testing.T) {
-	if testing.Short() {
-		t.Skip("skip test in short mode")
-	}
-	saver := &trace.Saver{}
-	// register twice - do we see events twice?
-	txp := httptransport.SaverPerformanceHTTPTransport{
-		HTTPTransport: netxlite.NewHTTPTransportStdlib(model.DiscardLogger),
-		Saver:         saver,
-	}
-	txp = httptransport.SaverPerformanceHTTPTransport{
-		HTTPTransport: txp,
-		Saver:         saver,
-	}
-	req, err := http.NewRequest("GET", "https://www.google.com", nil)
-	if err != nil {
-		t.Fatal(err)
-	}
-	resp, err := txp.RoundTrip(req)
-	if err != nil {
-		t.Fatal("not the error we expected")
-	}
-	if resp == nil {
-		t.Fatal("expected non nil response here")
-	}
-	ev := saver.Read()
-	// we should specifically see the events not attached to any
-	// context being submitted twice. This is fine because they are
-	// explicit, while the context is implicit and hence leads to
-	// more subtle bugs. For example, this happens when you measure
-	// every event and combine HTTP with DoH.
-	if len(ev) != 3 {
-		t.Fatal("expected three events")
-	}
-	expected := []string{
-		"http_wrote_headers",       // measured with context
-		"http_wrote_request",       // measured with context
-		"http_first_response_byte", // measured with context
-	}
-	for i := 0; i < len(expected); i++ {
-		if ev[i].Name != expected[i] {
-			t.Fatal("unexpected event name")
-		}
-	}
-}
-
 func TestSaverMetadataSuccess(t *testing.T) {
 	if testing.Short() {
 		t.Skip("skip test in short mode")

internal/engine/netx/netx.go

@@ -213,8 +213,6 @@ func NewHTTPTransport(config Config) model.HTTPTransport {
 			HTTPTransport: txp, Saver: config.HTTPSaver}
 		txp = httptransport.SaverBodyHTTPTransport{
 			HTTPTransport: txp, Saver: config.HTTPSaver}
-		txp = httptransport.SaverPerformanceHTTPTransport{
-			HTTPTransport: txp, Saver: config.HTTPSaver}
 		txp = httptransport.SaverTransactionHTTPTransport{
 			HTTPTransport: txp, Saver: config.HTTPSaver}
 	}

internal/engine/netx/netx_test.go

@@ -513,14 +513,7 @@ func TestNewWithSaver(t *testing.T) {
 	if stxptxp.Saver != saver {
 		t.Fatal("not the logger we expected")
 	}
-	sptxp, ok := stxptxp.HTTPTransport.(httptransport.SaverPerformanceHTTPTransport)
-	if !ok {
-		t.Fatal("not the transport we expected")
-	}
-	if sptxp.Saver != saver {
-		t.Fatal("not the logger we expected")
-	}
-	sbtxp, ok := sptxp.HTTPTransport.(httptransport.SaverBodyHTTPTransport)
+	sbtxp, ok := stxptxp.HTTPTransport.(httptransport.SaverBodyHTTPTransport)
 	if !ok {
 		t.Fatal("not the transport we expected")
 	}