From 15a36bcd643d37c05f52447adb32d0a149f3d324 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 21 Mar 2023 13:17:24 +0100 Subject: [PATCH 01/34] feat(sveltekit): Add performance monitoring to Sveltekit server handle (#7532) --- packages/sveltekit/src/server/handle.ts | 103 +++++++ packages/sveltekit/src/server/index.ts | 1 + packages/sveltekit/test/server/handle.test.ts | 251 ++++++++++++++++++ packages/sveltekit/test/utils.ts | 12 + 4 files changed, 367 insertions(+) create mode 100644 packages/sveltekit/src/server/handle.ts create mode 100644 packages/sveltekit/test/server/handle.test.ts create mode 100644 packages/sveltekit/test/utils.ts diff --git a/packages/sveltekit/src/server/handle.ts b/packages/sveltekit/src/server/handle.ts new file mode 100644 index 000000000000..90dda26dac55 --- /dev/null +++ b/packages/sveltekit/src/server/handle.ts @@ -0,0 +1,103 @@ +/* eslint-disable @sentry-internal/sdk/no-optional-chaining */ +import { captureException, getCurrentHub, startTransaction } from '@sentry/node'; +import type { Transaction } from '@sentry/types'; +import { + addExceptionMechanism, + baggageHeaderToDynamicSamplingContext, + extractTraceparentData, + isThenable, + objectify, +} from '@sentry/utils'; +import type { Handle } from '@sveltejs/kit'; +import * as domain from 'domain'; + +function sendErrorToSentry(e: unknown): unknown { + // In case we have a primitive, wrap it in the equivalent wrapper class (string -> String, etc.) so that we can + // store a seen flag on it. + const objectifiedErr = objectify(e); + + captureException(objectifiedErr, scope => { + scope.addEventProcessor(event => { + addExceptionMechanism(event, { + type: 'sveltekit', + handled: false, + data: { + function: 'handle', + }, + }); + return event; + }); + + return scope; + }); + + return objectifiedErr; +} + +/** + * A SvelteKit handle function that wraps the request for Sentry error and + * performance monitoring. + * + * Usage: + * ``` + * // src/hooks.server.ts + * import { sentryHandle } from '@sentry/sveltekit'; + * + * export const handle = sentryHandle; + * + * // Optionally use the sequence function to add additional handlers. + * // export const handle = sequence(sentryHandle, yourCustomHandle); + * ``` + */ +export const sentryHandle: Handle = ({ event, resolve }) => { + return domain.create().bind(() => { + let maybePromiseResult; + + const sentryTraceHeader = event.request.headers.get('sentry-trace'); + const baggageHeader = event.request.headers.get('baggage'); + const traceparentData = sentryTraceHeader ? extractTraceparentData(sentryTraceHeader) : undefined; + const dynamicSamplingContext = baggageHeaderToDynamicSamplingContext(baggageHeader); + + // transaction could be undefined if hub extensions were not added. + const transaction: Transaction | undefined = startTransaction({ + op: 'http.server', + name: `${event.request.method} ${event.route.id}`, + status: 'ok', + ...traceparentData, + metadata: { + source: 'route', + dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext, + }, + }); + + getCurrentHub().getScope()?.setSpan(transaction); + + try { + maybePromiseResult = resolve(event); + } catch (e) { + transaction?.setStatus('internal_error'); + const sentryError = sendErrorToSentry(e); + transaction?.finish(); + throw sentryError; + } + + if (isThenable(maybePromiseResult)) { + Promise.resolve(maybePromiseResult).then( + response => { + transaction?.setHttpStatus(response.status); + transaction?.finish(); + }, + e => { + transaction?.setStatus('internal_error'); + sendErrorToSentry(e); + transaction?.finish(); + }, + ); + } else { + transaction?.setHttpStatus(maybePromiseResult.status); + transaction?.finish(); + } + + return maybePromiseResult; + })(); +}; diff --git a/packages/sveltekit/src/server/index.ts b/packages/sveltekit/src/server/index.ts index c7784d870c56..9109f29499d4 100644 --- a/packages/sveltekit/src/server/index.ts +++ b/packages/sveltekit/src/server/index.ts @@ -3,3 +3,4 @@ export * from '@sentry/node'; export { init } from './sdk'; export { handleErrorWithSentry } from './handleError'; export { wrapLoadWithSentry } from './load'; +export { sentryHandle } from './handle'; diff --git a/packages/sveltekit/test/server/handle.test.ts b/packages/sveltekit/test/server/handle.test.ts new file mode 100644 index 000000000000..cf17b56aaa90 --- /dev/null +++ b/packages/sveltekit/test/server/handle.test.ts @@ -0,0 +1,251 @@ +import { addTracingExtensions, Hub, makeMain, Scope } from '@sentry/core'; +import { NodeClient } from '@sentry/node'; +import type { Transaction } from '@sentry/types'; +import type { Handle } from '@sveltejs/kit'; +import { vi } from 'vitest'; + +import { sentryHandle } from '../../src/server/handle'; +import { getDefaultNodeClientOptions } from '../utils'; + +const mockCaptureException = vi.fn(); +let mockScope = new Scope(); + +vi.mock('@sentry/node', async () => { + const original = (await vi.importActual('@sentry/node')) as any; + return { + ...original, + captureException: (err: unknown, cb: (arg0: unknown) => unknown) => { + cb(mockScope); + mockCaptureException(err, cb); + return original.captureException(err, cb); + }, + }; +}); + +const mockAddExceptionMechanism = vi.fn(); + +vi.mock('@sentry/utils', async () => { + const original = (await vi.importActual('@sentry/utils')) as any; + return { + ...original, + addExceptionMechanism: (...args: unknown[]) => mockAddExceptionMechanism(...args), + }; +}); + +function mockEvent(override: Record = {}): Parameters[0]['event'] { + const event: Parameters[0]['event'] = { + cookies: {} as any, + fetch: () => Promise.resolve({} as any), + getClientAddress: () => '', + locals: {}, + params: { id: '123' }, + platform: {}, + request: { + method: 'GET', + headers: { + get: () => null, + append: () => {}, + delete: () => {}, + forEach: () => {}, + has: () => false, + set: () => {}, + }, + } as any, + route: { id: '/users/[id]' }, + setHeaders: () => {}, + url: new URL('http://localhost:3000/users/123'), + isDataRequest: false, + + ...override, + }; + + return event; +} + +const mockResponse = { status: 200, headers: {}, body: '' } as any; + +const enum Type { + Sync = 'sync', + Async = 'async', +} + +function resolve(type: Type, isError: boolean): Parameters[0]['resolve'] { + if (type === Type.Sync) { + return (..._args: unknown[]) => { + if (isError) { + throw new Error(type); + } + + return mockResponse; + }; + } + + return (..._args: unknown[]) => { + return new Promise((resolve, reject) => { + if (isError) { + reject(new Error(type)); + } else { + resolve(mockResponse); + } + }); + }; +} + +let hub: Hub; +let client: NodeClient; + +describe('handleSentry', () => { + beforeAll(() => { + addTracingExtensions(); + }); + + beforeEach(() => { + mockScope = new Scope(); + const options = getDefaultNodeClientOptions({ tracesSampleRate: 1.0 }); + client = new NodeClient(options); + hub = new Hub(client); + makeMain(hub); + + mockCaptureException.mockClear(); + mockAddExceptionMechanism.mockClear(); + }); + + describe.each([ + // isSync, isError, expectedResponse + [Type.Sync, true, undefined], + [Type.Sync, false, mockResponse], + [Type.Async, true, undefined], + [Type.Async, false, mockResponse], + ])('%s resolve with error %s', (type, isError, mockResponse) => { + it('should return a response', async () => { + let response: any = undefined; + try { + response = await sentryHandle({ event: mockEvent(), resolve: resolve(type, isError) }); + } catch (e) { + expect(e).toBeInstanceOf(Error); + expect(e.message).toEqual(type); + } + + expect(response).toEqual(mockResponse); + }); + + it('creates a transaction', async () => { + let ref: any = undefined; + client.on('finishTransaction', (transaction: Transaction) => { + ref = transaction; + }); + + try { + await sentryHandle({ event: mockEvent(), resolve: resolve(type, isError) }); + } catch (e) { + // + } + + expect(ref).toBeDefined(); + + expect(ref.name).toEqual('GET /users/[id]'); + expect(ref.op).toEqual('http.server'); + expect(ref.status).toEqual(isError ? 'internal_error' : 'ok'); + expect(ref.metadata.source).toEqual('route'); + + expect(ref.endTimestamp).toBeDefined(); + }); + + it('creates a transaction from sentry-trace header', async () => { + const event = mockEvent({ + request: { + headers: { + get: (key: string) => { + if (key === 'sentry-trace') { + return '1234567890abcdef1234567890abcdef-1234567890abcdef-1'; + } + + return null; + }, + }, + }, + }); + + let ref: any = undefined; + client.on('finishTransaction', (transaction: Transaction) => { + ref = transaction; + }); + + try { + await sentryHandle({ event, resolve: resolve(type, isError) }); + } catch (e) { + // + } + + expect(ref).toBeDefined(); + expect(ref.traceId).toEqual('1234567890abcdef1234567890abcdef'); + expect(ref.parentSpanId).toEqual('1234567890abcdef'); + expect(ref.sampled).toEqual(true); + }); + + it('creates a transaction with dynamic sampling context from baggage header', async () => { + const event = mockEvent({ + request: { + headers: { + get: (key: string) => { + if (key === 'sentry-trace') { + return '1234567890abcdef1234567890abcdef-1234567890abcdef-1'; + } + + if (key === 'baggage') { + return ( + 'sentry-environment=production,sentry-release=1.0.0,sentry-transaction=dogpark,' + + 'sentry-user_segment=segmentA,sentry-public_key=dogsarebadatkeepingsecrets,' + + 'sentry-trace_id=1234567890abcdef1234567890abcdef,sentry-sample_rate=1' + ); + } + + return null; + }, + }, + }, + }); + + let ref: any = undefined; + client.on('finishTransaction', (transaction: Transaction) => { + ref = transaction; + }); + + try { + await sentryHandle({ event, resolve: resolve(type, isError) }); + } catch (e) { + // + } + + expect(ref).toBeDefined(); + expect(ref.metadata.dynamicSamplingContext).toEqual({ + environment: 'production', + release: '1.0.0', + public_key: 'dogsarebadatkeepingsecrets', + sample_rate: '1', + trace_id: '1234567890abcdef1234567890abcdef', + transaction: 'dogpark', + user_segment: 'segmentA', + }); + }); + + it('send errors to Sentry', async () => { + const addEventProcessorSpy = vi.spyOn(mockScope, 'addEventProcessor').mockImplementationOnce(callback => { + void callback({}, { event_id: 'fake-event-id' }); + return mockScope; + }); + + try { + await sentryHandle({ event: mockEvent(), resolve: resolve(type, isError) }); + } catch (e) { + expect(mockCaptureException).toBeCalledTimes(1); + expect(addEventProcessorSpy).toBeCalledTimes(1); + expect(mockAddExceptionMechanism).toBeCalledTimes(1); + expect(mockAddExceptionMechanism).toBeCalledWith( + {}, + { handled: false, type: 'sveltekit', data: { function: 'handle' } }, + ); + } + }); + }); +}); diff --git a/packages/sveltekit/test/utils.ts b/packages/sveltekit/test/utils.ts new file mode 100644 index 000000000000..993a6bd8823d --- /dev/null +++ b/packages/sveltekit/test/utils.ts @@ -0,0 +1,12 @@ +import { createTransport } from '@sentry/core'; +import type { ClientOptions } from '@sentry/types'; +import { resolvedSyncPromise } from '@sentry/utils'; + +export function getDefaultNodeClientOptions(options: Partial = {}): ClientOptions { + return { + integrations: [], + transport: () => createTransport({ recordDroppedEvent: () => undefined }, _ => resolvedSyncPromise({})), + stackParser: () => [], + ...options, + }; +} From 6426b58c7bf387ff922382c2577631002c1aa977 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 21 Mar 2023 15:19:20 +0100 Subject: [PATCH 02/34] feat(hub): Make scope always defined on the hub (#7551) --- packages/core/src/hub.ts | 56 +++++++++++++++++----------------------- 1 file changed, 23 insertions(+), 33 deletions(-) diff --git a/packages/core/src/hub.ts b/packages/core/src/hub.ts index 203241a35389..734b4c28711a 100644 --- a/packages/core/src/hub.ts +++ b/packages/core/src/hub.ts @@ -56,7 +56,7 @@ const DEFAULT_BREADCRUMBS = 100; */ export interface Layer { client?: Client; - scope?: Scope; + scope: Scope; } /** @@ -87,7 +87,7 @@ export interface Carrier { */ export class Hub implements HubInterface { /** Is a {@link Layer}[] containing the client and scope */ - private readonly _stack: Layer[] = [{}]; + private readonly _stack: Layer[]; /** Contains the last event id of a captured event. */ private _lastEventId?: string; @@ -101,7 +101,7 @@ export class Hub implements HubInterface { * @param version number, higher number means higher priority. */ public constructor(client?: Client, scope: Scope = new Scope(), private readonly _version: number = API_VERSION) { - this.getStackTop().scope = scope; + this._stack = [{ scope }]; if (client) { this.bindClient(client); } @@ -166,7 +166,7 @@ export class Hub implements HubInterface { } /** Returns the scope of the top stack. */ - public getScope(): Scope | undefined { + public getScope(): Scope { return this.getStackTop().scope; } @@ -256,7 +256,7 @@ export class Hub implements HubInterface { public addBreadcrumb(breadcrumb: Breadcrumb, hint?: BreadcrumbHint): void { const { scope, client } = this.getStackTop(); - if (!scope || !client) return; + if (!client) return; const { beforeBreadcrumb = null, maxBreadcrumbs = DEFAULT_BREADCRUMBS } = (client.getOptions && client.getOptions()) || {}; @@ -282,40 +282,35 @@ export class Hub implements HubInterface { * @inheritDoc */ public setUser(user: User | null): void { - const scope = this.getScope(); - if (scope) scope.setUser(user); + this.getScope().setUser(user); } /** * @inheritDoc */ public setTags(tags: { [key: string]: Primitive }): void { - const scope = this.getScope(); - if (scope) scope.setTags(tags); + this.getScope().setTags(tags); } /** * @inheritDoc */ public setExtras(extras: Extras): void { - const scope = this.getScope(); - if (scope) scope.setExtras(extras); + this.getScope().setExtras(extras); } /** * @inheritDoc */ public setTag(key: string, value: Primitive): void { - const scope = this.getScope(); - if (scope) scope.setTag(key, value); + this.getScope().setTag(key, value); } /** * @inheritDoc */ public setExtra(key: string, extra: Extra): void { - const scope = this.getScope(); - if (scope) scope.setExtra(key, extra); + this.getScope().setExtra(key, extra); } /** @@ -323,8 +318,7 @@ export class Hub implements HubInterface { */ // eslint-disable-next-line @typescript-eslint/no-explicit-any public setContext(name: string, context: { [key: string]: any } | null): void { - const scope = this.getScope(); - if (scope) scope.setContext(name, context); + this.getScope().setContext(name, context); } /** @@ -395,17 +389,15 @@ export class Hub implements HubInterface { */ public endSession(): void { const layer = this.getStackTop(); - const scope = layer && layer.scope; - const session = scope && scope.getSession(); + const scope = layer.scope; + const session = scope.getSession(); if (session) { closeSession(session); } this._sendSessionUpdate(); // the session is over; take it off of the scope - if (scope) { - scope.setSession(); - } + scope.setSession(); } /** @@ -426,17 +418,15 @@ export class Hub implements HubInterface { ...context, }); - if (scope) { - // End existing session if there's one - const currentSession = scope.getSession && scope.getSession(); - if (currentSession && currentSession.status === 'ok') { - updateSession(currentSession, { status: 'exited' }); - } - this.endSession(); - - // Afterwards we set the new session on the scope - scope.setSession(session); + // End existing session if there's one + const currentSession = scope.getSession && scope.getSession(); + if (currentSession && currentSession.status === 'ok') { + updateSession(currentSession, { status: 'exited' }); } + this.endSession(); + + // Afterwards we set the new session on the scope + scope.setSession(session); return session; } @@ -472,7 +462,7 @@ export class Hub implements HubInterface { * @param method The method to call on the client. * @param args Arguments to pass to the client function. */ - private _withClient(callback: (client: Client, scope: Scope | undefined) => void): void { + private _withClient(callback: (client: Client, scope: Scope) => void): void { const { scope, client } = this.getStackTop(); if (client) { callback(client, scope); From 5c5ac2ceb65aee04f50fd0b5a1eacc01a79c65d9 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 21 Mar 2023 15:46:08 +0100 Subject: [PATCH 03/34] ref(browser): Remove `sendBeacon` API usage (#7552) --- packages/browser/src/client.ts | 23 +++-------------------- 1 file changed, 3 insertions(+), 20 deletions(-) diff --git a/packages/browser/src/client.ts b/packages/browser/src/client.ts index 8cafc77a68c3..1d0bd091cf19 100644 --- a/packages/browser/src/client.ts +++ b/packages/browser/src/client.ts @@ -1,5 +1,5 @@ import type { Scope } from '@sentry/core'; -import { BaseClient, getEnvelopeEndpointWithUrlEncodedAuth, SDK_VERSION } from '@sentry/core'; +import { BaseClient, SDK_VERSION } from '@sentry/core'; import type { BrowserClientReplayOptions, ClientOptions, @@ -9,7 +9,7 @@ import type { Severity, SeverityLevel, } from '@sentry/types'; -import { createClientReportEnvelope, dsnToString, getSDKSource, logger, serializeEnvelope } from '@sentry/utils'; +import { createClientReportEnvelope, dsnToString, getSDKSource, logger } from '@sentry/utils'; import { eventFromException, eventFromMessage } from './eventbuilder'; import { WINDOW } from './helpers'; @@ -132,24 +132,7 @@ export class BrowserClient extends BaseClient { __DEBUG_BUILD__ && logger.log('Sending outcomes:', outcomes); - const url = getEnvelopeEndpointWithUrlEncodedAuth(this._dsn, this._options); const envelope = createClientReportEnvelope(outcomes, this._options.tunnel && dsnToString(this._dsn)); - - try { - const isRealNavigator = Object.prototype.toString.call(WINDOW && WINDOW.navigator) === '[object Navigator]'; - const hasSendBeacon = isRealNavigator && typeof WINDOW.navigator.sendBeacon === 'function'; - // Make sure beacon is not used if user configures custom transport options - if (hasSendBeacon && !this._options.transportOptions) { - // Prevent illegal invocations - https://xgwang.me/posts/you-may-not-know-beacon/#it-may-throw-error%2C-be-sure-to-catch - const sendBeacon = WINDOW.navigator.sendBeacon.bind(WINDOW.navigator); - sendBeacon(url, serializeEnvelope(envelope)); - } else { - // If beacon is not supported or if they are using the tunnel option - // use our regular transport to send client reports to Sentry. - void this._sendEnvelope(envelope); - } - } catch (e) { - __DEBUG_BUILD__ && logger.error(e); - } + void this._sendEnvelope(envelope); } } From 7b9198f24fa44271eda99c6f607e36a940b93463 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 21 Mar 2023 16:53:01 +0100 Subject: [PATCH 04/34] fix(browser): Ensure keepalive flag is correctly set for parallel requests (#7553) We noticed that sometimes request would remain in a seemingly pending state. After some investigation, we found out that the limit of 64kb for keepalive-enabled fetch requests is not per request but for all parallel requests running at the same time. This fixes this by keeping track of how large the pending body sizes are, plus the # of pending requests, and setting keepalive accordingly. --- packages/browser/src/transports/fetch.ts | 37 +++++++++---- .../test/unit/transports/fetch.test.ts | 55 +++++++++++++++++++ 2 files changed, 81 insertions(+), 11 deletions(-) diff --git a/packages/browser/src/transports/fetch.ts b/packages/browser/src/transports/fetch.ts index 83b75c82ba39..e996b6f8277a 100644 --- a/packages/browser/src/transports/fetch.ts +++ b/packages/browser/src/transports/fetch.ts @@ -13,7 +13,14 @@ export function makeFetchTransport( options: BrowserTransportOptions, nativeFetch: FetchImpl = getNativeFetchImplementation(), ): Transport { + let pendingBodySize = 0; + let pendingCount = 0; + function makeRequest(request: TransportRequest): PromiseLike { + const requestSize = request.body.length; + pendingBodySize += requestSize; + pendingCount++; + const requestOptions: RequestInit = { body: request.body, method: 'POST', @@ -25,23 +32,31 @@ export function makeFetchTransport( // frequently sending events right before the user is switching pages (eg. whenfinishing navigation transactions). // Gotchas: // - `keepalive` isn't supported by Firefox - // - As per spec (https://fetch.spec.whatwg.org/#http-network-or-cache-fetch), a request with `keepalive: true` - // and a content length of > 64 kibibytes returns a network error. We will therefore only activate the flag when - // we're below that limit. - keepalive: request.body.length <= 65536, + // - As per spec (https://fetch.spec.whatwg.org/#http-network-or-cache-fetch): + // If the sum of contentLength and inflightKeepaliveBytes is greater than 64 kibibytes, then return a network error. + // We will therefore only activate the flag when we're below that limit. + // There is also a limit of requests that can be open at the same time, so we also limit this to 15 + // See https://github.com/getsentry/sentry-javascript/pull/7553 for details + keepalive: pendingBodySize <= 60_000 && pendingCount < 15, ...options.fetchOptions, }; try { - return nativeFetch(options.url, requestOptions).then(response => ({ - statusCode: response.status, - headers: { - 'x-sentry-rate-limits': response.headers.get('X-Sentry-Rate-Limits'), - 'retry-after': response.headers.get('Retry-After'), - }, - })); + return nativeFetch(options.url, requestOptions).then(response => { + pendingBodySize -= requestSize; + pendingCount--; + return { + statusCode: response.status, + headers: { + 'x-sentry-rate-limits': response.headers.get('X-Sentry-Rate-Limits'), + 'retry-after': response.headers.get('Retry-After'), + }, + }; + }); } catch (e) { clearCachedFetchImplementation(); + pendingBodySize -= requestSize; + pendingCount--; return rejectedSyncPromise(e); } } diff --git a/packages/browser/test/unit/transports/fetch.test.ts b/packages/browser/test/unit/transports/fetch.test.ts index 53ed5c34e21b..e6b58eeaa110 100644 --- a/packages/browser/test/unit/transports/fetch.test.ts +++ b/packages/browser/test/unit/transports/fetch.test.ts @@ -16,6 +16,11 @@ const ERROR_ENVELOPE = createEnvelope({ event_id: 'aa3ff046696b4b [{ type: 'event' }, { event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2' }] as EventItem, ]); +const LARGE_ERROR_ENVELOPE = createEnvelope( + { event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2', sent_at: '123' }, + [[{ type: 'event' }, { event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2', message: 'x'.repeat(10 * 900) }] as EventItem], +); + class Headers { headers: { [key: string]: string } = {}; get(key: string) { @@ -107,4 +112,54 @@ describe('NewFetchTransport', () => { await expect(() => transport.send(ERROR_ENVELOPE)).not.toThrow(); expect(mockFetch).toHaveBeenCalledTimes(1); }); + + it('correctly sets keepalive flag', async () => { + const mockFetch = jest.fn(() => + Promise.resolve({ + headers: new Headers(), + status: 200, + text: () => Promise.resolve({}), + }), + ) as unknown as FetchImpl; + + const REQUEST_OPTIONS: RequestInit = { + referrerPolicy: 'strict-origin', + referrer: 'http://example.org', + }; + + const transport = makeFetchTransport( + { ...DEFAULT_FETCH_TRANSPORT_OPTIONS, fetchOptions: REQUEST_OPTIONS }, + mockFetch, + ); + + const promises: PromiseLike[] = []; + for (let i = 0; i < 30; i++) { + promises.push(transport.send(LARGE_ERROR_ENVELOPE)); + } + + await Promise.all(promises); + + for (let i = 1; i <= 30; i++) { + // After 7 requests, we hit the total limit of >64kb of size + // Starting there, keepalive should be false + const keepalive = i < 7; + expect(mockFetch).toHaveBeenNthCalledWith(i, expect.any(String), expect.objectContaining({ keepalive })); + } + + (mockFetch as jest.Mock).mockClear(); + + // Limit resets when requests have resolved + // Now try based on # of pending requests + const promises2 = []; + for (let i = 0; i < 20; i++) { + promises2.push(transport.send(ERROR_ENVELOPE)); + } + + await Promise.all(promises2); + + for (let i = 1; i <= 20; i++) { + const keepalive = i < 15; + expect(mockFetch).toHaveBeenNthCalledWith(i, expect.any(String), expect.objectContaining({ keepalive })); + } + }); }); From 72dca3e1bfe4e5756c44ef1e4f7b139c0ec6d2c5 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 21 Mar 2023 16:54:06 +0100 Subject: [PATCH 05/34] chore(otel): Update docs for using propagator (#7548) --- packages/opentelemetry-node/README.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/opentelemetry-node/README.md b/packages/opentelemetry-node/README.md index 551684a75c26..b75ca0ab0ef8 100644 --- a/packages/opentelemetry-node/README.md +++ b/packages/opentelemetry-node/README.md @@ -66,10 +66,9 @@ const sdk = new opentelemetry.NodeSDK({ // Sentry config spanProcessor: new SentrySpanProcessor(), + textMapPropagator: new SentryPropagator(), }); -otelApi.propagation.setGlobalPropagator(new SentryPropagator()); - sdk.start(); ``` From 2738b5ff3fca3816c506eb1736a4c0a055c0fcf7 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 21 Mar 2023 18:50:24 +0100 Subject: [PATCH 06/34] ref(core): Remove guards around scope usage (#7554) --- packages/core/src/hub.ts | 4 ++-- packages/core/src/sdk.ts | 4 +--- packages/core/src/sessionflusher.ts | 6 ++---- packages/core/src/tracing/hubextensions.ts | 14 ++++++-------- packages/core/src/tracing/idletransaction.ts | 7 ++----- packages/core/src/tracing/transaction.ts | 3 +-- packages/core/src/tracing/utils.ts | 2 +- 7 files changed, 15 insertions(+), 25 deletions(-) diff --git a/packages/core/src/hub.ts b/packages/core/src/hub.ts index 734b4c28711a..f67ae3773325 100644 --- a/packages/core/src/hub.ts +++ b/packages/core/src/hub.ts @@ -326,7 +326,7 @@ export class Hub implements HubInterface { */ public configureScope(callback: (scope: Scope) => void): void { const { scope, client } = this.getStackTop(); - if (scope && client) { + if (client) { callback(scope); } } @@ -413,7 +413,7 @@ export class Hub implements HubInterface { const session = makeSession({ release, environment, - ...(scope && { user: scope.getUser() }), + user: scope.getUser(), ...(userAgent && { userAgent }), ...context, }); diff --git a/packages/core/src/sdk.ts b/packages/core/src/sdk.ts index 7d2df2115ddd..5dc58a5fc944 100644 --- a/packages/core/src/sdk.ts +++ b/packages/core/src/sdk.ts @@ -28,9 +28,7 @@ export function initAndBind( } const hub = getCurrentHub(); const scope = hub.getScope(); - if (scope) { - scope.update(options.initialScope); - } + scope.update(options.initialScope); const client = new clientClass(options); hub.bindClient(client); diff --git a/packages/core/src/sessionflusher.ts b/packages/core/src/sessionflusher.ts index 9b4579ade486..4ef196819504 100644 --- a/packages/core/src/sessionflusher.ts +++ b/packages/core/src/sessionflusher.ts @@ -72,15 +72,13 @@ export class SessionFlusher implements SessionFlusherLike { return; } const scope = getCurrentHub().getScope(); - const requestSession = scope && scope.getRequestSession(); + const requestSession = scope.getRequestSession(); if (requestSession && requestSession.status) { this._incrementSessionStatusCount(requestSession.status, new Date()); // This is not entirely necessarily but is added as a safe guard to indicate the bounds of a request and so in // case captureRequestSession is called more than once to prevent double count - if (scope) { - scope.setRequestSession(undefined); - } + scope.setRequestSession(undefined); /* eslint-enable @typescript-eslint/no-unsafe-member-access */ } } diff --git a/packages/core/src/tracing/hubextensions.ts b/packages/core/src/tracing/hubextensions.ts index b5518f95e04b..ad6858d41e4a 100644 --- a/packages/core/src/tracing/hubextensions.ts +++ b/packages/core/src/tracing/hubextensions.ts @@ -11,15 +11,13 @@ import { Transaction } from './transaction'; /** Returns all trace headers that are currently on the top scope. */ function traceHeaders(this: Hub): { [key: string]: string } { const scope = this.getScope(); - if (scope) { - const span = scope.getSpan(); - if (span) { - return { + const span = scope.getSpan(); + + return span + ? { 'sentry-trace': span.toTraceparent(), - }; - } - } - return {}; + } + : {}; } /** diff --git a/packages/core/src/tracing/idletransaction.ts b/packages/core/src/tracing/idletransaction.ts index f4604a312519..77b7d5317da1 100644 --- a/packages/core/src/tracing/idletransaction.ts +++ b/packages/core/src/tracing/idletransaction.ts @@ -346,10 +346,7 @@ export class IdleTransaction extends Transaction { */ function clearActiveTransaction(hub: Hub): void { const scope = hub.getScope(); - if (scope) { - const transaction = scope.getTransaction(); - if (transaction) { - scope.setSpan(undefined); - } + if (scope.getTransaction()) { + scope.setSpan(undefined); } } diff --git a/packages/core/src/tracing/transaction.ts b/packages/core/src/tracing/transaction.ts index eba498b7e654..b1e6e74195be 100644 --- a/packages/core/src/tracing/transaction.ts +++ b/packages/core/src/tracing/transaction.ts @@ -256,8 +256,7 @@ export class Transaction extends SpanClass implements TransactionInterface { const maybeSampleRate = this.metadata.sampleRate; const sample_rate = maybeSampleRate !== undefined ? maybeSampleRate.toString() : undefined; - const scope = hub.getScope(); - const { segment: user_segment } = (scope && scope.getUser()) || {}; + const { segment: user_segment } = hub.getScope().getUser() || {}; const source = this.metadata.source; diff --git a/packages/core/src/tracing/utils.ts b/packages/core/src/tracing/utils.ts index 5dc64f98d400..624fff153270 100644 --- a/packages/core/src/tracing/utils.ts +++ b/packages/core/src/tracing/utils.ts @@ -21,7 +21,7 @@ export { TRACEPARENT_REGEXP, extractTraceparentData } from '@sentry/utils'; export function getActiveTransaction(maybeHub?: Hub): T | undefined { const hub = maybeHub || getCurrentHub(); const scope = hub.getScope(); - return scope && (scope.getTransaction() as T | undefined); + return scope.getTransaction() as T | undefined; } // so it can be used in manual instrumentation without necessitating a hard dependency on @sentry/utils From 95b0a6cdaa9d6f05e9775021fca0bb31d22f4dc4 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Tue, 21 Mar 2023 18:56:49 +0000 Subject: [PATCH 07/34] feat(node): Export tracing from `@sentry/node` (#7503) --- packages/node-integration-tests/package.json | 3 +- .../suites/express/tracing/server.ts | 3 +- .../tracing-new/apollo-graphql/scenario.ts | 43 ++++++++++ .../suites/tracing-new/apollo-graphql/test.ts | 35 ++++++++ .../auto-instrument/mongodb/scenario.ts | 46 ++++++++++ .../auto-instrument/mongodb/test.ts | 85 +++++++++++++++++++ .../auto-instrument/mysql/scenario.ts | 36 ++++++++ .../tracing-new/auto-instrument/mysql/test.ts | 23 +++++ .../auto-instrument/pg/scenario.ts | 25 ++++++ .../tracing-new/auto-instrument/pg/test.ts | 54 ++++++++++++ .../tracing-new/prisma-orm/docker-compose.yml | 13 +++ .../tracing-new/prisma-orm/package.json | 22 +++++ .../prisma/migrations/migration_lock.toml | 3 + .../migrations/sentry_test/migration.sql | 12 +++ .../prisma-orm/prisma/schema.prisma | 15 ++++ .../suites/tracing-new/prisma-orm/scenario.ts | 47 ++++++++++ .../suites/tracing-new/prisma-orm/setup.ts | 16 ++++ .../suites/tracing-new/prisma-orm/test.ts | 17 ++++ .../suites/tracing-new/prisma-orm/yarn.lock | 27 ++++++ .../tracePropagationTargets/scenario.ts | 26 ++++++ .../tracePropagationTargets/test.ts | 42 +++++++++ packages/node/package.json | 1 + packages/node/src/client.ts | 5 +- packages/node/src/index.ts | 3 + packages/node/src/tracing/index.ts | 24 ++++++ packages/node/src/tracing/integrations.ts | 1 + packages/tracing-internal/src/index.ts | 11 ++- .../src/node/integrations/index.ts | 1 + .../src/node/integrations/lazy.ts | 47 ++++++++++ 29 files changed, 681 insertions(+), 5 deletions(-) create mode 100644 packages/node-integration-tests/suites/tracing-new/apollo-graphql/scenario.ts create mode 100644 packages/node-integration-tests/suites/tracing-new/apollo-graphql/test.ts create mode 100644 packages/node-integration-tests/suites/tracing-new/auto-instrument/mongodb/scenario.ts create mode 100644 packages/node-integration-tests/suites/tracing-new/auto-instrument/mongodb/test.ts create mode 100644 packages/node-integration-tests/suites/tracing-new/auto-instrument/mysql/scenario.ts create mode 100644 packages/node-integration-tests/suites/tracing-new/auto-instrument/mysql/test.ts create mode 100644 packages/node-integration-tests/suites/tracing-new/auto-instrument/pg/scenario.ts create mode 100644 packages/node-integration-tests/suites/tracing-new/auto-instrument/pg/test.ts create mode 100644 packages/node-integration-tests/suites/tracing-new/prisma-orm/docker-compose.yml create mode 100644 packages/node-integration-tests/suites/tracing-new/prisma-orm/package.json create mode 100644 packages/node-integration-tests/suites/tracing-new/prisma-orm/prisma/migrations/migration_lock.toml create mode 100644 packages/node-integration-tests/suites/tracing-new/prisma-orm/prisma/migrations/sentry_test/migration.sql create mode 100644 packages/node-integration-tests/suites/tracing-new/prisma-orm/prisma/schema.prisma create mode 100644 packages/node-integration-tests/suites/tracing-new/prisma-orm/scenario.ts create mode 100755 packages/node-integration-tests/suites/tracing-new/prisma-orm/setup.ts create mode 100644 packages/node-integration-tests/suites/tracing-new/prisma-orm/test.ts create mode 100644 packages/node-integration-tests/suites/tracing-new/prisma-orm/yarn.lock create mode 100644 packages/node-integration-tests/suites/tracing-new/tracePropagationTargets/scenario.ts create mode 100644 packages/node-integration-tests/suites/tracing-new/tracePropagationTargets/test.ts create mode 100644 packages/node/src/tracing/index.ts create mode 100644 packages/node/src/tracing/integrations.ts create mode 100644 packages/tracing-internal/src/node/integrations/lazy.ts diff --git a/packages/node-integration-tests/package.json b/packages/node-integration-tests/package.json index f1661102a963..e63a6e940dca 100644 --- a/packages/node-integration-tests/package.json +++ b/packages/node-integration-tests/package.json @@ -9,6 +9,7 @@ "scripts": { "clean": "rimraf -g **/node_modules", "prisma:init": "(cd suites/tracing/prisma-orm && ts-node ./setup.ts)", + "prisma:init:new": "(cd suites/tracing-new/prisma-orm && ts-node ./setup.ts)", "lint": "run-s lint:prettier lint:eslint", "lint:eslint": "eslint . --format stylish", "lint:prettier": "prettier --check \"{suites,utils}/**/*.ts\"", @@ -16,7 +17,7 @@ "fix:eslint": "eslint . --format stylish --fix", "fix:prettier": "prettier --write \"{suites,utils}/**/*.ts\"", "type-check": "tsc", - "pretest": "run-s --silent prisma:init", + "pretest": "run-s --silent prisma:init prisma:init:new", "test": "ts-node ./utils/run-tests.ts", "test:watch": "yarn test --watch" }, diff --git a/packages/node-integration-tests/suites/express/tracing/server.ts b/packages/node-integration-tests/suites/express/tracing/server.ts index faf5a50f95ed..e857621ad22e 100644 --- a/packages/node-integration-tests/suites/express/tracing/server.ts +++ b/packages/node-integration-tests/suites/express/tracing/server.ts @@ -1,5 +1,4 @@ import * as Sentry from '@sentry/node'; -import * as Tracing from '@sentry/tracing'; import cors from 'cors'; import express from 'express'; @@ -8,7 +7,7 @@ const app = express(); Sentry.init({ dsn: 'https://public@dsn.ingest.sentry.io/1337', release: '1.0', - integrations: [new Sentry.Integrations.Http({ tracing: true }), new Tracing.Integrations.Express({ app })], + integrations: [new Sentry.Integrations.Http({ tracing: true }), new Sentry.Integrations.Express({ app })], tracesSampleRate: 1.0, }); diff --git a/packages/node-integration-tests/suites/tracing-new/apollo-graphql/scenario.ts b/packages/node-integration-tests/suites/tracing-new/apollo-graphql/scenario.ts new file mode 100644 index 000000000000..5bd8aa815cbe --- /dev/null +++ b/packages/node-integration-tests/suites/tracing-new/apollo-graphql/scenario.ts @@ -0,0 +1,43 @@ +import * as Sentry from '@sentry/node'; +import { ApolloServer, gql } from 'apollo-server'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + tracesSampleRate: 1.0, + integrations: [new Sentry.Integrations.GraphQL(), new Sentry.Integrations.Apollo()], +}); + +const typeDefs = gql` + type Query { + hello: String + } +`; + +const resolvers = { + Query: { + hello: () => { + return 'Hello world!'; + }, + }, +}; + +const server = new ApolloServer({ + typeDefs, + resolvers, +}); + +const transaction = Sentry.startTransaction({ name: 'test_transaction', op: 'transaction' }); + +Sentry.configureScope(scope => { + scope.setSpan(transaction); +}); + +void (async () => { + // Ref: https://www.apollographql.com/docs/apollo-server/testing/testing/#testing-using-executeoperation + await server.executeOperation({ + query: '{hello}', + }); + + transaction.finish(); +})(); diff --git a/packages/node-integration-tests/suites/tracing-new/apollo-graphql/test.ts b/packages/node-integration-tests/suites/tracing-new/apollo-graphql/test.ts new file mode 100644 index 000000000000..128f8a2f164b --- /dev/null +++ b/packages/node-integration-tests/suites/tracing-new/apollo-graphql/test.ts @@ -0,0 +1,35 @@ +import { assertSentryTransaction, conditionalTest, TestEnv } from '../../../utils'; + +// Node 10 is not supported by `graphql-js` +// Ref: https://github.com/graphql/graphql-js/blob/main/package.json +conditionalTest({ min: 12 })('GraphQL/Apollo Tests', () => { + test('should instrument GraphQL and Apollo Server.', async () => { + const env = await TestEnv.init(__dirname); + const envelope = await env.getEnvelopeRequest({ envelopeType: 'transaction' }); + + expect(envelope).toHaveLength(3); + + const transaction = envelope[2]; + const parentSpanId = (transaction as any)?.contexts?.trace?.span_id; + const graphqlSpanId = (transaction as any)?.spans?.[0].span_id; + + expect(parentSpanId).toBeDefined(); + expect(graphqlSpanId).toBeDefined(); + + assertSentryTransaction(transaction, { + transaction: 'test_transaction', + spans: [ + { + description: 'execute', + op: 'graphql.execute', + parent_span_id: parentSpanId, + }, + { + description: 'Query.hello', + op: 'graphql.resolve', + parent_span_id: graphqlSpanId, + }, + ], + }); + }); +}); diff --git a/packages/node-integration-tests/suites/tracing-new/auto-instrument/mongodb/scenario.ts b/packages/node-integration-tests/suites/tracing-new/auto-instrument/mongodb/scenario.ts new file mode 100644 index 000000000000..31d7356765e9 --- /dev/null +++ b/packages/node-integration-tests/suites/tracing-new/auto-instrument/mongodb/scenario.ts @@ -0,0 +1,46 @@ +import * as Sentry from '@sentry/node'; +import { MongoClient } from 'mongodb'; + +// suppress logging of the mongo download +global.console.log = () => null; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + tracesSampleRate: 1.0, + integrations: [...Sentry.autoDiscoverNodePerformanceMonitoringIntegrations()], +}); + +const client = new MongoClient(process.env.MONGO_URL || '', { + useUnifiedTopology: true, +}); + +async function run(): Promise { + const transaction = Sentry.startTransaction({ + name: 'Test Transaction', + op: 'transaction', + }); + + Sentry.configureScope(scope => { + scope.setSpan(transaction); + }); + + try { + await client.connect(); + + const database = client.db('admin'); + const collection = database.collection('movies'); + + await collection.insertOne({ title: 'Rick and Morty' }); + await collection.findOne({ title: 'Back to the Future' }); + await collection.updateOne({ title: 'Back to the Future' }, { $set: { title: 'South Park' } }); + await collection.findOne({ title: 'South Park' }); + + await collection.find({ title: 'South Park' }).toArray(); + } finally { + if (transaction) transaction.finish(); + await client.close(); + } +} + +void run(); diff --git a/packages/node-integration-tests/suites/tracing-new/auto-instrument/mongodb/test.ts b/packages/node-integration-tests/suites/tracing-new/auto-instrument/mongodb/test.ts new file mode 100644 index 000000000000..5664aac9422b --- /dev/null +++ b/packages/node-integration-tests/suites/tracing-new/auto-instrument/mongodb/test.ts @@ -0,0 +1,85 @@ +import { MongoMemoryServer } from 'mongodb-memory-server-global'; + +import { assertSentryTransaction, conditionalTest, TestEnv } from '../../../../utils'; + +// This test can take longer. +jest.setTimeout(15000); + +conditionalTest({ min: 12 })('MongoDB Test', () => { + let mongoServer: MongoMemoryServer; + + beforeAll(async () => { + mongoServer = await MongoMemoryServer.create(); + process.env.MONGO_URL = mongoServer.getUri(); + }, 10000); + + afterAll(async () => { + if (mongoServer) { + await mongoServer.stop(); + } + }); + + test('should auto-instrument `mongodb` package.', async () => { + const env = await TestEnv.init(__dirname); + const envelope = await env.getEnvelopeRequest({ envelopeType: 'transaction' }); + + expect(envelope).toHaveLength(3); + + assertSentryTransaction(envelope[2], { + transaction: 'Test Transaction', + spans: [ + { + data: { + collectionName: 'movies', + dbName: 'admin', + namespace: 'admin.movies', + doc: '{"title":"Rick and Morty"}', + }, + description: 'insertOne', + op: 'db', + }, + { + data: { + collectionName: 'movies', + dbName: 'admin', + namespace: 'admin.movies', + query: '{"title":"Back to the Future"}', + }, + description: 'findOne', + op: 'db', + }, + { + data: { + collectionName: 'movies', + dbName: 'admin', + namespace: 'admin.movies', + filter: '{"title":"Back to the Future"}', + update: '{"$set":{"title":"South Park"}}', + }, + description: 'updateOne', + op: 'db', + }, + { + data: { + collectionName: 'movies', + dbName: 'admin', + namespace: 'admin.movies', + query: '{"title":"South Park"}', + }, + description: 'findOne', + op: 'db', + }, + { + data: { + collectionName: 'movies', + dbName: 'admin', + namespace: 'admin.movies', + query: '{"title":"South Park"}', + }, + description: 'find', + op: 'db', + }, + ], + }); + }); +}); diff --git a/packages/node-integration-tests/suites/tracing-new/auto-instrument/mysql/scenario.ts b/packages/node-integration-tests/suites/tracing-new/auto-instrument/mysql/scenario.ts new file mode 100644 index 000000000000..0f576cb793aa --- /dev/null +++ b/packages/node-integration-tests/suites/tracing-new/auto-instrument/mysql/scenario.ts @@ -0,0 +1,36 @@ +import * as Sentry from '@sentry/node'; +import mysql from 'mysql'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + tracesSampleRate: 1.0, + integrations: [...Sentry.autoDiscoverNodePerformanceMonitoringIntegrations()], +}); + +const connection = mysql.createConnection({ + user: 'root', + password: 'docker', +}); + +connection.connect(function (err: unknown) { + if (err) { + return; + } +}); + +const transaction = Sentry.startTransaction({ + op: 'transaction', + name: 'Test Transaction', +}); + +Sentry.configureScope(scope => { + scope.setSpan(transaction); +}); + +connection.query('SELECT 1 + 1 AS solution', function () { + connection.query('SELECT NOW()', ['1', '2'], () => { + if (transaction) transaction.finish(); + connection.end(); + }); +}); diff --git a/packages/node-integration-tests/suites/tracing-new/auto-instrument/mysql/test.ts b/packages/node-integration-tests/suites/tracing-new/auto-instrument/mysql/test.ts new file mode 100644 index 000000000000..3b96f2cafec0 --- /dev/null +++ b/packages/node-integration-tests/suites/tracing-new/auto-instrument/mysql/test.ts @@ -0,0 +1,23 @@ +import { assertSentryTransaction, TestEnv } from '../../../../utils'; + +test('should auto-instrument `mysql` package.', async () => { + const env = await TestEnv.init(__dirname); + const envelope = await env.getEnvelopeRequest({ envelopeType: 'transaction' }); + + expect(envelope).toHaveLength(3); + + assertSentryTransaction(envelope[2], { + transaction: 'Test Transaction', + spans: [ + { + description: 'SELECT 1 + 1 AS solution', + op: 'db', + }, + + { + description: 'SELECT NOW()', + op: 'db', + }, + ], + }); +}); diff --git a/packages/node-integration-tests/suites/tracing-new/auto-instrument/pg/scenario.ts b/packages/node-integration-tests/suites/tracing-new/auto-instrument/pg/scenario.ts new file mode 100644 index 000000000000..a7859fd562a3 --- /dev/null +++ b/packages/node-integration-tests/suites/tracing-new/auto-instrument/pg/scenario.ts @@ -0,0 +1,25 @@ +import * as Sentry from '@sentry/node'; +import pg from 'pg'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + tracesSampleRate: 1.0, + integrations: [...Sentry.autoDiscoverNodePerformanceMonitoringIntegrations()], +}); + +const transaction = Sentry.startTransaction({ + op: 'transaction', + name: 'Test Transaction', +}); + +Sentry.configureScope(scope => { + scope.setSpan(transaction); +}); + +const client = new pg.Client(); +client.query('SELECT * FROM foo where bar ilike "baz%"', ['a', 'b'], () => + client.query('SELECT * FROM bazz', () => { + client.query('SELECT NOW()', () => transaction.finish()); + }), +); diff --git a/packages/node-integration-tests/suites/tracing-new/auto-instrument/pg/test.ts b/packages/node-integration-tests/suites/tracing-new/auto-instrument/pg/test.ts new file mode 100644 index 000000000000..edfa67cee9d7 --- /dev/null +++ b/packages/node-integration-tests/suites/tracing-new/auto-instrument/pg/test.ts @@ -0,0 +1,54 @@ +import { assertSentryTransaction, TestEnv } from '../../../../utils'; + +class PgClient { + // https://node-postgres.com/api/client#clientquery + public query(_text: unknown, values: unknown, callback?: () => void) { + if (typeof callback === 'function') { + callback(); + return; + } + + if (typeof values === 'function') { + values(); + return; + } + + return Promise.resolve(); + } +} + +beforeAll(() => { + jest.mock('pg', () => { + return { + Client: PgClient, + native: { + Client: PgClient, + }, + }; + }); +}); + +test('should auto-instrument `pg` package.', async () => { + const env = await TestEnv.init(__dirname); + const envelope = await env.getEnvelopeRequest({ envelopeType: 'transaction' }); + + expect(envelope).toHaveLength(3); + + assertSentryTransaction(envelope[2], { + transaction: 'Test Transaction', + spans: [ + { + description: 'SELECT * FROM foo where bar ilike "baz%"', + op: 'db', + }, + { + description: 'SELECT * FROM bazz', + op: 'db', + }, + { + description: 'SELECT NOW()', + op: 'db', + }, + ], + }); +}); diff --git a/packages/node-integration-tests/suites/tracing-new/prisma-orm/docker-compose.yml b/packages/node-integration-tests/suites/tracing-new/prisma-orm/docker-compose.yml new file mode 100644 index 000000000000..45caa4bb3179 --- /dev/null +++ b/packages/node-integration-tests/suites/tracing-new/prisma-orm/docker-compose.yml @@ -0,0 +1,13 @@ +version: '3.9' + +services: + db: + image: postgres:13 + restart: always + container_name: integration-tests-prisma + ports: + - '5433:5432' + environment: + POSTGRES_USER: prisma + POSTGRES_PASSWORD: prisma + POSTGRES_DB: tests diff --git a/packages/node-integration-tests/suites/tracing-new/prisma-orm/package.json b/packages/node-integration-tests/suites/tracing-new/prisma-orm/package.json new file mode 100644 index 000000000000..f8b24d7d0465 --- /dev/null +++ b/packages/node-integration-tests/suites/tracing-new/prisma-orm/package.json @@ -0,0 +1,22 @@ +{ + "name": "sentry-prisma-test", + "version": "1.0.0", + "description": "", + "main": "index.js", + "engines": { + "node": ">=12" + }, + "scripts": { + "db-up": "docker-compose up -d", + "generate": "prisma generate", + "migrate": "prisma migrate dev -n sentry-test", + "setup": "run-s --silent db-up generate migrate" + }, + "keywords": [], + "author": "", + "license": "ISC", + "dependencies": { + "@prisma/client": "3.12.0", + "prisma": "^3.12.0" + } +} diff --git a/packages/node-integration-tests/suites/tracing-new/prisma-orm/prisma/migrations/migration_lock.toml b/packages/node-integration-tests/suites/tracing-new/prisma-orm/prisma/migrations/migration_lock.toml new file mode 100644 index 000000000000..fbffa92c2bb7 --- /dev/null +++ b/packages/node-integration-tests/suites/tracing-new/prisma-orm/prisma/migrations/migration_lock.toml @@ -0,0 +1,3 @@ +# Please do not edit this file manually +# It should be added in your version-control system (i.e. Git) +provider = "postgresql" \ No newline at end of file diff --git a/packages/node-integration-tests/suites/tracing-new/prisma-orm/prisma/migrations/sentry_test/migration.sql b/packages/node-integration-tests/suites/tracing-new/prisma-orm/prisma/migrations/sentry_test/migration.sql new file mode 100644 index 000000000000..8619aaceb2b0 --- /dev/null +++ b/packages/node-integration-tests/suites/tracing-new/prisma-orm/prisma/migrations/sentry_test/migration.sql @@ -0,0 +1,12 @@ +-- CreateTable +CREATE TABLE "User" ( + "id" SERIAL NOT NULL, + "createdAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, + "email" TEXT NOT NULL, + "name" TEXT, + + CONSTRAINT "User_pkey" PRIMARY KEY ("id") +); + +-- CreateIndex +CREATE UNIQUE INDEX "User_email_key" ON "User"("email"); diff --git a/packages/node-integration-tests/suites/tracing-new/prisma-orm/prisma/schema.prisma b/packages/node-integration-tests/suites/tracing-new/prisma-orm/prisma/schema.prisma new file mode 100644 index 000000000000..4363c97738ee --- /dev/null +++ b/packages/node-integration-tests/suites/tracing-new/prisma-orm/prisma/schema.prisma @@ -0,0 +1,15 @@ +datasource db { + url = "postgresql://prisma:prisma@localhost:5433/tests" + provider = "postgresql" +} + +generator client { + provider = "prisma-client-js" +} + +model User { + id Int @id @default(autoincrement()) + createdAt DateTime @default(now()) + email String @unique + name String? +} diff --git a/packages/node-integration-tests/suites/tracing-new/prisma-orm/scenario.ts b/packages/node-integration-tests/suites/tracing-new/prisma-orm/scenario.ts new file mode 100644 index 000000000000..0eb40d9c83ee --- /dev/null +++ b/packages/node-integration-tests/suites/tracing-new/prisma-orm/scenario.ts @@ -0,0 +1,47 @@ +/* eslint-disable @typescript-eslint/no-unsafe-member-access */ +import { PrismaClient } from '@prisma/client'; +import * as Sentry from '@sentry/node'; +import { randomBytes } from 'crypto'; + +const client = new PrismaClient(); + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + tracesSampleRate: 1.0, + integrations: [new Sentry.Integrations.Prisma({ client })], +}); + +async function run(): Promise { + const transaction = Sentry.startTransaction({ + name: 'Test Transaction', + op: 'transaction', + }); + + Sentry.configureScope(scope => { + scope.setSpan(transaction); + }); + + try { + await client.user.create({ + data: { + name: 'Tilda', + email: `tilda_${randomBytes(4).toString('hex')}@sentry.io`, + }, + }); + + await client.user.findMany(); + + await client.user.deleteMany({ + where: { + email: { + contains: 'sentry.io', + }, + }, + }); + } finally { + if (transaction) transaction.finish(); + } +} + +void run(); diff --git a/packages/node-integration-tests/suites/tracing-new/prisma-orm/setup.ts b/packages/node-integration-tests/suites/tracing-new/prisma-orm/setup.ts new file mode 100755 index 000000000000..3c40d12f7337 --- /dev/null +++ b/packages/node-integration-tests/suites/tracing-new/prisma-orm/setup.ts @@ -0,0 +1,16 @@ +import { parseSemver } from '@sentry/utils'; +import { execSync } from 'child_process'; + +const NODE_VERSION = parseSemver(process.versions.node); + +if (NODE_VERSION.major && NODE_VERSION.major < 12) { + // eslint-disable-next-line no-console + console.warn(`Skipping Prisma tests on Node: ${NODE_VERSION.major}`); + process.exit(0); +} + +try { + execSync('yarn && yarn setup'); +} catch (_) { + process.exit(1); +} diff --git a/packages/node-integration-tests/suites/tracing-new/prisma-orm/test.ts b/packages/node-integration-tests/suites/tracing-new/prisma-orm/test.ts new file mode 100644 index 000000000000..e3393f5fe2f8 --- /dev/null +++ b/packages/node-integration-tests/suites/tracing-new/prisma-orm/test.ts @@ -0,0 +1,17 @@ +import { assertSentryTransaction, conditionalTest, TestEnv } from '../../../utils'; + +conditionalTest({ min: 12 })('Prisma ORM Integration', () => { + test('should instrument Prisma client for tracing.', async () => { + const env = await TestEnv.init(__dirname); + const envelope = await env.getEnvelopeRequest({ envelopeType: 'transaction' }); + + assertSentryTransaction(envelope[2], { + transaction: 'Test Transaction', + spans: [ + { description: 'User create', op: 'db.sql.prisma' }, + { description: 'User findMany', op: 'db.sql.prisma' }, + { description: 'User deleteMany', op: 'db.sql.prisma' }, + ], + }); + }); +}); diff --git a/packages/node-integration-tests/suites/tracing-new/prisma-orm/yarn.lock b/packages/node-integration-tests/suites/tracing-new/prisma-orm/yarn.lock new file mode 100644 index 000000000000..d228adebd621 --- /dev/null +++ b/packages/node-integration-tests/suites/tracing-new/prisma-orm/yarn.lock @@ -0,0 +1,27 @@ +# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. +# yarn lockfile v1 + + +"@prisma/client@3.12.0": + version "3.12.0" + resolved "https://registry.yarnpkg.com/@prisma/client/-/client-3.12.0.tgz#a0eb49ffea5c128dd11dffb896d7139a60073d12" + integrity sha512-4NEQjUcWja/NVBvfuDFscWSk1/rXg3+wj+TSkqXCb1tKlx/bsUE00rxsvOvGg7VZ6lw1JFpGkwjwmsOIc4zvQw== + dependencies: + "@prisma/engines-version" "3.12.0-37.22b822189f46ef0dc5c5b503368d1bee01213980" + +"@prisma/engines-version@3.12.0-37.22b822189f46ef0dc5c5b503368d1bee01213980": + version "3.12.0-37.22b822189f46ef0dc5c5b503368d1bee01213980" + resolved "https://registry.yarnpkg.com/@prisma/engines-version/-/engines-version-3.12.0-37.22b822189f46ef0dc5c5b503368d1bee01213980.tgz#829ca3d9d0d92555f44644606d4edfd45b2f5886" + integrity sha512-o+jo8d7ZEiVpcpNWUDh3fj2uPQpBxl79XE9ih9nkogJbhw6P33274SHnqheedZ7PyvPIK/mvU8MLNYgetgXPYw== + +"@prisma/engines@3.12.0-37.22b822189f46ef0dc5c5b503368d1bee01213980": + version "3.12.0-37.22b822189f46ef0dc5c5b503368d1bee01213980" + resolved "https://registry.yarnpkg.com/@prisma/engines/-/engines-3.12.0-37.22b822189f46ef0dc5c5b503368d1bee01213980.tgz#e52e364084c4d05278f62768047b788665e64a45" + integrity sha512-zULjkN8yhzS7B3yeEz4aIym4E2w1ChrV12i14pht3ePFufvsAvBSoZ+tuXMvfSoNTgBS5E4bolRzLbMmbwkkMQ== + +prisma@^3.12.0: + version "3.12.0" + resolved "https://registry.yarnpkg.com/prisma/-/prisma-3.12.0.tgz#9675e0e72407122759d3eadcb6d27cdccd3497bd" + integrity sha512-ltCMZAx1i0i9xuPM692Srj8McC665h6E5RqJom999sjtVSccHSD8Z+HSdBN2183h9PJKvC5dapkn78dd0NWMBg== + dependencies: + "@prisma/engines" "3.12.0-37.22b822189f46ef0dc5c5b503368d1bee01213980" diff --git a/packages/node-integration-tests/suites/tracing-new/tracePropagationTargets/scenario.ts b/packages/node-integration-tests/suites/tracing-new/tracePropagationTargets/scenario.ts new file mode 100644 index 000000000000..a6197e5ab743 --- /dev/null +++ b/packages/node-integration-tests/suites/tracing-new/tracePropagationTargets/scenario.ts @@ -0,0 +1,26 @@ +// eslint-disable-next-line @typescript-eslint/no-unused-vars +import * as Sentry from '@sentry/node'; +import * as http from 'http'; + +Sentry.addTracingExtensions(); + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + tracesSampleRate: 1.0, + tracePropagationTargets: [/\/v0/, 'v1'], + integrations: [new Sentry.Integrations.Http({ tracing: true })], +}); + +const transaction = Sentry.startTransaction({ name: 'test_transaction' }); + +Sentry.configureScope(scope => { + scope.setSpan(transaction); +}); + +http.get('http://match-this-url.com/api/v0'); +http.get('http://match-this-url.com/api/v1'); +http.get('http://dont-match-this-url.com/api/v2'); +http.get('http://dont-match-this-url.com/api/v3'); + +transaction.finish(); diff --git a/packages/node-integration-tests/suites/tracing-new/tracePropagationTargets/test.ts b/packages/node-integration-tests/suites/tracing-new/tracePropagationTargets/test.ts new file mode 100644 index 000000000000..1209c59da46a --- /dev/null +++ b/packages/node-integration-tests/suites/tracing-new/tracePropagationTargets/test.ts @@ -0,0 +1,42 @@ +import nock from 'nock'; + +import { runScenario, TestEnv } from '../../../utils'; + +test('HttpIntegration should instrument correct requests when tracePropagationTargets option is provided', async () => { + const match1 = nock('http://match-this-url.com') + .get('/api/v0') + .matchHeader('baggage', val => typeof val === 'string') + .matchHeader('sentry-trace', val => typeof val === 'string') + .reply(200); + + const match2 = nock('http://match-this-url.com') + .get('/api/v1') + .matchHeader('baggage', val => typeof val === 'string') + .matchHeader('sentry-trace', val => typeof val === 'string') + .reply(200); + + const match3 = nock('http://dont-match-this-url.com') + .get('/api/v2') + .matchHeader('baggage', val => val === undefined) + .matchHeader('sentry-trace', val => val === undefined) + .reply(200); + + const match4 = nock('http://dont-match-this-url.com') + .get('/api/v3') + .matchHeader('baggage', val => val === undefined) + .matchHeader('sentry-trace', val => val === undefined) + .reply(200); + + const env = await TestEnv.init(__dirname); + await runScenario(env.url); + + env.server.close(); + nock.cleanAll(); + + await new Promise(resolve => env.server.close(resolve)); + + expect(match1.isDone()).toBe(true); + expect(match2.isDone()).toBe(true); + expect(match3.isDone()).toBe(true); + expect(match4.isDone()).toBe(true); +}); diff --git a/packages/node/package.json b/packages/node/package.json index 7386be3912bb..e29e7290c495 100644 --- a/packages/node/package.json +++ b/packages/node/package.json @@ -19,6 +19,7 @@ "@sentry/core": "7.44.2", "@sentry/types": "7.44.2", "@sentry/utils": "7.44.2", + "@sentry-internal/tracing": "7.44.2", "cookie": "^0.4.1", "https-proxy-agent": "^5.0.0", "lru_map": "^0.3.3", diff --git a/packages/node/src/client.ts b/packages/node/src/client.ts index 0b3a925d775e..d0d0ae7424be 100644 --- a/packages/node/src/client.ts +++ b/packages/node/src/client.ts @@ -1,5 +1,5 @@ import type { Scope } from '@sentry/core'; -import { BaseClient, SDK_VERSION, SessionFlusher } from '@sentry/core'; +import { addTracingExtensions, BaseClient, SDK_VERSION, SessionFlusher } from '@sentry/core'; import type { Event, EventHint, Severity, SeverityLevel } from '@sentry/types'; import { logger, resolvedSyncPromise } from '@sentry/utils'; import * as os from 'os'; @@ -40,6 +40,9 @@ export class NodeClient extends BaseClient { ...options.transportOptions, }; + // The Node client always supports tracing + addTracingExtensions(); + super(options); } diff --git a/packages/node/src/index.ts b/packages/node/src/index.ts index 92d3cfcf835f..07db6be2f07d 100644 --- a/packages/node/src/index.ts +++ b/packages/node/src/index.ts @@ -45,6 +45,7 @@ export { setUser, withScope, } from '@sentry/core'; +export { autoDiscoverNodePerformanceMonitoringIntegrations } from './tracing'; export { NodeClient } from './client'; export { makeNodeTransport } from './transports'; @@ -57,10 +58,12 @@ import * as domain from 'domain'; import * as Handlers from './handlers'; import * as NodeIntegrations from './integrations'; +import * as TracingIntegrations from './tracing/integrations'; const INTEGRATIONS = { ...CoreIntegrations, ...NodeIntegrations, + ...TracingIntegrations, }; export { INTEGRATIONS as Integrations, Handlers }; diff --git a/packages/node/src/tracing/index.ts b/packages/node/src/tracing/index.ts new file mode 100644 index 000000000000..15c4e2889b3f --- /dev/null +++ b/packages/node/src/tracing/index.ts @@ -0,0 +1,24 @@ +import { lazyLoadedNodePerformanceMonitoringIntegrations } from '@sentry-internal/tracing'; +import type { Integration } from '@sentry/types'; +import { logger } from '@sentry/utils'; + +/** + * Automatically detects and returns integrations that will work with your dependencies. + */ +export function autoDiscoverNodePerformanceMonitoringIntegrations(): Integration[] { + const loadedIntegrations = lazyLoadedNodePerformanceMonitoringIntegrations + .map(tryLoad => { + try { + return tryLoad(); + } catch (_) { + return undefined; + } + }) + .filter(integration => !!integration) as Integration[]; + + if (loadedIntegrations.length === 0) { + logger.warn('Performance monitoring integrations could not be automatically loaded.'); + } + + return loadedIntegrations; +} diff --git a/packages/node/src/tracing/integrations.ts b/packages/node/src/tracing/integrations.ts new file mode 100644 index 000000000000..a37bf6bfd494 --- /dev/null +++ b/packages/node/src/tracing/integrations.ts @@ -0,0 +1 @@ +export { Apollo, Express, GraphQL, Mongo, Mysql, Postgres, Prisma } from '@sentry-internal/tracing'; diff --git a/packages/tracing-internal/src/index.ts b/packages/tracing-internal/src/index.ts index c735942fbe19..c4e17c25294f 100644 --- a/packages/tracing-internal/src/index.ts +++ b/packages/tracing-internal/src/index.ts @@ -1,6 +1,15 @@ export * from './exports'; -export { Apollo, Express, GraphQL, Mongo, Mysql, Postgres, Prisma } from './node/integrations'; +export { + Apollo, + Express, + GraphQL, + Mongo, + Mysql, + Postgres, + Prisma, + lazyLoadedNodePerformanceMonitoringIntegrations, +} from './node'; export { BrowserTracing, diff --git a/packages/tracing-internal/src/node/integrations/index.ts b/packages/tracing-internal/src/node/integrations/index.ts index 607a3e129984..0b69f4440f3a 100644 --- a/packages/tracing-internal/src/node/integrations/index.ts +++ b/packages/tracing-internal/src/node/integrations/index.ts @@ -5,3 +5,4 @@ export { Mongo } from './mongo'; export { Prisma } from './prisma'; export { GraphQL } from './graphql'; export { Apollo } from './apollo'; +export * from './lazy'; diff --git a/packages/tracing-internal/src/node/integrations/lazy.ts b/packages/tracing-internal/src/node/integrations/lazy.ts new file mode 100644 index 000000000000..f53ff756cd48 --- /dev/null +++ b/packages/tracing-internal/src/node/integrations/lazy.ts @@ -0,0 +1,47 @@ +import type { Integration, IntegrationClass } from '@sentry/types'; +import { dynamicRequire } from '@sentry/utils'; + +export const lazyLoadedNodePerformanceMonitoringIntegrations: (() => Integration)[] = [ + () => { + const integration = dynamicRequire(module, './apollo') as { + Apollo: IntegrationClass; + }; + return new integration.Apollo(); + }, + () => { + const integration = dynamicRequire(module, './apollo') as { + Apollo: IntegrationClass; + }; + return new integration.Apollo({ useNestjs: true }); + }, + () => { + const integration = dynamicRequire(module, './graphql') as { + GraphQL: IntegrationClass; + }; + return new integration.GraphQL(); + }, + () => { + const integration = dynamicRequire(module, './mongo') as { + Mongo: IntegrationClass; + }; + return new integration.Mongo(); + }, + () => { + const integration = dynamicRequire(module, './mongo') as { + Mongo: IntegrationClass; + }; + return new integration.Mongo({ mongoose: true }); + }, + () => { + const integration = dynamicRequire(module, './mysql') as { + Mysql: IntegrationClass; + }; + return new integration.Mysql(); + }, + () => { + const integration = dynamicRequire(module, './postgres') as { + Postgres: IntegrationClass; + }; + return new integration.Postgres(); + }, +]; From 86b89b9a271ba4bfe66dd74284992a07e1bc9c72 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Tue, 21 Mar 2023 19:11:05 +0000 Subject: [PATCH 08/34] feat(tracing): Migrate some imports away from `@sentry/tracing` (#7539) --- packages/nextjs/src/server/wrapApiHandlerWithSentry.ts | 2 +- packages/opentelemetry-node/src/spanprocessor.ts | 3 +-- .../opentelemetry-node/src/utils/map-otel-status.ts | 2 +- packages/remix/src/utils/instrumentServer.ts | 3 +-- packages/serverless/src/awslambda.ts | 10 ++++++++-- packages/serverless/src/gcpfunction/http.ts | 2 +- 6 files changed, 13 insertions(+), 9 deletions(-) diff --git a/packages/nextjs/src/server/wrapApiHandlerWithSentry.ts b/packages/nextjs/src/server/wrapApiHandlerWithSentry.ts index 5bed5dca3135..37ef93bf30cc 100644 --- a/packages/nextjs/src/server/wrapApiHandlerWithSentry.ts +++ b/packages/nextjs/src/server/wrapApiHandlerWithSentry.ts @@ -1,10 +1,10 @@ import { hasTracingEnabled } from '@sentry/core'; import { captureException, getCurrentHub, startTransaction } from '@sentry/node'; -import { extractTraceparentData } from '@sentry/tracing'; import type { Transaction } from '@sentry/types'; import { addExceptionMechanism, baggageHeaderToDynamicSamplingContext, + extractTraceparentData, isString, logger, objectify, diff --git a/packages/opentelemetry-node/src/spanprocessor.ts b/packages/opentelemetry-node/src/spanprocessor.ts index 08d14e9fa671..c2d141e05f56 100644 --- a/packages/opentelemetry-node/src/spanprocessor.ts +++ b/packages/opentelemetry-node/src/spanprocessor.ts @@ -2,8 +2,7 @@ import type { Context } from '@opentelemetry/api'; import { SpanKind, trace } from '@opentelemetry/api'; import type { Span as OtelSpan, SpanProcessor as OtelSpanProcessor } from '@opentelemetry/sdk-trace-base'; import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; -import { addGlobalEventProcessor, getCurrentHub } from '@sentry/core'; -import { Transaction } from '@sentry/tracing'; +import { addGlobalEventProcessor, getCurrentHub, Transaction } from '@sentry/core'; import type { DynamicSamplingContext, Span as SentrySpan, TraceparentData, TransactionContext } from '@sentry/types'; import { isString, logger } from '@sentry/utils'; diff --git a/packages/opentelemetry-node/src/utils/map-otel-status.ts b/packages/opentelemetry-node/src/utils/map-otel-status.ts index 968150852e6e..8fdc0e09ac8b 100644 --- a/packages/opentelemetry-node/src/utils/map-otel-status.ts +++ b/packages/opentelemetry-node/src/utils/map-otel-status.ts @@ -1,6 +1,6 @@ import type { Span as OtelSpan } from '@opentelemetry/sdk-trace-base'; import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; -import type { SpanStatusType as SentryStatus } from '@sentry/tracing'; +import type { SpanStatusType as SentryStatus } from '@sentry/core'; // canonicalCodesHTTPMap maps some HTTP codes to Sentry's span statuses. See possible mapping in https://develop.sentry.dev/sdk/event-payloads/span/ const canonicalCodesHTTPMap: Record = { diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index 43e4d8cd1bf0..825198426eec 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -1,8 +1,7 @@ /* eslint-disable max-lines */ -import { hasTracingEnabled } from '@sentry/core'; +import { getActiveTransaction, hasTracingEnabled } from '@sentry/core'; import type { Hub } from '@sentry/node'; import { captureException, getCurrentHub } from '@sentry/node'; -import { getActiveTransaction } from '@sentry/tracing'; import type { Transaction, TransactionSource, WrappedFunction } from '@sentry/types'; import { addExceptionMechanism, diff --git a/packages/serverless/src/awslambda.ts b/packages/serverless/src/awslambda.ts index 29dd10da6602..ccfc1e191024 100644 --- a/packages/serverless/src/awslambda.ts +++ b/packages/serverless/src/awslambda.ts @@ -2,9 +2,15 @@ import type { Scope } from '@sentry/node'; import * as Sentry from '@sentry/node'; import { captureException, captureMessage, flush, getCurrentHub, withScope } from '@sentry/node'; -import { extractTraceparentData } from '@sentry/tracing'; import type { Integration } from '@sentry/types'; -import { baggageHeaderToDynamicSamplingContext, dsnFromString, dsnToString, isString, logger } from '@sentry/utils'; +import { + baggageHeaderToDynamicSamplingContext, + dsnFromString, + dsnToString, + extractTraceparentData, + isString, + logger, +} from '@sentry/utils'; // NOTE: I have no idea how to fix this right now, and don't want to waste more time, as it builds just fine — Kamil // eslint-disable-next-line import/no-unresolved import type { Context, Handler } from 'aws-lambda'; diff --git a/packages/serverless/src/gcpfunction/http.ts b/packages/serverless/src/gcpfunction/http.ts index fbe5882f549a..8892353fd4bf 100644 --- a/packages/serverless/src/gcpfunction/http.ts +++ b/packages/serverless/src/gcpfunction/http.ts @@ -1,8 +1,8 @@ import type { AddRequestDataToEventOptions } from '@sentry/node'; import { captureException, flush, getCurrentHub } from '@sentry/node'; -import { extractTraceparentData } from '@sentry/tracing'; import { baggageHeaderToDynamicSamplingContext, + extractTraceparentData, isString, isThenable, logger, From baff7dd73f54a070250fd28e88fa30d99a41ce50 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Wed, 22 Mar 2023 10:05:18 +0000 Subject: [PATCH 09/34] feat(nextjs): Remove `@sentry/tracing` dependency from nextjs SDK (#7561) --- packages/browser/src/index.ts | 4 ++-- packages/nextjs/package.json | 1 - packages/nextjs/src/client/index.ts | 9 +++++++-- packages/nextjs/src/edge/edgeclient.ts | 5 ++++- packages/nextjs/src/edge/index.ts | 2 -- packages/nextjs/src/server/utils/wrapperUtils.ts | 3 +-- packages/nextjs/test/config/wrappers.test.ts | 5 +++++ packages/nextjs/test/edge/withSentryAPI.test.ts | 4 ++++ packages/node/src/index.ts | 1 + 9 files changed, 24 insertions(+), 10 deletions(-) diff --git a/packages/browser/src/index.ts b/packages/browser/src/index.ts index 59d814ac9412..dca32f05b35d 100644 --- a/packages/browser/src/index.ts +++ b/packages/browser/src/index.ts @@ -21,8 +21,8 @@ const INTEGRATIONS = { export { INTEGRATIONS as Integrations }; export { Replay } from '@sentry/replay'; -export { BrowserTracing } from '@sentry-internal/tracing'; -export { addTracingExtensions } from '@sentry/core'; +export { BrowserTracing, defaultRequestInstrumentationOptions } from '@sentry-internal/tracing'; +export { addTracingExtensions, getActiveTransaction } from '@sentry/core'; export { makeBrowserOfflineTransport } from './transports/offline'; export { onProfilingStartRouteTransaction } from './profiling/hubextensions'; export { BrowserProfilingIntegration } from './profiling/integration'; diff --git a/packages/nextjs/package.json b/packages/nextjs/package.json index f341b0609519..8b32553b1f9d 100644 --- a/packages/nextjs/package.json +++ b/packages/nextjs/package.json @@ -22,7 +22,6 @@ "@sentry/integrations": "7.44.2", "@sentry/node": "7.44.2", "@sentry/react": "7.44.2", - "@sentry/tracing": "7.44.2", "@sentry/types": "7.44.2", "@sentry/utils": "7.44.2", "@sentry/webpack-plugin": "1.20.0", diff --git a/packages/nextjs/src/client/index.ts b/packages/nextjs/src/client/index.ts index 79dcd5219cd4..ece6bf78db6a 100644 --- a/packages/nextjs/src/client/index.ts +++ b/packages/nextjs/src/client/index.ts @@ -1,8 +1,13 @@ import { hasTracingEnabled } from '@sentry/core'; import { RewriteFrames } from '@sentry/integrations'; import type { BrowserOptions } from '@sentry/react'; -import { configureScope, init as reactInit, Integrations } from '@sentry/react'; -import { BrowserTracing, defaultRequestInstrumentationOptions } from '@sentry/tracing'; +import { + BrowserTracing, + configureScope, + defaultRequestInstrumentationOptions, + init as reactInit, + Integrations, +} from '@sentry/react'; import type { EventProcessor } from '@sentry/types'; import { addOrUpdateIntegration } from '@sentry/utils'; diff --git a/packages/nextjs/src/edge/edgeclient.ts b/packages/nextjs/src/edge/edgeclient.ts index a5b38d651aed..16aed66d1ca4 100644 --- a/packages/nextjs/src/edge/edgeclient.ts +++ b/packages/nextjs/src/edge/edgeclient.ts @@ -1,5 +1,5 @@ import type { Scope } from '@sentry/core'; -import { BaseClient, SDK_VERSION } from '@sentry/core'; +import { addTracingExtensions, BaseClient, SDK_VERSION } from '@sentry/core'; import type { ClientOptions, Event, EventHint, Severity, SeverityLevel } from '@sentry/types'; import { eventFromMessage, eventFromUnknownInput } from './eventbuilder'; @@ -28,6 +28,9 @@ export class EdgeClient extends BaseClient { version: SDK_VERSION, }; + // The Edge client always supports tracing + addTracingExtensions(); + super(options); } diff --git a/packages/nextjs/src/edge/index.ts b/packages/nextjs/src/edge/index.ts index 6f8cd2f42cc4..f7785aaa06d6 100644 --- a/packages/nextjs/src/edge/index.ts +++ b/packages/nextjs/src/edge/index.ts @@ -1,5 +1,3 @@ -import '@sentry/tracing'; // Allow people to call tracing API methods without explicitly importing the tracing package. - import { getCurrentHub, getIntegrationsToSetup, initAndBind, Integrations as CoreIntegrations } from '@sentry/core'; import type { Options } from '@sentry/types'; import { diff --git a/packages/nextjs/src/server/utils/wrapperUtils.ts b/packages/nextjs/src/server/utils/wrapperUtils.ts index ae05b3f16b8d..9fa91fbbee5a 100644 --- a/packages/nextjs/src/server/utils/wrapperUtils.ts +++ b/packages/nextjs/src/server/utils/wrapperUtils.ts @@ -1,5 +1,4 @@ -import { captureException, getCurrentHub, startTransaction } from '@sentry/core'; -import { getActiveTransaction } from '@sentry/tracing'; +import { captureException, getActiveTransaction, getCurrentHub, startTransaction } from '@sentry/core'; import type { Transaction } from '@sentry/types'; import { baggageHeaderToDynamicSamplingContext, extractTraceparentData } from '@sentry/utils'; import * as domain from 'domain'; diff --git a/packages/nextjs/test/config/wrappers.test.ts b/packages/nextjs/test/config/wrappers.test.ts index 68c598e9707f..444d45513ccb 100644 --- a/packages/nextjs/test/config/wrappers.test.ts +++ b/packages/nextjs/test/config/wrappers.test.ts @@ -1,4 +1,5 @@ import * as SentryCore from '@sentry/core'; +import { addTracingExtensions } from '@sentry/core'; import * as SentryNode from '@sentry/node'; import type { IncomingMessage, ServerResponse } from 'http'; @@ -6,6 +7,10 @@ import { wrapGetInitialPropsWithSentry, wrapGetServerSidePropsWithSentry } from const startTransactionSpy = jest.spyOn(SentryCore, 'startTransaction'); +// The wrap* functions require the hub to have tracing extensions. This is normally called by the NodeClient +// constructor but the client isn't used in these tests. +addTracingExtensions(); + describe('data-fetching function wrappers', () => { const route = '/tricks/[trickName]'; let req: IncomingMessage; diff --git a/packages/nextjs/test/edge/withSentryAPI.test.ts b/packages/nextjs/test/edge/withSentryAPI.test.ts index 2ecbdf22a96e..08a91e0c5e11 100644 --- a/packages/nextjs/test/edge/withSentryAPI.test.ts +++ b/packages/nextjs/test/edge/withSentryAPI.test.ts @@ -2,6 +2,10 @@ import * as coreSdk from '@sentry/core'; import { wrapApiHandlerWithSentry } from '../../src/edge'; +// The wrap* functions require the hub to have tracing extensions. This is normally called by the EdgeClient +// constructor but the client isn't used in these tests. +coreSdk.addTracingExtensions(); + // @ts-ignore Request does not exist on type Global const origRequest = global.Request; // @ts-ignore Response does not exist on type Global diff --git a/packages/node/src/index.ts b/packages/node/src/index.ts index 07db6be2f07d..7f0d923e4ae2 100644 --- a/packages/node/src/index.ts +++ b/packages/node/src/index.ts @@ -30,6 +30,7 @@ export { captureMessage, configureScope, createTransport, + getActiveTransaction, getHubFromCarrier, getCurrentHub, Hub, From a73f58bb79c337cfbbcd66ea5565400892a2d144 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Wed, 22 Mar 2023 10:35:43 +0000 Subject: [PATCH 10/34] fix(node): Consider tracing error handler for process exit (#7558) --- packages/core/src/tracing/errors.ts | 4 +++ .../src/integrations/onuncaughtexception.ts | 31 ++++++++++++------- 2 files changed, 23 insertions(+), 12 deletions(-) diff --git a/packages/core/src/tracing/errors.ts b/packages/core/src/tracing/errors.ts index 3351f428fecf..8785bd86d448 100644 --- a/packages/core/src/tracing/errors.ts +++ b/packages/core/src/tracing/errors.ts @@ -29,3 +29,7 @@ function errorCallback(): void { activeTransaction.setStatus(status); } } + +// The function name will be lost when bundling but we need to be able to identify this listener later to maintain the +// node.js default exit behaviour +errorCallback.tag = 'sentry_tracingErrorCallback'; diff --git a/packages/node/src/integrations/onuncaughtexception.ts b/packages/node/src/integrations/onuncaughtexception.ts index 2d10ae61d696..e55de4d1fd8e 100644 --- a/packages/node/src/integrations/onuncaughtexception.ts +++ b/packages/node/src/integrations/onuncaughtexception.ts @@ -8,6 +8,10 @@ import { logAndExitProcess } from './utils/errorhandling'; type OnFatalErrorHandler = (firstError: Error, secondError?: Error) => void; +type TaggedListener = NodeJS.UncaughtExceptionListener & { + tag?: string; +}; + // CAREFUL: Please think twice before updating the way _options looks because the Next.js SDK depends on it in `index.server.ts` interface OnUncaughtExceptionOptions { // TODO(v8): Evaluate whether we should switch the default behaviour here. @@ -95,18 +99,21 @@ export class OnUncaughtException implements Integration { // exit behaviour of the SDK accordingly: // - If other listeners are attached, do not exit. // - If the only listener attached is ours, exit. - const userProvidedListenersCount = global.process - .listeners('uncaughtException') - .reduce((acc, listener) => { - if ( - listener.name === 'domainUncaughtExceptionClear' || // as soon as we're using domains this listener is attached by node itself - listener === this.handler // filter the handler we registered ourselves) - ) { - return acc; - } else { - return acc + 1; - } - }, 0); + const userProvidedListenersCount = ( + global.process.listeners('uncaughtException') as TaggedListener[] + ).reduce((acc, listener) => { + if ( + // There are 3 listeners we ignore: + listener.name === 'domainUncaughtExceptionClear' || // as soon as we're using domains this listener is attached by node itself + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + (listener.tag && listener.tag === 'sentry_tracingErrorCallback') || // the handler we register for tracing + listener === this.handler // the handler we register in this integration + ) { + return acc; + } else { + return acc + 1; + } + }, 0); const processWouldExit = userProvidedListenersCount === 0; const shouldApplyFatalHandlingLogic = this._options.exitEvenIfOtherHandlersAreRegistered || processWouldExit; From 4f34b5ae8900546a39ee112ebff4633dab12c3cd Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 22 Mar 2023 11:40:00 +0100 Subject: [PATCH 11/34] feat(core): Add trace function (#7556) ```js const fetchResult = Sentry.trace({ name: 'GET /users'}, () => fetch('/users'), handleError); ``` --- packages/core/src/tracing/index.ts | 1 + packages/core/src/tracing/trace.ts | 65 +++++++ packages/core/test/lib/tracing/trace.test.ts | 170 +++++++++++++++++++ 3 files changed, 236 insertions(+) create mode 100644 packages/core/src/tracing/trace.ts create mode 100644 packages/core/test/lib/tracing/trace.test.ts diff --git a/packages/core/src/tracing/index.ts b/packages/core/src/tracing/index.ts index fd4949257ceb..1afb556bce4d 100644 --- a/packages/core/src/tracing/index.ts +++ b/packages/core/src/tracing/index.ts @@ -6,3 +6,4 @@ export { extractTraceparentData, getActiveTransaction, stripUrlQueryAndFragment, // eslint-disable-next-line deprecation/deprecation export { SpanStatus } from './spanstatus'; export type { SpanStatusType } from './span'; +export { trace } from './trace'; diff --git a/packages/core/src/tracing/trace.ts b/packages/core/src/tracing/trace.ts new file mode 100644 index 000000000000..19911f7be91f --- /dev/null +++ b/packages/core/src/tracing/trace.ts @@ -0,0 +1,65 @@ +import type { TransactionContext } from '@sentry/types'; +import { isThenable } from '@sentry/utils'; + +import { getCurrentHub } from '../hub'; +import type { Span } from './span'; + +/** + * Wraps a function with a transaction/span and finishes the span after the function is done. + * + * This function is meant to be used internally and may break at any time. Use at your own risk. + * + * @internal + * @private + */ +export function trace( + context: TransactionContext, + callback: (span: Span) => T, + // eslint-disable-next-line @typescript-eslint/no-empty-function + onError: (error: unknown) => void = () => {}, +): T { + const ctx = { ...context }; + // If a name is set and a description is not, set the description to the name. + if (ctx.name !== undefined && ctx.description === undefined) { + ctx.description = ctx.name; + } + + const hub = getCurrentHub(); + const scope = hub.getScope(); + + const parentSpan = scope.getSpan(); + const activeSpan = parentSpan ? parentSpan.startChild(ctx) : hub.startTransaction(ctx); + scope.setSpan(activeSpan); + + function finishAndSetSpan(): void { + activeSpan.finish(); + hub.getScope().setSpan(parentSpan); + } + + let maybePromiseResult: T; + try { + maybePromiseResult = callback(activeSpan); + } catch (e) { + activeSpan.setStatus('internal_error'); + onError(e); + finishAndSetSpan(); + throw e; + } + + if (isThenable(maybePromiseResult)) { + Promise.resolve(maybePromiseResult).then( + () => { + finishAndSetSpan(); + }, + e => { + activeSpan.setStatus('internal_error'); + onError(e); + finishAndSetSpan(); + }, + ); + } else { + finishAndSetSpan(); + } + + return maybePromiseResult; +} diff --git a/packages/core/test/lib/tracing/trace.test.ts b/packages/core/test/lib/tracing/trace.test.ts new file mode 100644 index 000000000000..8a7aa4e09191 --- /dev/null +++ b/packages/core/test/lib/tracing/trace.test.ts @@ -0,0 +1,170 @@ +import { addTracingExtensions, Hub, makeMain } from '../../../src'; +import { trace } from '../../../src/tracing'; +import { getDefaultTestClientOptions, TestClient } from '../../mocks/client'; + +beforeAll(() => { + addTracingExtensions(); +}); + +const enum Type { + Sync = 'sync', + Async = 'async', +} + +let hub: Hub; +let client: TestClient; + +describe('trace', () => { + beforeEach(() => { + const options = getDefaultTestClientOptions({ tracesSampleRate: 0.0 }); + client = new TestClient(options); + hub = new Hub(client); + makeMain(hub); + }); + + describe.each([ + // isSync, isError, callback, expectedReturnValue + [Type.Async, false, () => Promise.resolve('async good'), 'async good'], + [Type.Sync, false, () => 'sync good', 'sync good'], + [Type.Async, true, () => Promise.reject('async bad'), 'async bad'], + [ + Type.Sync, + true, + () => { + throw 'sync bad'; + }, + 'sync bad', + ], + ])('with %s callback and error %s', (_type, isError, callback, expected) => { + it('should return the same value as the callback', async () => { + try { + const result = await trace({ name: 'GET users/[id]' }, () => { + return callback(); + }); + expect(result).toEqual(expected); + } catch (e) { + expect(e).toEqual(expected); + } + }); + + it('creates a transaction', async () => { + let ref: any = undefined; + client.on('finishTransaction', transaction => { + ref = transaction; + }); + try { + await trace({ name: 'GET users/[id]' }, () => { + return callback(); + }); + } catch (e) { + // + } + expect(ref).toBeDefined(); + + expect(ref.name).toEqual('GET users/[id]'); + expect(ref.status).toEqual(isError ? 'internal_error' : undefined); + }); + + it('allows traceparent information to be overriden', async () => { + let ref: any = undefined; + client.on('finishTransaction', transaction => { + ref = transaction; + }); + try { + await trace( + { + name: 'GET users/[id]', + parentSampled: true, + traceId: '12345678901234567890123456789012', + parentSpanId: '1234567890123456', + }, + () => { + return callback(); + }, + ); + } catch (e) { + // + } + expect(ref).toBeDefined(); + + expect(ref.sampled).toEqual(true); + expect(ref.traceId).toEqual('12345678901234567890123456789012'); + expect(ref.parentSpanId).toEqual('1234567890123456'); + }); + + it('allows for transaction to be mutated', async () => { + let ref: any = undefined; + client.on('finishTransaction', transaction => { + ref = transaction; + }); + try { + await trace({ name: 'GET users/[id]' }, span => { + span.op = 'http.server'; + return callback(); + }); + } catch (e) { + // + } + + expect(ref.op).toEqual('http.server'); + }); + + it('creates a span with correct description', async () => { + let ref: any = undefined; + client.on('finishTransaction', transaction => { + ref = transaction; + }); + try { + await trace({ name: 'GET users/[id]', parentSampled: true }, () => { + return trace({ name: 'SELECT * from users' }, () => { + return callback(); + }); + }); + } catch (e) { + // + } + + expect(ref.spanRecorder.spans).toHaveLength(2); + expect(ref.spanRecorder.spans[1].description).toEqual('SELECT * from users'); + expect(ref.spanRecorder.spans[1].parentSpanId).toEqual(ref.spanId); + expect(ref.spanRecorder.spans[1].status).toEqual(isError ? 'internal_error' : undefined); + }); + + it('allows for span to be mutated', async () => { + let ref: any = undefined; + client.on('finishTransaction', transaction => { + ref = transaction; + }); + try { + await trace({ name: 'GET users/[id]', parentSampled: true }, () => { + return trace({ name: 'SELECT * from users' }, childSpan => { + childSpan.op = 'db.query'; + return callback(); + }); + }); + } catch (e) { + // + } + + expect(ref.spanRecorder.spans).toHaveLength(2); + expect(ref.spanRecorder.spans[1].op).toEqual('db.query'); + }); + + it('calls `onError` hook', async () => { + const onError = jest.fn(); + try { + await trace( + { name: 'GET users/[id]' }, + () => { + return callback(); + }, + onError, + ); + } catch (e) { + expect(onError).toHaveBeenCalledTimes(1); + expect(onError).toHaveBeenCalledWith(e); + } + expect(onError).toHaveBeenCalledTimes(isError ? 1 : 0); + }); + }); +}); From 89b5720ad8f416848b239370a6a7956ee8c040a4 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 22 Mar 2023 14:38:03 +0100 Subject: [PATCH 12/34] fix(tracing): Account for case where startTransaction returns undefined (#7566) --- packages/core/src/tracing/trace.ts | 11 ++++++---- packages/core/test/lib/tracing/trace.test.ts | 23 ++++++++++++++++++-- 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/packages/core/src/tracing/trace.ts b/packages/core/src/tracing/trace.ts index 19911f7be91f..8e7844d23988 100644 --- a/packages/core/src/tracing/trace.ts +++ b/packages/core/src/tracing/trace.ts @@ -7,6 +7,9 @@ import type { Span } from './span'; /** * Wraps a function with a transaction/span and finishes the span after the function is done. * + * Note that if you have not enabled tracing extensions via `addTracingExtensions`, this function + * will not generate spans, and the `span` returned from the callback may be undefined. + * * This function is meant to be used internally and may break at any time. Use at your own risk. * * @internal @@ -14,7 +17,7 @@ import type { Span } from './span'; */ export function trace( context: TransactionContext, - callback: (span: Span) => T, + callback: (span?: Span) => T, // eslint-disable-next-line @typescript-eslint/no-empty-function onError: (error: unknown) => void = () => {}, ): T { @@ -32,7 +35,7 @@ export function trace( scope.setSpan(activeSpan); function finishAndSetSpan(): void { - activeSpan.finish(); + activeSpan && activeSpan.finish(); hub.getScope().setSpan(parentSpan); } @@ -40,7 +43,7 @@ export function trace( try { maybePromiseResult = callback(activeSpan); } catch (e) { - activeSpan.setStatus('internal_error'); + activeSpan && activeSpan.setStatus('internal_error'); onError(e); finishAndSetSpan(); throw e; @@ -52,7 +55,7 @@ export function trace( finishAndSetSpan(); }, e => { - activeSpan.setStatus('internal_error'); + activeSpan && activeSpan.setStatus('internal_error'); onError(e); finishAndSetSpan(); }, diff --git a/packages/core/test/lib/tracing/trace.test.ts b/packages/core/test/lib/tracing/trace.test.ts index 8a7aa4e09191..064c41dc123a 100644 --- a/packages/core/test/lib/tracing/trace.test.ts +++ b/packages/core/test/lib/tracing/trace.test.ts @@ -47,6 +47,21 @@ describe('trace', () => { } }); + it('should return the same value as the callback if transactions are undefined', async () => { + // @ts-ignore we are force overriding the transaction return to be undefined + // The `startTransaction` types are actually wrong - it can return undefined + // if tracingExtensions are not enabled + jest.spyOn(hub, 'startTransaction').mockReturnValue(undefined); + try { + const result = await trace({ name: 'GET users/[id]' }, () => { + return callback(); + }); + expect(result).toEqual(expected); + } catch (e) { + expect(e).toEqual(expected); + } + }); + it('creates a transaction', async () => { let ref: any = undefined; client.on('finishTransaction', transaction => { @@ -99,7 +114,9 @@ describe('trace', () => { }); try { await trace({ name: 'GET users/[id]' }, span => { - span.op = 'http.server'; + if (span) { + span.op = 'http.server'; + } return callback(); }); } catch (e) { @@ -138,7 +155,9 @@ describe('trace', () => { try { await trace({ name: 'GET users/[id]', parentSampled: true }, () => { return trace({ name: 'SELECT * from users' }, childSpan => { - childSpan.op = 'db.query'; + if (childSpan) { + childSpan.op = 'db.query'; + } return callback(); }); }); From e97b0970f9eefe927562efed19383b08144aede8 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 22 Mar 2023 15:12:40 +0100 Subject: [PATCH 13/34] feat(sveltekit): Add SvelteKit routing instrumentation (#7565) Add routing instrumentation to the client SvelteKit SDK. Pageload navigations are created on function call as always and updated with a proper name once the `page` store emits the route id. Navigation transactions are created by subscribing to the `navigating` store. No need for user configuration, as the instrumentation will be added automatically on SDK intialization. --- packages/sveltekit/package.json | 1 + packages/sveltekit/rollup.npm.config.js | 20 +-- packages/sveltekit/src/client/router.ts | 111 ++++++++++++++ packages/sveltekit/src/client/sdk.ts | 12 +- packages/sveltekit/test/client/router.test.ts | 139 ++++++++++++++++++ packages/sveltekit/test/client/sdk.test.ts | 25 +++- packages/sveltekit/test/vitest.setup.ts | 13 ++ packages/sveltekit/vite.config.ts | 13 +- yarn.lock | 5 + 9 files changed, 313 insertions(+), 26 deletions(-) create mode 100644 packages/sveltekit/src/client/router.ts create mode 100644 packages/sveltekit/test/client/router.test.ts create mode 100644 packages/sveltekit/test/vitest.setup.ts diff --git a/packages/sveltekit/package.json b/packages/sveltekit/package.json index ab2e9ee19ac7..2293cdcd42cb 100644 --- a/packages/sveltekit/package.json +++ b/packages/sveltekit/package.json @@ -30,6 +30,7 @@ }, "devDependencies": { "@sveltejs/kit": "^1.11.0", + "svelte": "^3.44.0", "typescript": "^4.9.3", "vite": "4.0.0" }, diff --git a/packages/sveltekit/rollup.npm.config.js b/packages/sveltekit/rollup.npm.config.js index f1f8240d5a7a..f9dfe71fd30c 100644 --- a/packages/sveltekit/rollup.npm.config.js +++ b/packages/sveltekit/rollup.npm.config.js @@ -1,14 +1,10 @@ import { makeBaseNPMConfig, makeNPMConfigVariants } from '../../rollup/index.js'; -export default - makeNPMConfigVariants( - makeBaseNPMConfig({ - entrypoints: [ - 'src/index.server.ts', - 'src/index.client.ts', - 'src/client/index.ts', - 'src/server/index.ts', - ], - }), - ) -; +export default makeNPMConfigVariants( + makeBaseNPMConfig({ + entrypoints: ['src/index.server.ts', 'src/index.client.ts', 'src/client/index.ts', 'src/server/index.ts'], + packageSpecificConfig: { + external: ['$app/stores'], + }, + }), +); diff --git a/packages/sveltekit/src/client/router.ts b/packages/sveltekit/src/client/router.ts new file mode 100644 index 000000000000..c4cb7a95c5cf --- /dev/null +++ b/packages/sveltekit/src/client/router.ts @@ -0,0 +1,111 @@ +import { getActiveTransaction } from '@sentry/core'; +import { WINDOW } from '@sentry/svelte'; +import type { Span, Transaction, TransactionContext } from '@sentry/types'; + +import { navigating, page } from '$app/stores'; + +const DEFAULT_TAGS = { + 'routing.instrumentation': '@sentry/sveltekit', +}; + +/** + * Automatically creates pageload and navigation transactions for the client-side SvelteKit router. + * + * This instrumentation makes use of SvelteKit's `page` and `navigating` stores which can be accessed + * anywhere on the client side. + * + * @param startTransactionFn the function used to start (idle) transactions + * @param startTransactionOnPageLoad controls if pageload transactions should be created (defaults to `true`) + * @param startTransactionOnLocationChange controls if navigation transactions should be created (defauls to `true`) + */ +export function svelteKitRoutingInstrumentation( + startTransactionFn: (context: TransactionContext) => T | undefined, + startTransactionOnPageLoad: boolean = true, + startTransactionOnLocationChange: boolean = true, +): void { + if (startTransactionOnPageLoad) { + instrumentPageload(startTransactionFn); + } + + if (startTransactionOnLocationChange) { + instrumentNavigations(startTransactionFn); + } +} + +function instrumentPageload(startTransactionFn: (context: TransactionContext) => Transaction | undefined): void { + const initialPath = WINDOW && WINDOW.location && WINDOW.location.pathname; + + const pageloadTransaction = startTransactionFn({ + name: initialPath, + op: 'pageload', + description: initialPath, + tags: { + ...DEFAULT_TAGS, + }, + }); + + page.subscribe(page => { + if (!page) { + return; + } + + const routeId = page.route && page.route.id; + + if (pageloadTransaction && routeId) { + pageloadTransaction.setName(routeId, 'route'); + } + }); +} + +/** + * Use the `navigating` store to start a transaction on navigations. + */ +function instrumentNavigations(startTransactionFn: (context: TransactionContext) => Transaction | undefined): void { + let routingSpan: Span | undefined = undefined; + let activeTransaction: Transaction | undefined; + + navigating.subscribe(navigation => { + if (!navigation) { + // `navigating` emits a 'null' value when the navigation is completed. + // So in this case, we can finish the routing span. If the transaction was an IdleTransaction, + // it will finish automatically and if it was user-created users also need to finish it. + if (routingSpan) { + routingSpan.finish(); + routingSpan = undefined; + } + return; + } + + const routeDestination = navigation.to && navigation.to.route.id; + const routeOrigin = navigation.from && navigation.from.route.id; + + if (routeOrigin === routeDestination) { + return; + } + + activeTransaction = getActiveTransaction(); + + if (!activeTransaction) { + activeTransaction = startTransactionFn({ + name: routeDestination || (WINDOW && WINDOW.location && WINDOW.location.pathname), + op: 'navigation', + metadata: { source: 'route' }, + tags: { + ...DEFAULT_TAGS, + }, + }); + } + + if (activeTransaction) { + if (routingSpan) { + // If a routing span is still open from a previous navigation, we finish it. + routingSpan.finish(); + } + routingSpan = activeTransaction.startChild({ + op: 'ui.sveltekit.routing', + description: 'SvelteKit Route Change', + }); + activeTransaction.setTag('from', routeOrigin); + } + }); +} diff --git a/packages/sveltekit/src/client/sdk.ts b/packages/sveltekit/src/client/sdk.ts index 50f44bdfa353..9bf1d2cb140b 100644 --- a/packages/sveltekit/src/client/sdk.ts +++ b/packages/sveltekit/src/client/sdk.ts @@ -1,17 +1,18 @@ -import { defaultRequestInstrumentationOptions } from '@sentry-internal/tracing'; import { hasTracingEnabled } from '@sentry/core'; import type { BrowserOptions } from '@sentry/svelte'; import { BrowserTracing, configureScope, init as initSvelteSdk } from '@sentry/svelte'; import { addOrUpdateIntegration } from '@sentry/utils'; import { applySdkMetadata } from '../common/metadata'; +import { svelteKitRoutingInstrumentation } from './router'; // Treeshakable guard to remove all code related to tracing declare const __SENTRY_TRACING__: boolean; /** + * Initialize the client side of the Sentry SvelteKit SDK. * - * @param options + * @param options Configuration options for the SDK. */ export function init(options: BrowserOptions): void { applySdkMetadata(options, ['sveltekit', 'svelte']); @@ -33,14 +34,11 @@ function addClientIntegrations(options: BrowserOptions): void { if (typeof __SENTRY_TRACING__ === 'undefined' || __SENTRY_TRACING__) { if (hasTracingEnabled(options)) { const defaultBrowserTracingIntegration = new BrowserTracing({ - tracePropagationTargets: [...defaultRequestInstrumentationOptions.tracePropagationTargets], - // TODO: Add SvelteKit router instrumentations - // routingInstrumentation: sveltekitRoutingInstrumentation, + routingInstrumentation: svelteKitRoutingInstrumentation, }); integrations = addOrUpdateIntegration(defaultBrowserTracingIntegration, integrations, { - // TODO: Add SvelteKit router instrumentations - // options.routingInstrumentation: sveltekitRoutingInstrumentation, + 'options.routingInstrumentation': svelteKitRoutingInstrumentation, }); } } diff --git a/packages/sveltekit/test/client/router.test.ts b/packages/sveltekit/test/client/router.test.ts new file mode 100644 index 000000000000..a517274ea505 --- /dev/null +++ b/packages/sveltekit/test/client/router.test.ts @@ -0,0 +1,139 @@ +/* eslint-disable @typescript-eslint/unbound-method */ +import type { Transaction } from '@sentry/types'; +import { writable } from 'svelte/store'; +import type { SpyInstance } from 'vitest'; +import { vi } from 'vitest'; + +import { navigating, page } from '$app/stores'; + +import { svelteKitRoutingInstrumentation } from '../../src/client/router'; + +// we have to overwrite the global mock from `vitest.setup.ts` here to reset the +// `navigating` store for each test. +vi.mock('$app/stores', async () => { + return { + get navigating() { + return navigatingStore; + }, + page: writable(), + }; +}); + +let navigatingStore = writable(); + +describe('sveltekitRoutingInstrumentation', () => { + let returnedTransaction: (Transaction & { returnedTransaction: SpyInstance }) | undefined; + const mockedStartTransaction = vi.fn().mockImplementation(txnCtx => { + returnedTransaction = { + ...txnCtx, + setName: vi.fn(), + startChild: vi.fn().mockImplementation(ctx => { + return { ...mockedRoutingSpan, ...ctx }; + }), + setTag: vi.fn(), + }; + return returnedTransaction; + }); + + const mockedRoutingSpan = { + finish: () => {}, + }; + + const routingSpanFinishSpy = vi.spyOn(mockedRoutingSpan, 'finish'); + + beforeEach(() => { + navigatingStore = writable(); + vi.clearAllMocks(); + }); + + it("starts a pageload transaction when it's called with default params", () => { + svelteKitRoutingInstrumentation(mockedStartTransaction); + + expect(mockedStartTransaction).toHaveBeenCalledTimes(1); + expect(mockedStartTransaction).toHaveBeenCalledWith({ + name: '/', + op: 'pageload', + description: '/', + tags: { + 'routing.instrumentation': '@sentry/sveltekit', + }, + }); + + // We emit an update to the `page` store to simulate the SvelteKit router lifecycle + // @ts-ignore This is fine because we testUtils/stores.ts defines `page` as a writable store + page.set({ route: { id: 'testRoute' } }); + + // This should update the transaction name with the parameterized route: + expect(returnedTransaction?.setName).toHaveBeenCalledTimes(1); + expect(returnedTransaction?.setName).toHaveBeenCalledWith('testRoute', 'route'); + }); + + it("doesn't start a pageload transaction if `startTransactionOnPageLoad` is false", () => { + svelteKitRoutingInstrumentation(mockedStartTransaction, false); + expect(mockedStartTransaction).toHaveBeenCalledTimes(0); + }); + + it("doesn't starts a navigation transaction when `startTransactionOnLocationChange` is false", () => { + svelteKitRoutingInstrumentation(mockedStartTransaction, false, false); + + // We emit an update to the `navigating` store to simulate the SvelteKit navigation lifecycle + // @ts-ignore This is fine because we testUtils/stores.ts defines `navigating` as a writable store + navigating.set( + { from: { route: { id: 'testNavigationOrigin' } } }, + { to: { route: { id: 'testNavigationDestination' } } }, + ); + + // This should update the transaction name with the parameterized route: + expect(mockedStartTransaction).toHaveBeenCalledTimes(0); + }); + + it('starts a navigation transaction when `startTransactionOnLocationChange` is true', () => { + svelteKitRoutingInstrumentation(mockedStartTransaction, false, true); + + // We emit an update to the `navigating` store to simulate the SvelteKit navigation lifecycle + // @ts-ignore This is fine because we testUtils/stores.ts defines `navigating` as a writable store + navigating.set({ + from: { route: { id: 'testNavigationOrigin' } }, + to: { route: { id: 'testNavigationDestination' } }, + }); + + // This should update the transaction name with the parameterized route: + expect(mockedStartTransaction).toHaveBeenCalledTimes(1); + expect(mockedStartTransaction).toHaveBeenCalledWith({ + name: 'testNavigationDestination', + op: 'navigation', + metadata: { + source: 'route', + }, + tags: { + 'routing.instrumentation': '@sentry/sveltekit', + }, + }); + + expect(returnedTransaction?.startChild).toHaveBeenCalledWith({ + op: 'ui.sveltekit.routing', + description: 'SvelteKit Route Change', + }); + + expect(returnedTransaction?.setTag).toHaveBeenCalledWith('from', 'testNavigationOrigin'); + + // We emit `null` here to simulate the end of the navigation lifecycle + // @ts-ignore this is fine + navigating.set(null); + + expect(routingSpanFinishSpy).toHaveBeenCalledTimes(1); + }); + + it("doesn't start a navigation transaction if navigation origin and destination are equal", () => { + svelteKitRoutingInstrumentation(mockedStartTransaction, false, true); + + // We emit an update to the `navigating` store to simulate the SvelteKit navigation lifecycle + // @ts-ignore This is fine because we testUtils/stores.ts defines `navigating` as a writable store + navigating.set({ + from: { route: { id: 'testRoute' } }, + to: { route: { id: 'testRoute' } }, + }); + + expect(mockedStartTransaction).toHaveBeenCalledTimes(0); + }); +}); diff --git a/packages/sveltekit/test/client/sdk.test.ts b/packages/sveltekit/test/client/sdk.test.ts index 8e404578883d..a8353a73df3e 100644 --- a/packages/sveltekit/test/client/sdk.test.ts +++ b/packages/sveltekit/test/client/sdk.test.ts @@ -5,6 +5,7 @@ import { SDK_VERSION, WINDOW } from '@sentry/svelte'; import { vi } from 'vitest'; import { BrowserTracing, init } from '../../src/client'; +import { svelteKitRoutingInstrumentation } from '../../src/client/router'; const svelteInit = vi.spyOn(SentrySvelte, 'init'); @@ -87,6 +88,7 @@ describe('Sentry client SDK', () => { // This is the closest we can get to unit-testing the `__SENTRY_TRACING__` tree-shaking guard // IRL, the code to add the integration would most likely be removed by the bundler. + // @ts-ignore this is fine in the test globalThis.__SENTRY_TRACING__ = false; init({ @@ -100,24 +102,35 @@ describe('Sentry client SDK', () => { expect(integrationsToInit).not.toContainEqual(expect.objectContaining({ name: 'BrowserTracing' })); expect(browserTracing).toBeUndefined(); + // @ts-ignore this is fine in the test delete globalThis.__SENTRY_TRACING__; }); - // TODO: this test is only meaningful once we have a routing instrumentation which we always want to add - // to a user-provided BrowserTracing integration (see NextJS SDK) - it.skip('Merges the user-provided BrowserTracing integration with the automatically added one', () => { + it('Merges a user-provided BrowserTracing integration with the automatically added one', () => { init({ dsn: 'https://public@dsn.ingest.sentry.io/1337', - integrations: [new BrowserTracing({ tracePropagationTargets: ['myDomain.com'] })], + integrations: [ + new BrowserTracing({ tracePropagationTargets: ['myDomain.com'], startTransactionOnLocationChange: false }), + ], enableTracing: true, }); const integrationsToInit = svelteInit.mock.calls[0][0].integrations; - const browserTracing = (getCurrentHub().getClient() as BrowserClient)?.getIntegrationById('BrowserTracing'); + + const browserTracing = (getCurrentHub().getClient() as BrowserClient)?.getIntegrationById( + 'BrowserTracing', + ) as BrowserTracing; + const options = browserTracing.options; expect(integrationsToInit).toContainEqual(expect.objectContaining({ name: 'BrowserTracing' })); expect(browserTracing).toBeDefined(); - expect((browserTracing as BrowserTracing).options.tracePropagationTargets).toEqual(['myDomain.com']); + + // This shows that the user-configured options are still here + expect(options.tracePropagationTargets).toEqual(['myDomain.com']); + expect(options.startTransactionOnLocationChange).toBe(false); + + // But we force the routing instrumentation to be ours + expect(options.routingInstrumentation).toEqual(svelteKitRoutingInstrumentation); }); }); }); diff --git a/packages/sveltekit/test/vitest.setup.ts b/packages/sveltekit/test/vitest.setup.ts new file mode 100644 index 000000000000..48c9b0e33528 --- /dev/null +++ b/packages/sveltekit/test/vitest.setup.ts @@ -0,0 +1,13 @@ +import { writable } from 'svelte/store'; +import { vi } from 'vitest'; + +export function setup() { + // mock $app/stores because vitest can't resolve this import from SvelteKit. + // Seems like $app/stores is only created at build time of a SvelteKit app. + vi.mock('$app/stores', async () => { + return { + navigating: writable(), + page: writable(), + }; + }); +} diff --git a/packages/sveltekit/vite.config.ts b/packages/sveltekit/vite.config.ts index f479704b7591..c1e4297e11ea 100644 --- a/packages/sveltekit/vite.config.ts +++ b/packages/sveltekit/vite.config.ts @@ -1,3 +1,14 @@ +import type { UserConfig } from 'vitest'; + import baseConfig from '../../vite/vite.config'; -export default baseConfig; +export default { + ...baseConfig, + test: { + // test exists, no idea why TS doesn't recognize it + // eslint-disable-next-line @typescript-eslint/no-explicit-any + ...(baseConfig as UserConfig & { test: any }).test, + environment: 'jsdom', + setupFiles: ['./test/vitest.setup.ts'], + }, +}; diff --git a/yarn.lock b/yarn.lock index 0243a33b9b4b..0407a15e420c 100644 --- a/yarn.lock +++ b/yarn.lock @@ -25270,6 +25270,11 @@ svelte@3.49.0: resolved "https://registry.yarnpkg.com/svelte/-/svelte-3.49.0.tgz#5baee3c672306de1070c3b7888fc2204e36a4029" integrity sha512-+lmjic1pApJWDfPCpUUTc1m8azDqYCG1JN9YEngrx/hUyIcFJo6VZhj0A1Ai0wqoHcEIuQy+e9tk+4uDgdtsFA== +svelte@^3.44.0: + version "3.57.0" + resolved "https://registry.yarnpkg.com/svelte/-/svelte-3.57.0.tgz#a3969cfe51f25f2a55e75f7b98dbd02c3af0980b" + integrity sha512-WMXEvF+RtAaclw0t3bPDTUe19pplMlfyKDsixbHQYgCWi9+O9VN0kXU1OppzrB9gPAvz4NALuoca2LfW2bOjTQ== + svgo@^1.0.0: version "1.3.2" resolved "https://registry.yarnpkg.com/svgo/-/svgo-1.3.2.tgz#b6dc511c063346c9e415b81e43401145b96d4167" From d265fe53190a82ffcb5b3c0b207b97e7a36130ec Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 22 Mar 2023 15:22:32 +0100 Subject: [PATCH 14/34] ref(sveltekit): Rewrite `sentryHandle` using trace func (#7559) --- packages/sveltekit/src/server/handle.ts | 67 ++++++++----------------- 1 file changed, 22 insertions(+), 45 deletions(-) diff --git a/packages/sveltekit/src/server/handle.ts b/packages/sveltekit/src/server/handle.ts index 90dda26dac55..aab69c085048 100644 --- a/packages/sveltekit/src/server/handle.ts +++ b/packages/sveltekit/src/server/handle.ts @@ -1,11 +1,11 @@ /* eslint-disable @sentry-internal/sdk/no-optional-chaining */ -import { captureException, getCurrentHub, startTransaction } from '@sentry/node'; -import type { Transaction } from '@sentry/types'; +import type { Span } from '@sentry/core'; +import { trace } from '@sentry/core'; +import { captureException } from '@sentry/node'; import { addExceptionMechanism, baggageHeaderToDynamicSamplingContext, extractTraceparentData, - isThenable, objectify, } from '@sentry/utils'; import type { Handle } from '@sveltejs/kit'; @@ -51,53 +51,30 @@ function sendErrorToSentry(e: unknown): unknown { */ export const sentryHandle: Handle = ({ event, resolve }) => { return domain.create().bind(() => { - let maybePromiseResult; - const sentryTraceHeader = event.request.headers.get('sentry-trace'); const baggageHeader = event.request.headers.get('baggage'); const traceparentData = sentryTraceHeader ? extractTraceparentData(sentryTraceHeader) : undefined; const dynamicSamplingContext = baggageHeaderToDynamicSamplingContext(baggageHeader); - // transaction could be undefined if hub extensions were not added. - const transaction: Transaction | undefined = startTransaction({ - op: 'http.server', - name: `${event.request.method} ${event.route.id}`, - status: 'ok', - ...traceparentData, - metadata: { - source: 'route', - dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext, - }, - }); - - getCurrentHub().getScope()?.setSpan(transaction); - - try { - maybePromiseResult = resolve(event); - } catch (e) { - transaction?.setStatus('internal_error'); - const sentryError = sendErrorToSentry(e); - transaction?.finish(); - throw sentryError; - } - - if (isThenable(maybePromiseResult)) { - Promise.resolve(maybePromiseResult).then( - response => { - transaction?.setHttpStatus(response.status); - transaction?.finish(); - }, - e => { - transaction?.setStatus('internal_error'); - sendErrorToSentry(e); - transaction?.finish(); + return trace( + { + op: 'http.server', + name: `${event.request.method} ${event.route.id}`, + status: 'ok', + ...traceparentData, + metadata: { + source: 'route', + dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext, }, - ); - } else { - transaction?.setHttpStatus(maybePromiseResult.status); - transaction?.finish(); - } - - return maybePromiseResult; + }, + async (span?: Span) => { + const res = await resolve(event); + if (span) { + span.setHttpStatus(res.status); + } + return res; + }, + sendErrorToSentry, + ); })(); }; From 714a9eb7f9a02a0a63273fce54a7b06e20eceb76 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 22 Mar 2023 15:34:24 +0100 Subject: [PATCH 15/34] feat(sveltekit): Add performance monitoring for server load (#7536) --- packages/sveltekit/src/server/load.ts | 45 +++++++---- packages/sveltekit/test/server/load.test.ts | 89 ++++++++++++++++++++- 2 files changed, 117 insertions(+), 17 deletions(-) diff --git a/packages/sveltekit/src/server/load.ts b/packages/sveltekit/src/server/load.ts index ef0433091a9e..5e773365e4a4 100644 --- a/packages/sveltekit/src/server/load.ts +++ b/packages/sveltekit/src/server/load.ts @@ -1,6 +1,14 @@ +/* eslint-disable @sentry-internal/sdk/no-optional-chaining */ +import { trace } from '@sentry/core'; import { captureException } from '@sentry/node'; -import { addExceptionMechanism, isThenable, objectify } from '@sentry/utils'; +import { + addExceptionMechanism, + baggageHeaderToDynamicSamplingContext, + extractTraceparentData, + objectify, +} from '@sentry/utils'; import type { HttpError, ServerLoad } from '@sveltejs/kit'; +import * as domain from 'domain'; function isHttpError(err: unknown): err is HttpError { return typeof err === 'object' && err !== null && 'status' in err && 'body' in err; @@ -44,21 +52,30 @@ function sendErrorToSentry(e: unknown): unknown { export function wrapLoadWithSentry(origLoad: ServerLoad): ServerLoad { return new Proxy(origLoad, { apply: (wrappingTarget, thisArg, args: Parameters) => { - let maybePromiseResult; + return domain.create().bind(() => { + const [event] = args; - try { - maybePromiseResult = wrappingTarget.apply(thisArg, args); - } catch (e) { - throw sendErrorToSentry(e); - } + const sentryTraceHeader = event.request.headers.get('sentry-trace'); + const baggageHeader = event.request.headers.get('baggage'); + const traceparentData = sentryTraceHeader ? extractTraceparentData(sentryTraceHeader) : undefined; + const dynamicSamplingContext = baggageHeaderToDynamicSamplingContext(baggageHeader); - if (isThenable(maybePromiseResult)) { - Promise.resolve(maybePromiseResult).then(null, e => { - sendErrorToSentry(e); - }); - } - - return maybePromiseResult; + const routeId = event.route.id; + return trace( + { + op: 'function.sveltekit.load', + name: routeId ? routeId : event.url.pathname, + status: 'ok', + ...traceparentData, + metadata: { + source: routeId ? 'route' : 'url', + dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext, + }, + }, + () => wrappingTarget.apply(thisArg, args), + sendErrorToSentry, + ); + })(); }, }); } diff --git a/packages/sveltekit/test/server/load.test.ts b/packages/sveltekit/test/server/load.test.ts index ec2503b945c4..81b067689093 100644 --- a/packages/sveltekit/test/server/load.test.ts +++ b/packages/sveltekit/test/server/load.test.ts @@ -1,3 +1,4 @@ +import { addTracingExtensions } from '@sentry/core'; import { Scope } from '@sentry/node'; import type { ServerLoad } from '@sveltejs/kit'; import { error } from '@sveltejs/kit'; @@ -20,6 +21,19 @@ vi.mock('@sentry/node', async () => { }; }); +const mockTrace = 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); + }, + }; +}); + const mockAddExceptionMechanism = vi.fn(); vi.mock('@sentry/utils', async () => { @@ -34,10 +48,42 @@ function getById(_id?: string) { throw new Error('error'); } +const MOCK_LOAD_ARGS: any = { + params: { id: '123' }, + route: { + id: '/users/[id]', + }, + url: new URL('http://localhost:3000/users/123'), + request: { + headers: { + get: (key: string) => { + if (key === 'sentry-trace') { + return '1234567890abcdef1234567890abcdef-1234567890abcdef-1'; + } + + if (key === 'baggage') { + return ( + 'sentry-environment=production,sentry-release=1.0.0,sentry-transaction=dogpark,' + + 'sentry-user_segment=segmentA,sentry-public_key=dogsarebadatkeepingsecrets,' + + 'sentry-trace_id=1234567890abcdef1234567890abcdef,sentry-sample_rate=1' + ); + } + + return null; + }, + }, + }, +}; + +beforeAll(() => { + addTracingExtensions(); +}); + describe('wrapLoadWithSentry', () => { beforeEach(() => { mockCaptureException.mockClear(); mockAddExceptionMechanism.mockClear(); + mockTrace.mockClear(); mockScope = new Scope(); }); @@ -49,12 +95,49 @@ describe('wrapLoadWithSentry', () => { } const wrappedLoad = wrapLoadWithSentry(load); - const res = wrappedLoad({ params: { id: '1' } } as any); + const res = wrappedLoad(MOCK_LOAD_ARGS); await expect(res).rejects.toThrow(); expect(mockCaptureException).toHaveBeenCalledTimes(1); }); + it('calls trace function', async () => { + async function load({ params }: Parameters[0]): Promise> { + return { + post: params.id, + }; + } + + const wrappedLoad = wrapLoadWithSentry(load); + await wrappedLoad(MOCK_LOAD_ARGS); + + expect(mockTrace).toHaveBeenCalledTimes(1); + expect(mockTrace).toHaveBeenCalledWith( + { + op: 'function.sveltekit.load', + name: '/users/[id]', + parentSampled: true, + parentSpanId: '1234567890abcdef', + status: 'ok', + traceId: '1234567890abcdef1234567890abcdef', + metadata: { + dynamicSamplingContext: { + environment: 'production', + public_key: 'dogsarebadatkeepingsecrets', + release: '1.0.0', + sample_rate: '1', + trace_id: '1234567890abcdef1234567890abcdef', + transaction: 'dogpark', + user_segment: 'segmentA', + }, + source: 'route', + }, + }, + expect.any(Function), + expect.any(Function), + ); + }); + describe('with error() helper', () => { it.each([ // [statusCode, timesCalled] @@ -75,7 +158,7 @@ describe('wrapLoadWithSentry', () => { } const wrappedLoad = wrapLoadWithSentry(load); - const res = wrappedLoad({ params: { id: '1' } } as any); + const res = wrappedLoad(MOCK_LOAD_ARGS); await expect(res).rejects.toThrow(); expect(mockCaptureException).toHaveBeenCalledTimes(times); @@ -95,7 +178,7 @@ describe('wrapLoadWithSentry', () => { } const wrappedLoad = wrapLoadWithSentry(load); - const res = wrappedLoad({ params: { id: '1' } } as any); + const res = wrappedLoad(MOCK_LOAD_ARGS); await expect(res).rejects.toThrow(); expect(addEventProcessorSpy).toBeCalledTimes(1); From 046c0c21d1454fe8365a45fc35efd4014815e5f5 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 22 Mar 2023 15:38:19 +0100 Subject: [PATCH 16/34] fix(nextjs): Add tracing extension methods in `wrapServerComponentWithSentry` (#7567) Our E2E tests were [failing](https://github.com/getsentry/sentry-javascript/actions/runs/4488942749/jobs/7894376591#step:6:6185) because apparently we don't initialize a client while building a NextJS app. This caused the tracing extension methods to not be added, resulting in `startTransaction` returning `undefined`. Not sure if this is the best way to fix this but it seems to work (h/t @timfish for the fix). --- packages/nextjs/src/server/wrapServerComponentWithSentry.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/nextjs/src/server/wrapServerComponentWithSentry.ts b/packages/nextjs/src/server/wrapServerComponentWithSentry.ts index eab32207236c..12ff9ceb5f72 100644 --- a/packages/nextjs/src/server/wrapServerComponentWithSentry.ts +++ b/packages/nextjs/src/server/wrapServerComponentWithSentry.ts @@ -1,4 +1,4 @@ -import { captureException, getCurrentHub, startTransaction } from '@sentry/core'; +import { addTracingExtensions, captureException, getCurrentHub, startTransaction } from '@sentry/core'; import { baggageHeaderToDynamicSamplingContext, extractTraceparentData } from '@sentry/utils'; import * as domain from 'domain'; @@ -11,6 +11,8 @@ export function wrapServerComponentWithSentry any> appDirComponent: F, context: ServerComponentContext, ): F { + addTracingExtensions(); + const { componentRoute, componentType } = context; // Even though users may define server components as async functions, for the client bundles From 09ee30bf068bf1e70971f63042ce873fad98bd8f Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 22 Mar 2023 16:13:12 +0100 Subject: [PATCH 17/34] feat(sveltekit): Add performance monitoring for client load (#7537) --- packages/sveltekit/src/client/load.ts | 39 +++++----- packages/sveltekit/test/client/load.test.ts | 82 +++++++++++++++++++-- 2 files changed, 96 insertions(+), 25 deletions(-) diff --git a/packages/sveltekit/src/client/load.ts b/packages/sveltekit/src/client/load.ts index fbaa5f98799f..32f310e3bd2d 100644 --- a/packages/sveltekit/src/client/load.ts +++ b/packages/sveltekit/src/client/load.ts @@ -1,6 +1,7 @@ +import { trace } from '@sentry/core'; import { captureException } from '@sentry/svelte'; -import { addExceptionMechanism, isThenable, objectify } from '@sentry/utils'; -import type { ServerLoad } from '@sveltejs/kit'; +import { addExceptionMechanism, objectify } from '@sentry/utils'; +import type { Load } from '@sveltejs/kit'; function sendErrorToSentry(e: unknown): unknown { // In case we have a primitive, wrap it in the equivalent wrapper class (string -> String, etc.) so that we can @@ -30,24 +31,24 @@ function sendErrorToSentry(e: unknown): unknown { * * @param origLoad SvelteKit user defined load function */ -export function wrapLoadWithSentry(origLoad: ServerLoad): ServerLoad { +export function wrapLoadWithSentry(origLoad: Load): Load { return new Proxy(origLoad, { - apply: (wrappingTarget, thisArg, args: Parameters) => { - let maybePromiseResult; - - try { - maybePromiseResult = wrappingTarget.apply(thisArg, args); - } catch (e) { - throw sendErrorToSentry(e); - } - - if (isThenable(maybePromiseResult)) { - Promise.resolve(maybePromiseResult).then(null, e => { - sendErrorToSentry(e); - }); - } - - return maybePromiseResult; + apply: (wrappingTarget, thisArg, args: Parameters) => { + const [event] = args; + + const routeId = event.route.id; + return trace( + { + op: 'function.sveltekit.load', + name: routeId ? routeId : event.url.pathname, + status: 'ok', + metadata: { + source: routeId ? 'route' : 'url', + }, + }, + () => wrappingTarget.apply(thisArg, args), + sendErrorToSentry, + ); }, }); } diff --git a/packages/sveltekit/test/client/load.test.ts b/packages/sveltekit/test/client/load.test.ts index 7cbfd3593c03..f4d18c9f9909 100644 --- a/packages/sveltekit/test/client/load.test.ts +++ b/packages/sveltekit/test/client/load.test.ts @@ -1,5 +1,5 @@ -import { Scope } from '@sentry/svelte'; -import type { ServerLoad } from '@sveltejs/kit'; +import { addTracingExtensions, Scope } from '@sentry/svelte'; +import type { Load } from '@sveltejs/kit'; import { vi } from 'vitest'; import { wrapLoadWithSentry } from '../../src/client/load'; @@ -19,6 +19,19 @@ vi.mock('@sentry/svelte', async () => { }; }); +const mockTrace = 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); + }, + }; +}); + const mockAddExceptionMechanism = vi.fn(); vi.mock('@sentry/utils', async () => { @@ -33,41 +46,98 @@ function getById(_id?: string) { throw new Error('error'); } +const MOCK_LOAD_ARGS: any = { + params: { id: '123' }, + route: { + id: '/users/[id]', + }, + url: new URL('http://localhost:3000/users/123'), + request: { + headers: { + get: (key: string) => { + if (key === 'sentry-trace') { + return '1234567890abcdef1234567890abcdef-1234567890abcdef-1'; + } + + if (key === 'baggage') { + return ( + 'sentry-environment=production,sentry-release=1.0.0,sentry-transaction=dogpark,' + + 'sentry-user_segment=segmentA,sentry-public_key=dogsarebadatkeepingsecrets,' + + 'sentry-trace_id=1234567890abcdef1234567890abcdef,sentry-sample_rate=1' + ); + } + + return null; + }, + }, + }, +}; + +beforeAll(() => { + addTracingExtensions(); +}); + describe('wrapLoadWithSentry', () => { beforeEach(() => { mockCaptureException.mockClear(); mockAddExceptionMechanism.mockClear(); + mockTrace.mockClear(); mockScope = new Scope(); }); it('calls captureException', async () => { - async function load({ params }: Parameters[0]): Promise> { + async function load({ params }: Parameters[0]): Promise> { return { post: getById(params.id), }; } const wrappedLoad = wrapLoadWithSentry(load); - const res = wrappedLoad({ params: { id: '1' } } as any); + const res = wrappedLoad(MOCK_LOAD_ARGS); await expect(res).rejects.toThrow(); expect(mockCaptureException).toHaveBeenCalledTimes(1); }); + it('calls trace function', async () => { + async function load({ params }: Parameters[0]): Promise> { + return { + post: params.id, + }; + } + + const wrappedLoad = wrapLoadWithSentry(load); + await wrappedLoad(MOCK_LOAD_ARGS); + + expect(mockTrace).toHaveBeenCalledTimes(1); + expect(mockTrace).toHaveBeenCalledWith( + { + op: 'function.sveltekit.load', + name: '/users/[id]', + status: 'ok', + metadata: { + source: 'route', + }, + }, + expect.any(Function), + expect.any(Function), + ); + }); + it('adds an exception mechanism', async () => { const addEventProcessorSpy = vi.spyOn(mockScope, 'addEventProcessor').mockImplementationOnce(callback => { void callback({}, { event_id: 'fake-event-id' }); return mockScope; }); - async function load({ params }: Parameters[0]): Promise> { + async function load({ params }: Parameters[0]): Promise> { return { post: getById(params.id), }; } const wrappedLoad = wrapLoadWithSentry(load); - const res = wrappedLoad({ params: { id: '1' } } as any); + const res = wrappedLoad(MOCK_LOAD_ARGS); await expect(res).rejects.toThrow(); expect(addEventProcessorSpy).toBeCalledTimes(1); From 8e78e6e5ec3b5894176555ee4e19b3368d64c969 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 22 Mar 2023 16:38:09 +0100 Subject: [PATCH 18/34] fix(react): Handle case where error.cause already defined (#7557) If `error.cause` is already defined, attempt to walk down the error chain to set the `ReactErrorBoundary` error. --- packages/react/src/errorboundary.tsx | 21 ++++- packages/react/test/errorboundary.test.tsx | 101 +++++++++++++++++++-- 2 files changed, 111 insertions(+), 11 deletions(-) diff --git a/packages/react/src/errorboundary.tsx b/packages/react/src/errorboundary.tsx index 1553028195a2..96a88cf31e73 100644 --- a/packages/react/src/errorboundary.tsx +++ b/packages/react/src/errorboundary.tsx @@ -66,6 +66,25 @@ const INITIAL_STATE = { eventId: null, }; +function setCause(error: Error & { cause?: Error }, cause: Error): void { + const seenErrors = new WeakMap(); + + function recurse(error: Error & { cause?: Error }, cause: Error): void { + // If we've already seen the error, there is a recursive loop somewhere in the error's + // cause chain. Let's just bail out then to prevent a stack overflow. + if (seenErrors.has(error)) { + return; + } + if (error.cause) { + seenErrors.set(error, true); + return recurse(error.cause, cause); + } + error.cause = cause; + } + + recurse(error, cause); +} + /** * A ErrorBoundary component that logs errors to Sentry. Requires React >= 16. * NOTE: If you are a Sentry user, and you are seeing this stack frame, it means the @@ -93,7 +112,7 @@ class ErrorBoundary extends React.Component; } -const TestApp: React.FC = ({ children, ...props }) => { +interface TestAppProps extends ErrorBoundaryProps { + errorComp?: JSX.Element; +} + +const TestApp: React.FC = ({ children, errorComp, ...props }) => { + // eslint-disable-next-line no-param-reassign + const customErrorComp = errorComp || ; const [isError, setError] = React.useState(false); return ( = ({ children, ...props }) => { } }} > - {isError ? : children} + {isError ? customErrorComp : children}