fix(nextjs): Re-enable OTEL fetch instrumentation and disable Next.js fetch instrumentation (#11686)

Author: lforst

dev-packages/e2e-tests/test-applications/nextjs-14/app/propagation/test-outgoing-fetch-external-disallowed/check/route.ts

@@ -0,0 +1,5 @@
+import { checkHandler } from '../../utils';
+
+export const dynamic = 'force-dynamic';
+
+export const GET = checkHandler;

dev-packages/e2e-tests/test-applications/nextjs-14/app/propagation/test-outgoing-fetch-external-disallowed/route.ts

@@ -0,0 +1,10 @@
+import { NextResponse } from 'next/server';
+
+export const dynamic = 'force-dynamic';
+
+export async function GET() {
+  const data = await fetch(`http://localhost:3030/propagation/test-outgoing-fetch-external-disallowed/check`).then(
+    res => res.json(),
+  );
+  return NextResponse.json(data);
+}

dev-packages/e2e-tests/test-applications/nextjs-14/app/propagation/test-outgoing-fetch/check/route.ts

@@ -0,0 +1,5 @@
+import { checkHandler } from '../../utils';
+
+export const dynamic = 'force-dynamic';
+
+export const GET = checkHandler;

dev-packages/e2e-tests/test-applications/nextjs-14/app/propagation/test-outgoing-fetch/route.ts

@@ -0,0 +1,8 @@
+import { NextResponse } from 'next/server';
+
+export const dynamic = 'force-dynamic';
+
+export async function GET() {
+  const data = await fetch(`http://localhost:3030/propagation/test-outgoing-fetch/check`).then(res => res.json());
+  return NextResponse.json(data);
+}

dev-packages/e2e-tests/test-applications/nextjs-14/app/propagation/test-outgoing-http-external-disallowed/check/route.ts

@@ -0,0 +1,5 @@
+import { checkHandler } from '../../utils';
+
+export const dynamic = 'force-dynamic';
+
+export const GET = checkHandler;

dev-packages/e2e-tests/test-applications/nextjs-14/app/propagation/test-outgoing-http-external-disallowed/route.ts

@@ -0,0 +1,9 @@
+import { NextResponse } from 'next/server';
+import { makeHttpRequest } from '../utils';
+
+export const dynamic = 'force-dynamic';
+
+export async function GET() {
+  const data = await makeHttpRequest(`http://localhost:3030/propagation/test-outgoing-http-external-disallowed/check`);
+  return NextResponse.json(data);
+}

dev-packages/e2e-tests/test-applications/nextjs-14/app/propagation/test-outgoing-http/check/route.ts

@@ -0,0 +1,5 @@
+import { checkHandler } from '../../utils';
+
+export const dynamic = 'force-dynamic';
+
+export const GET = checkHandler;

dev-packages/e2e-tests/test-applications/nextjs-14/app/propagation/test-outgoing-http/route.ts

@@ -0,0 +1,9 @@
+import { NextResponse } from 'next/server';
+import { makeHttpRequest } from '../utils';
+
+export const dynamic = 'force-dynamic';
+
+export async function GET() {
+  const data = await makeHttpRequest(`http://localhost:3030/propagation/test-outgoing-http/check`);
+  return NextResponse.json(data);
+}

dev-packages/e2e-tests/test-applications/nextjs-14/app/propagation/utils.ts

@@ -0,0 +1,38 @@
+import http from 'http';
+import { headers } from 'next/headers';
+import { NextResponse } from 'next/server';
+
+export function makeHttpRequest(url: string) {
+  return new Promise(resolve => {
+    const data: any[] = [];
+    http
+      .request(url, httpRes => {
+        httpRes.on('data', chunk => {
+          data.push(chunk);
+        });
+        httpRes.on('error', error => {
+          resolve({ error: error.message, url });
+        });
+        httpRes.on('end', () => {
+          try {
+            const json = JSON.parse(Buffer.concat(data).toString());
+            resolve(json);
+          } catch {
+            resolve({ data: Buffer.concat(data).toString(), url });
+          }
+        });
+      })
+      .end();
+  });
+}
+
+export function checkHandler() {
+  const headerList = headers();
+
+  const headerObj: Record<string, unknown> = {};
+  headerList.forEach((value, key) => {
+    headerObj[key] = value;
+  });
+
+  return NextResponse.json({ headers: headerObj });
+}

dev-packages/e2e-tests/test-applications/nextjs-14/app/request-instrumentation/page.tsx

@@ -3,7 +3,7 @@ import http from 'http';
 export const dynamic = 'force-dynamic';
 
 export default async function Page() {
-  await fetch('http://example.com/', { cache: 'no-cache' });
+  await fetch('http://example.com/', { cache: 'no-cache' }).then(res => res.text());
   await new Promise<void>(resolve => {
     http.get('http://example.com/', res => {
       res.on('data', () => {

dev-packages/e2e-tests/test-applications/nextjs-14/instrumentation.ts

@@ -12,6 +12,10 @@ export function register() {
         // We are doing a lot of events at once in this test
         bufferSize: 1000,
       },
+      tracePropagationTargets: [
+        'http://localhost:3030/propagation/test-outgoing-fetch/check',
+        'http://localhost:3030/propagation/test-outgoing-http/check',
+      ],
     });
   }
 }

dev-packages/e2e-tests/test-applications/nextjs-14/playwright.config.ts

@@ -22,7 +22,7 @@ const eventProxyPort = 3031;
 const config: PlaywrightTestConfig = {
   testDir: './tests',
   /* Maximum time one test can run for. */
-  timeout: 150_000,
+  timeout: 30_000,
   expect: {
     /**
      * Maximum time expect() should wait for the condition to be met.

dev-packages/e2e-tests/test-applications/nextjs-14/tests/propagation.test.ts

@@ -0,0 +1,113 @@
+import { expect, test } from '@playwright/test';
+import { waitForTransaction } from '@sentry-internal/event-proxy-server';
+
+test('Propagates trace for outgoing http requests', async ({ baseURL, request }) => {
+  const inboundTransactionPromise = waitForTransaction('nextjs-14', transactionEvent => {
+    return transactionEvent.transaction === 'GET /propagation/test-outgoing-http/check';
+  });
+
+  const outboundTransactionPromise = waitForTransaction('nextjs-14', transactionEvent => {
+    return transactionEvent.transaction === 'GET /propagation/test-outgoing-http';
+  });
+
+  const { headers } = await (await request.get(`${baseURL}/propagation/test-outgoing-http`)).json();
+
+  const inboundTransaction = await inboundTransactionPromise;
+  const outboundTransaction = await outboundTransactionPromise;
+
+  expect(inboundTransaction.contexts?.trace?.trace_id).toStrictEqual(expect.any(String));
+  expect(inboundTransaction.contexts?.trace?.trace_id).toBe(outboundTransaction.contexts?.trace?.trace_id);
+
+  const httpClientSpan = outboundTransaction.spans?.find(span => span.op === 'http.client');
+
+  expect(httpClientSpan).toBeDefined();
+  expect(httpClientSpan?.span_id).toStrictEqual(expect.any(String));
+  expect(inboundTransaction.contexts?.trace?.parent_span_id).toBe(httpClientSpan?.span_id);
+
+  expect(headers).toMatchObject({
+    baggage: expect.any(String),
+    'sentry-trace': `${outboundTransaction.contexts?.trace?.trace_id}-${httpClientSpan?.span_id}-1`,
+  });
+});
+
+test('Propagates trace for outgoing fetch requests', async ({ baseURL, request }) => {
+  const inboundTransactionPromise = waitForTransaction('nextjs-14', transactionEvent => {
+    return transactionEvent.transaction === 'GET /propagation/test-outgoing-fetch/check';
+  });
+
+  const outboundTransactionPromise = waitForTransaction('nextjs-14', transactionEvent => {
+    return transactionEvent.transaction === 'GET /propagation/test-outgoing-fetch';
+  });
+
+  const { headers } = await (await request.get(`${baseURL}/propagation/test-outgoing-fetch`)).json();
+
+  const inboundTransaction = await inboundTransactionPromise;
+  const outboundTransaction = await outboundTransactionPromise;
+
+  expect(inboundTransaction.contexts?.trace?.trace_id).toStrictEqual(expect.any(String));
+  expect(inboundTransaction.contexts?.trace?.trace_id).toBe(outboundTransaction.contexts?.trace?.trace_id);
+
+  const httpClientSpan = outboundTransaction.spans?.find(
+    span => span.op === 'http.client' && span.data?.['sentry.origin'] === 'auto.http.otel.node_fetch',
+  );
+
+  // Right now we assert that the OTEL span is the last span before propagating
+  expect(httpClientSpan).toBeDefined();
+  expect(httpClientSpan?.span_id).toStrictEqual(expect.any(String));
+  expect(inboundTransaction.contexts?.trace?.parent_span_id).toBe(httpClientSpan?.span_id);
+
+  expect(headers).toMatchObject({
+    baggage: expect.any(String),
+    'sentry-trace': `${outboundTransaction.contexts?.trace?.trace_id}-${httpClientSpan?.span_id}-1`,
+  });
+});
+
+test('Does not propagate outgoing http requests not covered by tracePropagationTargets', async ({
+  baseURL,
+  request,
+}) => {
+  const inboundTransactionPromise = waitForTransaction('nextjs-14', transactionEvent => {
+    return transactionEvent.transaction === 'GET /propagation/test-outgoing-http-external-disallowed/check';
+  });
+
+  const outboundTransactionPromise = waitForTransaction('nextjs-14', transactionEvent => {
+    return transactionEvent.transaction === 'GET /propagation/test-outgoing-http-external-disallowed';
+  });
+
+  const { headers } = await (await request.get(`${baseURL}/propagation/test-outgoing-http-external-disallowed`)).json();
+
+  expect(headers.baggage).toBeUndefined();
+  expect(headers['sentry-trace']).toBeUndefined();
+
+  const inboundTransaction = await inboundTransactionPromise;
+  const outboundTransaction = await outboundTransactionPromise;
+
+  expect(typeof outboundTransaction.contexts?.trace?.trace_id).toBe('string');
+  expect(inboundTransaction.contexts?.trace?.trace_id).not.toBe(outboundTransaction.contexts?.trace?.trace_id);
+});
+
+test('Does not propagate outgoing fetch requests not covered by tracePropagationTargets', async ({
+  baseURL,
+  request,
+}) => {
+  const inboundTransactionPromise = waitForTransaction('nextjs-14', transactionEvent => {
+    return transactionEvent.transaction === 'GET /propagation/test-outgoing-fetch-external-disallowed/check';
+  });
+
+  const outboundTransactionPromise = waitForTransaction('nextjs-14', transactionEvent => {
+    return transactionEvent.transaction === 'GET /propagation/test-outgoing-fetch-external-disallowed';
+  });
+
+  const { headers } = await (
+    await request.get(`${baseURL}/propagation/test-outgoing-fetch-external-disallowed`)
+  ).json();
+
+  expect(headers.baggage).toBeUndefined();
+  expect(headers['sentry-trace']).toBeUndefined();
+
+  const inboundTransaction = await inboundTransactionPromise;
+  const outboundTransaction = await outboundTransactionPromise;
+
+  expect(typeof outboundTransaction.contexts?.trace?.trace_id).toBe('string');
+  expect(inboundTransaction.contexts?.trace?.trace_id).not.toBe(outboundTransaction.contexts?.trace?.trace_id);
+});

dev-packages/e2e-tests/test-applications/nextjs-14/tests/request-instrumentation.test.ts

@@ -8,24 +8,27 @@ test('Should send a transaction with a fetch span', async ({ page }) => {
 
   await page.goto(`/request-instrumentation`);
 
-  expect((await transactionPromise).spans).toContainEqual(
+  await expect(transactionPromise).resolves.toBeDefined();
+
+  const transactionEvent = await transactionPromise;
+
+  expect(transactionEvent.spans).toContainEqual(
     expect.objectContaining({
       data: expect.objectContaining({
         'http.method': 'GET',
         'sentry.op': 'http.client',
-        'next.span_type': 'AppRender.fetch', // This span is created by Next.js own fetch instrumentation
+        'sentry.origin': 'auto.http.otel.node_fetch',
       }),
       description: 'GET http://example.com/',
     }),
   );
 
-  expect((await transactionPromise).spans).toContainEqual(
+  expect(transactionEvent.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',
+        'sentry.origin': 'auto.http.otel.http',
       }),
       description: 'GET http://example.com/',
     }),

packages/nextjs/src/server/index.ts

@@ -73,14 +73,16 @@ export function init(options: NodeOptions): void {
   const customDefaultIntegrations = [
     ...getDefaultIntegrations(options).filter(
       integration =>
-        // 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 would lead to double spans for route handler requests
         integration.name !== 'Http',
     ),
     httpIntegration(),
   ];
 
+  // Turn off Next.js' own fetch instrumentation
+  // https://github.com/lforst/nextjs-fork/blob/1994fd186defda77ad971c36dc3163db263c993f/packages/next/src/server/lib/patch-fetch.ts#L245
+  process.env.NEXT_OTEL_FETCH_DISABLED = '1';
+
   // This value is injected at build time, based on the output directory specified in the build config. Though a default
   // is set there, we set it here as well, just in case something has gone wrong with the injection.
   const distDirName = globalWithInjectedValues.__rewriteFramesDistDir__;