Leave span status unset for 4xx responses on server spans (#17)

* Leave span status unset for 4xx responses on server spans

Per the OTel HTTP semantic conventions, span status for HTTP status
codes in the 4xx range must be left unset for SpanKind.SERVER spans.
Decide the span status primarily from the status code resolved by
echo.ResolveResponseStatus instead of returning Error for any non-nil
handler error (e.g. echo.NewHTTPError(400)).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Remove Echo-specific references from extractor files

Keep extrator.go and extrator_test.go free of Echo-specific mentions
so they can be copied for use outside of Echo. The Echo-specific
scenarios remain covered in otel_test.go.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

Author: suzuki-safie

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