Merge branch 'develop' into lforst-set-headers-instead-of-append

.size-limit.js

@@ -180,7 +180,7 @@ module.exports = [
     name: 'CDN Bundle (incl. Tracing, Replay)',
     path: createCDNPath('bundle.tracing.replay.min.js'),
     gzip: true,
-    limit: '73.1 KB',
+    limit: '74 KB',
   },
   {
     name: 'CDN Bundle (incl. Tracing, Replay, Feedback)',

CHANGELOG.md

@@ -8,6 +8,13 @@
 
 ## Unreleased
 
+### Important Changes
+
+- **ref(nextjs): Remove dead code ([#13828](https://github.com/getsentry/sentry-javascript/pull/13903))**
+
+Relevant for users of the `@sentry/nextjs` package: If you have previously configured a
+`SENTRY_IGNORE_API_RESOLUTION_ERROR` environment variable, it is now safe to unset it.
+
 - "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott
 
 Work in this release was contributed by @trzeciak and @lizhiyao. Thank you for your contributions!

dev-packages/browser-integration-tests/suites/replay/dsc/subject.js

@@ -0,0 +1,5 @@
+import * as Sentry from '@sentry/browser';
+
+window._triggerError = function (errorCount) {
+  Sentry.captureException(new Error(`This is error #${errorCount}`));
+};

dev-packages/browser-integration-tests/suites/replay/dsc/test.ts

@@ -3,7 +3,12 @@ import type * as Sentry from '@sentry/browser';
 import type { EventEnvelopeHeaders } from '@sentry/types';
 
 import { sentryTest } from '../../../utils/fixtures';
-import { envelopeRequestParser, shouldSkipTracingTest, waitForTransactionRequest } from '../../../utils/helpers';
+import {
+  envelopeRequestParser,
+  shouldSkipTracingTest,
+  waitForErrorRequest,
+  waitForTransactionRequest,
+} from '../../../utils/helpers';
 import { getReplaySnapshot, shouldSkipReplayTest, waitForReplayRunning } from '../../../utils/replayHelpers';
 
 type TestWindow = Window & {
@@ -216,3 +221,77 @@ sentryTest(
     });
   },
 );
+
+sentryTest('should add replay_id to error DSC while replay is active', async ({ getLocalTestPath, page }) => {
+  if (shouldSkipReplayTest()) {
+    sentryTest.skip();
+  }
+
+  const hasTracing = !shouldSkipTracingTest();
+
+  await page.route('https://dsn.ingest.sentry.io/**/*', route => {
+    return route.fulfill({
+      status: 200,
+      contentType: 'application/json',
+      body: JSON.stringify({ id: 'test-id' }),
+    });
+  });
+
+  const url = await getLocalTestPath({ testDir: __dirname });
+  await page.goto(url);
+
+  const error1Req = waitForErrorRequest(page, event => event.exception?.values?.[0].value === 'This is error #1');
+  const error2Req = waitForErrorRequest(page, event => event.exception?.values?.[0].value === 'This is error #2');
+
+  // We want to wait for the transaction to be done, to ensure we have a consistent test
+  const transactionReq = hasTracing ? waitForTransactionRequest(page) : Promise.resolve();
+
+  // Wait for this to be available
+  await page.waitForFunction('!!window.Replay');
+
+  // We have to start replay before we finish the transaction, otherwise the DSC will not be frozen with the Replay ID
+  await page.evaluate('window.Replay.start();');
+  await waitForReplayRunning(page);
+  await transactionReq;
+
+  await page.evaluate('window._triggerError(1)');
+
+  const error1Header = envelopeRequestParser(await error1Req, 0) as EventEnvelopeHeaders;
+  const replay = await getReplaySnapshot(page);
+
+  expect(replay.session?.id).toBeDefined();
+
+  expect(error1Header.trace).toBeDefined();
+  expect(error1Header.trace).toEqual({
+    environment: 'production',
+    trace_id: expect.any(String),
+    public_key: 'public',
+    replay_id: replay.session?.id,
+    ...(hasTracing
+      ? {
+          sample_rate: '1',
+          sampled: 'true',
+        }
+      : {}),
+  });
+
+  // Now end replay and trigger another error, it should not have a replay_id in DSC anymore
+  await page.evaluate('window.Replay.stop();');
+  await page.waitForFunction('!window.Replay.getReplayId();');
+  await page.evaluate('window._triggerError(2)');
+
+  const error2Header = envelopeRequestParser(await error2Req, 0) as EventEnvelopeHeaders;
+
+  expect(error2Header.trace).toBeDefined();
+  expect(error2Header.trace).toEqual({
+    environment: 'production',
+    trace_id: expect.any(String),
+    public_key: 'public',
+    ...(hasTracing
+      ? {
+          sample_rate: '1',
+          sampled: 'true',
+        }
+      : {}),
+  });
+});

dev-packages/browser-integration-tests/utils/helpers.ts

@@ -199,7 +199,7 @@ export async function waitForTransactionRequestOnUrl(page: Page, url: string): P
   return req;
 }
 
-export function waitForErrorRequest(page: Page): Promise<Request> {
+export function waitForErrorRequest(page: Page, callback?: (event: Event) => boolean): Promise<Request> {
   return page.waitForRequest(req => {
     const postData = req.postData();
     if (!postData) {
@@ -209,7 +209,15 @@ export function waitForErrorRequest(page: Page): Promise<Request> {
     try {
       const event = envelopeRequestParser(req);
 
-      return !event.type;
+      if (event.type) {
+        return false;
+      }
+
+      if (callback) {
+        return callback(event);
+      }
+
+      return true;
     } catch {
       return false;
     }

packages/nextjs/src/common/types.ts

@@ -55,11 +55,6 @@ export type WrappedNextApiHandler = {
   __sentry_route__?: string;
   __sentry_wrapped__?: boolean;
 };
-
-export type AugmentedNextApiRequest = NextApiRequest & {
-  __withSentry_applied__?: boolean;
-};
-
 export type AugmentedNextApiResponse = NextApiResponse & {
   __sentryTransaction?: SentrySpan;
 };

packages/nextjs/src/common/wrapApiHandlerWithSentry.ts

@@ -6,21 +6,24 @@ import {
   startSpanManual,
   withIsolationScope,
 } from '@sentry/core';
-import { consoleSandbox, isString, logger, objectify, vercelWaitUntil } from '@sentry/utils';
-
 import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from '@sentry/core';
-import type { AugmentedNextApiRequest, AugmentedNextApiResponse, NextApiHandler } from './types';
+import { isString, logger, objectify, vercelWaitUntil } from '@sentry/utils';
+import type { NextApiRequest } from 'next';
+import type { AugmentedNextApiResponse, NextApiHandler } from './types';
 import { flushSafelyWithTimeout } from './utils/responseEnd';
 import { escapeNextjsTracing } from './utils/tracingUtils';
 
+export type AugmentedNextApiRequest = NextApiRequest & {
+  __withSentry_applied__?: boolean;
+};
+
 /**
- * Wrap the given API route handler for tracing and error capturing. Thin wrapper around `withSentry`, which only
- * applies it if it hasn't already been applied.
+ * Wrap the given API route handler with error nad performance monitoring.
  *
  * @param apiHandler The handler exported from the user's API page route file, which may or may not already be
  * wrapped with `withSentry`
  * @param parameterizedRoute The page's parameterized route.
- * @returns The wrapped handler
+ * @returns The wrapped handler which will always return a Promise.
  */
 export function wrapApiHandlerWithSentry(apiHandler: NextApiHandler, parameterizedRoute: string): NextApiHandler {
   return new Proxy(apiHandler, {
@@ -44,9 +47,7 @@ export function wrapApiHandlerWithSentry(apiHandler: NextApiHandler, parameteriz
           return wrappingTarget.apply(thisArg, args);
         }
 
-        // We're now auto-wrapping API route handlers using `wrapApiHandlerWithSentry` (which uses `withSentry` under the hood), but
-        // users still may have their routes manually wrapped with `withSentry`. This check makes `sentryWrappedHandler`
-        // idempotent so that those cases don't break anything.
+        // Prevent double wrapping of the same request.
         if (req.__withSentry_applied__) {
           return wrappingTarget.apply(thisArg, args);
         }
@@ -55,7 +56,6 @@ export function wrapApiHandlerWithSentry(apiHandler: NextApiHandler, parameteriz
         return withIsolationScope(isolationScope => {
           return continueTrace(
             {
-              // TODO(v8): Make it so that continue trace will allow null as sentryTrace value and remove this fallback here
               sentryTrace:
                 req.headers && isString(req.headers['sentry-trace']) ? req.headers['sentry-trace'] : undefined,
               baggage: req.headers?.baggage,
@@ -80,31 +80,15 @@ export function wrapApiHandlerWithSentry(apiHandler: NextApiHandler, parameteriz
                   // eslint-disable-next-line @typescript-eslint/unbound-method
                   res.end = new Proxy(res.end, {
                     apply(target, thisArg, argArray) {
-                      if (span.isRecording()) {
-                        setHttpStatus(span, res.statusCode);
-                        span.end();
-                      }
+                      setHttpStatus(span, res.statusCode);
+                      span.end();
                       vercelWaitUntil(flushSafelyWithTimeout());
-                      target.apply(thisArg, argArray);
+                      return target.apply(thisArg, argArray);
                     },
                   });
 
                   try {
-                    const handlerResult = await wrappingTarget.apply(thisArg, args);
-                    if (
-                      process.env.NODE_ENV === 'development' &&
-                      !process.env.SENTRY_IGNORE_API_RESOLUTION_ERROR &&
-                      !res.writableEnded
-                    ) {
-                      consoleSandbox(() => {
-                        // eslint-disable-next-line no-console
-                        console.warn(
-                          '[sentry] If Next.js logs a warning "API resolved without sending a response", it\'s a false positive, which may happen when you use `wrapApiHandlerWithSentry` manually to wrap your routes. To suppress this warning, set `SENTRY_IGNORE_API_RESOLUTION_ERROR` to 1 in your env. To suppress the nextjs warning, use the `externalResolver` API route option (see https://nextjs.org/docs/api-routes/api-middlewares#custom-config for details).',
-                        );
-                      });
-                    }
-
-                    return handlerResult;
+                    return await wrappingTarget.apply(thisArg, args);
                   } catch (e) {
                     // 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. (Because of the one-way-on-Vercel-one-way-off-of-Vercel approach we've been forced
@@ -123,16 +107,8 @@ export function wrapApiHandlerWithSentry(apiHandler: NextApiHandler, parameteriz
                       },
                     });
 
-                    // Because we're going to finish and send the transaction before passing the error onto nextjs, it won't yet
-                    // have had a chance to set the status to 500, so unless we do it ourselves now, we'll incorrectly report that
-                    // the transaction was error-free
-                    res.statusCode = 500;
-                    res.statusMessage = 'Internal Server Error';
-
-                    if (span.isRecording()) {
-                      setHttpStatus(span, res.statusCode);
-                      span.end();
-                    }
+                    setHttpStatus(span, 500);
+                    span.end();
 
                     vercelWaitUntil(flushSafelyWithTimeout());
 

packages/nextjs/src/server/index.ts

@@ -14,6 +14,7 @@ import { GLOBAL_OBJ, logger } from '@sentry/utils';
 import {
   ATTR_HTTP_REQUEST_METHOD,
   ATTR_HTTP_ROUTE,
+  ATTR_URL_QUERY,
   SEMATTRS_HTTP_METHOD,
   SEMATTRS_HTTP_TARGET,
 } from '@opentelemetry/semantic-conventions';
@@ -157,11 +158,14 @@ export function init(options: NodeOptions): NodeClient | undefined {
     // We need to drop these spans.
     if (
       // eslint-disable-next-line deprecation/deprecation
-      typeof spanAttributes[SEMATTRS_HTTP_TARGET] === 'string' &&
-      // eslint-disable-next-line deprecation/deprecation
-      spanAttributes[SEMATTRS_HTTP_TARGET].includes('sentry_key') &&
-      // eslint-disable-next-line deprecation/deprecation
-      spanAttributes[SEMATTRS_HTTP_TARGET].includes('sentry_client')
+      (typeof spanAttributes[SEMATTRS_HTTP_TARGET] === 'string' &&
+        // eslint-disable-next-line deprecation/deprecation
+        spanAttributes[SEMATTRS_HTTP_TARGET].includes('sentry_key') &&
+        // eslint-disable-next-line deprecation/deprecation
+        spanAttributes[SEMATTRS_HTTP_TARGET].includes('sentry_client')) ||
+      (typeof spanAttributes[ATTR_URL_QUERY] === 'string' &&
+        spanAttributes[ATTR_URL_QUERY].includes('sentry_key') &&
+        spanAttributes[ATTR_URL_QUERY].includes('sentry_client'))
     ) {
       samplingDecision.decision = false;
     }
@@ -238,8 +242,7 @@ export function init(options: NodeOptions): NodeClient | undefined {
             // Pages router
             event.transaction === '/404' ||
             // App router (could be "GET /404", "POST /404", ...)
-            event.transaction?.match(/^(GET|HEAD|POST|PUT|DELETE|CONNECT|OPTIONS|TRACE|PATCH) \/404$/) ||
-            event.transaction === 'GET /_not-found'
+            event.transaction?.match(/^(GET|HEAD|POST|PUT|DELETE|CONNECT|OPTIONS|TRACE|PATCH) \/(404|_not-found)$/)
           ) {
             return null;
           }

packages/node/src/integrations/tracing/graphql.ts

@@ -40,12 +40,7 @@ const INTEGRATION_NAME = 'Graphql';
 export const instrumentGraphql = generateInstrumentOnce<GraphqlOptions>(
   INTEGRATION_NAME,
   (_options: GraphqlOptions = {}) => {
-    const options = {
-      ignoreResolveSpans: true,
-      ignoreTrivialResolveSpans: true,
-      useOperationNameForRootSpan: true,
-      ..._options,
-    };
+    const options = getOptionsWithDefaults(_options);
 
     return new GraphQLInstrumentation({
       ...options,
@@ -89,7 +84,10 @@ const _graphqlIntegration = ((options: GraphqlOptions = {}) => {
   return {
     name: INTEGRATION_NAME,
     setupOnce() {
-      instrumentGraphql(options);
+      // We set defaults here, too, because otherwise we'd update the instrumentation config
+      // to the config without defaults, as `generateInstrumentOnce` automatically calls `setConfig(options)`
+      // when being called the second time
+      instrumentGraphql(getOptionsWithDefaults(options));
     },
   };
 }) satisfies IntegrationFn;
@@ -100,3 +98,12 @@ const _graphqlIntegration = ((options: GraphqlOptions = {}) => {
  * Capture tracing data for GraphQL.
  */
 export const graphqlIntegration = defineIntegration(_graphqlIntegration);
+
+function getOptionsWithDefaults(options?: GraphqlOptions): GraphqlOptions {
+  return {
+    ignoreResolveSpans: true,
+    ignoreTrivialResolveSpans: true,
+    useOperationNameForRootSpan: true,
+    ...options,
+  };
+}

packages/node/src/otel/instrument.ts

@@ -1,7 +1,8 @@
 import type { Instrumentation } from '@opentelemetry/instrumentation';
 import { addOpenTelemetryInstrumentation } from '@sentry/opentelemetry';
 
-const INSTRUMENTED: Record<string, Instrumentation> = {};
+/** Exported only for tests. */
+export const INSTRUMENTED: Record<string, Instrumentation> = {};
 
 /**
  * Instrument an OpenTelemetry instrumentation once.

packages/node/test/integrations/tracing/graphql.test.ts

@@ -0,0 +1,46 @@
+import { GraphQLInstrumentation } from '@opentelemetry/instrumentation-graphql';
+import { graphqlIntegration, instrumentGraphql } from '../../../src/integrations/tracing/graphql';
+import { INSTRUMENTED } from '../../../src/otel/instrument';
+
+jest.mock('@opentelemetry/instrumentation-graphql');
+
+describe('GraphQL', () => {
+  beforeEach(() => {
+    jest.clearAllMocks();
+    delete INSTRUMENTED.Graphql;
+
+    (GraphQLInstrumentation as unknown as jest.SpyInstance).mockImplementation(() => {
+      return {
+        setTracerProvider: () => undefined,
+        setMeterProvider: () => undefined,
+        getConfig: () => ({}),
+        setConfig: () => ({}),
+        enable: () => undefined,
+      };
+    });
+  });
+
+  it('defaults are correct for instrumentGraphql', () => {
+    instrumentGraphql({ ignoreTrivialResolveSpans: false });
+
+    expect(GraphQLInstrumentation).toHaveBeenCalledTimes(1);
+    expect(GraphQLInstrumentation).toHaveBeenCalledWith({
+      ignoreResolveSpans: true,
+      ignoreTrivialResolveSpans: false,
+      useOperationNameForRootSpan: true,
+      responseHook: expect.any(Function),
+    });
+  });
+
+  it('defaults are correct for _graphqlIntegration', () => {
+    graphqlIntegration({ ignoreTrivialResolveSpans: false }).setupOnce!();
+
+    expect(GraphQLInstrumentation).toHaveBeenCalledTimes(1);
+    expect(GraphQLInstrumentation).toHaveBeenCalledWith({
+      ignoreResolveSpans: true,
+      ignoreTrivialResolveSpans: false,
+      useOperationNameForRootSpan: true,
+      responseHook: expect.any(Function),
+    });
+  });
+});