fix(opentelemetry): Do not stomp span error status #11169
Conversation
size-limit report 📦
|
| @@ -10,6 +10,7 @@ export const SPAN_STATUS_ERROR = 2; | |||
| * @param httpStatus The HTTP response status code. | |||
| * @returns The span status or unknown_error. | |||
| */ | |||
| // https://develop.sentry.dev/sdk/event-payloads/span/ | |||
There was a problem hiding this comment.
l (feel free to ignore): Any reason for not adding this link directly to the JSDoc header?
There was a problem hiding this comment.
I am a bit cautious with links in js doc bc they might become stale. If it's just in our codebase it's not that bad but as a user it's frustrating if links are dead.
There was a problem hiding this comment.
Ok fair point, in this case I think it's totally fine. In other cases I think it's still worth to take that risk but admittedly, I didn't think too much about it until now 😅
| const statusCode = status && status.code; | ||
| if (statusCode === SpanStatusCode.OK || statusCode === SpanStatusCode.UNSET) { | ||
| return { code: SPAN_STATUS_OK }; | ||
| if (typeof grpcCodeAttribute === 'string') { |
There was a problem hiding this comment.
m: can grpcCodeAttribute be of any other type than string like httpCodeAttribute?
There was a problem hiding this comment.
I opened an issue to track this, as I didn't touch that particular logic: #11195
It may very well be that this is a leftover from our first initial OTEL implementation draft.
This fixes a bug I noticed when attempting to port the Next.js SDK to OTEL.
The behaviour of
mapStatus()(which is called when we serialize spans & transactions to set the span and transaction status) previously always looked at the HTTP and GRPC status attributes first when determining the status of a span, stomping any previously set status in the process. Additionally, and arguably worse, when there was no HTTP and GRPC status attribute on the span, and the span status wasERROR, we completely stomped the error message withunknown_error.This PR changes the implementation so that we first prefer the status code that is already set on the span when it is
OKandERROR, otherwise we try to infer the span status from the HTTP and GRPC codes, we default to setting the span status toOKwhen the otel span status isUNSET, and we default toERRORwhen we see an unknown otel span status.