From d0e20646c1d7b3d84c6297c0cb706c32b78a2732 Mon Sep 17 00:00:00 2001 From: Yingrong Zhao <22300958+VinozzZ@users.noreply.github.com> Date: Wed, 31 Jul 2024 11:34:25 -0400 Subject: [PATCH] implement error.type for failed RoundTrip --- .../net/http/otelhttp/internal/semconv/env.go | 8 +++ .../otelhttp/internal/semconv/httpconv.go | 31 +++++--- .../internal/semconv/httpconv_test.go | 70 +++++++++++++++++-- .../net/http/otelhttp/transport.go | 11 ++- 4 files changed, 105 insertions(+), 15 deletions(-) diff --git a/instrumentation/net/http/otelhttp/internal/semconv/env.go b/instrumentation/net/http/otelhttp/internal/semconv/env.go index 077de04f157..25970e3e9eb 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/env.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/env.go @@ -116,3 +116,11 @@ func (c HTTPClient) Status(code int) (codes.Code, string) { } return codes.Unset, "" } + +func (c HTTPClient) ErrorType(err error) attribute.KeyValue { + if c.duplicate { + return newHTTPClient{}.ErrorType(err) + } + + return attribute.KeyValue{} +} diff --git a/instrumentation/net/http/otelhttp/internal/semconv/httpconv.go b/instrumentation/net/http/otelhttp/internal/semconv/httpconv.go index b0cec2e6f71..745b8c67bc4 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/httpconv.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/httpconv.go @@ -4,7 +4,10 @@ package semconv // import "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/internal/semconv" import ( + "fmt" "net/http" + "reflect" + "strconv" "strings" "go.opentelemetry.io/otel/attribute" @@ -207,7 +210,6 @@ func (n newHTTPClient) RequestTraceAttrs(req *http.Request) []attribute.KeyValue - url.full - server.address - server.port - - user.agent.original - network.protocol.name - network.protocol.version */ @@ -271,11 +273,6 @@ func (n newHTTPClient) RequestTraceAttrs(req *http.Request) []attribute.KeyValue attrs = append(attrs, semconvNew.ServerPort(eligiblePort)) } - // TODO: should we emit this attribute if it's opt-in only? - if useragent != "" { - attrs = append(attrs, semconvNew.UserAgentOriginal(useragent)) - } - if protoName != "" && protoName != "http" { attrs = append(attrs, semconvNew.NetworkProtocolName(protoName)) } @@ -308,15 +305,29 @@ func (n newHTTPClient) ResponseTraceAttrs(resp *http.Response) []attribute.KeyVa } if isErrorStatusCode(resp.StatusCode) { - errorType := http.StatusText(resp.StatusCode) - if errorType == "" { - errorType = semconvNew.ErrorTypeOther.Value.AsString() - } + errorType := strconv.Itoa(resp.StatusCode) attrs = append(attrs, semconvNew.ErrorTypeKey.String(errorType)) } return attrs } +func (n newHTTPClient) ErrorType(err error) attribute.KeyValue { + t := reflect.TypeOf(err) + var value string + if t.PkgPath() == "" && t.Name() == "" { + // Likely a builtin type. + value = t.String() + } else { + value = fmt.Sprintf("%s.%s", t.PkgPath(), t.Name()) + } + + if value == "" { + return semconvNew.ErrorTypeOther + } + + return semconvNew.ErrorTypeKey.String(value) +} + func (n newHTTPClient) method(method string) (attribute.KeyValue, attribute.KeyValue) { if method == "" { return semconvNew.HTTPRequestMethodGet, attribute.KeyValue{} diff --git a/instrumentation/net/http/otelhttp/internal/semconv/httpconv_test.go b/instrumentation/net/http/otelhttp/internal/semconv/httpconv_test.go index f1b955d329a..49b70a04535 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/httpconv_test.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/httpconv_test.go @@ -4,8 +4,10 @@ package semconv import ( + "errors" "fmt" "net/http" + "net/http/httptest" "strings" "testing" @@ -128,20 +130,59 @@ func TestNewMethod(t *testing.T) { } } -func TestClientRequest(t *testing.T) { +func TestNewTraceRequest_Client(t *testing.T) { + t.Setenv("OTEL_HTTP_CLIENT_COMPATIBILITY_MODE", "http/dup") body := strings.NewReader("Hello, world!") url := "https://example.com:8888/foo/bar?stuff=morestuff" - req, err := http.NewRequest("pOST", url, body) - assert.NoError(t, err) + req := httptest.NewRequest("pOST", url, body) req.Header.Set("User-Agent", "go-test-agent") want := []attribute.KeyValue{ attribute.String("http.request.method", "POST"), attribute.String("http.request.method_original", "pOST"), + attribute.String("http.method", "pOST"), attribute.String("url.full", url), + attribute.String("http.url", url), attribute.String("server.address", "example.com"), attribute.Int("server.port", 8888), + attribute.String("network.protocol.version", "1.1"), + attribute.String("net.peer.name", "example.com"), + attribute.Int("net.peer.port", 8888), attribute.String("user_agent.original", "go-test-agent"), + attribute.Int("http.request_content_length", 13), + } + client := NewHTTPClient() + assert.ElementsMatch(t, want, client.RequestTraceAttrs(req)) +} + +func TestNewTraceResponse_Client(t *testing.T) { + t.Setenv("OTEL_HTTP_CLIENT_COMPATIBILITY_MODE", "http/dup") + testcases := []struct { + resp http.Response + want []attribute.KeyValue + }{ + {resp: http.Response{StatusCode: 200, ContentLength: 123}, want: []attribute.KeyValue{attribute.Int("http.response.status_code", 200), attribute.Int("http.status_code", 200), attribute.Int("http.response_content_length", 123)}}, + {resp: http.Response{StatusCode: 404, ContentLength: 0}, want: []attribute.KeyValue{attribute.Int("http.response.status_code", 404), attribute.Int("http.status_code", 404), attribute.String("error.type", "404")}}, + } + + for _, tt := range testcases { + client := NewHTTPClient() + assert.ElementsMatch(t, tt.want, client.ResponseTraceAttrs(&tt.resp)) + } +} + +func TestClientRequest(t *testing.T) { + body := strings.NewReader("Hello, world!") + url := "https://example.com:8888/foo/bar?stuff=morestuff" + req := httptest.NewRequest("pOST", url, body) + req.Header.Set("User-Agent", "go-test-agent") + + want := []attribute.KeyValue{ + attribute.String("http.request.method", "POST"), + attribute.String("http.request.method_original", "pOST"), + attribute.String("url.full", url), + attribute.String("server.address", "example.com"), + attribute.Int("server.port", 8888), attribute.String("network.protocol.version", "1.1"), } got := newHTTPClient{}.RequestTraceAttrs(req) @@ -154,7 +195,7 @@ func TestClientResponse(t *testing.T) { want []attribute.KeyValue }{ {resp: http.Response{StatusCode: 200, ContentLength: 123}, want: []attribute.KeyValue{attribute.Int("http.response.status_code", 200)}}, - {resp: http.Response{StatusCode: 404, ContentLength: 0}, want: []attribute.KeyValue{attribute.Int("http.response.status_code", 404), attribute.String("error.type", http.StatusText(404))}}, + {resp: http.Response{StatusCode: 404, ContentLength: 0}, want: []attribute.KeyValue{attribute.Int("http.response.status_code", 404), attribute.String("error.type", "404")}}, } for _, tt := range testcases { @@ -162,3 +203,24 @@ func TestClientResponse(t *testing.T) { assert.ElementsMatch(t, tt.want, got) } } + +func TestRequestErrorType(t *testing.T) { + testcases := []struct { + err error + want attribute.KeyValue + }{ + {err: errors.New("http: nil Request.URL"), want: attribute.String("error.type", "*errors.errorString")}, + {err: customError{}, want: attribute.String("error.type", "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/internal/semconv.customError")}, + } + + for _, tt := range testcases { + got := newHTTPClient{}.ErrorType(tt.err) + assert.Equal(t, tt.want, got) + } +} + +type customError struct{} + +func (customError) Error() string { + return "custom error" +} diff --git a/instrumentation/net/http/otelhttp/transport.go b/instrumentation/net/http/otelhttp/transport.go index 06948c11cb1..5ee65a65496 100644 --- a/instrumentation/net/http/otelhttp/transport.go +++ b/instrumentation/net/http/otelhttp/transport.go @@ -163,7 +163,16 @@ func (t *Transport) RoundTrip(r *http.Request) (*http.Response, error) { res, err := t.rt.RoundTrip(r) if err != nil { - span.RecordError(err) + + // set error type attribute if the error is part of the predefined + // error types. + // otherwise, record it as an exception + if errType := t.semconv.ErrorType(err); errType.Valid() { + span.SetAttributes(errType) + } else { + span.RecordError(err) + } + span.SetStatus(codes.Error, err.Error()) span.End() return res, err