Leave span status unset for 4xx responses on server spans

extrator.go

@@ -560,27 +560,39 @@ func SpanNameFormatter(v Values) string {
 	return method
 }
 
-// SpanStatus returns the span status code and error description based on the HTTP status code and error.
+// SpanStatus returns the span status code and error description for a server span, based on the
+// resolved HTTP response status code and the error that occurred while handling the request.
 //
 // Spec:
 //
 //	Span Status MUST be left unset if HTTP status code was in the 1xx, 2xx or 3xx ranges, unless there was another error
 //	 (e.g., network error receiving the response body; or 3xx codes with max redirects exceeded), in which case status
 //	 MUST be set to Error.
 //
+//	For HTTP status codes in the 4xx range span status MUST be left unset in case of SpanKind.SERVER and SHOULD be set
+//	 to Error in case of SpanKind.CLIENT.
+//
 // Reference:
 // - [Span.Status](https://opentelemetry.io/docs/specs/semconv/http/http-spans/#status)
 // - [Recording errors on spans](https://opentelemetry.io/docs/specs/semconv/general/recording-errors/)
 func SpanStatus(code int, err error) (codes.Code, string) {
-	if err != nil { // When the operation ends with an error, instrumentation: SHOULD set the span status code to Error
-		return codes.Error, err.Error()
-	}
-
 	if code < 100 || code >= 600 {
 		return codes.Error, fmt.Sprintf("Invalid HTTP status code %d", code)
 	}
 	if code >= 500 {
+		if err != nil {
+			return codes.Error, err.Error()
+		}
 		return codes.Error, ""
 	}
+	if code >= 400 {
+		// this instrumentation creates server spans, and for those the convention is to leave
+		// the status unset on 4xx responses, even when the error is what the status code was
+		// resolved from.
+		return codes.Unset, ""
+	}
+	if err != nil { // an error alongside a 1xx-3xx status indicates another error occurred
+		return codes.Error, err.Error()
+	}
 	return codes.Unset, ""
 }

extrator_test.go

@@ -701,6 +701,13 @@ func TestSpanStatus(t *testing.T) {
 			expectCode: codes.Error,
 			expectDesc: "network error",
 		},
+		{
+			name:       "error with invalid status code",
+			whenStatus: 0,
+			whenError:  errors.New("network error"),
+			expectCode: codes.Error,
+			expectDesc: "Invalid HTTP status code 0",
+		},
 		{
 			name:       "invalid status code below range",
 			whenStatus: 99,
@@ -729,6 +736,27 @@ func TestSpanStatus(t *testing.T) {
 			expectCode: codes.Error,
 			expectDesc: "",
 		},
+		{
+			name:       "server error 500 with error keeps error description",
+			whenStatus: 500,
+			whenError:  errors.New("something failed"),
+			expectCode: codes.Error,
+			expectDesc: "something failed",
+		},
+		{
+			name:       "client error 404 is left unset for server span",
+			whenStatus: 404,
+			whenError:  nil,
+			expectCode: codes.Unset,
+			expectDesc: "",
+		},
+		{
+			name:       "client error 400 with error is left unset for server span",
+			whenStatus: 400,
+			whenError:  errors.New("invalid request"),
+			expectCode: codes.Unset,
+			expectDesc: "",
+		},
 		{
 			name:       "informational status 100",
 			whenStatus: 100,

otel_test.go

@@ -13,8 +13,8 @@ import (
 	"github.com/stretchr/testify/assert"
 	"go.opentelemetry.io/otel"
 	"go.opentelemetry.io/otel/attribute"
-	"go.opentelemetry.io/otel/metric"
 	"go.opentelemetry.io/otel/codes"
+	"go.opentelemetry.io/otel/metric"
 	"go.opentelemetry.io/otel/propagation"
 	"go.opentelemetry.io/otel/semconv/v1.39.0/httpconv"
 	"go.opentelemetry.io/otel/trace"
@@ -493,6 +493,70 @@ func TestSpanStatusOnHTTP500(t *testing.T) {
 	}
 }
 
+// statusCoderError implements echo.HTTPStatusCoder so echo.ResolveResponseStatus can resolve its status code.
+type statusCoderError struct {
+	code int
+	msg  string
+}
+
+func (e *statusCoderError) Error() string   { return e.msg }
+func (e *statusCoderError) StatusCode() int { return e.code }
+
+func TestSpanStatusOnHTTP4xx(t *testing.T) {
+	tests := []struct {
+		name         string
+		handler      echo.HandlerFunc
+		expectStatus int
+	}{
+		{
+			name: "handler writes 400 status code directly",
+			handler: func(c *echo.Context) error {
+				return c.String(http.StatusBadRequest, "bad request")
+			},
+			expectStatus: http.StatusBadRequest,
+		},
+		{
+			name: "handler returns echo HTTP error with 400",
+			handler: func(c *echo.Context) error {
+				return echo.NewHTTPError(http.StatusBadRequest, "bad request")
+			},
+			expectStatus: http.StatusBadRequest,
+		},
+		{
+			name: "handler returns HTTPStatusCoder error with 422",
+			handler: func(c *echo.Context) error {
+				return &statusCoderError{code: http.StatusUnprocessableEntity, msg: "validation failed"}
+			},
+			expectStatus: http.StatusUnprocessableEntity,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			exporter := tracetest.NewInMemoryExporter()
+			tp := sdktrace.NewTracerProvider(sdktrace.WithSyncer(exporter))
+
+			e := echo.New()
+			e.Use(NewMiddlewareWithConfig(Config{
+				ServerName:     "foobar",
+				TracerProvider: tp,
+			}))
+			e.GET("/error", tt.handler)
+
+			r := httptest.NewRequest(http.MethodGet, "/error", http.NoBody)
+			w := httptest.NewRecorder()
+			e.ServeHTTP(w, r)
+
+			assert.Equal(t, tt.expectStatus, w.Result().StatusCode)
+
+			spans := exporter.GetSpans()
+			assert.Len(t, spans, 1)
+			// per the semantic conventions, span status is left unset for 4xx responses on server spans
+			assert.Equal(t, codes.Unset, spans[0].Status.Code)
+		})
+	}
+}
+
 func TestConfig_OnNextError(t *testing.T) {
 	tests := []struct {
 		name              string