From bf3c8bcdc3ed3d351ba3c4cf3febed514b4d6307 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 9 Feb 2022 15:08:19 +0100 Subject: [PATCH] [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`. --- .../urlgetter/getter_integration_test.go | 36 --------------- internal/engine/netx/httptransport/saver.go | 30 ------------ .../engine/netx/httptransport/saver_test.go | 46 ------------------- internal/engine/netx/netx.go | 2 - internal/engine/netx/netx_test.go | 9 +--- 5 files changed, 1 insertion(+), 122 deletions(-) diff --git a/internal/engine/experiment/urlgetter/getter_integration_test.go b/internal/engine/experiment/urlgetter/getter_integration_test.go index b7f6717..ef68465 100644 --- a/internal/engine/experiment/urlgetter/getter_integration_test.go +++ b/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 diff --git a/internal/engine/netx/httptransport/saver.go b/internal/engine/netx/httptransport/saver.go index 1e23e91..772aac1 100644 --- a/internal/engine/netx/httptransport/saver.go +++ b/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{} diff --git a/internal/engine/netx/httptransport/saver_test.go b/internal/engine/netx/httptransport/saver_test.go index 989bf36..a5d45d8 100644 --- a/internal/engine/netx/httptransport/saver_test.go +++ b/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") diff --git a/internal/engine/netx/netx.go b/internal/engine/netx/netx.go index d0222c7..c40d78e 100644 --- a/internal/engine/netx/netx.go +++ b/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} } diff --git a/internal/engine/netx/netx_test.go b/internal/engine/netx/netx_test.go index 0fc35fe..9ab809a 100644 --- a/internal/engine/netx/netx_test.go +++ b/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") }