From a52e6f4f04cf481ff787afb2a1976ee8fa84c6bb Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 14 Feb 2024 11:36:07 +0100 Subject: [PATCH 1/6] wip --- .../sveltekit-2/package.json | 1 + .../sveltekit-2/src/routes/+layout.svelte | 17 ++++-- .../sveltekit-2/src/routes/+page.svelte | 5 +- .../sveltekit-2/src/routes/redirect1/+page.ts | 5 ++ .../sveltekit-2/src/routes/redirect2/+page.ts | 5 ++ .../sveltekit-2/test/performance.test.ts | 47 ++++++++++++++++ .../src/client/browserTracingIntegration.ts | 54 +++++++++---------- 7 files changed, 98 insertions(+), 36 deletions(-) create mode 100644 dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/redirect1/+page.ts create mode 100644 dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/redirect2/+page.ts diff --git a/dev-packages/e2e-tests/test-applications/sveltekit-2/package.json b/dev-packages/e2e-tests/test-applications/sveltekit-2/package.json index a323c3c415be..dbdb431960de 100644 --- a/dev-packages/e2e-tests/test-applications/sveltekit-2/package.json +++ b/dev-packages/e2e-tests/test-applications/sveltekit-2/package.json @@ -6,6 +6,7 @@ "dev": "vite dev", "build": "vite build", "preview": "vite preview", + "proxy": "ts-node-script start-event-proxy.ts", "clean": "npx rimraf node_modules,pnpm-lock.yaml", "check": "svelte-kit sync && svelte-check --tsconfig ./tsconfig.json", "check:watch": "svelte-kit sync && svelte-check --tsconfig ./tsconfig.json --watch", diff --git a/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/+layout.svelte b/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/+layout.svelte index 08c4b6468a93..5c4ee5f2ea76 100644 --- a/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/+layout.svelte +++ b/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/+layout.svelte @@ -1,10 +1,17 @@

Sveltekit E2E Test app

diff --git a/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/+page.svelte b/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/+page.svelte index aeb12d58603f..29ce9ec693a8 100644 --- a/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/+page.svelte +++ b/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/+page.svelte @@ -15,7 +15,7 @@ Server Route error
  • - Route with Params + Route with Params
  • Route with Server Load @@ -23,4 +23,7 @@
  • Route with fetch in universal load
    • +
  • + Redirect +
    • diff --git a/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/redirect1/+page.ts b/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/redirect1/+page.ts new file mode 100644 index 000000000000..3f462bf810fd --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/redirect1/+page.ts @@ -0,0 +1,5 @@ +import { redirect } from '@sveltejs/kit'; + +export const load = async () => { + redirect(301, '/redirect2'); +}; diff --git a/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/redirect2/+page.ts b/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/redirect2/+page.ts new file mode 100644 index 000000000000..99a810761d18 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/redirect2/+page.ts @@ -0,0 +1,5 @@ +import { redirect } from '@sveltejs/kit'; + +export const load = async () => { + redirect(301, '/users/789'); +}; diff --git a/dev-packages/e2e-tests/test-applications/sveltekit-2/test/performance.test.ts b/dev-packages/e2e-tests/test-applications/sveltekit-2/test/performance.test.ts index aed2040392e7..c76f8990a78b 100644 --- a/dev-packages/e2e-tests/test-applications/sveltekit-2/test/performance.test.ts +++ b/dev-packages/e2e-tests/test-applications/sveltekit-2/test/performance.test.ts @@ -184,4 +184,51 @@ test.describe('performance events', () => { }, }); }); + + test.only('captures a navigation transaction directly after pageload', async ({ page }) => { + await page.goto('/'); + + const clientPageloadTxnPromise = waitForTransaction('sveltekit-2', txnEvent => { + return txnEvent?.contexts?.trace?.op === 'pageload'; + }); + + const clientNavigationTxnPromise = waitForTransaction('sveltekit-2', txnEvent => { + console.log(txnEvent); + return txnEvent?.contexts?.trace?.op === 'navigation'; + }); + + const navigationClickPromise = page.locator('#routeWithParamsLink').click(); + + const [pageloadTxnEvent, navigationTxnEvent, _] = await Promise.all([ + clientPageloadTxnPromise, + clientNavigationTxnPromise, + navigationClickPromise, + ]); + + expect(pageloadTxnEvent).toMatchObject({ + transaction: '/', + tags: { runtime: 'browser' }, + transaction_info: { source: 'route' }, + type: 'transaction', + contexts: { + trace: { + op: 'pageload', + origin: 'auto.pageload.sveltekit', + }, + }, + }); + + expect(navigationTxnEvent).toMatchObject({ + transaction: '/users/[id]', + tags: { runtime: 'browser' }, + transaction_info: { source: 'route' }, + type: 'transaction', + contexts: { + trace: { + op: 'http.server', + origin: 'auto.http.sveltekit', + }, + }, + }); + }); }); diff --git a/packages/sveltekit/src/client/browserTracingIntegration.ts b/packages/sveltekit/src/client/browserTracingIntegration.ts index 5e80cdc92f28..060c5c297137 100644 --- a/packages/sveltekit/src/client/browserTracingIntegration.ts +++ b/packages/sveltekit/src/client/browserTracingIntegration.ts @@ -98,7 +98,7 @@ function _instrumentPageload(client: Client): void { */ function _instrumentNavigations(client: Client): void { let routingSpan: Span | undefined; - let activeSpan: Span | undefined; + let activeNavigationSpan: Span | undefined; navigating.subscribe(navigation => { if (!navigation) { @@ -129,36 +129,30 @@ function _instrumentNavigations(client: Client): void { const parameterizedRouteOrigin = from && from.route.id; const parameterizedRouteDestination = to && to.route.id; - activeSpan = getActiveSpan(); - - if (!activeSpan) { - startBrowserTracingNavigationSpan(client, { - name: parameterizedRouteDestination || rawRouteDestination || 'unknown', - op: 'navigation', - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.sveltekit', - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: parameterizedRouteDestination ? 'route' : 'url', - }, - tags: { - 'routing.instrumentation': '@sentry/sveltekit', - }, - }); - activeSpan = getActiveSpan(); - } + // if (!activeNavigationSpan) { + activeNavigationSpan = startBrowserTracingNavigationSpan(client, { + name: parameterizedRouteDestination || rawRouteDestination || 'unknown', + op: 'navigation', + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.sveltekit', + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: parameterizedRouteDestination ? 'route' : 'url', + 'sentry.sveltekit.navigation.from': parameterizedRouteOrigin || undefined, + }, + }); + // } - 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); + 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', + }, + onlyIfParent: true, + }); }); } From d1a8467b43743e548c84767de2f13f1dd8e329fd Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 14 Feb 2024 15:03:05 +0100 Subject: [PATCH 2/6] ... --- .../sveltekit-2/test/performance.test.ts | 81 +++++++++++++++++-- .../src/client/browserTracingIntegration.ts | 34 +++++--- 2 files changed, 97 insertions(+), 18 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/sveltekit-2/test/performance.test.ts b/dev-packages/e2e-tests/test-applications/sveltekit-2/test/performance.test.ts index c76f8990a78b..cf4b615d4150 100644 --- a/dev-packages/e2e-tests/test-applications/sveltekit-2/test/performance.test.ts +++ b/dev-packages/e2e-tests/test-applications/sveltekit-2/test/performance.test.ts @@ -189,12 +189,11 @@ test.describe('performance events', () => { await page.goto('/'); const clientPageloadTxnPromise = waitForTransaction('sveltekit-2', txnEvent => { - return txnEvent?.contexts?.trace?.op === 'pageload'; + return txnEvent?.contexts?.trace?.op === 'pageload' && txnEvent?.tags?.runtime === 'browser'; }); const clientNavigationTxnPromise = waitForTransaction('sveltekit-2', txnEvent => { - console.log(txnEvent); - return txnEvent?.contexts?.trace?.op === 'navigation'; + return txnEvent?.contexts?.trace?.op === 'navigation' && txnEvent?.tags?.runtime === 'browser'; }); const navigationClickPromise = page.locator('#routeWithParamsLink').click(); @@ -225,10 +224,82 @@ test.describe('performance events', () => { type: 'transaction', contexts: { trace: { - op: 'http.server', - origin: 'auto.http.sveltekit', + op: 'navigation', + origin: 'auto.navigation.sveltekit', + }, + }, + }); + + const routingSpans = navigationTxnEvent.spans?.filter(s => s.op === 'ui.sveltekit.routing'); + expect(routingSpans).toHaveLength(1); + + const routingSpan = routingSpans && routingSpans[0]; + expect(routingSpan).toMatchObject({ + op: 'ui.sveltekit.routing', + description: 'SvelteKit Route Change', + data: { + 'sentry.op': 'ui.sveltekit.routing', + 'sentry.origin': 'auto.ui.sveltekit', + }, + }); + }); + + test.only('captures a navigation transaction directly after pageload', async ({ page }) => { + await page.goto('/'); + + const clientPageloadTxnPromise = waitForTransaction('sveltekit-2', txnEvent => { + return txnEvent?.contexts?.trace?.op === 'pageload' && txnEvent?.tags?.runtime === 'browser'; + }); + + const clientNavigationTxnPromise = waitForTransaction('sveltekit-2', txnEvent => { + return txnEvent?.contexts?.trace?.op === 'navigation' && txnEvent?.tags?.runtime === 'browser'; + }); + + const navigationClickPromise = page.locator('#routeWithParamsLink').click(); + + const [pageloadTxnEvent, navigationTxnEvent, _] = await Promise.all([ + clientPageloadTxnPromise, + clientNavigationTxnPromise, + navigationClickPromise, + ]); + + expect(pageloadTxnEvent).toMatchObject({ + transaction: '/', + tags: { runtime: 'browser' }, + transaction_info: { source: 'route' }, + type: 'transaction', + contexts: { + trace: { + op: 'pageload', + origin: 'auto.pageload.sveltekit', }, }, }); + + expect(navigationTxnEvent).toMatchObject({ + transaction: '/users/[id]', + tags: { runtime: 'browser' }, + transaction_info: { source: 'route' }, + type: 'transaction', + contexts: { + trace: { + op: 'navigation', + origin: 'auto.navigation.sveltekit', + }, + }, + }); + + const routingSpans = navigationTxnEvent.spans?.filter(s => s.op === 'ui.sveltekit.routing'); + expect(routingSpans).toHaveLength(1); + + const routingSpan = routingSpans && routingSpans[0]; + expect(routingSpan).toMatchObject({ + op: 'ui.sveltekit.routing', + description: 'SvelteKit Route Change', + data: { + 'sentry.op': 'ui.sveltekit.routing', + 'sentry.origin': 'auto.ui.sveltekit', + }, + }); }); }); diff --git a/packages/sveltekit/src/client/browserTracingIntegration.ts b/packages/sveltekit/src/client/browserTracingIntegration.ts index 060c5c297137..6a923ee98c54 100644 --- a/packages/sveltekit/src/client/browserTracingIntegration.ts +++ b/packages/sveltekit/src/client/browserTracingIntegration.ts @@ -64,7 +64,7 @@ export function browserTracingIntegration( function _instrumentPageload(client: Client): void { const initialPath = WINDOW && WINDOW.location && WINDOW.location.pathname; - startBrowserTracingPageLoadSpan(client, { + const pageloadSpan = startBrowserTracingPageLoadSpan(client, { name: initialPath, op: 'pageload', description: initialPath, @@ -76,8 +76,9 @@ function _instrumentPageload(client: Client): void { [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', }, }); - - const pageloadSpan = getActiveSpan(); + if (!pageloadSpan) { + return; + } page.subscribe(page => { if (!page) { @@ -86,7 +87,7 @@ function _instrumentPageload(client: Client): void { const routeId = page.route && page.route.id; - if (pageloadSpan && routeId) { + if (routeId) { pageloadSpan.updateName(routeId); pageloadSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route'); } @@ -98,7 +99,6 @@ function _instrumentPageload(client: Client): void { */ function _instrumentNavigations(client: Client): void { let routingSpan: Span | undefined; - let activeNavigationSpan: Span | undefined; navigating.subscribe(navigation => { if (!navigation) { @@ -129,22 +129,30 @@ function _instrumentNavigations(client: Client): void { const parameterizedRouteOrigin = from && from.route.id; const parameterizedRouteDestination = to && to.route.id; - // if (!activeNavigationSpan) { - activeNavigationSpan = startBrowserTracingNavigationSpan(client, { + if (routingSpan) { + // If a routing span is still open from a previous navigation, we finish it. + // This is important for e.g. redirects when a new navigation root span finishes + // the first root span. If we don't `.end()` the previous span, it will get + // status 'cancelled' which isn't entirely correct. + routingSpan.end(); + } + + startBrowserTracingNavigationSpan(client, { name: parameterizedRouteDestination || rawRouteDestination || 'unknown', op: 'navigation', attributes: { [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.sveltekit', [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: parameterizedRouteDestination ? 'route' : 'url', 'sentry.sveltekit.navigation.from': parameterizedRouteOrigin || undefined, + 'sentry.sveltekit.navigation.to': parameterizedRouteDestination || undefined, + + // `navigation.type` denotes the origin of the navigation. e.g.: + // - link (clicking on a link) + // - goto (programmatic via goto() or redirect()) + // - popstate (back/forward navigation) + 'sentry.sveltekit.navigation.type': navigation.type, }, }); - // } - - if (routingSpan) { - // If a routing span is still open from a previous navigation, we finish it. - routingSpan.end(); - } routingSpan = startInactiveSpan({ op: 'ui.sveltekit.routing', From 85800b2a68b3a8ad161b0559ad67eda3b65d0f0f Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 14 Feb 2024 15:51:08 +0100 Subject: [PATCH 3/6] tests --- .../sveltekit-2/test/performance.test.ts | 138 +++++++++++++++--- .../src/client/browserTracingIntegration.ts | 20 ++- 2 files changed, 131 insertions(+), 27 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/sveltekit-2/test/performance.test.ts b/dev-packages/e2e-tests/test-applications/sveltekit-2/test/performance.test.ts index cf4b615d4150..53b1ea128f00 100644 --- a/dev-packages/e2e-tests/test-applications/sveltekit-2/test/performance.test.ts +++ b/dev-packages/e2e-tests/test-applications/sveltekit-2/test/performance.test.ts @@ -185,7 +185,7 @@ test.describe('performance events', () => { }); }); - test.only('captures a navigation transaction directly after pageload', async ({ page }) => { + test('captures a navigation transaction directly after pageload', async ({ page }) => { await page.goto('/'); const clientPageloadTxnPromise = waitForTransaction('sveltekit-2', txnEvent => { @@ -226,6 +226,11 @@ test.describe('performance events', () => { trace: { op: 'navigation', origin: 'auto.navigation.sveltekit', + data: { + 'sentry.sveltekit.navigation.from': '/', + 'sentry.sveltekit.navigation.to': '/users/[id]', + 'sentry.sveltekit.navigation.type': 'link', + }, }, }, }); @@ -240,43 +245,126 @@ test.describe('performance events', () => { data: { 'sentry.op': 'ui.sveltekit.routing', 'sentry.origin': 'auto.ui.sveltekit', + 'sentry.sveltekit.navigation.from': '/', + 'sentry.sveltekit.navigation.to': '/users/[id]', + 'sentry.sveltekit.navigation.type': 'link', }, }); }); - test.only('captures a navigation transaction directly after pageload', async ({ page }) => { + test('captures one navigation transaction per redirect', async ({ page }) => { await page.goto('/'); - const clientPageloadTxnPromise = waitForTransaction('sveltekit-2', txnEvent => { - return txnEvent?.contexts?.trace?.op === 'pageload' && txnEvent?.tags?.runtime === 'browser'; + const clientNavigationRedirect1TxnPromise = waitForTransaction('sveltekit-2', txnEvent => { + return ( + txnEvent?.contexts?.trace?.op === 'navigation' && + txnEvent?.tags?.runtime === 'browser' && + txnEvent?.transaction === '/redirect1' + ); }); - const clientNavigationTxnPromise = waitForTransaction('sveltekit-2', txnEvent => { - return txnEvent?.contexts?.trace?.op === 'navigation' && txnEvent?.tags?.runtime === 'browser'; + const clientNavigationRedirect2TxnPromise = waitForTransaction('sveltekit-2', txnEvent => { + return ( + txnEvent?.contexts?.trace?.op === 'navigation' && + txnEvent?.tags?.runtime === 'browser' && + txnEvent?.transaction === '/redirect2' + ); }); - const navigationClickPromise = page.locator('#routeWithParamsLink').click(); + const clientNavigationRedirect3TxnPromise = waitForTransaction('sveltekit-2', txnEvent => { + return ( + txnEvent?.contexts?.trace?.op === 'navigation' && + txnEvent?.tags?.runtime === 'browser' && + txnEvent?.transaction === '/users/[id]' + ); + }); - const [pageloadTxnEvent, navigationTxnEvent, _] = await Promise.all([ - clientPageloadTxnPromise, - clientNavigationTxnPromise, + const navigationClickPromise = page.locator('#redirectLink').click(); + + const [redirect1TxnEvent, redirect2TxnEvent, redirect3TxnEvent, _] = await Promise.all([ + clientNavigationRedirect1TxnPromise, + clientNavigationRedirect2TxnPromise, + clientNavigationRedirect3TxnPromise, navigationClickPromise, ]); - expect(pageloadTxnEvent).toMatchObject({ - transaction: '/', + expect(redirect1TxnEvent).toMatchObject({ + transaction: '/redirect1', tags: { runtime: 'browser' }, transaction_info: { source: 'route' }, type: 'transaction', contexts: { trace: { - op: 'pageload', - origin: 'auto.pageload.sveltekit', + op: 'navigation', + origin: 'auto.navigation.sveltekit', + data: { + 'sentry.origin': 'auto.navigation.sveltekit', + 'sentry.op': 'navigation', + 'sentry.source': 'route', + 'sentry.sveltekit.navigation.type': 'link', + 'sentry.sveltekit.navigation.from': '/', + 'sentry.sveltekit.navigation.to': '/redirect1', + 'sentry.sample_rate': 1, + }, }, }, }); - expect(navigationTxnEvent).toMatchObject({ + const redirect1Spans = redirect1TxnEvent.spans?.filter(s => s.op === 'ui.sveltekit.routing'); + expect(redirect1Spans).toHaveLength(1); + + const redirect1Span = redirect1Spans && redirect1Spans[0]; + expect(redirect1Span).toMatchObject({ + op: 'ui.sveltekit.routing', + description: 'SvelteKit Route Change', + data: { + 'sentry.op': 'ui.sveltekit.routing', + 'sentry.origin': 'auto.ui.sveltekit', + 'sentry.sveltekit.navigation.from': '/', + 'sentry.sveltekit.navigation.to': '/redirect1', + 'sentry.sveltekit.navigation.type': 'link', + }, + }); + + expect(redirect2TxnEvent).toMatchObject({ + transaction: '/redirect2', + tags: { runtime: 'browser' }, + transaction_info: { source: 'route' }, + type: 'transaction', + contexts: { + trace: { + op: 'navigation', + origin: 'auto.navigation.sveltekit', + data: { + 'sentry.origin': 'auto.navigation.sveltekit', + 'sentry.op': 'navigation', + 'sentry.source': 'route', + 'sentry.sveltekit.navigation.type': 'goto', + 'sentry.sveltekit.navigation.from': '/', + 'sentry.sveltekit.navigation.to': '/redirect2', + 'sentry.sample_rate': 1, + }, + }, + }, + }); + + const redirect2Spans = redirect2TxnEvent.spans?.filter(s => s.op === 'ui.sveltekit.routing'); + expect(redirect2Spans).toHaveLength(1); + + const redirect2Span = redirect2Spans && redirect2Spans[0]; + expect(redirect2Span).toMatchObject({ + op: 'ui.sveltekit.routing', + description: 'SvelteKit Route Change', + data: { + 'sentry.op': 'ui.sveltekit.routing', + 'sentry.origin': 'auto.ui.sveltekit', + 'sentry.sveltekit.navigation.from': '/', + 'sentry.sveltekit.navigation.to': '/redirect2', + 'sentry.sveltekit.navigation.type': 'goto', + }, + }); + + expect(redirect3TxnEvent).toMatchObject({ transaction: '/users/[id]', tags: { runtime: 'browser' }, transaction_info: { source: 'route' }, @@ -285,20 +373,32 @@ test.describe('performance events', () => { trace: { op: 'navigation', origin: 'auto.navigation.sveltekit', + data: { + 'sentry.origin': 'auto.navigation.sveltekit', + 'sentry.op': 'navigation', + 'sentry.source': 'route', + 'sentry.sveltekit.navigation.type': 'goto', + 'sentry.sveltekit.navigation.from': '/', + 'sentry.sveltekit.navigation.to': '/users/[id]', + 'sentry.sample_rate': 1, + }, }, }, }); - const routingSpans = navigationTxnEvent.spans?.filter(s => s.op === 'ui.sveltekit.routing'); - expect(routingSpans).toHaveLength(1); + const redirect3Spans = redirect3TxnEvent.spans?.filter(s => s.op === 'ui.sveltekit.routing'); + expect(redirect3Spans).toHaveLength(1); - const routingSpan = routingSpans && routingSpans[0]; - expect(routingSpan).toMatchObject({ + const redirect3Span = redirect3Spans && redirect3Spans[0]; + expect(redirect3Span).toMatchObject({ op: 'ui.sveltekit.routing', description: 'SvelteKit Route Change', data: { 'sentry.op': 'ui.sveltekit.routing', 'sentry.origin': 'auto.ui.sveltekit', + 'sentry.sveltekit.navigation.from': '/', + 'sentry.sveltekit.navigation.to': '/users/[id]', + 'sentry.sveltekit.navigation.type': 'goto', }, }); }); diff --git a/packages/sveltekit/src/client/browserTracingIntegration.ts b/packages/sveltekit/src/client/browserTracingIntegration.ts index 6a923ee98c54..a56d066ae8ac 100644 --- a/packages/sveltekit/src/client/browserTracingIntegration.ts +++ b/packages/sveltekit/src/client/browserTracingIntegration.ts @@ -137,20 +137,23 @@ function _instrumentNavigations(client: Client): void { routingSpan.end(); } + const navigationInfo = { + // `navigation.type` denotes the origin of the navigation. e.g.: + // - link (clicking on a link) + // - goto (programmatic via goto() or redirect()) + // - popstate (back/forward navigation) + 'sentry.sveltekit.navigation.type': navigation.type, + 'sentry.sveltekit.navigation.from': parameterizedRouteOrigin || undefined, + 'sentry.sveltekit.navigation.to': parameterizedRouteDestination || undefined, + }; + startBrowserTracingNavigationSpan(client, { name: parameterizedRouteDestination || rawRouteDestination || 'unknown', op: 'navigation', attributes: { [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.sveltekit', [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: parameterizedRouteDestination ? 'route' : 'url', - 'sentry.sveltekit.navigation.from': parameterizedRouteOrigin || undefined, - 'sentry.sveltekit.navigation.to': parameterizedRouteDestination || undefined, - - // `navigation.type` denotes the origin of the navigation. e.g.: - // - link (clicking on a link) - // - goto (programmatic via goto() or redirect()) - // - popstate (back/forward navigation) - 'sentry.sveltekit.navigation.type': navigation.type, + ...navigationInfo, }, }); @@ -159,6 +162,7 @@ function _instrumentNavigations(client: Client): void { name: 'SvelteKit Route Change', attributes: { [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.ui.sveltekit', + ...navigationInfo, }, onlyIfParent: true, }); From e8d722bb11a1c819177cee48b66b4c862499afe3 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 14 Feb 2024 16:03:51 +0100 Subject: [PATCH 4/6] fix unit tests --- .../src/client/browserTracingIntegration.ts | 5 ++-- .../client/browserTracingIntegration.test.ts | 27 ++++++++++--------- 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/packages/sveltekit/src/client/browserTracingIntegration.ts b/packages/sveltekit/src/client/browserTracingIntegration.ts index a56d066ae8ac..5926df031c62 100644 --- a/packages/sveltekit/src/client/browserTracingIntegration.ts +++ b/packages/sveltekit/src/client/browserTracingIntegration.ts @@ -10,6 +10,7 @@ import { startInactiveSpan, } from '@sentry/svelte'; import type { Client, Integration, Span } from '@sentry/types'; +import { dropUndefinedKeys } from '@sentry/utils'; import { svelteKitRoutingInstrumentation } from './router'; /** @@ -137,7 +138,7 @@ function _instrumentNavigations(client: Client): void { routingSpan.end(); } - const navigationInfo = { + const navigationInfo = dropUndefinedKeys({ // `navigation.type` denotes the origin of the navigation. e.g.: // - link (clicking on a link) // - goto (programmatic via goto() or redirect()) @@ -145,7 +146,7 @@ function _instrumentNavigations(client: Client): void { 'sentry.sveltekit.navigation.type': navigation.type, 'sentry.sveltekit.navigation.from': parameterizedRouteOrigin || undefined, 'sentry.sveltekit.navigation.to': parameterizedRouteDestination || undefined, - }; + }); startBrowserTracingNavigationSpan(client, { name: parameterizedRouteDestination || rawRouteDestination || 'unknown', diff --git a/packages/sveltekit/test/client/browserTracingIntegration.test.ts b/packages/sveltekit/test/client/browserTracingIntegration.test.ts index 415637c05560..fba7136bd5eb 100644 --- a/packages/sveltekit/test/client/browserTracingIntegration.test.ts +++ b/packages/sveltekit/test/client/browserTracingIntegration.test.ts @@ -45,6 +45,7 @@ describe('browserTracingIntegration', () => { }), setTag: vi.fn(), }; + return createdRootSpan; }); const startBrowserTracingNavigationSpanSpy = vi @@ -56,6 +57,7 @@ describe('browserTracingIntegration', () => { setAttribute: vi.fn(), setTag: vi.fn(), }; + return createdRootSpan; }); const fakeClient = { getOptions: () => ({}), on: () => {} }; @@ -173,6 +175,7 @@ describe('browserTracingIntegration', () => { navigating.set({ from: { route: { id: '/users' }, url: { pathname: '/users' } }, to: { route: { id: '/users/[id]' }, url: { pathname: '/users/7762' } }, + type: 'link', }); // This should update the transaction name with the parameterized route: @@ -183,9 +186,9 @@ describe('browserTracingIntegration', () => { attributes: { [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.sveltekit', [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', - }, - tags: { - 'routing.instrumentation': '@sentry/sveltekit', + 'sentry.sveltekit.navigation.from': '/users', + 'sentry.sveltekit.navigation.to': '/users/[id]', + 'sentry.sveltekit.navigation.type': 'link', }, }); @@ -195,12 +198,13 @@ describe('browserTracingIntegration', () => { name: 'SvelteKit Route Change', attributes: { [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.ui.sveltekit', + 'sentry.sveltekit.navigation.from': '/users', + 'sentry.sveltekit.navigation.to': '/users/[id]', + 'sentry.sveltekit.navigation.type': 'link', }, + onlyIfParent: true, }); - // 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); @@ -246,9 +250,8 @@ describe('browserTracingIntegration', () => { attributes: { [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.sveltekit', - }, - tags: { - 'routing.instrumentation': '@sentry/sveltekit', + 'sentry.sveltekit.navigation.from': '/users/[id]', + 'sentry.sveltekit.navigation.to': '/users/[id]', }, }); @@ -258,11 +261,11 @@ describe('browserTracingIntegration', () => { name: 'SvelteKit Route Change', attributes: { [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.ui.sveltekit', + 'sentry.sveltekit.navigation.from': '/users/[id]', + 'sentry.sveltekit.navigation.to': '/users/[id]', }, + onlyIfParent: true, }); - - // 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', () => { From c9ebdc2ba9caf1d4b4f7c07b050a307707add68f Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 14 Feb 2024 16:09:29 +0100 Subject: [PATCH 5/6] remove unused import --- packages/sveltekit/src/client/browserTracingIntegration.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/sveltekit/src/client/browserTracingIntegration.ts b/packages/sveltekit/src/client/browserTracingIntegration.ts index 5926df031c62..07de01e86c11 100644 --- a/packages/sveltekit/src/client/browserTracingIntegration.ts +++ b/packages/sveltekit/src/client/browserTracingIntegration.ts @@ -4,7 +4,6 @@ import { BrowserTracing as OriginalBrowserTracing, WINDOW, browserTracingIntegration as originalBrowserTracingIntegration, - getActiveSpan, startBrowserTracingNavigationSpan, startBrowserTracingPageLoadSpan, startInactiveSpan, From f1a85bdeaa220a68905d8ffcdb962b1a1840aea3 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 14 Feb 2024 18:04:30 +0100 Subject: [PATCH 6/6] Update dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/+layout.svelte Co-authored-by: Francesco Novy --- .../test-applications/sveltekit-2/src/routes/+layout.svelte | 3 --- 1 file changed, 3 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/+layout.svelte b/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/+layout.svelte index 5c4ee5f2ea76..8eb402c9eda6 100644 --- a/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/+layout.svelte +++ b/dev-packages/e2e-tests/test-applications/sveltekit-2/src/routes/+layout.svelte @@ -8,9 +8,6 @@ document.body.classList.add("hydrated"); }); - navigating.subscribe((navigation) => { - console.log("XXX navigating", navigation); - });