New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(opentelemetry): Do not stomp span status when startSpan
callback throws
#11170
Conversation
// Note: Because of this, we currently have a circular type dependency (which we opted out of in package.json). | ||
// This is not avoidable as we need `spanToJSON` in `spanUtils.ts`, which in turn is needed by `span.ts` for backwards compatibility. | ||
// And `spanToJSON` needs the Span class from `span.ts` to check here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I stumbled onto this but found that this comment had no place in JSDoc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that all fields returned here are optional and need to be guarded against.
I'd argue this might still be a helpful message but the types should hint that anyway.
Also just to double-check: Putting a line comment between the JSDoc block and the function declaration doesn't prevent the JSDoc from being associated with the function, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep the JSdoc still works!
size-limit report 📦
|
I will not merge this yet. I'd like to add tests but I had trouble writing some that verified the behaviour I want here... |
Mergin to unblock #11016 This is implicitly covered by Next.js tests |
This PR fixes span status stomping in the Otel
starSpan
andstartSpanManual
implementations.When the callbacks threw, we didn't check whether there was already a more useful status on there and just stomped it with an
ERROR
status. Now we only set the status toERROR
when it wasn't defined already.