feat(nestjs): Add function-level span decorator to nestjs #12721
Conversation
packages/nestjs/README.md
Outdated
| @@ -38,6 +38,23 @@ Sentry.init({ | |||
|
|
|||
| Note that it is necessary to initialize Sentry **before you import any package that may be instrumented by us**. | |||
|
|
|||
| ## Span Decorator | |||
|
|
|||
| Use the @GetSentrySpan() decorator to gain additional performance insights for any function within your NestJS application. | |||
There was a problem hiding this comment.
l: Is GetX a pattern that is common for nest? To me it reads a bit weird, something like WrapWithSentrySpan or something along these lines would feel more explanatory to me, but no strong feelings!
There was a problem hiding this comment.
No, GetX is not super common in nest was just an initial idea. I like SentryTraced if everyone is happy with that
There was a problem hiding this comment.
Updated to SentryTraced
| try { | ||
| return await originalMethod.apply(this, args); | ||
| } catch (e) { | ||
| captureException(e); | ||
| } |
There was a problem hiding this comment.
h: We do not want to wrap this with a try-catch-captureException. We only want to capture errors when they bubble up to an unexpected place.
packages/nestjs/src/index.ts
Outdated
| @@ -1,3 +1,5 @@ | |||
| export * from '@sentry/node'; | |||
|
|
|||
| export { init } from './sdk'; | |||
|
|
|||
| export * from './span-decorator'; | |||
There was a problem hiding this comment.
h: In the index file, let's be specific about what we export. This is to avoid accidentally widening the API surface.
| const originalMethod = descriptor.value as (...args: any[]) => Promise<any>; | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| descriptor.value = async function (...args: any[]) { |
There was a problem hiding this comment.
h: We should not modify the return value. Currently we are wrapping all non-promise return values in a Promise. To fix this we can just remove all async and await keywords and everything will work perfectly fine.
| expect.objectContaining({ | ||
| span_id: expect.any(String), | ||
| trace_id: expect.any(String), | ||
| data: expect.any(Object), |
There was a problem hiding this comment.
m: Let's assert on the sentry.origin and sentry.source attibutes in the data object.
There was a problem hiding this comment.
I explicitly assert the data object now, there is no sentry.source property though
| @SentryTraced('wait function') | ||
| async wait() { | ||
| return new Promise(resolve => setTimeout(resolve, 500)); | ||
| } |
There was a problem hiding this comment.
l: We could add a second method that is not async and assert on it in the test.
There was a problem hiding this comment.
Do you mean a second non-async function and then check whether the transaction includes spans for both?
Adds a decorator @SentryTraced() to the nestjs sdk that can be used on functions to wrap them in a span.
Manual test from my sample application see screenshot below.