Skip to content

Commit

Permalink
fix(netx): stop collecting HTTP performance metrics
Browse files Browse the repository at this point in the history 
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`.
  • Loading branch information
@bassosimone
bassosimone committed Feb 9, 2022
1 parent f5e10e0 commit b6db4f6
Show file tree
Hide file tree
Showing 5 changed files with 1 addition and 122 deletions.
36 changes: 0 additions & 36 deletions internal/engine/experiment/urlgetter/getter_integration_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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":
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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":
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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":
Expand All @@ -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
Expand Down
30 changes: 0 additions & 30 deletions internal/engine/netx/httptransport/saver.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,42 +5,13 @@ import (
"context"
"io"
"net/http"
"net/http/httptrace"
"time"

"github.com/ooni/probe-cli/v3/internal/engine/netx/trace"
"github.com/ooni/probe-cli/v3/internal/model"
"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 {
Expand Down Expand Up @@ -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{}
46 changes: 0 additions & 46 deletions internal/engine/netx/httptransport/saver_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
2 changes: 0 additions & 2 deletions internal/engine/netx/netx.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}
}
Expand Down
9 changes: 1 addition & 8 deletions internal/engine/netx/netx_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Expand Down

0 comments on commit b6db4f6

Please sign in to comment.