Merge remote-tracking branch 'origin/develop' into kw-add-user-feedback-api

Author: krystofwoldrich

packages/core/src/tracing/idletransaction.ts

@@ -111,9 +111,6 @@ export class IdleTransaction extends Transaction {
     super(transactionContext, _idleHub);
 
     if (_onScope) {
-      // There should only be one active transaction on the scope
-      clearActiveTransaction(_idleHub);
-
       // We set the transaction here on the scope so error events pick up the trace
       // context and attach it to the error.
       __DEBUG_BUILD__ && logger.log(`Setting idle transaction on scope. Span ID: ${this.spanId}`);
@@ -179,7 +176,10 @@ export class IdleTransaction extends Transaction {
 
     // if `this._onScope` is `true`, the transaction put itself on the scope when it started
     if (this._onScope) {
-      clearActiveTransaction(this._idleHub);
+      const scope = this._idleHub.getScope();
+      if (scope.getTransaction() === this) {
+        scope.setSpan(undefined);
+      }
     }
 
     return super.finish(endTimestamp);
@@ -353,13 +353,3 @@ export class IdleTransaction extends Transaction {
     }, this._heartbeatInterval);
   }
 }
-
-/**
- * Reset transaction on scope to `undefined`
- */
-function clearActiveTransaction(hub: Hub): void {
-  const scope = hub.getScope();
-  if (scope.getTransaction()) {
-    scope.setSpan(undefined);
-  }
-}

packages/sveltekit/src/client/load.ts

@@ -3,11 +3,18 @@ import { captureException } from '@sentry/svelte';
 import { addExceptionMechanism, objectify } from '@sentry/utils';
 import type { LoadEvent } from '@sveltejs/kit';
 
+import { isRedirect } from '../common/utils';
+
 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);
 
+  // We don't want to capture thrown `Redirect`s as these are not errors but expected behaviour
+  if (isRedirect(objectifiedErr)) {
+    return objectifiedErr;
+  }
+
   captureException(objectifiedErr, scope => {
     scope.addEventProcessor(event => {
       addExceptionMechanism(event, {

packages/sveltekit/src/common/utils.ts

@@ -0,0 +1,16 @@
+import type { Redirect } from '@sveltejs/kit';
+
+/**
+ * Determines if a thrown "error" is a Redirect object which SvelteKit users can throw to redirect to another route
+ * see: https://kit.svelte.dev/docs/modules#sveltejs-kit-redirect
+ * @param error the potential redirect error
+ */
+export function isRedirect(error: unknown): error is Redirect {
+  if (error == null || typeof error !== 'object') {
+    return false;
+  }
+  const hasValidLocation = 'location' in error && typeof error.location === 'string';
+  const hasValidStatus =
+    'status' in error && typeof error.status === 'number' && error.status >= 300 && error.status <= 308;
+  return hasValidLocation && hasValidStatus;
+}

packages/sveltekit/src/server/load.ts

@@ -5,6 +5,7 @@ import type { TransactionContext } from '@sentry/types';
 import { addExceptionMechanism, objectify } from '@sentry/utils';
 import type { HttpError, LoadEvent, ServerLoadEvent } from '@sveltejs/kit';
 
+import { isRedirect } from '../common/utils';
 import { getTracePropagationData } from './utils';
 
 function isHttpError(err: unknown): err is HttpError {
@@ -19,7 +20,12 @@ function sendErrorToSentry(e: unknown): unknown {
   // The error() helper is commonly used to throw errors in load functions: https://kit.svelte.dev/docs/modules#sveltejs-kit-error
   // If we detect a thrown error that is an instance of HttpError, we don't want to capture 4xx errors as they
   // could be noisy.
-  if (isHttpError(objectifiedErr) && objectifiedErr.status < 500 && objectifiedErr.status >= 400) {
+  // Also the `redirect(...)` helper is used to redirect users from one page to another. We don't want to capture thrown
+  // `Redirect`s as they're not errors but expected behaviour
+  if (
+    isRedirect(objectifiedErr) ||
+    (isHttpError(objectifiedErr) && objectifiedErr.status < 500 && objectifiedErr.status >= 400)
+  ) {
     return objectifiedErr;
   }
 

packages/sveltekit/test/client/load.test.ts

@@ -1,5 +1,6 @@
 import { addTracingExtensions, Scope } from '@sentry/svelte';
 import type { Load } from '@sveltejs/kit';
+import { redirect } from '@sveltejs/kit';
 import { vi } from 'vitest';
 
 import { wrapLoadWithSentry } from '../../src/client/load';
@@ -99,6 +100,18 @@ describe('wrapLoadWithSentry', () => {
     expect(mockCaptureException).toHaveBeenCalledTimes(1);
   });
 
+  it("doesn't call captureException for thrown `Redirect`s", async () => {
+    async function load(_: Parameters<Load>[0]): Promise<ReturnType<Load>> {
+      throw redirect(300, 'other/route');
+    }
+
+    const wrappedLoad = wrapLoadWithSentry(load);
+    const res = wrappedLoad(MOCK_LOAD_ARGS);
+    await expect(res).rejects.toThrow();
+
+    expect(mockCaptureException).not.toHaveBeenCalled();
+  });
+
   it('calls trace function', async () => {
     async function load({ params }: Parameters<Load>[0]): Promise<ReturnType<Load>> {
       return {

packages/sveltekit/test/common/utils.test.ts

@@ -0,0 +1,25 @@
+import { isRedirect } from '../../src/common/utils';
+
+describe('isRedirect', () => {
+  it.each([
+    { location: '/users/id', status: 300 },
+    { location: '/users/id', status: 304 },
+    { location: '/users/id', status: 308 },
+    { location: '', status: 308 },
+  ])('returns `true` for valid Redirect objects', redirectObject => {
+    expect(isRedirect(redirectObject)).toBe(true);
+  });
+
+  it.each([
+    300,
+    'redirect',
+    { location: { route: { id: 'users/id' } }, status: 300 },
+    { status: 308 },
+    { location: '/users/id' },
+    { location: '/users/id', status: 201 },
+    { location: '/users/id', status: 400 },
+    { location: '/users/id', status: 500 },
+  ])('returns `false` for invalid Redirect objects', redirectObject => {
+    expect(isRedirect(redirectObject)).toBe(false);
+  });
+});

packages/sveltekit/test/server/load.test.ts

@@ -1,7 +1,7 @@
 import { addTracingExtensions } from '@sentry/core';
 import { Scope } from '@sentry/node';
 import type { Load, ServerLoad } from '@sveltejs/kit';
-import { error } from '@sveltejs/kit';
+import { error, redirect } from '@sveltejs/kit';
 import { vi } from 'vitest';
 
 import { wrapLoadWithSentry, wrapServerLoadWithSentry } from '../../src/server/load';
@@ -166,6 +166,18 @@ describe.each([
     });
   });
 
+  it("doesn't call captureException for thrown `Redirect`s", async () => {
+    async function load(_: Parameters<Load>[0]): Promise<ReturnType<Load>> {
+      throw redirect(300, 'other/route');
+    }
+
+    const wrappedLoad = wrapLoadWithSentry(load);
+    const res = wrappedLoad(MOCK_LOAD_ARGS);
+    await expect(res).rejects.toThrow();
+
+    expect(mockCaptureException).not.toHaveBeenCalled();
+  });
+
   it('adds an exception mechanism', async () => {
     const addEventProcessorSpy = vi.spyOn(mockScope, 'addEventProcessor').mockImplementationOnce(callback => {
       void callback({}, { event_id: 'fake-event-id' });

packages/tracing/test/idletransaction.test.ts

@@ -1,5 +1,5 @@
 import { BrowserClient } from '@sentry/browser';
-import { TRACING_DEFAULTS } from '@sentry/core';
+import { TRACING_DEFAULTS, Transaction } from '@sentry/core';
 
 import { Hub, IdleTransaction, Span } from '../../core/src';
 import { IdleTransactionSpanRecorder } from '../../core/src/tracing/idletransaction';
@@ -75,6 +75,29 @@ describe('IdleTransaction', () => {
         expect(s.getTransaction()).toBe(undefined);
       });
     });
+
+    it('does not remove transaction from scope on finish if another transaction was set there', () => {
+      const transaction = new IdleTransaction(
+        { name: 'foo' },
+        hub,
+        TRACING_DEFAULTS.idleTimeout,
+        TRACING_DEFAULTS.finalTimeout,
+        TRACING_DEFAULTS.heartbeatInterval,
+        true,
+      );
+      transaction.initSpanRecorder(10);
+
+      // @ts-ignore need to pass in hub
+      const otherTransaction = new Transaction({ name: 'bar' }, hub);
+      hub.getScope().setSpan(otherTransaction);
+
+      transaction.finish();
+      jest.runAllTimers();
+
+      hub.configureScope(s => {
+        expect(s.getTransaction()).toBe(otherTransaction);
+      });
+    });
   });
 
   beforeEach(() => {

yarn.lock

@@ -4215,9 +4215,9 @@
     highlight.js "^9.15.6"
 
 "@sveltejs/kit@^1.11.0":
-  version "1.11.0"
-  resolved "https://registry.yarnpkg.com/@sveltejs/kit/-/kit-1.11.0.tgz#23f233c351e5956356ba6f3206e40637c5f5dbda"
-  integrity sha512-PwViZcMoLgEU/jhLoSyjf5hSrHS67wvSm0ifBo4prP9irpGa5HuPOZeVDTL5tPDSBoKxtdYi1zlGdoiJfO86jA==
+  version "1.15.1"
+  resolved "https://registry.yarnpkg.com/@sveltejs/kit/-/kit-1.15.1.tgz#241f1d7e6cf457112b8c098ca26fd2eb85f8db5f"
+  integrity sha512-Wexy3N+COoClTuRawVJRbLoH5HFxNrXG3uoHt/Yd5IGx8WAcJM9Nj/CcBLw/tjCR9uDDYMnx27HxuPy3YIYQUA==
   dependencies:
     "@sveltejs/vite-plugin-svelte" "^2.0.0"
     "@types/cookie" "^0.5.1"
@@ -23936,12 +23936,7 @@ set-blocking@^2.0.0, set-blocking@~2.0.0:
   resolved "https://registry.yarnpkg.com/set-blocking/-/set-blocking-2.0.0.tgz#045f9782d011ae9a6803ddd382b24392b3d890f7"
   integrity sha1-BF+XgtARrppoA93TgrJDkrPYkPc=
 
-set-cookie-parser@^2.4.8:
-  version "2.4.8"
-  resolved "https://registry.yarnpkg.com/set-cookie-parser/-/set-cookie-parser-2.4.8.tgz#d0da0ed388bc8f24e706a391f9c9e252a13c58b2"
-  integrity sha512-edRH8mBKEWNVIVMKejNnuJxleqYE/ZSdcT8/Nem9/mmosx12pctd80s2Oy00KNZzrogMZS5mauK2/ymL1bvlvg==
-
-set-cookie-parser@^2.5.1:
+set-cookie-parser@^2.4.8, set-cookie-parser@^2.5.1:
   version "2.5.1"
   resolved "https://registry.yarnpkg.com/set-cookie-parser/-/set-cookie-parser-2.5.1.tgz#ddd3e9a566b0e8e0862aca974a6ac0e01349430b"
   integrity sha512-1jeBGaKNGdEq4FgIrORu/N570dwoPYio8lSoYLWmX7sQ//0JY08Xh9o5pBcgmHQ/MbsYp/aZnOe1s1lIsbLprQ==