implement error.type for failed RoundTrip

open-telemetry · Jul 31, 2024 · d0e2064 · d0e2064
1 parent 07a4a6c
commit d0e2064
Show file tree

Hide file tree

Showing 4 changed files with 105 additions and 15 deletions.
diff --git 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
@@ -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
@@ -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,11 +195,32 @@ 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 {
 		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
@@ -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