fix(nextjs): Remove Http integration from Next.js (#11304)

s1gr1d · lforst · mydea · web-flow · commit 4676ca33373c · 2024-04-09T15:27:23.000+02:00
Next.js provides their own OTel http integration, which conflicts with ours ref #11016 added commit from this PR: #11319 --------- Co-authored-by: Luca Forstner <luca.forstner@sentry.io> Co-authored-by: Francesco Novy <francesco.novy@sentry.io>
diff --git a/dev-packages/e2e-tests/test-applications/nextjs-14/tests/request-instrumentation.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-14/tests/request-instrumentation.test.ts
@@ -19,16 +19,15 @@ test('Should send a transaction with a fetch span', async ({ page }) => {
     }),
   );
 
-  // TODO: Uncomment the below when fixed. For whatever reason that we now have accepted, spans created with Node.js' http.get() will not attach themselves to transactions.
-  // More info: https://github.com/getsentry/sentry-javascript/pull/11016/files#diff-10fa195789425786c6e5e769380be18790768f0b76319ee41bbb488d9fe50405R4
-  // expect((await transactionPromise).spans).toContainEqual(
-  //   expect.objectContaining({
-  //     data: expect.objectContaining({
-  //       'http.method': 'GET',
-  //       'sentry.op': 'http.client',
-  //       'sentry.origin': 'auto.http.otel.http',
-  //     }),
-  //     description: 'GET http://example.com/',
-  //   }),
-  // );
+  expect((await transactionPromise).spans).toContainEqual(
+    expect.objectContaining({
+      data: expect.objectContaining({
+        'http.method': 'GET',
+        'sentry.op': 'http.client',
+        // todo: without the HTTP integration in the Next.js SDK, this is set to 'manual' -> we could rename this to be more specific
+        'sentry.origin': 'manual',
+      }),
+      description: 'GET http://example.com/',
+    }),
+  );
 });
diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/route-handlers.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/route-handlers.test.ts
@@ -56,6 +56,7 @@ test('Should record exceptions and transactions for faulty route handlers', asyn
 
   expect(routehandlerTransaction.contexts?.trace?.status).toBe('unknown_error');
   expect(routehandlerTransaction.contexts?.trace?.op).toBe('http.server');
+  expect(routehandlerTransaction.contexts?.trace?.origin).toBe('auto.function.nextjs');
 
   expect(routehandlerError.exception?.values?.[0].value).toBe('route-handler-error');
   // TODO: Uncomment once we update the scope transaction name on the server side
diff --git a/packages/nextjs/package.json b/packages/nextjs/package.json
@@ -65,12 +65,14 @@
     "access": "public"
   },
   "dependencies": {
+    "@opentelemetry/api": "1.7.0",
     "@rollup/plugin-commonjs": "24.0.0",
     "@sentry/core": "8.0.0-alpha.9",
     "@sentry/node": "8.0.0-alpha.9",
     "@sentry/react": "8.0.0-alpha.9",
     "@sentry/types": "8.0.0-alpha.9",
     "@sentry/utils": "8.0.0-alpha.9",
+    "@sentry/opentelemetry": "8.0.0-alpha.9",
     "@sentry/vercel-edge": "8.0.0-alpha.9",
     "@sentry/webpack-plugin": "2.16.0",
     "chalk": "3.0.0",
diff --git a/packages/nextjs/src/common/wrapRouteHandlerWithSentry.ts b/packages/nextjs/src/common/wrapRouteHandlerWithSentry.ts
@@ -1,22 +1,58 @@
 import {
+  SEMANTIC_ATTRIBUTE_SENTRY_OP,
   SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
   SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
   SPAN_STATUS_ERROR,
   addTracingExtensions,
   captureException,
-  continueTrace,
+  getActiveSpan,
+  getRootSpan,
   handleCallbackErrors,
   setHttpStatus,
   startSpan,
 } from '@sentry/core';
+import type { Span } from '@sentry/types';
 import { winterCGHeadersToDict } from '@sentry/utils';
-
 import { isNotFoundNavigationError, isRedirectNavigationError } from './nextNavigationErrorUtils';
 import type { RouteHandlerContext } from './types';
 import { platformSupportsStreaming } from './utils/platformSupportsStreaming';
 import { flushQueue } from './utils/responseEnd';
 import { withIsolationScopeOrReuseFromRootSpan } from './utils/withIsolationScopeOrReuseFromRootSpan';
 
+/** As our own HTTP integration is disabled (src/server/index.ts) the rootSpan comes from Next.js.
+ * In case there is no root span, we start a new span. */
+function startOrUpdateSpan(spanName: string, cb: (rootSpan: Span) => Promise<Response>): Promise<Response> {
+  const activeSpan = getActiveSpan();
+  const rootSpan = activeSpan && getRootSpan(activeSpan);
+
+  if (rootSpan) {
+    rootSpan.updateName(spanName);
+    rootSpan.setAttributes({
+      [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route',
+      [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'http.server',
+      [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.function.nextjs',
+    });
+
+    return cb(rootSpan);
+  } else {
+    return startSpan(
+      {
+        op: 'http.server',
+        name: spanName,
+        forceTransaction: true,
+        attributes: {
+          [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route',
+          [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'http.server',
+          [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.function.nextjs',
+        },
+      },
+      (span: Span) => {
+        return cb(span);
+      },
+    );
+  }
+}
+
 /**
  * Wraps a Next.js route handler with performance and error instrumentation.
  */
@@ -26,7 +62,9 @@ export function wrapRouteHandlerWithSentry<F extends (...args: any[]) => any>(
   context: RouteHandlerContext,
 ): (...args: Parameters<F>) => ReturnType<F> extends Promise<unknown> ? ReturnType<F> : Promise<ReturnType<F>> {
   addTracingExtensions();
+
   const { method, parameterizedRoute, headers } = context;
+
   return new Proxy(routeHandler, {
     apply: (originalFunction, thisArg, args) => {
       return withIsolationScopeOrReuseFromRootSpan(async isolationScope => {
@@ -35,63 +73,44 @@ export function wrapRouteHandlerWithSentry<F extends (...args: any[]) => any>(
             headers: headers ? winterCGHeadersToDict(headers) : undefined,
           },
         });
-        return continueTrace(
-          {
-            // TODO(v8): Make it so that continue trace will allow null as sentryTrace value and remove this fallback here
-            sentryTrace: headers?.get('sentry-trace') ?? undefined,
-            baggage: headers?.get('baggage'),
-          },
-          async () => {
-            try {
-              return await startSpan(
-                {
-                  op: 'http.server',
-                  name: `${method} ${parameterizedRoute}`,
-                  forceTransaction: true,
-                  attributes: {
-                    [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route',
-                    [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.function.nextjs',
-                  },
-                },
-                async span => {
-                  const response: Response = await handleCallbackErrors(
-                    () => originalFunction.apply(thisArg, args),
-                    error => {
-                      // Next.js throws errors when calling `redirect()`. We don't wanna report these.
-                      if (isRedirectNavigationError(error)) {
-                        // Don't do anything
-                      } else if (isNotFoundNavigationError(error)) {
-                        span.setStatus({ code: SPAN_STATUS_ERROR, message: 'not_found' });
-                      } else {
-                        captureException(error, {
-                          mechanism: {
-                            handled: false,
-                          },
-                        });
-                      }
-                    },
-                  );
 
-                  try {
-                    if (span && response.status) {
-                      setHttpStatus(span, response.status);
-                    }
-                  } catch {
-                    // best effort - response may be undefined?
-                  }
+        try {
+          return await startOrUpdateSpan(`${method} ${parameterizedRoute}`, async (rootSpan: Span) => {
+            const response: Response = await handleCallbackErrors(
+              () => originalFunction.apply(thisArg, args),
+              error => {
+                // Next.js throws errors when calling `redirect()`. We don't wanna report these.
+                if (isRedirectNavigationError(error)) {
+                  // Don't do anything
+                } else if (isNotFoundNavigationError(error) && rootSpan) {
+                  rootSpan.setStatus({ code: SPAN_STATUS_ERROR, message: 'not_found' });
+                } else {
+                  captureException(error, {
+                    mechanism: {
+                      handled: false,
+                    },
+                  });
+                }
+              },
+            );
 
-                  return response;
-                },
-              );
-            } finally {
-              if (!platformSupportsStreaming() || process.env.NEXT_RUNTIME === 'edge') {
-                // 1. Edge transport requires manual flushing
-                // 2. Lambdas require manual flushing to prevent execution freeze before the event is sent
-                await flushQueue();
+            try {
+              if (rootSpan && response.status) {
+                setHttpStatus(rootSpan, response.status);
               }
+            } catch {
+              // best effort - response may be undefined?
             }
-          },
-        );
+
+            return response;
+          });
+        } finally {
+          if (!platformSupportsStreaming() || process.env.NEXT_RUNTIME === 'edge') {
+            // 1. Edge transport requires manual flushing
+            // 2. Lambdas require manual flushing to prevent execution freeze before the event is sent
+            await flushQueue();
+          }
+        }
       });
     },
   });
diff --git a/packages/nextjs/src/server/index.ts b/packages/nextjs/src/server/index.ts
@@ -12,6 +12,7 @@ import { onUncaughtExceptionIntegration } from './onUncaughtExceptionIntegration
 
 export * from '@sentry/node';
 import type { EventProcessor } from '@sentry/types';
+import { requestIsolationScopeIntegration } from './requestIsolationScopeIntegration';
 
 export { captureUnderscoreErrorException } from '../common/_error';
 export { onUncaughtExceptionIntegration } from './onUncaughtExceptionIntegration';
@@ -75,10 +76,13 @@ export function init(options: NodeOptions): void {
     ...getDefaultIntegrations(options).filter(
       integration =>
         integration.name !== 'OnUncaughtException' &&
-        // Next.js comes with its own Node-Fetch instrumentation so we shouldn't add ours on-top
-        integration.name !== 'NodeFetch',
+        // Next.js comes with its own Node-Fetch instrumentation, so we shouldn't add ours on-top
+        integration.name !== 'NodeFetch' &&
+        // Next.js comes with its own Http instrumentation for OTel which lead to double spans for route handler requests
+        integration.name !== 'Http',
     ),
     onUncaughtExceptionIntegration(),
+    requestIsolationScopeIntegration(),
   ];
 
   // This value is injected at build time, based on the output directory specified in the build config. Though a default
diff --git a/packages/nextjs/src/server/requestIsolationScopeIntegration.ts b/packages/nextjs/src/server/requestIsolationScopeIntegration.ts
@@ -0,0 +1,44 @@
+import { SpanKind } from '@opentelemetry/api';
+import {
+  defineIntegration,
+  getCapturedScopesOnSpan,
+  getCurrentScope,
+  getIsolationScope,
+  getRootSpan,
+  setCapturedScopesOnSpan,
+  spanToJSON,
+} from '@sentry/core';
+import { getSpanKind } from '@sentry/opentelemetry';
+
+/**
+ * This integration is responsible for creating isolation scopes for incoming Http requests.
+ * We do so by waiting for http spans to be created and then forking the isolation scope.
+ *
+ * Normally the isolation scopes would be created by our Http instrumentation, however Next.js brings it's own Http
+ * instrumentation so we had to disable ours.
+ */
+export const requestIsolationScopeIntegration = defineIntegration(() => {
+  return {
+    name: 'RequestIsolationScope',
+    setup(client) {
+      client.on('spanStart', span => {
+        const spanJson = spanToJSON(span);
+        const data = spanJson.data || {};
+
+        // The following check is a heuristic to determine whether the started span is a span that tracks an incoming HTTP request
+        if (
+          (getSpanKind(span) === SpanKind.SERVER && data['http.method']) ||
+          (span === getRootSpan(span) && data['next.route'])
+        ) {
+          const scopes = getCapturedScopesOnSpan(span);
+
+          // Update the isolation scope, isolate this request
+          const isolationScope = (scopes.isolationScope || getIsolationScope()).clone();
+          const scope = scopes.scope || getCurrentScope();
+
+          setCapturedScopesOnSpan(span, scope, isolationScope);
+        }
+      });
+    },
+  };
+});
diff --git a/packages/opentelemetry/src/utils/mapStatus.ts b/packages/opentelemetry/src/utils/mapStatus.ts
@@ -26,6 +26,10 @@ const canonicalGrpcErrorCodesMap: Record<string, SpanStatus['message']> = {
   '16': 'unauthenticated',
 } as const;
 
+const isStatusErrorMessageValid = (message: string): boolean => {
+  return Object.values(canonicalGrpcErrorCodesMap).includes(message as SpanStatus['message']);
+};
+
 /**
  * Get a Sentry span status from an otel span.
  */
@@ -39,7 +43,11 @@ export function mapStatus(span: AbstractSpan): SpanStatus {
       return { code: SPAN_STATUS_OK };
       // If the span is already marked as erroneous we return that exact status
     } else if (status.code === SpanStatusCode.ERROR) {
-      return { code: SPAN_STATUS_ERROR, message: status.message };
+      if (typeof status.message === 'undefined' || isStatusErrorMessageValid(status.message)) {
+        return { code: SPAN_STATUS_ERROR, message: status.message };
+      } else {
+        return { code: SPAN_STATUS_ERROR, message: 'unknown_error' };
+      }
     }
   }
 
