[BUG] fix: set HTTP span status code for 4xx/5xx REST responses (#22663)

[waterWang]

Description

Fixes #22663 — HTTP-layer SERVER/CLIENT tracing spans remain status.code = 0 (UNSET) for 4xx/5xx REST responses, making it impossible for observability backends to detect failed requests from the root span.

Root cause

TraceableRestChannel.sendResponse() and TraceableStreamingRestChannel.prepareResponse() did not check the HTTP response status code before ending the span. The http.status_code attribute was never set, and the span status was never set to ERROR for failed requests.

Changes

1. Span.java — Added setError(String errorMessage) method to the interface for recording error status without requiring an Exception object.

2. NoopSpan.java — No-op implementation of the new setError(String) method.

3. OTelSpan.java — Implementation delegates to delegateSpan.setStatus(StatusCode.ERROR, errorMessage).

4. AttributeNames.java — Added HTTP_STATUS_CODE = "http.status_code" constant.

5. TraceableRestChannel.java — In sendResponse(), adds http.status_code attribute and calls setError("HTTP <code>") when status >= 500.

6. TraceableStreamingRestChannel.java — In prepareResponse(), adds http.status_code attribute and calls setError("HTTP <code>") when status >= 500.

Testing

• Verified that http.status_code attribute is set on every REST response span
• Verified that status.code = ERROR (2) is set for HTTP 5xx responses
• Non-streaming path: sendResponse() in TraceableRestChannel
• Streaming path: prepareResponse() in TraceableStreamingRestChannel
• Existing tests continue to pass (no behavioral change for successful responses)

Related

• OTel HTTP semantic conventions: https://opentelemetry.io/docs/specs/semconv/http/http-spans/
• Per OTel spec, server span status should be ERROR for HTTP 5xx responses

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 No relevant tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Missing 4xx error handling The PR description states the intent is to flag both 4xx and 5xx as errors, but the implementation only calls setError when statusCode >= 500. 4xx client errors (e.g., 400, 404, 500-independent failures) will still leave the span status as UNSET. Per OTel HTTP semantic conventions, SERVER spans should be ERROR for 5xx, but if the intent stated in the PR title/description ("4xx/5xx") is to cover 4xx as well, this branch is incorrect. Please align implementation with intent. int statusCode = response.status().getStatus(); span.addAttribute(AttributeNames.HTTP_STATUS_CODE, (long) statusCode); if (statusCode >= 500) { span.setError("HTTP " + statusCode); } Attribute may not be recorded on failure addAttribute and setError are executed before delegate.prepareResponse(...) inside the try-with-resources, but there is no finally block. If delegate.prepareResponse throws, the attribute/error is set but the span is not ended here (span ends in sendResponse/sendChunk elsewhere), which may be acceptable — however, note that in the non-streaming counterpart the attribute is set in finally, while here it is set before the delegate call. Consider moving these calls to after the delegate to keep symmetry, or into a try/finally, to avoid recording attributes for a response that failed to be prepared. try (SpanScope ignored = tracer.withSpanInScope(span)) { int statusCode = status.getStatus(); span.addAttribute(AttributeNames.HTTP_STATUS_CODE, (long) statusCode); if (statusCode >= 500) { span.setError("HTTP " + statusCode); } delegate.prepareResponse(status, headers);

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Guard against null response in finally If response or response.status() is null, the finally block will throw an NPE, which will mask the original exception from delegate.sendResponse(response) and prevent span.endSpan() from being called. Guard the attribute/error recording with a null check to ensure the span is always ended. server/src/main/java/org/opensearch/telemetry/tracing/channels/TraceableRestChannel.java [109-119] try (SpanScope scope = tracer.withSpanInScope(span)) { delegate.sendResponse(response); } finally { - int statusCode = response.status().getStatus(); - span.addAttribute(AttributeNames.HTTP_STATUS_CODE, (long) statusCode); - if (statusCode >= 500) { - span.setError("HTTP " + statusCode); + if (response != null && response.status() != null) { + int statusCode = response.status().getStatus(); + span.addAttribute(AttributeNames.HTTP_STATUS_CODE, (long) statusCode); + if (statusCode >= 500) { + span.setError("HTTP " + statusCode); + } } span.endSpan(); } Suggestion importance[1-10]: 5 __ Why: A valid defensive coding suggestion; an NPE in the finally block could mask original exceptions and prevent span.endSpan() from being called, though in practice response is unlikely to be null.	Low
General	Mark 4xx responses as errors too The PR title mentions setting status for 4xx/5xx responses, but the code only sets error status for 5xx. Consider also marking 4xx client errors as errors, or at least align the implementation with the stated intent to avoid missing error tracking for client-side failures. server/src/main/java/org/opensearch/telemetry/tracing/channels/TraceableRestChannel.java [113-118] int statusCode = response.status().getStatus(); span.addAttribute(AttributeNames.HTTP_STATUS_CODE, (long) statusCode); -if (statusCode >= 500) { +if (statusCode >= 400) { span.setError("HTTP " + statusCode); } span.endSpan(); Suggestion importance[1-10]: 3 __ Why: This is a debatable design choice; OpenTelemetry semantic conventions typically only mark 5xx as errors on the server side, so the current 5xx-only behavior is reasonable. The PR title mentioning 4xx/5xx is a weak justification.	Low

[reta]

Let's reuse the new method here
:

Suggested change

	setError(exception.getMessage());

[reta]

Why only 5xx?

Suggested change

	if (statusCode >= 500) {
	if (statusCode >= 400) {

[reta]

Just a suggestion, introduce reportError method to TraceableRestChannel and reuse in TraceableStreamingRestChannel (instead of duplicating same code), since TraceableStreamingRestChannel extends TraceableRestChannel

[reta]

Thanks @waterWang, could you please fix DCO check?