-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat(nestjs): Add function-level span decorator to nestjs #12721
Changes from 12 commits
dd2ef7e
7d802e0
326969b
9afd76f
cef0a49
b297c41
2b4c8b1
b344930
dff2bef
f4c9618
059705a
d38d712
30f1dc3
394b324
4d16c2b
6b588cf
322c645
17739bb
e4ffdfd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
import { expect, test } from '@playwright/test'; | ||
import { waitForTransaction } from '@sentry-internal/test-utils'; | ||
|
||
test('Transaction includes span for decorated function', async ({ baseURL }) => { | ||
const transactionEventPromise = waitForTransaction('nestjs', transactionEvent => { | ||
return ( | ||
transactionEvent?.contexts?.trace?.op === 'http.server' && | ||
transactionEvent?.transaction === 'GET /test-span-decorator' | ||
); | ||
}); | ||
|
||
await fetch(`${baseURL}/test-span-decorator`); | ||
|
||
const transactionEvent = await transactionEventPromise; | ||
|
||
expect(transactionEvent.spans).toEqual( | ||
expect.arrayContaining([ | ||
expect.objectContaining({ | ||
span_id: expect.any(String), | ||
trace_id: expect.any(String), | ||
data: expect.any(Object), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. m: Let's assert on the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I explicitly assert the data object now, there is no |
||
description: 'wait', | ||
parent_span_id: expect.any(String), | ||
start_timestamp: expect.any(Number), | ||
status: 'ok', | ||
op: 'wait function', | ||
origin: 'manual', | ||
}), | ||
]), | ||
); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,5 @@ | ||
export * from '@sentry/node'; | ||
|
||
export { init } from './sdk'; | ||
|
||
export * from './span-decorator'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. h: In the index file, let's be specific about what we export. This is to avoid accidentally widening the API surface. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
import { captureException, startSpan } from '@sentry/node'; | ||
|
||
/** | ||
* A decorator usable to wrap arbitrary functions with spans. | ||
*/ | ||
export function SentryTraced(op: string = 'function') { | ||
return function (target: unknown, propertyKey: string, descriptor: PropertyDescriptor) { | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
await startSpan( | ||
{ | ||
op: op, | ||
name: propertyKey, | ||
}, | ||
async () => { | ||
try { | ||
return await originalMethod.apply(this, args); | ||
} catch (e) { | ||
captureException(e); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated |
||
}, | ||
); | ||
}; | ||
return descriptor; | ||
}; | ||
} |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean a second non-async function and then check whether the transaction includes spans for both?
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.
updated