Leave span status unset for 4xx responses on server spans

[suzuki-safie]

Summary

Thank you for maintaining this middleware!

The middleware currently sets the span status to Error whenever the handler (or a downstream middleware) returns a non-nil error — even when that error resolves to a 4xx response, e.g. echo.NewHTTPError(http.StatusBadRequest, ...) or any error implementing echo.HTTPStatusCoder.

Per the OpenTelemetry HTTP semantic conventions (HTTP spans / Status):

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.

The middleware creates spans with trace.SpanKindServer, so this rule applies to the spans it produces. Returning an *echo.HTTPError is the idiomatic way for an Echo handler to produce a 4xx response, so in practice 400/404/422 responses produced this way are currently reported as errored spans, which makes it harder to distinguish server-side failures (5xx) from ordinary client errors (4xx) in monitoring backends.

Root cause

In otel.go the resolved response status is passed to SpanStatus:

resp, status := echo.ResolveResponseStatus(c.Response(), err)
span.SetStatus(SpanStatus(status, err))

echo-opentelemetry/otel.go

Lines 223 to 224 in 41c8bfd

	resp, status := echo.ResolveResponseStatus(c.Response(), err)
	span.SetStatus(SpanStatus(status, err))

echo.ResolveResponseStatus correctly resolves HTTPError(400) (and any HTTPStatusCoder error) to status = 400. However, SpanStatus returns early when err != nil, so the resolved status code is not taken into account:

func SpanStatus(code int, err error) (codes.Code, string) {
	if err != nil { // returns Error regardless of the resolved code
		return codes.Error, err.Error()
	}
	...
}

echo-opentelemetry/extrator.go

Lines 574 to 577 in 41c8bfd

	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()
	}

Note that the same resolved status is recorded as the http.response.status_code span attribute, so the emitted span ends up a little inconsistent: the attribute reports 400 while the span status reports Error.

Reproduction

func TestSpanStatusOn4xxHTTPError(t *testing.T) {
	recorder := tracetest.NewSpanRecorder()
	provider := sdktrace.NewTracerProvider(sdktrace.WithSpanProcessor(recorder))

	e := echo.New()
	e.Use(NewMiddlewareWithConfig(Config{TracerProvider: provider}))
	e.GET("/test", func(c *echo.Context) error {
		return echo.NewHTTPError(http.StatusBadRequest, "invalid request")
	})

	w := httptest.NewRecorder()
	e.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/test", http.NoBody))

	assert.Equal(t, http.StatusBadRequest, w.Result().StatusCode)
	spans := recorder.Ended()
	assert.Len(t, spans, 1)
	// FAILS on main: got codes.Error (description "code=400, message=invalid request")
	assert.Equal(t, codes.Unset, spans[0].Status().Code)
}

Observed behavior on main (41c8bfd):

Handler	HTTP response	Span status (expected per spec)	Span status (actual)
writes 200, returns nil	200	Unset	Unset ✅
writes 400 directly, returns nil	400	Unset	Unset ✅
returns echo.NewHTTPError(400)	400	Unset	Error ❌
returns custom HTTPStatusCoder error (422)	422	Unset	Error ❌
returns echo.NewHTTPError(500)	500	Error	Error ✅
returns plain error (resolved to 500)	500	Error	Error ✅

Changes

• SpanStatus now bases the decision primarily on the resolved status code, and uses the error mainly for the status description:
  • 5xx → Error (description taken from err when present)
  • 4xx → Unset (server spans; the error is still returned to the caller and handled by Echo as before)
  • 1xx–3xx with a non-nil err → Error ("unless there was another error" clause of the spec, e.g. an error returned after a 2xx response was already committed)
  • invalid status code → Error (unchanged)
• Added unit test cases for SpanStatus (4xx with *HTTPError / HTTPStatusCoder errors) and a middleware-level test asserting the span status for a handler returning echo.NewHTTPError(400).
• Updated the doc comment on SpanStatus to quote the SpanKind.SERVER 4xx rule.

Existing tests are unaffected: TestSpanStatus's "error overrides status code" case (200 + error → Error) still holds under the new logic, and TestSpanStatusOnHTTP500 keeps asserting Error for 5xx.

Related issues

• This complements span error based on http status #11 / Set span status to Error when http status is >= 500 #12 / Improve span status code handling and docs #13: those addressed the err == nil path (a 5xx status written directly by the handler is now reported as Error based on the resolved status code). This PR applies the same status-code-based decision to the err != nil path.
• otel.go also sets the error.type attribute (semconv.ErrorType(err)) whenever err != nil. Per the conventions this attribute is only expected when the request actually ended with an error (which for server spans excludes 4xx), and its value has its own requirements (e.g. the status code number as a string). Since http span and error.type #10 already tracks error.type handling, I left that attribute untouched here to keep this PR focused on the span status — happy to help with http span and error.type #10 in a follow-up if useful.

Please let me know if you'd like any changes to the approach or the tests — happy to update the PR.

🤖 Generated with Claude Code

[aldas]

Looks good, but please remove refences to Echo specifics in extractor file. I would like it to be as general as can be so other people can use copy if needed for things other than Echo related.

[suzuki-safie]

@aldas
Thank you for the review! I've removed the Echo-specific references from the extractor files.

The Echo-specific scenarios (a handler returning echo.NewHTTPError(400) or an HTTPStatusCoder error) are still covered in otel_test.go.

[aldas]

LGTM

[aldas]

I'll do a next release in couple of days. you can try/use it already if you update dependency to specific commit

[aldas]

this is now in lastest release.