finalizeResponseSpanHook and finalizeNotFoundSpanHook set OK status on non-5xx responses, violating OTel spec #126
Description
Prerequisites
- I have written a descriptive issue title
- I have searched existing issues to ensure the issue has not already been raised
Issue
Problem
finalizeResponseSpanHook (for normal routes) and finalizeNotFoundSpanHook (for 404) unconditionally call setStatus(OK) for all responses with HTTP status codes below 500.
// finalizeResponseSpanHook
if (reply.statusCode < 500) {
span.setStatus({ code: SpanStatusCode.OK, message: 'OK' })
}
// finalizeNotFoundSpanHook
span.setStatus({ code: SpanStatusCode.OK, message: 'OK' })Violation of OTel Spec
Generally, Instrumentation Libraries SHOULD NOT set the status code to Ok, unless explicitly configured to do so. Instrumentation Libraries SHOULD leave the status code as Unset unless there is an error, as described above.
Generally, Only Application Developers and Operators are permitted to set the status to Ok.
Proposed Fix
Remove the setStatus(OK) calls and only set setStatus(ERROR) for 5xx responses.
// finalizeResponseSpanHook
function finalizeResponseSpanHook(request, reply, payload, hookDone) {
const span = request[kRequestSpan]
if (span != null) {
if (reply.statusCode >= 500) {
span.setStatus({ code: SpanStatusCode.ERROR })
}
// Leave 1xx / 2xx / 3xx / 4xx as UNSET
span.setAttributes({ [ATTR_HTTP_RESPONSE_STATUS_CODE]: reply.statusCode })
span.end()
}
request[kRequestSpan] = null
hookDone(null, payload)
}
// finalizeNotFoundSpanHook
instance.addHook('onResponse', function finalizeNotFoundSpanHook(request, reply, hookDone) {
const span = request[kRequestSpan]
if (span != null) {
// Remove setStatus(OK) call, leave status as UNSET
span.setAttributes({ [ATTR_HTTP_RESPONSE_STATUS_CODE]: reply.statusCode })
span.end()
}
request[kRequestSpan] = null
hookDone()
})If this approach looks good, I'd be happy to open a Pull Request. Would that be welcome?