c/ooni-probe-cli
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

refactor(tracex): internally represent errors as strings (#786)

Browse Source 
There are two reasons why this is beneficial:

1. github.com/google/go-cmp is more annoying to use for comparing
data structures when there are interfaces to compare. Sure, there's
a recipe for teaching it to compare errors, but how about making
the errors trivially comparable instead?

2. if we want to send errors over the network, JSON serialization
works but we cannot unmarshal the resulting string back to an error,
so how about making this representation trivial to serialize (we
are not going this now, but we need this property for websteps and
it may be sensible to try to avoid to have duplicate code because
of that -- measurex currently duplicates many tracex functionality
and this is quite unfortunate because it slows development down)

Additionally, if an error is a string:

3. we can very easily use a switch for comparing its possible
values with "" representing the absence of errors, while it is
more complex to do the same when using a nullable string or even
an error (i.e., an interface)

4. if a type is not nullable, it's easier to write safe code for
it and we may want to refactor experiments to use the internal
representation of measurements for more robust processing code

For all these reasons, let's internally use strings in tracex.

The overall aim here is to reduce the duplicated code between pre
and post-measurex measurements (see https://github.com/ooni/probe/issues/2035).
This commit is contained in:
Simone Basso 2022-06-02 10:37:07 +02:00 committed by GitHub
parent 2556e93050
commit 83e3167ce2
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
13 changed files with 115 additions and 68 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

41
internal/tracex/archival.go
View File

@ -64,8 +64,8 @@ func NewTCPConnectList(begin time.Time, events []Event) (out []TCPConnectEntry)
Port: iport,
Status: TCPConnectStatus{
Blocked: nil, // only used by Web Connectivity
Failure: NewFailure(event.Err),
Success: event.Err == nil,
Failure: event.Err.ToFailure(),
Success: event.Err.IsNil(),
},
T: event.Time.Sub(begin).Seconds(),
})
@ -73,21 +73,10 @@ func NewTCPConnectList(begin time.Time, events []Event) (out []TCPConnectEntry)
return
}
// NewFailure creates a failure nullable string from the given error
// NewFailure creates a failure nullable string from the given error. This function
// is equivalent to NewFailureStr(err).ToFailure().
func NewFailure(err error) *string {
if err == nil {
return nil
}
// The following code guarantees that the error is always wrapped even
// when we could not actually hit our code that does the wrapping. A case
// in which this happen is with context deadline for HTTP.
err = netxlite.NewTopLevelGenericErrWrapper(err)
errWrapper := err.(*netxlite.ErrWrapper)
s := errWrapper.Failure
if s == "" {
s = "unknown_failure: errWrapper.Failure is empty"
}
return &s
return NewFailureStr(err).ToFailure()
}
// NewFailedOperation creates a failed operation string from the given error.
@ -158,7 +147,7 @@ func newRequestList(begin time.Time, events []Event) (out []RequestEntry) {
entry.Response.Locations = ev.HTTPResponseHeaders.Values("Location")
entry.Response.Body.Value = string(ev.HTTPResponseBody)
entry.Response.BodyIsTruncated = ev.HTTPResponseBodyIsTruncated
entry.Failure = NewFailure(ev.Err)
entry.Failure = ev.Err.ToFailure()
out = append(out, entry)
}
}
@ -183,7 +172,7 @@ func NewDNSQueriesList(begin time.Time, events []Event) (out []DNSQueryEntry) {
entry.Answers, qtype.makeAnswerEntry(addr))
}
}
if len(entry.Answers) <= 0 && ev.Err == nil {
if len(entry.Answers) <= 0 && ev.Err.IsNil() {
// This allows us to skip cases where the server does not have
// an IPv6 address but has an IPv4 address. Instead, when we
// receive an error, we want to track its existence. The main
@ -228,7 +217,7 @@ func (qtype dnsQueryType) makeAnswerEntry(addr string) DNSAnswerEntry {
func (qtype dnsQueryType) makeQueryEntry(begin time.Time, ev *EventValue) DNSQueryEntry {
return DNSQueryEntry{
Engine: ev.Proto,
Failure: NewFailure(ev.Err),
Failure: ev.Err.ToFailure(),
Hostname: ev.Hostname,
QueryType: string(qtype),
ResolverAddress: ev.Address,
@ -244,21 +233,21 @@ func NewNetworkEventsList(begin time.Time, events []Event) (out []NetworkEvent)
case *EventConnectOperation:
out = append(out, NetworkEvent{
Address: ev.Address,
Failure: NewFailure(ev.Err),
Failure: ev.Err.ToFailure(),
Operation: wrapper.Name(),
Proto: ev.Proto,
T: ev.Time.Sub(begin).Seconds(),
})
case *EventReadOperation:
out = append(out, NetworkEvent{
Failure: NewFailure(ev.Err),
Failure: ev.Err.ToFailure(),
Operation: wrapper.Name(),
NumBytes: int64(ev.NumBytes),
T: ev.Time.Sub(begin).Seconds(),
})
case *EventWriteOperation:
out = append(out, NetworkEvent{
Failure: NewFailure(ev.Err),
Failure: ev.Err.ToFailure(),
Operation: wrapper.Name(),
NumBytes: int64(ev.NumBytes),
T: ev.Time.Sub(begin).Seconds(),
@ -266,7 +255,7 @@ func NewNetworkEventsList(begin time.Time, events []Event) (out []NetworkEvent)
case *EventReadFromOperation:
out = append(out, NetworkEvent{
Address: ev.Address,
Failure: NewFailure(ev.Err),
Failure: ev.Err.ToFailure(),
Operation: wrapper.Name(),
NumBytes: int64(ev.NumBytes),
T: ev.Time.Sub(begin).Seconds(),
@ -274,14 +263,14 @@ func NewNetworkEventsList(begin time.Time, events []Event) (out []NetworkEvent)
case *EventWriteToOperation:
out = append(out, NetworkEvent{
Address: ev.Address,
Failure: NewFailure(ev.Err),
Failure: ev.Err.ToFailure(),
Operation: wrapper.Name(),
NumBytes: int64(ev.NumBytes),
T: ev.Time.Sub(begin).Seconds(),
})
default: // For example, "tls_handshake_done" (used in data analysis!)
out = append(out, NetworkEvent{
Failure: NewFailure(ev.Err),
Failure: ev.Err.ToFailure(),
Operation: wrapper.Name(),
T: ev.Time.Sub(begin).Seconds(),
})
@ -302,7 +291,7 @@ func NewTLSHandshakesList(begin time.Time, events []Event) (out []TLSHandshake)
out = append(out, TLSHandshake{
Address: ev.Address,
CipherSuite: ev.TLSCipherSuite,
Failure: NewFailure(ev.Err),
Failure: ev.Err.ToFailure(),
NegotiatedProtocol: ev.TLSNegotiatedProto,
NoTLSVerify: ev.NoTLSVerify,
PeerCertificates: tlsMakePeerCerts(ev.TLSPeerCerts),

22
internal/tracex/archival_test.go
View File

@ -92,7 +92,7 @@ func TestNewTCPConnectList(t *testing.T) {
}}, &EventConnectOperation{&EventValue{
Address: "8.8.4.4:53",
Duration: 50 * time.Millisecond,
Err: io.EOF,
Err: netxlite.FailureEOFError,
Proto: "tcp",
Time: begin.Add(180 * time.Millisecond),
}}},
@ -165,7 +165,7 @@ func TestNewRequestList(t *testing.T) {
},
HTTPMethod: "GET",
HTTPURL: "https://www.example.com/result",
Err: io.EOF,
Err: netxlite.FailureEOFError,
Time: begin.Add(20 * time.Millisecond),
}}},
},
@ -332,7 +332,7 @@ func TestNewDNSQueriesList(t *testing.T) {
}}, &EventConnectOperation{&EventValue{ // skipped because not relevant
Address: "8.8.4.4:53",
Duration: 50 * time.Millisecond,
Err: io.EOF,
Err: netxlite.FailureEOFError,
Proto: "tcp",
Time: begin.Add(180 * time.Millisecond),
}}},
@ -381,7 +381,7 @@ func TestNewDNSQueriesList(t *testing.T) {
args: args{
begin: begin,
events: []Event{&EventResolveDone{&EventValue{
Err: &netxlite.ErrWrapper{Failure: netxlite.FailureDNSNXDOMAINError},
Err: netxlite.FailureDNSNXDOMAINError,
Hostname: "dns.google.com",
Time: begin.Add(200 * time.Millisecond),
}}},
@ -435,25 +435,25 @@ func TestNewNetworkEventsList(t *testing.T) {
begin: begin,
events: []Event{&EventConnectOperation{&EventValue{
Address: "8.8.8.8:853",
Err: io.EOF,
Err: netxlite.FailureEOFError,
Proto: "tcp",
Time: begin.Add(7 * time.Millisecond),
}}, &EventReadOperation{&EventValue{
Err: context.Canceled,
Err: netxlite.FailureInterrupted,
NumBytes: 7117,
Time: begin.Add(11 * time.Millisecond),
}}, &EventReadFromOperation{&EventValue{
Address: "8.8.8.8:853",
Err: context.Canceled,
Err: netxlite.FailureInterrupted,
NumBytes: 7117,
Time: begin.Add(11 * time.Millisecond),
}}, &EventWriteOperation{&EventValue{
Err: websocket.ErrBadHandshake,
Err: NewFailureStr(websocket.ErrBadHandshake),
NumBytes: 4114,
Time: begin.Add(14 * time.Millisecond),
}}, &EventWriteToOperation{&EventValue{
Address: "8.8.8.8:853",
Err: websocket.ErrBadHandshake,
Err: NewFailureStr(websocket.ErrBadHandshake),
NumBytes: 4114,
Time: begin.Add(14 * time.Millisecond),
}}, &EventResolveStart{&EventValue{
@ -528,7 +528,7 @@ func TestNewTLSHandshakesList(t *testing.T) {
begin: begin,
events: []Event{&EventTLSHandshakeDone{&EventValue{
Address: "131.252.210.176:443",
Err: io.EOF,
Err: netxlite.FailureEOFError,
NoTLSVerify: false,
Proto: "tcp",
TLSCipherSuite: "SUITE",
@ -564,7 +564,7 @@ func TestNewTLSHandshakesList(t *testing.T) {
begin: begin,
events: []Event{&EventQUICHandshakeDone{&EventValue{
Address: "131.252.210.176:443",
Err: io.EOF,
Err: netxlite.FailureEOFError,
NoTLSVerify: false,
Proto: "quic",
TLSCipherSuite: "SUITE",

6
internal/tracex/dialer.go
View File

@ -54,7 +54,7 @@ func (d *DialerSaver) DialContext(ctx context.Context, network, address string)
d.Saver.Write(&EventConnectOperation{&EventValue{
Address: address,
Duration: stop.Sub(start),
Err: err,
Err: NewFailureStr(err),
Proto: network,
Time: stop,
}})
@ -128,7 +128,7 @@ func (c *dialerConnWrapper) Read(p []byte) (int, error) {
Address: remoteAddr,
Data: p[:count],
Duration: stop.Sub(start),
Err: err,
Err: NewFailureStr(err),
NumBytes: count,
Proto: proto,
Time: stop,
@ -146,7 +146,7 @@ func (c *dialerConnWrapper) Write(p []byte) (int, error) {
Address: remoteAddr,
Data: p[:count],
Duration: stop.Sub(start),
Err: err,
Err: NewFailureStr(err),
NumBytes: count,
Proto: proto,
Time: stop,

6
internal/tracex/dialer_test.go
View File

@ -57,7 +57,7 @@ func TestDialerSaver(t *testing.T) {
if ev[0].Value().Duration <= 0 {
t.Fatal("unexpected Duration")
}
if !errors.Is(ev[0].Value().Err, expected) {
if ev[0].Value().Err != "unknown_failure: mocked error" {
t.Fatal("unexpected Err")
}
if ev[0].Name() != netxlite.ConnectOperation {
@ -210,7 +210,7 @@ func TestDialerConnWrapper(t *testing.T) {
if ev[0].Value().Duration <= 0 {
t.Fatal("unexpected Duration")
}
if !errors.Is(ev[0].Value().Err, io.EOF) {
if ev[0].Value().Err != netxlite.FailureEOFError {
t.Fatal("unexpected Err")
}
if ev[0].Name() != netxlite.ReadOperation {
@ -263,7 +263,7 @@ func TestDialerConnWrapper(t *testing.T) {
if ev[0].Value().Duration <= 0 {
t.Fatal("unexpected Duration")
}
if !errors.Is(ev[0].Value().Err, io.EOF) {
if ev[0].Value().Err != netxlite.FailureEOFError {
t.Fatal("unexpected Err")
}
if ev[0].Name() != netxlite.WriteOperation {

59
internal/tracex/event.go
View File

@ -6,12 +6,69 @@ package tracex
import (
"crypto/x509"
"errors"
"net/http"
"time"
"github.com/ooni/probe-cli/v3/internal/netxlite"
"github.com/ooni/probe-cli/v3/internal/runtimex"
)
// FailureStr is the string representation of an error. The empty
// string represents the absence of any error.
type FailureStr string
// NewFailureStr creates a FailureStr from an error. If the error is not
// already an ErrWrapper, it's converted to an ErrWrapper. If the ErrWrapper's
// Failure is not empty, we return that. Otherwise we return a string
// indicating that an ErrWrapper has an empty failure (should not happen).
func NewFailureStr(err error) FailureStr {
if err == nil {
return ""
}
// The following code guarantees that the error is always wrapped even
// when we could not actually hit our code that does the wrapping. A case
// in which this could happen is with context deadline for HTTP when you
// have wrapped the underlying dialers but not the Transport.
var errWrapper *netxlite.ErrWrapper
if !errors.As(err, &errWrapper) {
err := netxlite.NewTopLevelGenericErrWrapper(err)
couldConvert := errors.As(err, &errWrapper)
runtimex.PanicIfFalse(couldConvert, "we should have an ErrWrapper here")
}
s := errWrapper.Failure
if s == "" {
s = "unknown_failure: errWrapper.Failure is empty"
}
return FailureStr(s)
}
// IsNil returns whether this FailureStr is nil. Technically speaking, the
// failure cannot be nil, but an empty string is equivalent to nil after
// we convert using ToFailure(). Also, this type is often called Err, Error,
// or Failure. So, the resulting code actually reads correct.
func (fs FailureStr) IsNil() bool {
return fs == ""
}
// IsNotNil is the opposite of IsNil. Technically speaking, the
// failure cannot be nil, but an empty string is equivalent to nil after
// we convert using ToFailure(). Also, this type is often called Err, Error,
// or Failure. So, the resulting code actually reads correct.
func (fs FailureStr) IsNotNil() bool {
return !fs.IsNil()
}
// ToFailure converts a FailureStr to a OONI failure (i.e., a string
// on error and nil in case of success).
func (fs FailureStr) ToFailure() (out *string) {
if fs != "" {
s := string(fs)
out = &s
}
return
}
// Event is one of the events within a trace.
type Event interface {
// Value returns the event value
@ -224,7 +281,7 @@ type EventValue struct {
DNSResponse []byte `json:",omitempty"`
Data []byte `json:",omitempty"`
Duration time.Duration `json:",omitempty"`
Err error `json:",omitempty"`
Err FailureStr `json:",omitempty"`
HTTPMethod string `json:",omitempty"`
HTTPRequestHeaders http.Header `json:",omitempty"`
HTTPResponseHeaders http.Header `json:",omitempty"`

4
internal/tracex/http.go
View File

@ -73,7 +73,7 @@ func (txp *HTTPTransportSaver) RoundTrip(req *http.Request) (*http.Response, err
if err != nil {
ev.Duration = time.Since(started)
ev.Err = err
ev.Err = NewFailureStr(err)
return nil, err
}
@ -86,7 +86,7 @@ func (txp *HTTPTransportSaver) RoundTrip(req *http.Request) (*http.Response, err
if err != nil {
ev.Duration = time.Since(started)
ev.Err = err
ev.Err = NewFailureStr(err)
return nil, err
}

4
internal/tracex/http_test.go
View File

@ -178,7 +178,7 @@ func TestHTTPTransportSaver(t *testing.T) {
if value.Duration <= 0 {
t.Fatal("expected nonzero duration")
}
if value.Err.Error() != "connection_reset" {
if value.Err != netxlite.FailureConnectionReset {
t.Fatal("unexpected Err value")
}
if len(value.HTTPResponseHeaders) > 0 {
@ -268,7 +268,7 @@ func TestHTTPTransportSaver(t *testing.T) {
if ev[1].Value().HTTPResponseHeaders.Get("Server") != "antani" {
t.Fatal("invalid Server header")
}
if ev[1].Value().Err.Error() != "unknown_failure: mocked error" {
if ev[1].Value().Err != "unknown_failure: mocked error" {
t.Fatal("invalid error")
}
})

6
internal/tracex/quic.go
View File

@ -60,7 +60,7 @@ func (h *QUICDialerSaver) DialContext(ctx context.Context, network string,
h.Saver.Write(&EventQUICHandshakeDone{&EventValue{
Address: host,
Duration: stop.Sub(start),
Err: err,
Err: NewFailureStr(err),
NoTLSVerify: tlsCfg.InsecureSkipVerify,
Proto: network,
TLSNextProtos: tlsCfg.NextProtos,
@ -149,7 +149,7 @@ func (c *quicPacketConnWrapper) WriteTo(p []byte, addr net.Addr) (int, error) {
Address: addr.String(),
Data: p[:count],
Duration: stop.Sub(start),
Err: err,
Err: NewFailureStr(err),
NumBytes: count,
Time: stop,
}})
@ -168,7 +168,7 @@ func (c *quicPacketConnWrapper) ReadFrom(b []byte) (int, net.Addr, error) {
Address: c.safeAddrString(addr),
Data: data,
Duration: stop.Sub(start),
Err: err,
Err: NewFailureStr(err),
NumBytes: n,
Time: stop,
}})

12
internal/tracex/quic_test.go
View File

@ -50,7 +50,7 @@ func TestQUICDialerSaver(t *testing.T) {
if value.Duration <= 0 {
t.Fatal("expected non-zero duration")
}
if value.Err != nil {
if value.Err.IsNotNil() {
t.Fatal("expected no error here")
}
if value.TLSCipherSuite != "TLS_RSA_WITH_RC4_128_SHA" {
@ -120,7 +120,7 @@ func TestQUICDialerSaver(t *testing.T) {
if value.Duration <= 0 {
t.Fatal("expected non-zero duration")
}
if value.Err == nil {
if value.Err.IsNil() {
t.Fatal("expected non-nil error here")
}
}
@ -266,7 +266,7 @@ func TestQUICPacketConnWrapper(t *testing.T) {
if value.Duration <= 0 {
t.Fatal("expected nonzero duration")
}
if !errors.Is(value.Err, expected) {
if value.Err != "unknown_failure: mocked error" {
t.Fatal("unexpected value.Err", value.Err)
}
if value.NumBytes != 0 {
@ -326,7 +326,7 @@ func TestQUICPacketConnWrapper(t *testing.T) {
if value.Duration <= 0 {
t.Fatal("expected nonzero duration")
}
if value.Err != nil {
if value.Err.IsNotNil() {
t.Fatal("unexpected value.Err", value.Err)
}
if value.NumBytes != 4 {
@ -382,7 +382,7 @@ func TestQUICPacketConnWrapper(t *testing.T) {
if value.Duration <= 0 {
t.Fatal("expected nonzero duration")
}
if !errors.Is(value.Err, expected) {
if value.Err != "unknown_failure: mocked error" {
t.Fatal("unexpected value.Err", value.Err)
}
if value.NumBytes != 0 {
@ -434,7 +434,7 @@ func TestQUICPacketConnWrapper(t *testing.T) {
if value.Duration <= 0 {
t.Fatal("expected nonzero duration")
}
if value.Err != nil {
if value.Err.IsNotNil() {
t.Fatal("unexpected value.Err", value.Err)
}
if value.NumBytes != 1 {

4
internal/tracex/resolver.go
View File

@ -51,7 +51,7 @@ func (r *ResolverSaver) LookupHost(ctx context.Context, hostname string) ([]stri
Addresses: addrs,
Address: r.Resolver.Address(),
Duration: stop.Sub(start),
Err: err,
Err: NewFailureStr(err),
Hostname: hostname,
Proto: r.Resolver.Network(),
Time: stop,
@ -122,7 +122,7 @@ func (txp *DNSTransportSaver) RoundTrip(
DNSQuery: dnsMaybeQueryBytes(query),
DNSResponse: dnsMaybeResponseBytes(response),
Duration: stop.Sub(start),
Err: err,
Err: NewFailureStr(err),
Proto: txp.DNSTransport.Network(),
Time: stop,
}})

13
internal/tracex/resolver_test.go
View File

@ -11,12 +11,13 @@ import (
"github.com/ooni/probe-cli/v3/internal/model"
"github.com/ooni/probe-cli/v3/internal/model/mocks"
"github.com/ooni/probe-cli/v3/internal/netxlite"
"github.com/ooni/probe-cli/v3/internal/runtimex"
)
func TestResolverSaver(t *testing.T) {
t.Run("on failure", func(t *testing.T) {
expected := errors.New("no such host")
expected := netxlite.ErrOODNSNoSuchHost
saver := &Saver{}
reso := saver.WrapResolver(newFakeResolverWithExplicitError(expected))
addrs, err := reso.LookupHost(context.Background(), "www.google.com")
@ -45,7 +46,7 @@ func TestResolverSaver(t *testing.T) {
if ev[1].Value().Duration <= 0 {
t.Fatal("unexpected Duration")
}
if !errors.Is(ev[1].Value().Err, expected) {
if ev[1].Value().Err != netxlite.FailureDNSNXDOMAINError {
t.Fatal("unexpected Err")
}
if ev[1].Value().Hostname != "www.google.com" {
@ -89,7 +90,7 @@ func TestResolverSaver(t *testing.T) {
if ev[1].Value().Duration <= 0 {
t.Fatal("unexpected Duration")
}
if ev[1].Value().Err != nil {
if ev[1].Value().Err.IsNotNil() {
t.Fatal("unexpected Err")
}
if ev[1].Value().Hostname != "www.google.com" {
@ -106,7 +107,7 @@ func TestResolverSaver(t *testing.T) {
func TestDNSTransportSaver(t *testing.T) {
t.Run("on failure", func(t *testing.T) {
expected := errors.New("no such host")
expected := netxlite.ErrOODNSNoSuchHost
saver := &Saver{}
txp := saver.WrapDNSTransport(&mocks.DNSTransport{
MockRoundTrip: func(ctx context.Context, query model.DNSQuery) (model.DNSResponse, error) {
@ -154,7 +155,7 @@ func TestDNSTransportSaver(t *testing.T) {
if ev[1].Value().Duration <= 0 {
t.Fatal("unexpected Duration")
}
if !errors.Is(ev[1].Value().Err, expected) {
if ev[1].Value().Err != netxlite.FailureDNSNXDOMAINError {
t.Fatal("unexpected Err")
}
if ev[1].Name() != "dns_round_trip_done" {
@ -219,7 +220,7 @@ func TestDNSTransportSaver(t *testing.T) {
if ev[1].Value().Duration <= 0 {
t.Fatal("unexpected Duration")
}
if ev[1].Value().Err != nil {
if ev[1].Value().Err.IsNotNil() {
t.Fatal("unexpected Err")
}
if ev[1].Name() != "dns_round_trip_done" {

2
internal/tracex/tls.go
View File

@ -59,7 +59,7 @@ func (h *TLSHandshakerSaver) Handshake(
h.Saver.Write(&EventTLSHandshakeDone{&EventValue{
Address: remoteAddr,
Duration: stop.Sub(start),
Err: err,
Err: NewFailureStr(err),
NoTLSVerify: config.InsecureSkipVerify,
Proto: proto,
TLSCipherSuite: netxlite.TLSCipherSuiteString(state.CipherSuite),

4
internal/tracex/tls_test.go
View File

@ -48,7 +48,7 @@ func TestTLSHandshakerSaver(t *testing.T) {
if value.Duration <= 0 {
t.Fatal("expected non-zero duration")
}
if value.Err != nil {
if value.Err.IsNotNil() {
t.Fatal("expected no error here")
}
if value.TLSCipherSuite != "TLS_RSA_WITH_RC4_128_SHA" {
@ -130,7 +130,7 @@ func TestTLSHandshakerSaver(t *testing.T) {
if value.Duration <= 0 {
t.Fatal("expected non-zero duration")
}
if value.Err == nil {
if value.Err.IsNil() {
t.Fatal("expected non-nil error here")
}
if value.TLSCipherSuite != "" {