From 5a64fb9ae86b59ea11b5b8458106d1526f8e3bde Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 2 Jan 2024 14:53:17 +0100 Subject: [PATCH 1/3] feat(core): Deprecate `trace` in favor of `startSpan` Also add a `handleCallbackErrors` utility to replace this. --- packages/astro/src/index.server.ts | 1 + packages/browser/src/index.ts | 1 + packages/bun/src/index.ts | 1 + packages/core/src/index.ts | 1 + packages/core/src/tracing/index.ts | 1 + packages/core/src/tracing/trace.ts | 123 ++++---------- .../core/src/utils/handleCallbackErrors.ts | 63 ++++++++ .../lib/utils/handleCallbackErrors.test.ts | 150 ++++++++++++++++++ packages/deno/src/index.ts | 1 + .../src/common/utils/edgeWrapperUtils.ts | 31 ++-- .../common/withServerActionInstrumentation.ts | 12 +- .../wrapGenerationFunctionWithSentry.ts | 28 ++-- .../src/common/wrapRouteHandlerWithSentry.ts | 40 +++-- .../common/wrapServerComponentWithSentry.ts | 64 +++++--- .../nextjs/test/edge/edgeWrapperUtils.test.ts | 7 +- .../nextjs/test/edge/withSentryAPI.test.ts | 12 +- packages/node-experimental/src/index.ts | 1 + packages/node/src/index.ts | 1 + packages/opentelemetry/src/trace.ts | 64 ++------ packages/remix/src/index.server.ts | 1 + .../src/gcpfunction/cloud_events.ts | 23 +-- packages/serverless/src/gcpfunction/events.ts | 23 +-- packages/serverless/src/gcpfunction/http.ts | 24 +-- packages/sveltekit/src/client/load.ts | 7 +- packages/sveltekit/src/server/index.ts | 1 + packages/sveltekit/test/client/load.test.ts | 22 ++- .../src/node/integrations/prisma.ts | 4 +- .../test/integrations/node/prisma.test.ts | 18 +-- packages/vercel-edge/src/index.ts | 1 + 29 files changed, 429 insertions(+), 297 deletions(-) create mode 100644 packages/core/src/utils/handleCallbackErrors.ts create mode 100644 packages/core/test/lib/utils/handleCallbackErrors.test.ts diff --git a/packages/astro/src/index.server.ts b/packages/astro/src/index.server.ts index 73dce6f0fa7f..d06c1140d41b 100644 --- a/packages/astro/src/index.server.ts +++ b/packages/astro/src/index.server.ts @@ -41,6 +41,7 @@ export { setTags, setUser, spanStatusfromHttpCode, + // eslint-disable-next-line deprecation/deprecation trace, withScope, autoDiscoverNodePerformanceMonitoringIntegrations, diff --git a/packages/browser/src/index.ts b/packages/browser/src/index.ts index e357c05f14f1..d9afa470b2ba 100644 --- a/packages/browser/src/index.ts +++ b/packages/browser/src/index.ts @@ -48,6 +48,7 @@ export { extractTraceparentData, getActiveTransaction, spanStatusfromHttpCode, + // eslint-disable-next-line deprecation/deprecation trace, makeMultiplexedTransport, ModuleMetadata, diff --git a/packages/bun/src/index.ts b/packages/bun/src/index.ts index bc29dcd908b5..7ac0c9e0037e 100644 --- a/packages/bun/src/index.ts +++ b/packages/bun/src/index.ts @@ -60,6 +60,7 @@ export { setTags, setUser, spanStatusfromHttpCode, + // eslint-disable-next-line deprecation/deprecation trace, withScope, captureCheckIn, diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 385352470b34..bc49c8802dad 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -69,6 +69,7 @@ export { prepareEvent } from './utils/prepareEvent'; export { createCheckInEnvelope } from './checkin'; export { hasTracingEnabled } from './utils/hasTracingEnabled'; export { isSentryRequestUrl } from './utils/isSentryRequestUrl'; +export { handleCallbackErrors } from './utils/handleCallbackErrors'; export { spanToTraceHeader } from './utils/spanUtils'; export { DEFAULT_ENVIRONMENT } from './constants'; export { ModuleMetadata } from './integrations/metadata'; diff --git a/packages/core/src/tracing/index.ts b/packages/core/src/tracing/index.ts index fcb208a797aa..759a42cbdfe0 100644 --- a/packages/core/src/tracing/index.ts +++ b/packages/core/src/tracing/index.ts @@ -9,6 +9,7 @@ export { extractTraceparentData, getActiveTransaction } from './utils'; export { SpanStatus } from './spanstatus'; export type { SpanStatusType } from './span'; export { + // eslint-disable-next-line deprecation/deprecation trace, getActiveSpan, startSpan, diff --git a/packages/core/src/tracing/trace.ts b/packages/core/src/tracing/trace.ts index 6ff670c8cf7f..b5d49f4767a7 100644 --- a/packages/core/src/tracing/trace.ts +++ b/packages/core/src/tracing/trace.ts @@ -1,10 +1,11 @@ import type { Span, TransactionContext } from '@sentry/types'; -import { dropUndefinedKeys, isThenable, logger, tracingContextFromHeaders } from '@sentry/utils'; +import { dropUndefinedKeys, logger, tracingContextFromHeaders } from '@sentry/utils'; import { DEBUG_BUILD } from '../debug-build'; import { getCurrentScope, withScope } from '../exports'; import type { Hub } from '../hub'; import { getCurrentHub } from '../hub'; +import { handleCallbackErrors } from '../utils/handleCallbackErrors'; import { hasTracingEnabled } from '../utils/hasTracingEnabled'; /** @@ -18,6 +19,8 @@ import { hasTracingEnabled } from '../utils/hasTracingEnabled'; * * @internal * @private + * + * @deprecated Use `startSpan` instead. */ export function trace( context: TransactionContext, @@ -37,43 +40,18 @@ export function trace( scope.setSpan(activeSpan); - function finishAndSetSpan(): void { - activeSpan && activeSpan.end(); - scope.setSpan(parentSpan); - } - - let maybePromiseResult: T; - try { - maybePromiseResult = callback(activeSpan); - } catch (e) { - activeSpan && activeSpan.setStatus('internal_error'); - onError(e, activeSpan); - finishAndSetSpan(); - afterFinish(); - throw e; - } - - if (isThenable(maybePromiseResult)) { - // @ts-expect-error - the isThenable check returns the "wrong" type here - return maybePromiseResult.then( - res => { - finishAndSetSpan(); - afterFinish(); - return res; - }, - e => { - activeSpan && activeSpan.setStatus('internal_error'); - onError(e, activeSpan); - finishAndSetSpan(); - afterFinish(); - throw e; - }, - ); - } - - finishAndSetSpan(); - afterFinish(); - return maybePromiseResult; + return handleCallbackErrors( + () => callback(activeSpan), + error => { + activeSpan && activeSpan.setStatus('internal_error'); + onError(error, activeSpan); + }, + () => { + activeSpan && activeSpan.end(); + scope.setSpan(parentSpan); + afterFinish(); + }, + ); } /** @@ -90,7 +68,6 @@ export function trace( export function startSpan(context: TransactionContext, callback: (span: Span | undefined) => T): T { const ctx = normalizeContext(context); - // @ts-expect-error - isThenable returns the wrong type return withScope(scope => { const hub = getCurrentHub(); const parentSpan = scope.getSpan(); @@ -98,35 +75,16 @@ export function startSpan(context: TransactionContext, callback: (span: Span const activeSpan = createChildSpanOrTransaction(hub, parentSpan, ctx); scope.setSpan(activeSpan); - function finishAndSetSpan(): void { - activeSpan && activeSpan.end(); - } - - let maybePromiseResult: T; - try { - maybePromiseResult = callback(activeSpan); - } catch (e) { - activeSpan && activeSpan.setStatus('internal_error'); - finishAndSetSpan(); - throw e; - } - - if (isThenable(maybePromiseResult)) { - return maybePromiseResult.then( - res => { - finishAndSetSpan(); - return res; - }, - e => { - activeSpan && activeSpan.setStatus('internal_error'); - finishAndSetSpan(); - throw e; - }, - ); - } - - finishAndSetSpan(); - return maybePromiseResult; + return handleCallbackErrors( + () => callback(activeSpan), + () => { + // Only update the span status if it hasn't been changed yet + if (activeSpan && (!activeSpan.status || activeSpan.status === 'ok')) { + activeSpan.setStatus('internal_error'); + } + }, + () => activeSpan && activeSpan.end(), + ); }); } @@ -152,7 +110,6 @@ export function startSpanManual( ): T { const ctx = normalizeContext(context); - // @ts-expect-error - isThenable returns the wrong type return withScope(scope => { const hub = getCurrentHub(); const parentSpan = scope.getSpan(); @@ -164,25 +121,15 @@ export function startSpanManual( activeSpan && activeSpan.end(); } - let maybePromiseResult: T; - try { - maybePromiseResult = callback(activeSpan, finishAndSetSpan); - } catch (e) { - activeSpan && activeSpan.setStatus('internal_error'); - throw e; - } - - if (isThenable(maybePromiseResult)) { - return maybePromiseResult.then( - res => res, - e => { - activeSpan && activeSpan.setStatus('internal_error'); - throw e; - }, - ); - } - - return maybePromiseResult; + return handleCallbackErrors( + () => callback(activeSpan, finishAndSetSpan), + () => { + // Only update the span status if it hasn't been changed yet, and the span is not yet finished + if (activeSpan && !activeSpan.endTimestamp && (!activeSpan.status || activeSpan.status === 'ok')) { + activeSpan.setStatus('internal_error'); + } + }, + ); }); } diff --git a/packages/core/src/utils/handleCallbackErrors.ts b/packages/core/src/utils/handleCallbackErrors.ts new file mode 100644 index 000000000000..b3bec065581d --- /dev/null +++ b/packages/core/src/utils/handleCallbackErrors.ts @@ -0,0 +1,63 @@ +import { isThenable } from '@sentry/utils'; + +/** + * Wrap a callback function with error handling. + * If an error is thrown, it will be passed to the `onError` callback and re-thrown. + * + * If the return value of the function is a promise, it will be handled with `maybeHandlePromiseRejection`. + * + * If an `onFinally` callback is provided, this will be called when the callback has finished + * - so if it returns a promise, once the promise resolved/rejected, + * else once the callback has finished executing. + * The `onFinally` callback will _always_ be called, no matter if an error was thrown or not. + */ +export function handleCallbackErrors< + // eslint-disable-next-line @typescript-eslint/no-explicit-any + Fn extends () => any, +>( + fn: Fn, + onError: (error: unknown) => void, + // eslint-disable-next-line @typescript-eslint/no-empty-function + onFinally: () => void = () => {}, +): ReturnType { + let maybePromiseResult: ReturnType; + try { + maybePromiseResult = fn(); + } catch (e) { + onError(e); + onFinally(); + throw e; + } + + return maybeHandlePromiseRejection(maybePromiseResult, onError, onFinally); +} + +/** + * Maybe handle a promise rejection. + * This expects to be given a value that _may_ be a promise, or any other value. + * If it is a promise, and it rejects, it will call the `onError` callback. + * Other than this, it will generally return the given value as-is. + */ +function maybeHandlePromiseRejection( + value: MaybePromise, + onError: (error: unknown) => void, + onFinally: () => void, +): MaybePromise { + if (isThenable(value)) { + // @ts-expect-error - the isThenable check returns the "wrong" type here + return value.then( + res => { + onFinally(); + return res; + }, + e => { + onError(e); + onFinally(); + throw e; + }, + ); + } + + onFinally(); + return value; +} diff --git a/packages/core/test/lib/utils/handleCallbackErrors.test.ts b/packages/core/test/lib/utils/handleCallbackErrors.test.ts new file mode 100644 index 000000000000..3c6bb1e19302 --- /dev/null +++ b/packages/core/test/lib/utils/handleCallbackErrors.test.ts @@ -0,0 +1,150 @@ +import { handleCallbackErrors } from '../../../src/utils/handleCallbackErrors'; + +describe('handleCallbackErrors', () => { + it('works with a simple callback', () => { + const onError = jest.fn(); + + const fn = jest.fn(() => 'aa'); + + const res = handleCallbackErrors(fn, onError); + + expect(res).toBe('aa'); + expect(fn).toHaveBeenCalledTimes(1); + expect(onError).not.toHaveBeenCalled(); + }); + + it('triggers onError when callback has sync error', () => { + const error = new Error('test error'); + + const onError = jest.fn(); + + const fn = jest.fn(() => { + throw error; + }); + + expect(() => handleCallbackErrors(fn, onError)).toThrow(error); + + expect(fn).toHaveBeenCalledTimes(1); + expect(onError).toHaveBeenCalledTimes(1); + expect(onError).toHaveBeenCalledWith(error); + }); + + it('works with an async callback', async () => { + const onError = jest.fn(); + + const fn = jest.fn(async () => 'aa'); + + const res = handleCallbackErrors(fn, onError); + + expect(res).toBeInstanceOf(Promise); + + expect(fn).toHaveBeenCalledTimes(1); + expect(onError).not.toHaveBeenCalled(); + + const value = await res; + expect(value).toBe('aa'); + }); + + it('triggers onError when callback returns promise that rejects', async () => { + const onError = jest.fn(); + + const error = new Error('test error'); + + const fn = jest.fn(async () => { + await new Promise(resolve => setTimeout(resolve, 10)); + throw error; + }); + + const res = handleCallbackErrors(fn, onError); + + expect(res).toBeInstanceOf(Promise); + + expect(fn).toHaveBeenCalledTimes(1); + expect(onError).not.toHaveBeenCalled(); + + await expect(res).rejects.toThrow(error); + + expect(onError).toHaveBeenCalledTimes(1); + expect(onError).toHaveBeenCalledWith(error); + }); + + describe('onFinally', () => { + it('triggers after successfuly sync callback', () => { + const onError = jest.fn(); + const onFinally = jest.fn(); + + const fn = jest.fn(() => 'aa'); + + const res = handleCallbackErrors(fn, onError, onFinally); + + expect(res).toBe('aa'); + expect(fn).toHaveBeenCalledTimes(1); + expect(onError).not.toHaveBeenCalled(); + expect(onFinally).toHaveBeenCalledTimes(1); + }); + + it('triggers after error in sync callback', () => { + const error = new Error('test error'); + + const onError = jest.fn(); + const onFinally = jest.fn(); + + const fn = jest.fn(() => { + throw error; + }); + + expect(() => handleCallbackErrors(fn, onError, onFinally)).toThrow(error); + + expect(fn).toHaveBeenCalledTimes(1); + expect(onError).toHaveBeenCalledTimes(1); + expect(onError).toHaveBeenCalledWith(error); + expect(onFinally).toHaveBeenCalledTimes(1); + }); + + it('triggers after successful async callback', async () => { + const onError = jest.fn(); + const onFinally = jest.fn(); + + const fn = jest.fn(async () => 'aa'); + + const res = handleCallbackErrors(fn, onError, onFinally); + + expect(res).toBeInstanceOf(Promise); + + expect(fn).toHaveBeenCalledTimes(1); + expect(onError).not.toHaveBeenCalled(); + expect(onFinally).not.toHaveBeenCalled(); + + const value = await res; + expect(value).toBe('aa'); + + expect(onFinally).toHaveBeenCalledTimes(1); + }); + + it('triggers after error in async callback', async () => { + const onError = jest.fn(); + const onFinally = jest.fn(); + + const error = new Error('test error'); + + const fn = jest.fn(async () => { + await new Promise(resolve => setTimeout(resolve, 10)); + throw error; + }); + + const res = handleCallbackErrors(fn, onError, onFinally); + + expect(res).toBeInstanceOf(Promise); + + expect(fn).toHaveBeenCalledTimes(1); + expect(onError).not.toHaveBeenCalled(); + expect(onFinally).not.toHaveBeenCalled(); + + await expect(res).rejects.toThrow(error); + + expect(onError).toHaveBeenCalledTimes(1); + expect(onError).toHaveBeenCalledWith(error); + expect(onFinally).toHaveBeenCalledTimes(1); + }); + }); +}); diff --git a/packages/deno/src/index.ts b/packages/deno/src/index.ts index fe0b5ee4b620..1615b36064e2 100644 --- a/packages/deno/src/index.ts +++ b/packages/deno/src/index.ts @@ -59,6 +59,7 @@ export { setTags, setUser, spanStatusfromHttpCode, + // eslint-disable-next-line deprecation/deprecation trace, withScope, captureCheckIn, diff --git a/packages/nextjs/src/common/utils/edgeWrapperUtils.ts b/packages/nextjs/src/common/utils/edgeWrapperUtils.ts index 0128339a9b72..9c479a88ceeb 100644 --- a/packages/nextjs/src/common/utils/edgeWrapperUtils.ts +++ b/packages/nextjs/src/common/utils/edgeWrapperUtils.ts @@ -1,4 +1,4 @@ -import { addTracingExtensions, captureException, continueTrace, trace } from '@sentry/core'; +import { addTracingExtensions, captureException, continueTrace, handleCallbackErrors, startSpan } from '@sentry/core'; import { winterCGRequestToRequestData } from '@sentry/utils'; import type { EdgeRouteHandler } from '../../edge/types'; @@ -28,7 +28,7 @@ export function withEdgeWrapping( baggage, }); - return trace( + return startSpan( { ...transactionContext, name: options.spanDescription, @@ -41,7 +41,20 @@ export function withEdgeWrapping( }, }, async span => { - const handlerResult = await handler.apply(this, args); + const handlerResult = await handleCallbackErrors( + () => handler.apply(this, args), + error => { + captureException(error, { + mechanism: { + type: 'instrument', + handled: false, + data: { + function: options.mechanismFunctionName, + }, + }, + }); + }, + ); if (handlerResult instanceof Response) { span?.setHttpStatus(handlerResult.status); @@ -51,18 +64,6 @@ export function withEdgeWrapping( return handlerResult; }, - (err, span) => { - span?.setStatus('internal_error'); - captureException(err, { - mechanism: { - type: 'instrument', - handled: false, - data: { - function: options.mechanismFunctionName, - }, - }, - }); - }, ).finally(() => flushQueue()); }; } diff --git a/packages/nextjs/src/common/withServerActionInstrumentation.ts b/packages/nextjs/src/common/withServerActionInstrumentation.ts index d850b3e362f9..85872ac7d703 100644 --- a/packages/nextjs/src/common/withServerActionInstrumentation.ts +++ b/packages/nextjs/src/common/withServerActionInstrumentation.ts @@ -3,8 +3,9 @@ import { captureException, getClient, getCurrentScope, + handleCallbackErrors, runWithAsyncContext, - trace, + startSpan, } from '@sentry/core'; import { logger, tracingContextFromHeaders } from '@sentry/utils'; @@ -83,7 +84,7 @@ async function withServerActionInstrumentationImplementation { - const result = await callback(); + const result = await handleCallbackErrors(callback, error => { + captureException(error, { mechanism: { handled: false } }); + }); if (options.recordResponse !== undefined ? options.recordResponse : sendDefaultPii) { span?.setData('server_action_result', result); @@ -118,9 +121,6 @@ async function withServerActionInstrumentationImplementation { - captureException(error, { mechanism: { handled: false } }); - }, ); } finally { if (!platformSupportsStreaming()) { diff --git a/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts b/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts index 3acaa849ff79..d1765aa2c41e 100644 --- a/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts +++ b/packages/nextjs/src/common/wrapGenerationFunctionWithSentry.ts @@ -4,8 +4,9 @@ import { continueTrace, getClient, getCurrentScope, + handleCallbackErrors, runWithAsyncContext, - trace, + startSpan, } from '@sentry/core'; import type { WebFetchHeaders } from '@sentry/types'; import { winterCGHeadersToDict } from '@sentry/utils'; @@ -60,7 +61,7 @@ export function wrapGenerationFunctionWithSentry a transactionContext.parentSpanId = commonSpanId; } - return trace( + return startSpan( { op: 'function.nextjs', name: `${componentType}.${generationFunctionIdentifier} (${componentRoute})`, @@ -76,17 +77,18 @@ export function wrapGenerationFunctionWithSentry a }, }, () => { - return originalFunction.apply(thisArg, args); - }, - err => { - captureException(err, { - mechanism: { - handled: false, - data: { - function: 'wrapGenerationFunctionWithSentry', - }, - }, - }); + return handleCallbackErrors( + () => originalFunction.apply(thisArg, args), + err => + captureException(err, { + mechanism: { + handled: false, + data: { + function: 'wrapGenerationFunctionWithSentry', + }, + }, + }), + ); }, ); }); diff --git a/packages/nextjs/src/common/wrapRouteHandlerWithSentry.ts b/packages/nextjs/src/common/wrapRouteHandlerWithSentry.ts index 1f294283c7d8..a1067b1577e4 100644 --- a/packages/nextjs/src/common/wrapRouteHandlerWithSentry.ts +++ b/packages/nextjs/src/common/wrapRouteHandlerWithSentry.ts @@ -1,4 +1,11 @@ -import { addTracingExtensions, captureException, getCurrentScope, runWithAsyncContext, trace } from '@sentry/core'; +import { + addTracingExtensions, + captureException, + getCurrentScope, + handleCallbackErrors, + runWithAsyncContext, + startSpan, +} from '@sentry/core'; import { tracingContextFromHeaders, winterCGHeadersToDict } from '@sentry/utils'; import { isRedirectNavigationError } from './nextNavigationErrorUtils'; @@ -26,9 +33,10 @@ export function wrapRouteHandlerWithSentry any>( ); getCurrentScope().setPropagationContext(propagationContext); - let res; + let result; + try { - res = await trace( + result = await startSpan( { op: 'http.server', name: `${method} ${parameterizedRoute}`, @@ -43,7 +51,19 @@ export function wrapRouteHandlerWithSentry any>( }, }, async span => { - const response: Response = await originalFunction.apply(thisArg, args); + const response: Response = await handleCallbackErrors( + () => originalFunction.apply(thisArg, args), + error => { + // Next.js throws errors when calling `redirect()`. We don't wanna report these. + if (!isRedirectNavigationError(error)) { + captureException(error, { + mechanism: { + handled: false, + }, + }); + } + }, + ); try { span?.setHttpStatus(response.status); @@ -53,16 +73,6 @@ export function wrapRouteHandlerWithSentry any>( return response; }, - error => { - // Next.js throws errors when calling `redirect()`. We don't wanna report these. - if (!isRedirectNavigationError(error)) { - captureException(error, { - mechanism: { - handled: false, - }, - }); - } - }, ); } finally { if (!platformSupportsStreaming() || process.env.NEXT_RUNTIME === 'edge') { @@ -72,7 +82,7 @@ export function wrapRouteHandlerWithSentry any>( } } - return res; + return result; }); }, }); diff --git a/packages/nextjs/src/common/wrapServerComponentWithSentry.ts b/packages/nextjs/src/common/wrapServerComponentWithSentry.ts index f8e47e8047b3..2dddbbdd7a68 100644 --- a/packages/nextjs/src/common/wrapServerComponentWithSentry.ts +++ b/packages/nextjs/src/common/wrapServerComponentWithSentry.ts @@ -4,7 +4,7 @@ import { continueTrace, getCurrentScope, runWithAsyncContext, - trace, + startSpanManual, } from '@sentry/core'; import { winterCGHeadersToDict } from '@sentry/utils'; @@ -53,7 +53,7 @@ export function wrapServerComponentWithSentry any> transactionContext.parentSpanId = commonSpanId; } - const res = trace( + const res = startSpanManual( { ...transactionContext, op: 'function.nextjs', @@ -68,32 +68,50 @@ export function wrapServerComponentWithSentry any> source: 'component', }, }, - () => originalFunction.apply(thisArg, args), - (e, span) => { - if (isNotFoundNavigationError(e)) { - // We don't want to report "not-found"s - span?.setStatus('not_found'); - } else if (isRedirectNavigationError(e)) { - // We don't want to report redirects - // Since `trace` will automatically set the span status to "internal_error" we need to set it back to "ok" - span?.setStatus('ok'); - } else { - span?.setStatus('internal_error'); + span => { + try { + const res = originalFunction.apply(thisArg, args); + span?.end(); + return res; + } catch (error) { + if (isNotFoundNavigationError(error)) { + // We don't want to report "not-found"s + span?.setStatus('not_found'); + } else if (isRedirectNavigationError(error)) { + // We don't want to report redirects + // Since `startSpan` will automatically set the span status to "internal_error" when an error is thrown, + // We cannot set it to `ok` as that will lead to `startSpanManual` overwriting it when the error bubbles up, + // so instead we temp. set this to `cancelled` and handle this later before we end the span. + span?.setStatus('ok'); + } else { + span?.setStatus('internal_error'); - captureException(e, { - mechanism: { - handled: false, - }, - }); + captureException(error, { + mechanism: { + handled: false, + }, + }); + } + + span?.end(); + + // flushQueue should not throw + // eslint-disable-next-line @typescript-eslint/no-floating-promises + flushQueue(); + + // flushQueue should not throw + // eslint-disable-next-line @typescript-eslint/no-floating-promises + flushQueue(); + + throw error; } }, - () => { - // flushQueue should not throw - // eslint-disable-next-line @typescript-eslint/no-floating-promises - flushQueue(); - }, ); + // flushQueue should not throw + // eslint-disable-next-line @typescript-eslint/no-floating-promises + flushQueue(); + return res; }); }, diff --git a/packages/nextjs/test/edge/edgeWrapperUtils.test.ts b/packages/nextjs/test/edge/edgeWrapperUtils.test.ts index 0003c07e756a..2a782250c7c5 100644 --- a/packages/nextjs/test/edge/edgeWrapperUtils.test.ts +++ b/packages/nextjs/test/edge/edgeWrapperUtils.test.ts @@ -65,7 +65,7 @@ describe('withEdgeWrapping', () => { }); it('should return a function that calls trace', async () => { - const traceSpy = jest.spyOn(coreSdk, 'trace'); + const startSpanSpy = jest.spyOn(coreSdk, 'startSpan'); const request = new Request('https://sentry.io/'); const origFunction = jest.fn(_req => new Response()); @@ -78,8 +78,8 @@ describe('withEdgeWrapping', () => { await wrappedFunction(request); - expect(traceSpy).toHaveBeenCalledTimes(1); - expect(traceSpy).toHaveBeenCalledWith( + expect(startSpanSpy).toHaveBeenCalledTimes(1); + expect(startSpanSpy).toHaveBeenCalledWith( expect.objectContaining({ metadata: { request: { headers: {} }, source: 'route' }, name: 'some label', @@ -87,7 +87,6 @@ describe('withEdgeWrapping', () => { origin: 'auto.function.nextjs.withEdgeWrapping', }), expect.any(Function), - expect.any(Function), ); }); diff --git a/packages/nextjs/test/edge/withSentryAPI.test.ts b/packages/nextjs/test/edge/withSentryAPI.test.ts index db1fd3defab9..b51ce3dfeca6 100644 --- a/packages/nextjs/test/edge/withSentryAPI.test.ts +++ b/packages/nextjs/test/edge/withSentryAPI.test.ts @@ -34,7 +34,7 @@ afterAll(() => { global.Response = origResponse; }); -const traceSpy = jest.spyOn(coreSdk, 'trace'); +const startSpanSpy = jest.spyOn(coreSdk, 'startSpan'); afterEach(() => { jest.clearAllMocks(); @@ -49,8 +49,8 @@ describe('wrapApiHandlerWithSentry', () => { await wrappedFunction(request); - expect(traceSpy).toHaveBeenCalledTimes(1); - expect(traceSpy).toHaveBeenCalledWith( + expect(startSpanSpy).toHaveBeenCalledTimes(1); + expect(startSpanSpy).toHaveBeenCalledWith( expect.objectContaining({ metadata: { request: { headers: {}, method: 'POST', url: 'https://sentry.io/' }, source: 'route' }, name: 'POST /user/[userId]/post/[postId]', @@ -58,7 +58,6 @@ describe('wrapApiHandlerWithSentry', () => { origin: 'auto.function.nextjs.withEdgeWrapping', }), expect.any(Function), - expect.any(Function), ); }); @@ -69,8 +68,8 @@ describe('wrapApiHandlerWithSentry', () => { await wrappedFunction(); - expect(traceSpy).toHaveBeenCalledTimes(1); - expect(traceSpy).toHaveBeenCalledWith( + expect(startSpanSpy).toHaveBeenCalledTimes(1); + expect(startSpanSpy).toHaveBeenCalledWith( expect.objectContaining({ metadata: { source: 'route' }, name: 'handler (/user/[userId]/post/[postId])', @@ -78,7 +77,6 @@ describe('wrapApiHandlerWithSentry', () => { origin: 'auto.function.nextjs.withEdgeWrapping', }), expect.any(Function), - expect.any(Function), ); }); }); diff --git a/packages/node-experimental/src/index.ts b/packages/node-experimental/src/index.ts index c6771601c16f..0b5cbc7ec296 100644 --- a/packages/node-experimental/src/index.ts +++ b/packages/node-experimental/src/index.ts @@ -60,6 +60,7 @@ export { runWithAsyncContext, SDK_VERSION, spanStatusfromHttpCode, + // eslint-disable-next-line deprecation/deprecation trace, captureCheckIn, withMonitor, diff --git a/packages/node/src/index.ts b/packages/node/src/index.ts index c1db9de54194..cd7eadf58d3c 100644 --- a/packages/node/src/index.ts +++ b/packages/node/src/index.ts @@ -59,6 +59,7 @@ export { setTags, setUser, spanStatusfromHttpCode, + // eslint-disable-next-line deprecation/deprecation trace, withScope, captureCheckIn, diff --git a/packages/opentelemetry/src/trace.ts b/packages/opentelemetry/src/trace.ts index b5b1e48a8e4b..e2f517900114 100644 --- a/packages/opentelemetry/src/trace.ts +++ b/packages/opentelemetry/src/trace.ts @@ -1,8 +1,7 @@ import type { Span, Tracer } from '@opentelemetry/api'; import { SpanStatusCode, trace } from '@opentelemetry/api'; -import { SDK_VERSION } from '@sentry/core'; +import { SDK_VERSION, handleCallbackErrors } from '@sentry/core'; import type { Client } from '@sentry/types'; -import { isThenable } from '@sentry/utils'; import { getClient } from './custom/hub'; import { InternalSentrySemanticAttributes } from './semanticAttributes'; @@ -24,36 +23,15 @@ export function startSpan(spanContext: OpenTelemetrySpanContext, callback: (s const { name } = spanContext; return tracer.startActiveSpan(name, spanContext, span => { - function finishSpan(): void { - span.end(); - } - _applySentryAttributesToSpan(span, spanContext); - let maybePromiseResult: T; - try { - maybePromiseResult = callback(span); - } catch (e) { - span.setStatus({ code: SpanStatusCode.ERROR }); - finishSpan(); - throw e; - } - - if (isThenable(maybePromiseResult)) { - Promise.resolve(maybePromiseResult).then( - () => { - finishSpan(); - }, - () => { - span.setStatus({ code: SpanStatusCode.ERROR }); - finishSpan(); - }, - ); - } else { - finishSpan(); - } - - return maybePromiseResult; + return handleCallbackErrors( + () => callback(span), + () => { + span.setStatus({ code: SpanStatusCode.ERROR }); + }, + () => span.end(), + ); }); } @@ -71,29 +49,15 @@ export function startSpanManual(spanContext: OpenTelemetrySpanContext, callba const { name } = spanContext; - // @ts-expect-error - isThenable returns the wrong type return tracer.startActiveSpan(name, spanContext, span => { _applySentryAttributesToSpan(span, spanContext); - let maybePromiseResult: T; - try { - maybePromiseResult = callback(span); - } catch (e) { - span.setStatus({ code: SpanStatusCode.ERROR }); - throw e; - } - - if (isThenable(maybePromiseResult)) { - return maybePromiseResult.then( - res => res, - e => { - span.setStatus({ code: SpanStatusCode.ERROR }); - throw e; - }, - ); - } - - return maybePromiseResult; + return handleCallbackErrors( + () => callback(span), + () => { + span.setStatus({ code: SpanStatusCode.ERROR }); + }, + ); }); } diff --git a/packages/remix/src/index.server.ts b/packages/remix/src/index.server.ts index c62b3c9c729c..f7d987d9c12e 100644 --- a/packages/remix/src/index.server.ts +++ b/packages/remix/src/index.server.ts @@ -40,6 +40,7 @@ export { setTags, setUser, spanStatusfromHttpCode, + // eslint-disable-next-line deprecation/deprecation trace, withScope, autoDiscoverNodePerformanceMonitoringIntegrations, diff --git a/packages/serverless/src/gcpfunction/cloud_events.ts b/packages/serverless/src/gcpfunction/cloud_events.ts index 233d6c9a79f8..cde99b9707b5 100644 --- a/packages/serverless/src/gcpfunction/cloud_events.ts +++ b/packages/serverless/src/gcpfunction/cloud_events.ts @@ -1,5 +1,6 @@ +import { handleCallbackErrors } from '@sentry/core'; import { captureException, flush, getCurrentScope, startSpanManual } from '@sentry/node'; -import { isThenable, logger } from '@sentry/utils'; +import { logger } from '@sentry/utils'; import { DEBUG_BUILD } from '../debug-build'; import { domainify, markEventUnhandled, proxyFunction } from '../utils'; @@ -58,22 +59,12 @@ function _wrapCloudEventFunction( }); if (fn.length > 1) { - let fnResult; - try { - fnResult = (fn as CloudEventFunctionWithCallback)(context, newCallback); - } catch (err) { - captureException(err, scope => markEventUnhandled(scope)); - throw err; - } - - if (isThenable(fnResult)) { - fnResult.then(null, err => { + return handleCallbackErrors( + () => (fn as CloudEventFunctionWithCallback)(context, newCallback), + err => { captureException(err, scope => markEventUnhandled(scope)); - throw err; - }); - } - - return fnResult; + }, + ); } return Promise.resolve() diff --git a/packages/serverless/src/gcpfunction/events.ts b/packages/serverless/src/gcpfunction/events.ts index a3d1af662d3c..539c0ee80094 100644 --- a/packages/serverless/src/gcpfunction/events.ts +++ b/packages/serverless/src/gcpfunction/events.ts @@ -1,5 +1,6 @@ +import { handleCallbackErrors } from '@sentry/core'; import { captureException, flush, getCurrentScope, startSpanManual } from '@sentry/node'; -import { isThenable, logger } from '@sentry/utils'; +import { logger } from '@sentry/utils'; import { DEBUG_BUILD } from '../debug-build'; import { domainify, markEventUnhandled, proxyFunction } from '../utils'; @@ -63,22 +64,12 @@ function _wrapEventFunction }); if (fn.length > 2) { - let fnResult; - try { - fnResult = (fn as EventFunctionWithCallback)(data, context, newCallback); - } catch (err) { - captureException(err, scope => markEventUnhandled(scope)); - throw err; - } - - if (isThenable(fnResult)) { - fnResult.then(null, err => { + return handleCallbackErrors( + () => (fn as EventFunctionWithCallback)(data, context, newCallback), + err => { captureException(err, scope => markEventUnhandled(scope)); - throw err; - }); - } - - return fnResult; + }, + ); } return Promise.resolve() diff --git a/packages/serverless/src/gcpfunction/http.ts b/packages/serverless/src/gcpfunction/http.ts index 26d156010373..609a75c81c1e 100644 --- a/packages/serverless/src/gcpfunction/http.ts +++ b/packages/serverless/src/gcpfunction/http.ts @@ -1,9 +1,9 @@ -import { Transaction } from '@sentry/core'; +import { Transaction, handleCallbackErrors } from '@sentry/core'; import type { AddRequestDataToEventOptions } from '@sentry/node'; import { continueTrace, startSpanManual } from '@sentry/node'; import { getCurrentScope } from '@sentry/node'; import { captureException, flush } from '@sentry/node'; -import { isString, isThenable, logger, stripUrlQueryAndFragment } from '@sentry/utils'; +import { isString, logger, stripUrlQueryAndFragment } from '@sentry/utils'; import { DEBUG_BUILD } from '../debug-build'; import { domainify, markEventUnhandled, proxyFunction } from './../utils'; @@ -116,22 +116,12 @@ function _wrapHttpFunction(fn: HttpFunction, wrapOptions: Partial markEventUnhandled(scope)); - throw err; - } - - if (isThenable(fnResult)) { - fnResult.then(null, err => { + return handleCallbackErrors( + () => fn(req, res), + err => { captureException(err, scope => markEventUnhandled(scope)); - throw err; - }); - } - - return fnResult; + }, + ); }, ); }; diff --git a/packages/sveltekit/src/client/load.ts b/packages/sveltekit/src/client/load.ts index ebc21c35eaf0..545105e49c43 100644 --- a/packages/sveltekit/src/client/load.ts +++ b/packages/sveltekit/src/client/load.ts @@ -1,4 +1,4 @@ -import { trace } from '@sentry/core'; +import { handleCallbackErrors, startSpan } from '@sentry/core'; import { captureException } from '@sentry/svelte'; import { addNonEnumerableProperty, objectify } from '@sentry/utils'; import type { LoadEvent } from '@sveltejs/kit'; @@ -77,7 +77,7 @@ export function wrapLoadWithSentry any>(origLoad: T) // `event.route.id` directly. This will still cause invalidations but we get a route name. const routeId = routeIdFromDescriptor || event.route.id; - return trace( + return startSpan( { op: 'function.sveltekit.load', origin: 'auto.function.sveltekit', @@ -87,8 +87,7 @@ export function wrapLoadWithSentry any>(origLoad: T) source: routeId ? 'route' : 'url', }, }, - () => wrappingTarget.apply(thisArg, [patchedEvent]), - sendErrorToSentry, + () => handleCallbackErrors(() => wrappingTarget.apply(thisArg, [patchedEvent]), sendErrorToSentry), ); }, }); diff --git a/packages/sveltekit/src/server/index.ts b/packages/sveltekit/src/server/index.ts index b5d7d64a58a3..e4c8a543fcfd 100644 --- a/packages/sveltekit/src/server/index.ts +++ b/packages/sveltekit/src/server/index.ts @@ -38,6 +38,7 @@ export { setTags, setUser, spanStatusfromHttpCode, + // eslint-disable-next-line deprecation/deprecation trace, withScope, autoDiscoverNodePerformanceMonitoringIntegrations, diff --git a/packages/sveltekit/test/client/load.test.ts b/packages/sveltekit/test/client/load.test.ts index bd6e38fa7a2f..15d2850b4a8c 100644 --- a/packages/sveltekit/test/client/load.test.ts +++ b/packages/sveltekit/test/client/load.test.ts @@ -7,15 +7,15 @@ import { wrapLoadWithSentry } from '../../src/client/load'; const mockCaptureException = vi.spyOn(SentrySvelte, 'captureException').mockImplementation(() => 'xx'); -const mockTrace = vi.fn(); +const mockStartSpan = vi.fn(); vi.mock('@sentry/core', async () => { const original = (await vi.importActual('@sentry/core')) as any; return { ...original, - trace: (...args: unknown[]) => { - mockTrace(...args); - return original.trace(...args); + startSpan: (...args: unknown[]) => { + mockStartSpan(...args); + return original.startSpan(...args); }, }; }); @@ -39,7 +39,7 @@ beforeAll(() => { describe('wrapLoadWithSentry', () => { beforeEach(() => { mockCaptureException.mockClear(); - mockTrace.mockClear(); + mockStartSpan.mockClear(); }); it('calls captureException', async () => { @@ -79,8 +79,8 @@ describe('wrapLoadWithSentry', () => { const wrappedLoad = wrapLoadWithSentry(load); await wrappedLoad(MOCK_LOAD_ARGS); - expect(mockTrace).toHaveBeenCalledTimes(1); - expect(mockTrace).toHaveBeenCalledWith( + expect(mockStartSpan).toHaveBeenCalledTimes(1); + expect(mockStartSpan).toHaveBeenCalledWith( { op: 'function.sveltekit.load', origin: 'auto.function.sveltekit', @@ -91,7 +91,6 @@ describe('wrapLoadWithSentry', () => { }, }, expect.any(Function), - expect.any(Function), ); }); @@ -108,8 +107,8 @@ describe('wrapLoadWithSentry', () => { await wrappedLoad(MOCK_LOAD_ARGS); - expect(mockTrace).toHaveBeenCalledTimes(1); - expect(mockTrace).toHaveBeenCalledWith( + expect(mockStartSpan).toHaveBeenCalledTimes(1); + expect(mockStartSpan).toHaveBeenCalledWith( { op: 'function.sveltekit.load', origin: 'auto.function.sveltekit', @@ -120,7 +119,6 @@ describe('wrapLoadWithSentry', () => { }, }, expect.any(Function), - expect.any(Function), ); }); }); @@ -152,6 +150,6 @@ describe('wrapLoadWithSentry', () => { const wrappedLoad = wrapLoadWithSentry(wrapLoadWithSentry(load)); await wrappedLoad(MOCK_LOAD_ARGS); - expect(mockTrace).toHaveBeenCalledTimes(1); + expect(mockStartSpan).toHaveBeenCalledTimes(1); }); }); diff --git a/packages/tracing-internal/src/node/integrations/prisma.ts b/packages/tracing-internal/src/node/integrations/prisma.ts index b13fa2af7f53..6a9a4ac6b52e 100644 --- a/packages/tracing-internal/src/node/integrations/prisma.ts +++ b/packages/tracing-internal/src/node/integrations/prisma.ts @@ -1,4 +1,4 @@ -import { getCurrentHub, trace } from '@sentry/core'; +import { getCurrentHub, startSpan } from '@sentry/core'; import type { Integration } from '@sentry/types'; import { addNonEnumerableProperty, logger } from '@sentry/utils'; @@ -99,7 +99,7 @@ export class Prisma implements Integration { const action = params.action; const model = params.model; - return trace( + return startSpan( { name: model ? `${model} ${action}` : action, op: 'db.prisma', diff --git a/packages/tracing/test/integrations/node/prisma.test.ts b/packages/tracing/test/integrations/node/prisma.test.ts index 4debafd7de35..552edb1b78c2 100644 --- a/packages/tracing/test/integrations/node/prisma.test.ts +++ b/packages/tracing/test/integrations/node/prisma.test.ts @@ -5,15 +5,15 @@ import { Hub } from '@sentry/core'; import { Integrations } from '../../../src'; import { getTestClient } from '../../testutils'; -const mockTrace = jest.fn(); +const mockStartSpan = jest.fn(); jest.mock('@sentry/core', () => { const original = jest.requireActual('@sentry/core'); return { ...original, - trace: (...args: unknown[]) => { - mockTrace(...args); - return original.trace(...args); + startSpan: (...args: unknown[]) => { + mockStartSpan(...args); + return original.startSpan(...args); }, }; }); @@ -43,16 +43,16 @@ class PrismaClient { describe('setupOnce', function () { beforeEach(() => { - mockTrace.mockClear(); - mockTrace.mockReset(); + mockStartSpan.mockClear(); + mockStartSpan.mockReset(); }); it('should add middleware with $use method correctly', done => { const prismaClient = new PrismaClient(); new Integrations.Prisma({ client: prismaClient }); void prismaClient.user.create()?.then(() => { - expect(mockTrace).toHaveBeenCalledTimes(1); - expect(mockTrace).toHaveBeenLastCalledWith( + expect(mockStartSpan).toHaveBeenCalledTimes(1); + expect(mockStartSpan).toHaveBeenLastCalledWith( { name: 'user create', op: 'db.prisma', @@ -75,7 +75,7 @@ describe('setupOnce', function () { jest.spyOn(sentryCore, 'getCurrentHub').mockReturnValue(hub); void prismaClient.user.create()?.then(() => { - expect(mockTrace).not.toHaveBeenCalled(); + expect(mockStartSpan).not.toHaveBeenCalled(); done(); }); }); diff --git a/packages/vercel-edge/src/index.ts b/packages/vercel-edge/src/index.ts index 2ef1217ab117..daed81643ab5 100644 --- a/packages/vercel-edge/src/index.ts +++ b/packages/vercel-edge/src/index.ts @@ -59,6 +59,7 @@ export { setTags, setUser, spanStatusfromHttpCode, + // eslint-disable-next-line deprecation/deprecation trace, withScope, captureCheckIn, From a1e560b0f936baab6dea467e0db28f7418b3fb18 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 3 Jan 2024 14:38:48 +0100 Subject: [PATCH 2/3] use util --- .../common/wrapServerComponentWithSentry.ts | 66 ++++++++----------- 1 file changed, 26 insertions(+), 40 deletions(-) diff --git a/packages/nextjs/src/common/wrapServerComponentWithSentry.ts b/packages/nextjs/src/common/wrapServerComponentWithSentry.ts index 2dddbbdd7a68..713d1c9a0ef7 100644 --- a/packages/nextjs/src/common/wrapServerComponentWithSentry.ts +++ b/packages/nextjs/src/common/wrapServerComponentWithSentry.ts @@ -3,6 +3,7 @@ import { captureException, continueTrace, getCurrentScope, + handleCallbackErrors, runWithAsyncContext, startSpanManual, } from '@sentry/core'; @@ -69,50 +70,35 @@ export function wrapServerComponentWithSentry any> }, }, span => { - try { - const res = originalFunction.apply(thisArg, args); - span?.end(); - return res; - } catch (error) { - if (isNotFoundNavigationError(error)) { - // We don't want to report "not-found"s - span?.setStatus('not_found'); - } else if (isRedirectNavigationError(error)) { - // We don't want to report redirects - // Since `startSpan` will automatically set the span status to "internal_error" when an error is thrown, - // We cannot set it to `ok` as that will lead to `startSpanManual` overwriting it when the error bubbles up, - // so instead we temp. set this to `cancelled` and handle this later before we end the span. - span?.setStatus('ok'); - } else { - span?.setStatus('internal_error'); + return handleCallbackErrors( + () => originalFunction.apply(thisArg, args), + error => { + if (isNotFoundNavigationError(error)) { + // We don't want to report "not-found"s + span?.setStatus('not_found'); + } else if (isRedirectNavigationError(error)) { + // We don't want to report redirects + span?.setStatus('ok'); + } else { + span?.setStatus('internal_error'); - captureException(error, { - mechanism: { - handled: false, - }, - }); - } - - span?.end(); - - // flushQueue should not throw - // eslint-disable-next-line @typescript-eslint/no-floating-promises - flushQueue(); - - // flushQueue should not throw - // eslint-disable-next-line @typescript-eslint/no-floating-promises - flushQueue(); + captureException(error, { + mechanism: { + handled: false, + }, + }); + } + }, + () => { + span?.end(); - throw error; - } + // flushQueue should not throw + // eslint-disable-next-line @typescript-eslint/no-floating-promises + flushQueue(); + }, + ); }, ); - - // flushQueue should not throw - // eslint-disable-next-line @typescript-eslint/no-floating-promises - flushQueue(); - - return res; }); }, }); From c603d11c0447b0642150e6637274aa163d38aae8 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Thu, 4 Jan 2024 08:57:02 +0000 Subject: [PATCH 3/3] Return response --- packages/nextjs/src/common/wrapServerComponentWithSentry.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/nextjs/src/common/wrapServerComponentWithSentry.ts b/packages/nextjs/src/common/wrapServerComponentWithSentry.ts index 713d1c9a0ef7..427879b3e843 100644 --- a/packages/nextjs/src/common/wrapServerComponentWithSentry.ts +++ b/packages/nextjs/src/common/wrapServerComponentWithSentry.ts @@ -99,6 +99,8 @@ export function wrapServerComponentWithSentry any> ); }, ); + + return res; }); }, });