From b68c918df238e9f512048dca2e1eb9e4c03675db Mon Sep 17 00:00:00 2001 From: Atsushi Suzuki Date: Tue, 7 Jul 2026 21:08:37 +0900 Subject: [PATCH 1/2] 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 --- extrator.go | 22 ++++++++++++---- extrator_test.go | 29 +++++++++++++++++++++ otel_test.go | 66 +++++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 111 insertions(+), 6 deletions(-) diff --git a/extrator.go b/extrator.go index 166d5d6..8c6f0a0 100644 --- a/extrator.go +++ b/extrator.go @@ -560,7 +560,8 @@ 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 status code (see [echo.ResolveResponseStatus]) and the error returned from the handler. // // Spec: // @@ -568,19 +569,30 @@ func SpanNameFormatter(v Values) string { // (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 handler returned an error that + // resolves to 4xx (e.g. echo.NewHTTPError(400)). + 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, "" } diff --git a/extrator_test.go b/extrator_test.go index 7b531be..9e2463b 100644 --- a/extrator_test.go +++ b/extrator_test.go @@ -6,6 +6,7 @@ import ( "net/http" "testing" + "github.com/labstack/echo/v5" "github.com/stretchr/testify/assert" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/codes" @@ -701,6 +702,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 +737,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 handler error is left unset for server span", + whenStatus: 400, + whenError: echo.NewHTTPError(http.StatusBadRequest, "invalid request"), + expectCode: codes.Unset, + expectDesc: "", + }, { name: "informational status 100", whenStatus: 100, diff --git a/otel_test.go b/otel_test.go index d021ec2..b4ccb73 100644 --- a/otel_test.go +++ b/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 From df60b585a6aafc58df58bc11e9e622ddf2cfb578 Mon Sep 17 00:00:00 2001 From: Atsushi Suzuki Date: Wed, 8 Jul 2026 15:41:37 +0900 Subject: [PATCH 2/2] 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 --- extrator.go | 6 +++--- extrator_test.go | 5 ++--- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/extrator.go b/extrator.go index 8c6f0a0..bd10bad 100644 --- a/extrator.go +++ b/extrator.go @@ -561,7 +561,7 @@ func SpanNameFormatter(v Values) string { } // SpanStatus returns the span status code and error description for a server span, based on the -// resolved HTTP status code (see [echo.ResolveResponseStatus]) and the error returned from the handler. +// resolved HTTP response status code and the error that occurred while handling the request. // // Spec: // @@ -587,8 +587,8 @@ func SpanStatus(code int, err error) (codes.Code, string) { } 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 handler returned an error that - // resolves to 4xx (e.g. echo.NewHTTPError(400)). + // 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 diff --git a/extrator_test.go b/extrator_test.go index 9e2463b..4dc050f 100644 --- a/extrator_test.go +++ b/extrator_test.go @@ -6,7 +6,6 @@ import ( "net/http" "testing" - "github.com/labstack/echo/v5" "github.com/stretchr/testify/assert" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/codes" @@ -752,9 +751,9 @@ func TestSpanStatus(t *testing.T) { expectDesc: "", }, { - name: "client error 400 with handler error is left unset for server span", + name: "client error 400 with error is left unset for server span", whenStatus: 400, - whenError: echo.NewHTTPError(http.StatusBadRequest, "invalid request"), + whenError: errors.New("invalid request"), expectCode: codes.Unset, expectDesc: "", },