Uh oh!
There was an error while loading. Please reload this page.
feat(js): Document new performance APIs - #7707
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Lms24
left a comment
There was a problem hiding this comment.
LGTM generally, just had some minor comments.
Side note/no action required as too late: I'm only now realizing that startActiveSpan to me conveys that we only start a span but don't finish it after running the callback. Super sorry for not bringing this up earlier! But it's also not the end of the world.
| }); | ||
| const result = await Sentry.startActiveSpan( | ||
| { name: "Important Function" }, |
There was a problem hiding this comment.
l: should we shop op as well here or do we consider this not necessary for custom instrumentation?
There was a problem hiding this comment.
op is optional so I didn't include it. We need to go back and expand documentation on it, but that can happen later on.
| <PlatformContent includePath="performance/add-independent-span" /> | ||
| ## Start Transaction |
There was a problem hiding this comment.
WDYT about
| ## Start Transaction | |
| ## [Deprecated]Start Transaction |
or something similar that discourages folks from usingstartTransaction?
We probably need to make this JS-specific though so maybe throwing in a note/alert might also work
There was a problem hiding this comment.
I don't necessarily want to deprecate this method for now, but once we're more comfortable with the API we can come back and adjust.
| @@ -0,0 +1,5 @@ | |||
| <Note> | |||
| The span APIs require SDK version `7.65.0` or higher. If you are using an older version of the SDK, you can use the [explicit transaction APIs](#start-transaction) for custom instrumentation. | |||
There was a problem hiding this comment.
l: Is there a reason why this note is different to the Node version? I'd prefer the Node version as it explicitly names the APIs.
There was a problem hiding this comment.
nope, I just forgot lol
good call!
AbhiPrasad
commented
Aug 30, 2023
Yeah I thought about this too, but the UX of just not having to think about span finish meant that it kinda just got included with the name. We can alias the name to something else in a future version if users get confused, we have room to move here. |
| { name: "Important Function" }, | ||
| (span) => { | ||
| // Can access the span to add data or set specific status | ||
| span?.setData("foo", "bar"); |
There was a problem hiding this comment.
m: Should we mention/explain when/why span is undefined? May not be entirely clear from the outside why this may be unset and why I need to guard here.
resolvesgetsentry/sentry-javascript#8724
Motivation
Documents the new performance APIs under the custom instrumentation section.
In the future we'll want to revamp both the automatic and custom instrumentation sections, but for now this is a good starting point.
The APIs are detailed in the RFC about the new performance API, and were introduced in getsentry/sentry-javascript#8803
Details
Sentry.startActiveSpanautomatically wraps a callback with a span and makes that span the active span for the execution context of the callback. The callback can be async or sync, and can return arbitrary values.Sentry.startSpanjust creates a span, but does not wrap it in a callback. It needs to be explicitly set on the scope just likeSentry.startTransaction.Under the hood these methods will create a transaction or span depending on if there is already an active span on the scope.