feat(core): Deprecate trace in favor of startSpan
#10012
Conversation
| () => callback(activeSpan), | ||
| () => { | ||
| // Only update the span status if it hasn't been changed yet | ||
| if (activeSpan && (!activeSpan.status || activeSpan.status === 'ok')) { |
There was a problem hiding this comment.
Small adjustment here as well to make sure we don't overwrite the span status if it was set to something else before.
| () => callback(activeSpan), | ||
| () => { | ||
| // Only update the span status if it hasn't been changed yet | ||
| if (activeSpan && (!activeSpan.status || activeSpan.status === 'ok')) { |
There was a problem hiding this comment.
I am not sure if startSpan should concern itself with the span status. I believe the OTEL equivalent primitive also doesn't do this.
There was a problem hiding this comment.
yeah, this is a "feature" that we do that automatically. Otherwise users would need to do that themselves all the time 🤔 no strong feelings, but this is what the startSpan API is supposed to do afaik cc @AbhiPrasad
There was a problem hiding this comment.
given we are also an error monitoring SDK, I think it makes sense to set error status where appropriate, I think this bit of DX is useful for people.
size-limit report 📦
|
| @@ -28,7 +28,7 @@ export function withEdgeWrapping<H extends EdgeRouteHandler>( | |||
| baggage, | |||
| }); | |||
|
|
|||
| return trace( | |||
| return startSpan( | |||
There was a problem hiding this comment.
@lforst can you especially check that the logic for the next js spans is still correct? Just to be sure 😅
There was a problem hiding this comment.
It looks like we didn't wrap this apply call in handleCallbackErrors:
There was a problem hiding this comment.
ah yeah, forgot about this - initially wrote this to just use try-catch but safer to use the util here too 👍
Also add a `handleCallbackErrors` utility to replace this.
|
|
||
| if (isThenable(maybePromiseResult)) { | ||
| // @ts-expect-error - the isThenable check returns the "wrong" type here | ||
| return maybePromiseResult.then( |
There was a problem hiding this comment.
I think the failures might be legit. I don't think there was anything being swallowed. It seems like we're timing out on waiting for elements to appear on screen. Can you verify that you are returning all the values and not stalling anything? Are we returning the response here? https://github.com/getsentry/sentry-javascript/pull/10012/files#diff-6a177c0f974e405d7e55f4d0155181376d92b37749269c0904cdc6e94b17ef84L97
There was a problem hiding this comment.
Seems like an oversight a linter should have caught...
There was a problem hiding this comment.
Re added the lint rule here #10049
Also add a
handleCallbackErrorsutility to replace this.We've been using this in a few places, and since it has a bit of a different usage than
startSpanI had to add a new utility to properly make this work. The upside is that we now can ensure to use the same implementation for this "maybe promise" handling everywhere!