feat(sveltekit): Add SvelteKit routing instrumentation (#7565)

Lms24 · web-flow · commit e97b0970f9ee · 2023-03-22T15:12:40.000+01:00
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.
diff --git 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
@@ -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
@@ -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<T extends Transaction>(
+  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
@@ -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
@@ -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
@@ -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
@@ -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
diff --git a/yarn.lock b/yarn.lock
