diff --git a/extrator.go b/extrator.go index 166d5d6..bd10bad 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 response status code and the error that occurred while handling the request. // // 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 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, "" } diff --git a/extrator_test.go b/extrator_test.go index 7b531be..4dc050f 100644 --- a/extrator_test.go +++ b/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, 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