From 7ee1471785902a847d08a99e546d2bed0609ae84 Mon Sep 17 00:00:00 2001 From: Yingrong Zhao <22300958+VinozzZ@users.noreply.github.com> Date: Thu, 1 Aug 2024 03:49:25 -0400 Subject: [PATCH] instrumentation/net/http/otelhttp: support duplicate both old and new attributes for HTTPClient (#5945) This PR implements the HTTPClient that can duplicate attributes for old semconv v1.20.0 and new semconv v1.26.0. It's part of the migration plan #5332 --------- Co-authored-by: Damien Mathieu <42@dmathieu.com> --- .../net/http/otelhttp/internal/semconv/env.go | 23 ++- .../otelhttp/internal/semconv/httpconv.go | 151 ++++++++++++++++++ .../internal/semconv/httpconv_test.go | 98 ++++++++++++ .../net/http/otelhttp/transport.go | 10 +- 4 files changed, 276 insertions(+), 6 deletions(-) diff --git a/instrumentation/net/http/otelhttp/internal/semconv/env.go b/instrumentation/net/http/otelhttp/internal/semconv/env.go index 9948abe44b8..25970e3e9eb 100644 --- a/instrumentation/net/http/otelhttp/internal/semconv/env.go +++ b/instrumentation/net/http/otelhttp/internal/semconv/env.go @@ -82,23 +82,28 @@ func ServerStatus(code int) (codes.Code, string) { } type HTTPClient struct { - // TODO (#5332): Support for new semantic conventions - // duplicate bool + duplicate bool } func NewHTTPClient() HTTPClient { - // TODO (#5332): Support for new semantic conventions - // env := strings.ToLower(os.Getenv("OTEL_HTTP_CLIENT_COMPATIBILITY_MODE")) - return HTTPClient{} + env := strings.ToLower(os.Getenv("OTEL_HTTP_CLIENT_COMPATIBILITY_MODE")) + return HTTPClient{duplicate: env == "http/dup"} } // RequestTraceAttrs returns attributes for an HTTP request made by a client. func (c HTTPClient) RequestTraceAttrs(req *http.Request) []attribute.KeyValue { + if c.duplicate { + return append(oldHTTPClient{}.RequestTraceAttrs(req), newHTTPClient{}.RequestTraceAttrs(req)...) + } return oldHTTPClient{}.RequestTraceAttrs(req) } // ResponseTraceAttrs returns metric attributes for an HTTP request made by a client. func (c HTTPClient) ResponseTraceAttrs(resp *http.Response) []attribute.KeyValue { + if c.duplicate { + return append(oldHTTPClient{}.ResponseTraceAttrs(resp), newHTTPClient{}.ResponseTraceAttrs(resp)...) + } + return oldHTTPClient{}.ResponseTraceAttrs(resp) } @@ -111,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 90000ad364d..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" @@ -195,3 +198,151 @@ func (n newHTTPServer) ResponseTraceAttrs(resp ResponseTelemetry) []attribute.Ke func (n newHTTPServer) Route(route string) attribute.KeyValue { return semconvNew.HTTPRoute(route) } + +type newHTTPClient struct{} + +// RequestTraceAttrs returns trace attributes for an HTTP request made by a client. +func (n newHTTPClient) RequestTraceAttrs(req *http.Request) []attribute.KeyValue { + /* + below attributes are returned: + - http.request.method + - http.request.method.original + - url.full + - server.address + - server.port + - network.protocol.name + - network.protocol.version + */ + numOfAttributes := 3 // URL, server address, proto, and method. + + var urlHost string + if req.URL != nil { + urlHost = req.URL.Host + } + var requestHost string + var requestPort int + for _, hostport := range []string{urlHost, req.Header.Get("Host")} { + requestHost, requestPort = splitHostPort(hostport) + if requestHost != "" || requestPort > 0 { + break + } + } + + eligiblePort := requiredHTTPPort(req.URL != nil && req.URL.Scheme == "https", requestPort) + if eligiblePort > 0 { + numOfAttributes++ + } + useragent := req.UserAgent() + if useragent != "" { + numOfAttributes++ + } + + protoName, protoVersion := netProtocol(req.Proto) + if protoName != "" && protoName != "http" { + numOfAttributes++ + } + if protoVersion != "" { + numOfAttributes++ + } + + method, originalMethod := n.method(req.Method) + if originalMethod != (attribute.KeyValue{}) { + numOfAttributes++ + } + + attrs := make([]attribute.KeyValue, 0, numOfAttributes) + + attrs = append(attrs, method) + if originalMethod != (attribute.KeyValue{}) { + attrs = append(attrs, originalMethod) + } + + var u string + if req.URL != nil { + // Remove any username/password info that may be in the URL. + userinfo := req.URL.User + req.URL.User = nil + u = req.URL.String() + // Restore any username/password info that was removed. + req.URL.User = userinfo + } + attrs = append(attrs, semconvNew.URLFull(u)) + + attrs = append(attrs, semconvNew.ServerAddress(requestHost)) + if eligiblePort > 0 { + attrs = append(attrs, semconvNew.ServerPort(eligiblePort)) + } + + if protoName != "" && protoName != "http" { + attrs = append(attrs, semconvNew.NetworkProtocolName(protoName)) + } + if protoVersion != "" { + attrs = append(attrs, semconvNew.NetworkProtocolVersion(protoVersion)) + } + + return attrs +} + +// ResponseTraceAttrs returns trace attributes for an HTTP response made by a client. +func (n newHTTPClient) ResponseTraceAttrs(resp *http.Response) []attribute.KeyValue { + /* + below attributes are returned: + - http.response.status_code + - error.type + */ + var count int + if resp.StatusCode > 0 { + count++ + } + + if isErrorStatusCode(resp.StatusCode) { + count++ + } + + attrs := make([]attribute.KeyValue, 0, count) + if resp.StatusCode > 0 { + attrs = append(attrs, semconvNew.HTTPResponseStatusCode(resp.StatusCode)) + } + + if isErrorStatusCode(resp.StatusCode) { + 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{} + } + if attr, ok := methodLookup[method]; ok { + return attr, attribute.KeyValue{} + } + + orig := semconvNew.HTTPRequestMethodOriginal(method) + if attr, ok := methodLookup[strings.ToUpper(method)]; ok { + return attr, orig + } + return semconvNew.HTTPRequestMethodGet, orig +} + +func isErrorStatusCode(code int) bool { + return code >= 400 || code < 100 +} diff --git a/instrumentation/net/http/otelhttp/internal/semconv/httpconv_test.go b/instrumentation/net/http/otelhttp/internal/semconv/httpconv_test.go index 8b0f781597e..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,11 @@ package semconv import ( + "errors" "fmt" "net/http" + "net/http/httptest" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -126,3 +129,98 @@ func TestNewMethod(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 := 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) + assert.ElementsMatch(t, want, got) +} + +func TestClientResponse(t *testing.T) { + 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)}}, + {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 { + got := newHTTPClient{}.ResponseTraceAttrs(&tt.resp) + 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..be17d4c755d 100644 --- a/instrumentation/net/http/otelhttp/transport.go +++ b/instrumentation/net/http/otelhttp/transport.go @@ -163,7 +163,15 @@ 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