Merge pull request #8076 from getsentry/prepare-release/7.51.2

Luca Forstner · web-flow · commit ad75f149e9e3 · 2023-05-08T20:12:43.000+02:00
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
@@ -808,6 +808,7 @@ jobs:
         job_node_integration_tests,
         job_browser_playwright_tests,
         job_browser_integration_tests,
+        job_browser_loader_tests,
         job_remix_integration_tests,
         job_e2e_tests,
       ]
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,6 +4,11 @@
 
 - "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott
 
+## 7.51.2
+
+- fix(nextjs): Continue traces in data fetchers when there is an already active transaction on the hub (#8073)
+- fix(sveltekit): Avoid creating the Sentry Vite plugin in dev mode (#8065)
+
 ## 7.51.1
 
 - feat(replay): Add event to capture options on checkouts (#8011)
diff --git a/packages/e2e-tests/test-applications/create-react-app/test-recipe.json b/packages/e2e-tests/test-applications/create-react-app/test-recipe.json
@@ -2,5 +2,13 @@
   "$schema": "../../test-recipe-schema.json",
   "testApplicationName": "create-react-app",
   "buildCommand": "pnpm install && pnpm build",
-  "tests": []
+  "tests": [],
+  "canaryVersions": [
+    {
+      "dependencyOverrides": {
+        "react": "canary",
+        "react-dom": "canary"
+      }
+    }
+  ]
 }
diff --git a/packages/nextjs/src/server/utils/wrapperUtils.ts b/packages/nextjs/src/server/utils/wrapperUtils.ts
@@ -5,7 +5,7 @@ import {
   runWithAsyncContext,
   startTransaction,
 } from '@sentry/core';
-import type { Transaction } from '@sentry/types';
+import type { Span, Transaction } from '@sentry/types';
 import { baggageHeaderToDynamicSamplingContext, extractTraceparentData } from '@sentry/utils';
 import type { IncomingMessage, ServerResponse } from 'http';
 
@@ -82,7 +82,8 @@ export function withTracedServerSideDataFetcher<F extends (...args: any[]) => Pr
   return async function (this: unknown, ...args: Parameters<F>): Promise<ReturnType<F>> {
     return runWithAsyncContext(async () => {
       const hub = getCurrentHub();
-      let requestTransaction: Transaction | undefined = getTransactionFromRequest(req);
+      const scope = hub.getScope();
+      const previousSpan: Span | undefined = getTransactionFromRequest(req) ?? scope.getSpan();
       let dataFetcherSpan;
 
       const sentryTraceHeader = req.headers['sentry-trace'];
@@ -93,7 +94,8 @@ export function withTracedServerSideDataFetcher<F extends (...args: any[]) => Pr
       const dynamicSamplingContext = baggageHeaderToDynamicSamplingContext(rawBaggageString);
 
       if (platformSupportsStreaming()) {
-        if (requestTransaction === undefined) {
+        let spanToContinue: Span;
+        if (previousSpan === undefined) {
           const newTransaction = startTransaction(
             {
               op: 'http.server',
@@ -109,8 +111,6 @@ export function withTracedServerSideDataFetcher<F extends (...args: any[]) => Pr
             { request: req },
           );
 
-          requestTransaction = newTransaction;
-
           if (platformSupportsStreaming()) {
             // On platforms that don't support streaming, doing things after res.end() is unreliable.
             autoEndTransactionOnResponseEnd(newTransaction, res);
@@ -119,9 +119,12 @@ export function withTracedServerSideDataFetcher<F extends (...args: any[]) => Pr
           // Link the transaction and the request together, so that when we would normally only have access to one, it's
           // still possible to grab the other.
           setTransactionOnRequest(newTransaction, req);
+          spanToContinue = newTransaction;
+        } else {
+          spanToContinue = previousSpan;
         }
 
-        dataFetcherSpan = requestTransaction.startChild({
+        dataFetcherSpan = spanToContinue.startChild({
           op: 'function.nextjs',
           description: `${options.dataFetchingMethodName} (${options.dataFetcherRouteName})`,
           status: 'ok',
@@ -140,22 +143,20 @@ export function withTracedServerSideDataFetcher<F extends (...args: any[]) => Pr
         });
       }
 
-      const currentScope = hub.getScope();
-      if (currentScope) {
-        currentScope.setSpan(dataFetcherSpan);
-        currentScope.setSDKProcessingMetadata({ request: req });
-      }
+      scope.setSpan(dataFetcherSpan);
+      scope.setSDKProcessingMetadata({ request: req });
 
       try {
         return await origDataFetcher.apply(this, args);
       } catch (e) {
         // Since we finish the span before the error can bubble up and trigger the handlers in `registerErrorInstrumentation`
         // that set the transaction status, we need to manually set the status of the span & transaction
         dataFetcherSpan.setStatus('internal_error');
-        requestTransaction?.setStatus('internal_error');
+        previousSpan?.setStatus('internal_error');
         throw e;
       } finally {
         dataFetcherSpan.finish();
+        scope.setSpan(previousSpan);
         if (!platformSupportsStreaming()) {
           await flushQueue();
         }
diff --git a/packages/nextjs/src/server/wrapAppGetInitialPropsWithSentry.ts b/packages/nextjs/src/server/wrapAppGetInitialPropsWithSentry.ts
@@ -31,7 +31,8 @@ export function wrapAppGetInitialPropsWithSentry(origAppGetInitialProps: AppGetI
       const { req, res } = context.ctx;
 
       const errorWrappedAppGetInitialProps = withErrorInstrumentation(wrappingTarget);
-      const options = getCurrentHub().getClient()?.getOptions();
+      const hub = getCurrentHub();
+      const options = hub.getClient()?.getOptions();
 
       // Generally we can assume that `req` and `res` are always defined on the server:
       // https://nextjs.org/docs/api-reference/data-fetching/get-initial-props#context-object
@@ -51,7 +52,7 @@ export function wrapAppGetInitialPropsWithSentry(origAppGetInitialProps: AppGetI
           };
         } = await tracedGetInitialProps.apply(thisArg, args);
 
-        const requestTransaction = getTransactionFromRequest(req);
+        const requestTransaction = getTransactionFromRequest(req) ?? hub.getScope().getTransaction();
 
         // Per definition, `pageProps` is not optional, however an increased amount of users doesn't seem to call
         // `App.getInitialProps(appContext)` in their custom `_app` pages which is required as per
diff --git a/packages/nextjs/src/server/wrapErrorGetInitialPropsWithSentry.ts b/packages/nextjs/src/server/wrapErrorGetInitialPropsWithSentry.ts
@@ -34,7 +34,8 @@ export function wrapErrorGetInitialPropsWithSentry(
       const { req, res } = context;
 
       const errorWrappedGetInitialProps = withErrorInstrumentation(wrappingTarget);
-      const options = getCurrentHub().getClient()?.getOptions();
+      const hub = getCurrentHub();
+      const options = hub.getClient()?.getOptions();
 
       // Generally we can assume that `req` and `res` are always defined on the server:
       // https://nextjs.org/docs/api-reference/data-fetching/get-initial-props#context-object
@@ -52,7 +53,7 @@ export function wrapErrorGetInitialPropsWithSentry(
           _sentryBaggage?: string;
         } = await tracedGetInitialProps.apply(thisArg, args);
 
-        const requestTransaction = getTransactionFromRequest(req);
+        const requestTransaction = getTransactionFromRequest(req) ?? hub.getScope().getTransaction();
         if (requestTransaction) {
           errorGetInitialProps._sentryTraceData = requestTransaction.toTraceparent();
 
diff --git a/packages/nextjs/src/server/wrapGetInitialPropsWithSentry.ts b/packages/nextjs/src/server/wrapGetInitialPropsWithSentry.ts
@@ -30,7 +30,8 @@ export function wrapGetInitialPropsWithSentry(origGetInitialProps: GetInitialPro
       const { req, res } = context;
 
       const errorWrappedGetInitialProps = withErrorInstrumentation(wrappingTarget);
-      const options = getCurrentHub().getClient()?.getOptions();
+      const hub = getCurrentHub();
+      const options = hub.getClient()?.getOptions();
 
       // Generally we can assume that `req` and `res` are always defined on the server:
       // https://nextjs.org/docs/api-reference/data-fetching/get-initial-props#context-object
@@ -48,7 +49,7 @@ export function wrapGetInitialPropsWithSentry(origGetInitialProps: GetInitialPro
           _sentryBaggage?: string;
         } = await tracedGetInitialProps.apply(thisArg, args);
 
-        const requestTransaction = getTransactionFromRequest(req);
+        const requestTransaction = getTransactionFromRequest(req) ?? hub.getScope().getTransaction();
         if (requestTransaction) {
           initialProps._sentryTraceData = requestTransaction.toTraceparent();
 
diff --git a/packages/nextjs/src/server/wrapGetServerSidePropsWithSentry.ts b/packages/nextjs/src/server/wrapGetServerSidePropsWithSentry.ts
@@ -31,7 +31,8 @@ export function wrapGetServerSidePropsWithSentry(
       const { req, res } = context;
 
       const errorWrappedGetServerSideProps = withErrorInstrumentation(wrappingTarget);
-      const options = getCurrentHub().getClient()?.getOptions();
+      const hub = getCurrentHub();
+      const options = hub.getClient()?.getOptions();
 
       if (hasTracingEnabled() && options?.instrumenter === 'sentry') {
         const tracedGetServerSideProps = withTracedServerSideDataFetcher(errorWrappedGetServerSideProps, req, res, {
@@ -45,7 +46,7 @@ export function wrapGetServerSidePropsWithSentry(
         >);
 
         if (serverSideProps && 'props' in serverSideProps) {
-          const requestTransaction = getTransactionFromRequest(req);
+          const requestTransaction = getTransactionFromRequest(req) ?? hub.getScope().getTransaction();
           if (requestTransaction) {
             serverSideProps.props._sentryTraceData = requestTransaction.toTraceparent();
 
diff --git a/packages/nextjs/test/config/wrappers.test.ts b/packages/nextjs/test/config/wrappers.test.ts
@@ -6,6 +6,7 @@ import type { IncomingMessage, ServerResponse } from 'http';
 import { wrapGetInitialPropsWithSentry, wrapGetServerSidePropsWithSentry } from '../../src/server';
 
 const startTransactionSpy = jest.spyOn(SentryCore, 'startTransaction');
+const originalGetCurrentHub = jest.requireActual('@sentry/node').getCurrentHub;
 
 // 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.
@@ -21,13 +22,18 @@ describe('data-fetching function wrappers', () => {
       req = { headers: {}, url: 'http://dogs.are.great/tricks/kangaroo' } as IncomingMessage;
       res = { end: jest.fn() } as unknown as ServerResponse;
 
-      jest.spyOn(SentryCore, 'hasTracingEnabled').mockReturnValueOnce(true);
-      jest.spyOn(SentryNode, 'getCurrentHub').mockReturnValueOnce({
-        getClient: () =>
+      jest.spyOn(SentryCore, 'hasTracingEnabled').mockReturnValue(true);
+      jest.spyOn(SentryNode, 'getCurrentHub').mockImplementation(() => {
+        const hub = originalGetCurrentHub();
+
+        hub.getClient = () =>
           ({
             getOptions: () => ({ instrumenter: 'sentry' }),
-          } as any),
-      } as any);
+            getDsn: () => {},
+          } as any);
+
+        return hub;
+      });
     });
 
     afterEach(() => {
diff --git a/packages/sveltekit/src/vite/sentryVitePlugins.ts b/packages/sveltekit/src/vite/sentryVitePlugins.ts
@@ -78,7 +78,7 @@ export async function sentrySvelteKit(options: SentrySvelteKitPluginOptions = {}
     );
   }
 
-  if (mergedOptions.autoUploadSourceMaps) {
+  if (mergedOptions.autoUploadSourceMaps && process.env.NODE_ENV !== 'development') {
     const pluginOptions = {
       ...mergedOptions.sourceMapsUploadOptions,
       debug: mergedOptions.debug, // override the plugin's debug flag with the one from the top-level options
diff --git a/packages/sveltekit/test/vite/sentrySvelteKitPlugins.test.ts b/packages/sveltekit/test/vite/sentrySvelteKitPlugins.test.ts
@@ -38,6 +38,19 @@ describe('sentryVite()', () => {
     expect(plugins).toHaveLength(1);
   });
 
+  it("doesn't return the custom sentry source maps plugin if `NODE_ENV` is development", async () => {
+    const previousEnv = process.env.NODE_ENV;
+
+    process.env.NODE_ENV = 'development';
+    const plugins = await sentrySvelteKit({ autoUploadSourceMaps: true, autoInstrument: true });
+    const instrumentPlugin = plugins[0];
+
+    expect(plugins).toHaveLength(1);
+    expect(instrumentPlugin.name).toEqual('sentry-auto-instrumentation');
+
+    process.env.NODE_ENV = previousEnv;
+  });
+
   it("doesn't return the auto instrument plugin if autoInstrument is `false`", async () => {
     const plugins = await sentrySvelteKit({ autoInstrument: false });
     expect(plugins).toHaveLength(1);

Original file line number	Diff line number	Diff line change
`@@ -808,6 +808,7 @@ jobs:`
`808`	`808`	`job_node_integration_tests,`
`809`	`809`	`job_browser_playwright_tests,`
`810`	`810`	`job_browser_integration_tests,`
	`811`	`+ job_browser_loader_tests,`
`811`	`812`	`job_remix_integration_tests,`
`812`	`813`	`job_e2e_tests,`
`813`	`814`	`]`
Original file line number	Diff line number	Diff line change
`@@ -78,7 +78,7 @@ export async function sentrySvelteKit(options: SentrySvelteKitPluginOptions = {}`
`78`	`78`	`);`
`79`	`79`	`}`
`80`	`80`
`81`		`- if (mergedOptions.autoUploadSourceMaps) {`
	`81`	`+ if (mergedOptions.autoUploadSourceMaps && process.env.NODE_ENV !== 'development') {`
`82`	`82`	`const pluginOptions = {`
`83`	`83`	`...mergedOptions.sourceMapsUploadOptions,`
`84`	`84`	`debug: mergedOptions.debug, // override the plugin's debug flag with the one from the top-level options`