Prerequisites
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
Trace API: Set Status
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?
Prerequisites
Issue
Problem
finalizeResponseSpanHook(for normal routes) andfinalizeNotFoundSpanHook(for 404) unconditionally callsetStatus(OK)for all responses with HTTP status codes below 500.Violation of OTel Spec
Trace API: Set Status
Generally, Only Application Developers and Operators are permitted to set the status to
Ok.Proposed Fix
Remove the
setStatus(OK)calls and only setsetStatus(ERROR)for 5xx responses.If this approach looks good, I'd be happy to open a Pull Request. Would that be welcome?