Skip to content

Report the response status of a request that raised #1612

Description

@unflxw

In collector mode, the root span of a request that raised carries no http.response.status_code, while the root span of a successful request carries one. The status is not unknown: the same span reports it as a response_status tag.

A real example, GET /error in a Grape app, reported by main:

Kind: server          Status: UNSET
Tags
  response_status: 500
Span Attributes
  http.request.method: GET
  url.path: /error
  url.scheme: http
  server.address: localhost
  server.port: 4019
  network.protocol.version: 1.1

There is no http.response.status_code. Every 2xx root span in the same run has one.

This is deliberate. Appsignal::Rack::EventHandler#on_finish says so:

# Only a response the app actually produced counts. The 500 below
# stands in for a status that was never sent, so it is reported as a
# tag and a metric but not as this attribute.
transaction.add_opentelemetry_attributes(
  Appsignal::OpenTelemetry::HttpResponse.attributes_for(response&.status)
)

The reasoning is that when the app raises, response is nil and the 500 is our own stand-in rather than a status the app returned.

I think it is worth revisiting, because the client did receive a status. The request above returned 500 to curl. Whoever reads the trace wants to know what the client saw, and http.response.status_code is where they will look for it. Reporting the status as a tag but withholding it from the attribute means the one request where the status matters most is the one where the convention attribute is missing, and a consumer that groups or alerts on http.response.status_code cannot see it.

The counter-argument is that the 500 is an assumption. It comes from APPSIGNAL_EVENT_HANDLER_HAS_ERROR rather than from a response, and an app server with a custom error handler could send something else. If that is the blocker, the value is still knowable in the places that do have a response: Appsignal::Rack::AbstractMiddleware reads a real status, and so does the Webmachine integration through response.code.

Related, and possibly the same fix: the root span's status stays UNSET on an erroring request. OpenTelemetryBackend#set_error sets the status on current_span, which at that point is the innermost open event span, so the error status lands on process_request.rack (callback: on_finish) and not on the transaction's own span. The semantic conventions ask for Error on a server span that returned a 5xx. Ingestion copes and the error incident is created correctly, so this is about what the span itself says rather than about anything being lost.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementAn improvement to an existing feature.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions