From 62cad3d885f95639b34653b01e02b1570f57416f Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 1 Feb 2024 10:12:15 +0100 Subject: [PATCH 1/5] feat(sveltekit): Add custom `browserTracingIntegration()` --- .../sveltekit/vite.config.js | 4 + .../src/client/browserTracingIntegration.ts | 153 +++++++++++++++++- packages/sveltekit/src/client/index.ts | 1 + packages/sveltekit/src/client/router.ts | 2 + packages/sveltekit/src/client/sdk.ts | 9 +- 5 files changed, 165 insertions(+), 4 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/sveltekit/vite.config.js b/dev-packages/e2e-tests/test-applications/sveltekit/vite.config.js index 1a410bee7e11..be04ee6ea470 100644 --- a/dev-packages/e2e-tests/test-applications/sveltekit/vite.config.js +++ b/dev-packages/e2e-tests/test-applications/sveltekit/vite.config.js @@ -1,3 +1,4 @@ +import { version } from '$app/environment'; import { sentrySvelteKit } from '@sentry/sveltekit'; import { sveltekit } from '@sveltejs/kit/vite'; import { defineConfig } from 'vite'; @@ -6,6 +7,9 @@ export default defineConfig({ plugins: [ sentrySvelteKit({ autoUploadSourceMaps: false, + sourceMapsUploadOptions: { + release: version, + }, }), sveltekit(), ], diff --git a/packages/sveltekit/src/client/browserTracingIntegration.ts b/packages/sveltekit/src/client/browserTracingIntegration.ts index 9968f8b6de5f..ac2384354cde 100644 --- a/packages/sveltekit/src/client/browserTracingIntegration.ts +++ b/packages/sveltekit/src/client/browserTracingIntegration.ts @@ -1,14 +1,165 @@ -import { BrowserTracing as OriginalBrowserTracing } from '@sentry/svelte'; +import { navigating, page } from '$app/stores'; +import { + SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, + SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, + getActiveSpan, + startInactiveSpan, +} from '@sentry/core'; +import { + BrowserTracing as OriginalBrowserTracing, + WINDOW, + browserTracingIntegration as originalBrowserTracingIntegration, + startBrowserTracingNavigationSpan, + startBrowserTracingPageLoadSpan, +} from '@sentry/svelte'; +import type { Client, Integration, Span } from '@sentry/types'; import { svelteKitRoutingInstrumentation } from './router'; /** * A custom BrowserTracing integration for Sveltekit. + * + * @deprecated use `browserTracingIntegration()` instead. */ export class BrowserTracing extends OriginalBrowserTracing { public constructor(options?: ConstructorParameters[0]) { super({ + // eslint-disable-next-line deprecation/deprecation routingInstrumentation: svelteKitRoutingInstrumentation, ...options, }); } } + +/** + * A custom `BrowserTracing` integration for SvelteKit. + * + * @param options + */ +export function browserTracingIntegration( + options: Parameters[0] = {}, +): Integration { + const integration = { + ...originalBrowserTracingIntegration({ + ...options, + instrumentNavigation: false, + instrumentPageLoad: false, + }), + }; + + return { + ...integration, + afterAllSetup: client => { + integration.afterAllSetup(client); + + if (options.instrumentPageLoad !== false) { + _instrumentPageload(client); + } + + if (options.instrumentNavigation !== false) { + _instrumentNavigations(client); + } + }, + }; +} + +function _instrumentPageload(client: Client): void { + const initialPath = WINDOW && WINDOW.location && WINDOW.location.pathname; + + startBrowserTracingPageLoadSpan(client, { + name: initialPath, + op: 'pageload', + origin: 'auto.pageload.sveltekit', + description: initialPath, + tags: { + 'routing.instrumentation': '@sentry/sveltekit', + }, + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', + }, + }); + + const pageloadSpan = getActiveSpan(); + + page.subscribe(page => { + if (!page) { + return; + } + + const routeId = page.route && page.route.id; + + if (pageloadSpan && routeId) { + pageloadSpan.updateName(routeId); + pageloadSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route'); + } + }); +} + +/** + * Use the `navigating` store to start a transaction on navigations. + */ +function _instrumentNavigations(client: Client): void { + let routingSpan: Span | undefined = undefined; + let activeSpan: Span | 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.end(); + routingSpan = undefined; + } + return; + } + + const from = navigation.from; + const to = navigation.to; + + // for the origin we can fall back to window.location.pathname because in this emission, it still is set to the origin path + const rawRouteOrigin = (from && from.url.pathname) || (WINDOW && WINDOW.location && WINDOW.location.pathname); + + const rawRouteDestination = to && to.url.pathname; + + // We don't want to create transactions for navigations of same origin and destination. + // We need to look at the raw URL here because parameterized routes can still differ in their raw parameters. + if (rawRouteOrigin === rawRouteDestination) { + return; + } + + const parameterizedRouteOrigin = from && from.route.id; + const parameterizedRouteDestination = to && to.route.id; + + activeSpan = getActiveSpan(); + + if (!activeSpan) { + startBrowserTracingNavigationSpan(client, { + name: parameterizedRouteDestination || rawRouteDestination || 'unknown', + op: 'navigation', + origin: 'auto.navigation.sveltekit', + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: parameterizedRouteDestination ? 'route' : 'url', + }, + tags: { + 'routing.instrumentation': '@sentry/sveltekit', + }, + }); + activeSpan = getActiveSpan(); + } + + if (activeSpan) { + if (routingSpan) { + // If a routing span is still open from a previous navigation, we finish it. + routingSpan.end(); + } + routingSpan = startInactiveSpan({ + op: 'ui.sveltekit.routing', + name: 'SvelteKit Route Change', + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.ui.sveltekit', + }, + }); + activeSpan.setAttribute('sentry.sveltekit.navigation.from', parameterizedRouteOrigin || undefined); + } + }); +} diff --git a/packages/sveltekit/src/client/index.ts b/packages/sveltekit/src/client/index.ts index f60a353d8b1d..558526b1f318 100644 --- a/packages/sveltekit/src/client/index.ts +++ b/packages/sveltekit/src/client/index.ts @@ -3,3 +3,4 @@ export * from '@sentry/svelte'; export { init } from './sdk'; export { handleErrorWithSentry } from './handleError'; export { wrapLoadWithSentry } from './load'; +export { browserTracingIntegration } from './browserTracingIntegration'; diff --git a/packages/sveltekit/src/client/router.ts b/packages/sveltekit/src/client/router.ts index 2b36d4adb4f2..d1d9d3d33d35 100644 --- a/packages/sveltekit/src/client/router.ts +++ b/packages/sveltekit/src/client/router.ts @@ -17,6 +17,8 @@ const DEFAULT_TAGS = { * @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`) + * + * @deprecated use `browserTracingIntegration()` instead which includes SvelteKit-specific routing instrumentation out of the box. */ export function svelteKitRoutingInstrumentation( startTransactionFn: (context: TransactionContext) => T | undefined, diff --git a/packages/sveltekit/src/client/sdk.ts b/packages/sveltekit/src/client/sdk.ts index 920b2db75193..76b75bb4d300 100644 --- a/packages/sveltekit/src/client/sdk.ts +++ b/packages/sveltekit/src/client/sdk.ts @@ -1,10 +1,13 @@ import { applySdkMetadata, hasTracingEnabled } from '@sentry/core'; -import type { BrowserOptions, browserTracingIntegration } from '@sentry/svelte'; +import { BrowserOptions } from '@sentry/svelte'; import { getDefaultIntegrations as getDefaultSvelteIntegrations } from '@sentry/svelte'; import { WINDOW, getCurrentScope, init as initSvelteSdk } from '@sentry/svelte'; import type { Integration } from '@sentry/types'; -import { BrowserTracing } from './browserTracingIntegration'; +import { + BrowserTracing, + browserTracingIntegration as svelteKitBrowserTracingIntegration, +} from './browserTracingIntegration'; type WindowWithSentryFetchProxy = typeof WINDOW & { _sentryFetchProxy?: typeof fetch; @@ -97,7 +100,7 @@ function getDefaultIntegrations(options: BrowserOptions): Integration[] | undefi // will get treeshaken away if (typeof __SENTRY_TRACING__ === 'undefined' || __SENTRY_TRACING__) { if (hasTracingEnabled(options)) { - return [...getDefaultSvelteIntegrations(options), new BrowserTracing()]; + return [...getDefaultSvelteIntegrations(options), svelteKitBrowserTracingIntegration()]; } } From 9c86f1c0c0bfc53731159a0f49144f90aad8599b Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 1 Feb 2024 10:25:30 +0100 Subject: [PATCH 2/5] rm vite config e2e --- .../e2e-tests/test-applications/sveltekit/vite.config.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/sveltekit/vite.config.js b/dev-packages/e2e-tests/test-applications/sveltekit/vite.config.js index be04ee6ea470..1a410bee7e11 100644 --- a/dev-packages/e2e-tests/test-applications/sveltekit/vite.config.js +++ b/dev-packages/e2e-tests/test-applications/sveltekit/vite.config.js @@ -1,4 +1,3 @@ -import { version } from '$app/environment'; import { sentrySvelteKit } from '@sentry/sveltekit'; import { sveltekit } from '@sveltejs/kit/vite'; import { defineConfig } from 'vite'; @@ -7,9 +6,6 @@ export default defineConfig({ plugins: [ sentrySvelteKit({ autoUploadSourceMaps: false, - sourceMapsUploadOptions: { - release: version, - }, }), sveltekit(), ], From aaf9f3f3d36fea373fd08d218d4e8ccd7df97cf1 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 1 Feb 2024 10:29:01 +0100 Subject: [PATCH 3/5] lint, deprecations, etc --- packages/sveltekit/src/client/sdk.ts | 7 ++++++- packages/sveltekit/test/client/router.test.ts | 14 +++++++------- packages/sveltekit/test/client/sdk.test.ts | 4 ++-- 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/packages/sveltekit/src/client/sdk.ts b/packages/sveltekit/src/client/sdk.ts index 76b75bb4d300..b0dc7ee6af2d 100644 --- a/packages/sveltekit/src/client/sdk.ts +++ b/packages/sveltekit/src/client/sdk.ts @@ -1,5 +1,5 @@ import { applySdkMetadata, hasTracingEnabled } from '@sentry/core'; -import { BrowserOptions } from '@sentry/svelte'; +import type { BrowserOptions, browserTracingIntegration } from '@sentry/svelte'; import { getDefaultIntegrations as getDefaultSvelteIntegrations } from '@sentry/svelte'; import { WINDOW, getCurrentScope, init as initSvelteSdk } from '@sentry/svelte'; import type { Integration } from '@sentry/types'; @@ -67,6 +67,7 @@ function fixBrowserTracingIntegration(options: BrowserOptions): void { function isNewBrowserTracingIntegration( integration: Integration, ): integration is Integration & { options?: Parameters[0] } { + // eslint-disable-next-line deprecation/deprecation return !!integration.afterAllSetup && !!(integration as BrowserTracing).options; } @@ -80,15 +81,19 @@ function maybeUpdateBrowserTracingIntegration(integrations: Integration[]): Inte // If `browserTracingIntegration()` was added, we need to force-convert it to our custom one if (isNewBrowserTracingIntegration(browserTracing)) { const { options } = browserTracing; + // eslint-disable-next-line deprecation/deprecation integrations[integrations.indexOf(browserTracing)] = new BrowserTracing(options); } // If BrowserTracing was added, but it is not our forked version, // replace it with our forked version with the same options + // eslint-disable-next-line deprecation/deprecation if (!(browserTracing instanceof BrowserTracing)) { + // eslint-disable-next-line deprecation/deprecation const options: ConstructorParameters[0] = (browserTracing as BrowserTracing).options; // This option is overwritten by the custom integration delete options.routingInstrumentation; + // eslint-disable-next-line deprecation/deprecation integrations[integrations.indexOf(browserTracing)] = new BrowserTracing(options); } diff --git a/packages/sveltekit/test/client/router.test.ts b/packages/sveltekit/test/client/router.test.ts index 29037c28461f..a359a9dedbf0 100644 --- a/packages/sveltekit/test/client/router.test.ts +++ b/packages/sveltekit/test/client/router.test.ts @@ -49,6 +49,7 @@ describe('sveltekitRoutingInstrumentation', () => { }); it("starts a pageload transaction when it's called with default params", () => { + // eslint-disable-next-line deprecation/deprecation svelteKitRoutingInstrumentation(mockedStartTransaction); expect(mockedStartTransaction).toHaveBeenCalledTimes(1); @@ -66,7 +67,6 @@ describe('sveltekitRoutingInstrumentation', () => { }); // We emit an update to the `page` store to simulate the SvelteKit router lifecycle - // @ts-expect-error 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: @@ -76,15 +76,16 @@ describe('sveltekitRoutingInstrumentation', () => { }); it("doesn't start a pageload transaction if `startTransactionOnPageLoad` is false", () => { + // eslint-disable-next-line deprecation/deprecation svelteKitRoutingInstrumentation(mockedStartTransaction, false); expect(mockedStartTransaction).toHaveBeenCalledTimes(0); }); it("doesn't start a navigation transaction when `startTransactionOnLocationChange` is false", () => { + // eslint-disable-next-line deprecation/deprecation svelteKitRoutingInstrumentation(mockedStartTransaction, false, false); // We emit an update to the `navigating` store to simulate the SvelteKit navigation lifecycle - // @ts-expect-error This is fine because we testUtils/stores.ts defines `navigating` as a writable store navigating.set({ from: { route: { id: '/users' }, url: { pathname: '/users' } }, to: { route: { id: '/users/[id]' }, url: { pathname: '/users/7762' } }, @@ -95,10 +96,10 @@ describe('sveltekitRoutingInstrumentation', () => { }); it('starts a navigation transaction when `startTransactionOnLocationChange` is true', () => { + // eslint-disable-next-line deprecation/deprecation svelteKitRoutingInstrumentation(mockedStartTransaction, false, true); // We emit an update to the `navigating` store to simulate the SvelteKit navigation lifecycle - // @ts-expect-error This is fine because we testUtils/stores.ts defines `navigating` as a writable store navigating.set({ from: { route: { id: '/users' }, url: { pathname: '/users' } }, to: { route: { id: '/users/[id]' }, url: { pathname: '/users/7762' } }, @@ -127,7 +128,6 @@ describe('sveltekitRoutingInstrumentation', () => { expect(returnedTransaction?.setTag).toHaveBeenCalledWith('from', '/users'); // We emit `null` here to simulate the end of the navigation lifecycle - // @ts-expect-error this is fine navigating.set(null); expect(routingSpanFinishSpy).toHaveBeenCalledTimes(1); @@ -135,10 +135,10 @@ describe('sveltekitRoutingInstrumentation', () => { describe('handling same origin and destination navigations', () => { it("doesn't start a navigation transaction if the raw navigation origin and destination are equal", () => { + // eslint-disable-next-line deprecation/deprecation svelteKitRoutingInstrumentation(mockedStartTransaction, false, true); // We emit an update to the `navigating` store to simulate the SvelteKit navigation lifecycle - // @ts-expect-error This is fine because we testUtils/stores.ts defines `navigating` as a writable store navigating.set({ from: { route: { id: '/users/[id]' }, url: { pathname: '/users/7762' } }, to: { route: { id: '/users/[id]' }, url: { pathname: '/users/7762' } }, @@ -148,9 +148,9 @@ describe('sveltekitRoutingInstrumentation', () => { }); it('starts a navigation transaction if the raw navigation origin and destination are not equal', () => { + // eslint-disable-next-line deprecation/deprecation svelteKitRoutingInstrumentation(mockedStartTransaction, false, true); - // @ts-expect-error This is fine navigating.set({ from: { route: { id: '/users/[id]' }, url: { pathname: '/users/7762' } }, to: { route: { id: '/users/[id]' }, url: { pathname: '/users/223412' } }, @@ -179,11 +179,11 @@ describe('sveltekitRoutingInstrumentation', () => { }); it('falls back to `window.location.pathname` to determine the raw origin', () => { + // eslint-disable-next-line deprecation/deprecation svelteKitRoutingInstrumentation(mockedStartTransaction, false, true); // window.location.pathame is "/" in tests - // @ts-expect-error This is fine navigating.set({ to: { route: {}, url: { pathname: '/' } }, }); diff --git a/packages/sveltekit/test/client/sdk.test.ts b/packages/sveltekit/test/client/sdk.test.ts index 4b0afb85bcd8..c6eca47e5e79 100644 --- a/packages/sveltekit/test/client/sdk.test.ts +++ b/packages/sveltekit/test/client/sdk.test.ts @@ -82,7 +82,6 @@ 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-expect-error this is fine in the test globalThis.__SENTRY_TRACING__ = false; init({ @@ -93,7 +92,6 @@ describe('Sentry client SDK', () => { const browserTracing = getClient()?.getIntegrationByName('BrowserTracing'); expect(browserTracing).toBeUndefined(); - // @ts-expect-error this is fine in the test delete globalThis.__SENTRY_TRACING__; }); @@ -113,6 +111,7 @@ describe('Sentry client SDK', () => { expect(options.finalTimeout).toEqual(10); // But we force the routing instrumentation to be ours + // eslint-disable-next-line deprecation/deprecation expect(options.routingInstrumentation).toEqual(svelteKitRoutingInstrumentation); }); @@ -132,6 +131,7 @@ describe('Sentry client SDK', () => { expect(options.finalTimeout).toEqual(10); // But we force the routing instrumentation to be ours + // eslint-disable-next-line deprecation/deprecation expect(options.routingInstrumentation).toEqual(svelteKitRoutingInstrumentation); }); }); From 97896f9d1d90ec22e5775e2f85e8ad20409c57b5 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 2 Feb 2024 10:54:07 +0100 Subject: [PATCH 4/5] add tests --- .vscode/settings.json | 3 +- .../src/client/browserTracingIntegration.ts | 19 +- packages/sveltekit/src/client/router.ts | 1 + .../client/browserTracingIntegration.test.ts | 285 ++++++++++++++++++ 4 files changed, 297 insertions(+), 11 deletions(-) create mode 100644 packages/sveltekit/test/client/browserTracingIntegration.test.ts diff --git a/.vscode/settings.json b/.vscode/settings.json index 2950621966b9..154a8937601b 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -38,5 +38,6 @@ "editor.codeActionsOnSave": { "source.organizeImports.biome": "explicit", }, - "editor.defaultFormatter": "biomejs.biome" + "editor.defaultFormatter": "biomejs.biome", + "cSpell.words": ["pageloads"] } diff --git a/packages/sveltekit/src/client/browserTracingIntegration.ts b/packages/sveltekit/src/client/browserTracingIntegration.ts index ac2384354cde..f81caf414c6c 100644 --- a/packages/sveltekit/src/client/browserTracingIntegration.ts +++ b/packages/sveltekit/src/client/browserTracingIntegration.ts @@ -1,16 +1,13 @@ import { navigating, page } from '$app/stores'; -import { - SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, - SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, - getActiveSpan, - startInactiveSpan, -} from '@sentry/core'; +import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '@sentry/core'; import { BrowserTracing as OriginalBrowserTracing, WINDOW, browserTracingIntegration as originalBrowserTracingIntegration, + getActiveSpan, startBrowserTracingNavigationSpan, startBrowserTracingPageLoadSpan, + startInactiveSpan, } from '@sentry/svelte'; import type { Client, Integration, Span } from '@sentry/types'; import { svelteKitRoutingInstrumentation } from './router'; @@ -18,7 +15,9 @@ import { svelteKitRoutingInstrumentation } from './router'; /** * A custom BrowserTracing integration for Sveltekit. * - * @deprecated use `browserTracingIntegration()` instead. + * @deprecated use `browserTracingIntegration()` instead. The new `browserTracingIntegration()` + * includes SvelteKit-specific routing instrumentation out of the box. Therefore there's no need + * to pass in `svelteKitRoutingInstrumentation` anymore. */ export class BrowserTracing extends OriginalBrowserTracing { public constructor(options?: ConstructorParameters[0]) { @@ -68,12 +67,12 @@ function _instrumentPageload(client: Client): void { startBrowserTracingPageLoadSpan(client, { name: initialPath, op: 'pageload', - origin: 'auto.pageload.sveltekit', description: initialPath, tags: { 'routing.instrumentation': '@sentry/sveltekit', }, attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.pageload.sveltekit', [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', }, }); @@ -98,7 +97,7 @@ function _instrumentPageload(client: Client): void { * Use the `navigating` store to start a transaction on navigations. */ function _instrumentNavigations(client: Client): void { - let routingSpan: Span | undefined = undefined; + let routingSpan: Span | undefined; let activeSpan: Span | undefined; navigating.subscribe(navigation => { @@ -136,8 +135,8 @@ function _instrumentNavigations(client: Client): void { startBrowserTracingNavigationSpan(client, { name: parameterizedRouteDestination || rawRouteDestination || 'unknown', op: 'navigation', - origin: 'auto.navigation.sveltekit', attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.sveltekit', [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: parameterizedRouteDestination ? 'route' : 'url', }, tags: { diff --git a/packages/sveltekit/src/client/router.ts b/packages/sveltekit/src/client/router.ts index d1d9d3d33d35..593eeb97b1a2 100644 --- a/packages/sveltekit/src/client/router.ts +++ b/packages/sveltekit/src/client/router.ts @@ -19,6 +19,7 @@ const DEFAULT_TAGS = { * @param startTransactionOnLocationChange controls if navigation transactions should be created (defauls to `true`) * * @deprecated use `browserTracingIntegration()` instead which includes SvelteKit-specific routing instrumentation out of the box. + * Therefore, this function will be removed in v8. */ export function svelteKitRoutingInstrumentation( startTransactionFn: (context: TransactionContext) => T | undefined, diff --git a/packages/sveltekit/test/client/browserTracingIntegration.test.ts b/packages/sveltekit/test/client/browserTracingIntegration.test.ts new file mode 100644 index 000000000000..83984c0b19f5 --- /dev/null +++ b/packages/sveltekit/test/client/browserTracingIntegration.test.ts @@ -0,0 +1,285 @@ +/* eslint-disable @typescript-eslint/unbound-method */ +import type { Span } from '@sentry/types'; +import { writable } from 'svelte/store'; +import { vi } from 'vitest'; + +import { navigating, page } from '$app/stores'; + +import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '@sentry/core'; +import { browserTracingIntegration } from '../../src/client'; + +import * as SentrySvelte from '@sentry/svelte'; + +// 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('browserTracingIntegration', () => { + const svelteBrowserTracingIntegrationSpy = vi.spyOn(SentrySvelte, 'browserTracingIntegration'); + + let createdRootSpan: Partial | undefined; + + // @ts-expect-error - only returning a partial span here, that's fine + vi.spyOn(SentrySvelte, 'getActiveSpan').mockImplementation(() => { + return createdRootSpan; + }); + + const startBrowserTracingPageLoadSpanSpy = vi + .spyOn(SentrySvelte, 'startBrowserTracingPageLoadSpan') + .mockImplementation((_client, txnCtx) => { + createdRootSpan = { + ...txnCtx, + updateName: vi.fn(), + setAttribute: vi.fn(), + startChild: vi.fn().mockImplementation(ctx => { + return { ...mockedRoutingSpan, ...ctx }; + }), + setTag: vi.fn(), + }; + }); + + const startBrowserTracingNavigationSpanSpy = vi + .spyOn(SentrySvelte, 'startBrowserTracingNavigationSpan') + .mockImplementation((_client, txnCtx) => { + createdRootSpan = { + ...txnCtx, + updateName: vi.fn(), + setAttribute: vi.fn(), + setTag: vi.fn(), + }; + }); + + const fakeClient = { getOptions: () => undefined }; + + const mockedRoutingSpan = { + end: () => {}, + }; + + const routingSpanEndSpy = vi.spyOn(mockedRoutingSpan, 'end'); + + // @ts-expect-error - mockedRoutingSpan is not a complete Span, that's fine + const startInactiveSpanSpy = vi.spyOn(SentrySvelte, 'startInactiveSpan').mockImplementation(() => mockedRoutingSpan); + + beforeEach(() => { + createdRootSpan = undefined; + navigatingStore = writable(); + vi.clearAllMocks(); + }); + + it('implements required hooks', () => { + const integration = browserTracingIntegration(); + expect(integration.name).toEqual('BrowserTracing'); + expect(integration.setupOnce).toBeDefined(); + expect(integration.afterAllSetup).toBeDefined(); + }); + + it('passes on the options to the original integration', () => { + browserTracingIntegration({ enableLongTask: true, idleTimeout: 4242 }); + expect(svelteBrowserTracingIntegrationSpy).toHaveBeenCalledTimes(1); + expect(svelteBrowserTracingIntegrationSpy).toHaveBeenCalledWith({ + enableLongTask: true, + idleTimeout: 4242, + instrumentNavigation: false, + instrumentPageLoad: false, + }); + }); + + it('always disables `instrumentNavigation` and `instrumentPageLoad` in the original integration', () => { + browserTracingIntegration({ instrumentNavigation: true, instrumentPageLoad: true }); + expect(svelteBrowserTracingIntegrationSpy).toHaveBeenCalledTimes(1); + // This is fine and expected because we don't want to start the default instrumentation + // SvelteKit's browserTracingIntegration takes care of instrumenting pageloads and navigations on its own. + expect(svelteBrowserTracingIntegrationSpy).toHaveBeenCalledWith({ + instrumentNavigation: false, + instrumentPageLoad: false, + }); + }); + + it("starts a pageload span when it's called with default params", () => { + const integration = browserTracingIntegration(); + // @ts-expect-error - the fakeClient doesn't satisfy Client but that's fine + integration.afterAllSetup(fakeClient); + + expect(startBrowserTracingPageLoadSpanSpy).toHaveBeenCalledTimes(1); + expect(startBrowserTracingPageLoadSpanSpy).toHaveBeenCalledWith(fakeClient, { + name: '/', + op: 'pageload', + description: '/', + tags: { + 'routing.instrumentation': '@sentry/sveltekit', + }, + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.pageload.sveltekit', + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', + }, + }); + + // We emit an update to the `page` store to simulate the SvelteKit router lifecycle + // @ts-expect-error - page is a writable but the types say it's just readable + page.set({ route: { id: 'testRoute' } }); + + // This should update the transaction name with the parameterized route: + expect(createdRootSpan?.updateName).toHaveBeenCalledTimes(1); + expect(createdRootSpan?.updateName).toHaveBeenCalledWith('testRoute'); + expect(createdRootSpan?.setAttribute).toHaveBeenCalledWith(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route'); + }); + + it("doesn't start a pageload span if `instrumentPageLoad` is false", () => { + const integration = browserTracingIntegration({ + instrumentPageLoad: false, + }); + // @ts-expect-error - the fakeClient doesn't satisfy Client but that's fine + integration.afterAllSetup(fakeClient); + + expect(startBrowserTracingPageLoadSpanSpy).toHaveBeenCalledTimes(0); + }); + + it("doesn't start a navigation span when `instrumentNavigation` is false", () => { + const integration = browserTracingIntegration({ + instrumentNavigation: false, + }); + // @ts-expect-error - the fakeClient doesn't satisfy Client but that's fine + integration.afterAllSetup(fakeClient); + + // We emit an update to the `navigating` store to simulate the SvelteKit navigation lifecycle + // @ts-expect-error - page is a writable but the types say it's just readable + navigating.set({ + from: { route: { id: '/users' }, url: { pathname: '/users' } }, + to: { route: { id: '/users/[id]' }, url: { pathname: '/users/7762' } }, + }); + + // This should update the transaction name with the parameterized route: + expect(startBrowserTracingNavigationSpanSpy).toHaveBeenCalledTimes(0); + }); + + it('starts a navigation span when `startTransactionOnLocationChange` is true', () => { + const integration = browserTracingIntegration({ + instrumentPageLoad: false, + }); + // @ts-expect-error - the fakeClient doesn't satisfy Client but that's fine + integration.afterAllSetup(fakeClient); + + // We emit an update to the `navigating` store to simulate the SvelteKit navigation lifecycle + // @ts-expect-error - page is a writable but the types say it's just readable + navigating.set({ + from: { route: { id: '/users' }, url: { pathname: '/users' } }, + to: { route: { id: '/users/[id]' }, url: { pathname: '/users/7762' } }, + }); + + // This should update the transaction name with the parameterized route: + expect(startBrowserTracingNavigationSpanSpy).toHaveBeenCalledTimes(1); + expect(startBrowserTracingNavigationSpanSpy).toHaveBeenCalledWith(fakeClient, { + name: '/users/[id]', + op: 'navigation', + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.sveltekit', + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', + }, + tags: { + 'routing.instrumentation': '@sentry/sveltekit', + }, + }); + + // eslint-disable-next-line deprecation/deprecation + expect(startInactiveSpanSpy).toHaveBeenCalledWith({ + op: 'ui.sveltekit.routing', + name: 'SvelteKit Route Change', + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.ui.sveltekit', + }, + }); + + // eslint-disable-next-line deprecation/deprecation + expect(createdRootSpan?.setAttribute).toHaveBeenCalledWith('sentry.sveltekit.navigation.from', '/users'); + + // We emit `null` here to simulate the end of the navigation lifecycle + // @ts-expect-error - page is a writable but the types say it's just readable + navigating.set(null); + + expect(routingSpanEndSpy).toHaveBeenCalledTimes(1); + }); + + describe('handling same origin and destination navigations', () => { + it("doesn't start a navigation span if the raw navigation origin and destination are equal", () => { + const integration = browserTracingIntegration({ + instrumentPageLoad: false, + }); + // @ts-expect-error - the fakeClient doesn't satisfy Client but that's fine + integration.afterAllSetup(fakeClient); + + // We emit an update to the `navigating` store to simulate the SvelteKit navigation lifecycle + // @ts-expect-error - page is a writable but the types say it's just readable + navigating.set({ + from: { route: { id: '/users/[id]' }, url: { pathname: '/users/7762' } }, + to: { route: { id: '/users/[id]' }, url: { pathname: '/users/7762' } }, + }); + + expect(startBrowserTracingNavigationSpanSpy).toHaveBeenCalledTimes(0); + }); + + it('starts a navigation transaction if the raw navigation origin and destination are not equal', () => { + const integration = browserTracingIntegration({ + instrumentPageLoad: false, + }); + // @ts-expect-error - the fakeClient doesn't satisfy Client but that's fine + integration.afterAllSetup(fakeClient); + + // @ts-expect-error - page is a writable but the types say it's just readable + navigating.set({ + from: { route: { id: '/users/[id]' }, url: { pathname: '/users/7762' } }, + to: { route: { id: '/users/[id]' }, url: { pathname: '/users/223412' } }, + }); + + expect(startBrowserTracingNavigationSpanSpy).toHaveBeenCalledTimes(1); + expect(startBrowserTracingNavigationSpanSpy).toHaveBeenCalledWith(fakeClient, { + name: '/users/[id]', + op: 'navigation', + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.sveltekit', + }, + tags: { + 'routing.instrumentation': '@sentry/sveltekit', + }, + }); + + // eslint-disable-next-line deprecation/deprecation + expect(startInactiveSpanSpy).toHaveBeenCalledWith({ + op: 'ui.sveltekit.routing', + name: 'SvelteKit Route Change', + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.ui.sveltekit', + }, + }); + + // eslint-disable-next-line deprecation/deprecation + expect(createdRootSpan?.setAttribute).toHaveBeenCalledWith('sentry.sveltekit.navigation.from', '/users/[id]'); + }); + + it('falls back to `window.location.pathname` to determine the raw origin', () => { + const integration = browserTracingIntegration({ + instrumentPageLoad: false, + }); + // @ts-expect-error - the fakeClient doesn't satisfy Client but that's fine + integration.afterAllSetup(fakeClient); + + // window.location.pathame is "/" in tests + + // @ts-expect-error - page is a writable but the types say it's just readable + navigating.set({ + to: { route: {}, url: { pathname: '/' } }, + }); + + expect(startBrowserTracingNavigationSpanSpy).toHaveBeenCalledTimes(0); + }); + }); +}); From 230e1f02dfc13aa4f65ed79f6841de65377a779f Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 2 Feb 2024 12:34:44 +0100 Subject: [PATCH 5/5] cleanup --- .vscode/settings.json | 3 +-- packages/sveltekit/src/client/browserTracingIntegration.ts | 2 -- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/.vscode/settings.json b/.vscode/settings.json index 154a8937601b..2950621966b9 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -38,6 +38,5 @@ "editor.codeActionsOnSave": { "source.organizeImports.biome": "explicit", }, - "editor.defaultFormatter": "biomejs.biome", - "cSpell.words": ["pageloads"] + "editor.defaultFormatter": "biomejs.biome" } diff --git a/packages/sveltekit/src/client/browserTracingIntegration.ts b/packages/sveltekit/src/client/browserTracingIntegration.ts index f81caf414c6c..ffabc2a374a7 100644 --- a/packages/sveltekit/src/client/browserTracingIntegration.ts +++ b/packages/sveltekit/src/client/browserTracingIntegration.ts @@ -31,8 +31,6 @@ export class BrowserTracing extends OriginalBrowserTracing { /** * A custom `BrowserTracing` integration for SvelteKit. - * - * @param options */ export function browserTracingIntegration( options: Parameters[0] = {},