Remove Http integration from Next.js

s1gr1d · s1gr1d · commit 218433434e75 · 2024-03-27T11:33:26.000+01:00
diff --git a/packages/nextjs/src/common/wrapRouteHandlerWithSentry.ts b/packages/nextjs/src/common/wrapRouteHandlerWithSentry.ts
@@ -1,17 +1,14 @@
 import {
-  SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
-  SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
   SPAN_STATUS_ERROR,
   addTracingExtensions,
   captureException,
-  continueTrace,
+  getActiveSpan,
+  getIsolationScope,
+  getRootSpan,
   handleCallbackErrors,
   setHttpStatus,
-  startSpan,
-  withIsolationScope,
 } from '@sentry/core';
 import { winterCGHeadersToDict } from '@sentry/utils';
-
 import { isNotFoundNavigationError, isRedirectNavigationError } from './nextNavigationErrorUtils';
 import type { RouteHandlerContext } from './types';
 import { platformSupportsStreaming } from './utils/platformSupportsStreaming';
@@ -26,73 +23,55 @@ 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;
+
+  const { headers } = context;
+
   return new Proxy(routeHandler, {
-    apply: (originalFunction, thisArg, args) => {
-      return withIsolationScope(async isolationScope => {
-        isolationScope.setSDKProcessingMetadata({
-          request: {
-            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,
-                          },
-                        });
-                      }
-                    },
-                  );
+    apply: async (originalFunction, thisArg, args) => {
+      getIsolationScope().setSDKProcessingMetadata({
+        request: {
+          headers: headers ? winterCGHeadersToDict(headers) : undefined,
+        },
+      });
 
-                  try {
-                    if (span && response.status) {
-                      setHttpStatus(span, response.status);
-                    }
-                  } catch {
-                    // best effort - response may be undefined?
-                  }
+      try {
+        const activeSpan = getActiveSpan();
+        const rootSpan = activeSpan && getRootSpan(activeSpan);
 
-                  return response;
+        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,
                 },
-              );
-            } 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
@@ -75,8 +75,9 @@ 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 and Http instrumentation, so we shouldn't add ours on-top
+        integration.name !== 'NodeFetch' &&
+        integration.name !== 'Http',
     ),
     onUncaughtExceptionIntegration(),
   ];
diff --git a/packages/node-experimental/src/integrations/http.ts b/packages/node-experimental/src/integrations/http.ts
@@ -65,12 +65,6 @@ const _httpIntegration = ((options: HttpOptions = {}) => {
           ignoreIncomingRequestHook: request => {
             const url = getRequestUrl(request);
 
-            const method = request.method?.toUpperCase();
-            // We do not capture OPTIONS/HEAD requests as transactions
-            if (method === 'OPTIONS' || method === 'HEAD') {
-              return true;
-            }
-
             if (_ignoreIncomingRequests && _ignoreIncomingRequests(url)) {
               return true;
             }
diff --git a/packages/opentelemetry/src/sampler.ts b/packages/opentelemetry/src/sampler.ts
@@ -4,10 +4,11 @@ import { TraceState } from '@opentelemetry/core';
 import type { Sampler, SamplingResult } from '@opentelemetry/sdk-trace-base';
 import { SamplingDecision } from '@opentelemetry/sdk-trace-base';
 import { SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, hasTracingEnabled } from '@sentry/core';
-import type { Client, ClientOptions, SamplingContext } from '@sentry/types';
+import type { Client, ClientOptions, SamplingContext, SpanAttributes } from '@sentry/types';
 import { isNaN, logger } from '@sentry/utils';
 import { SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING } from './constants';
 
+import { SemanticAttributes } from '@opentelemetry/semantic-conventions';
 import { DEBUG_BUILD } from './debug-build';
 import { getPropagationContextFromSpanContext, getSamplingDecision } from './propagator';
 import { setIsSetup } from './utils/setupCheck';
@@ -29,7 +30,7 @@ export class SentrySampler implements Sampler {
     traceId: string,
     spanName: string,
     _spanKind: unknown,
-    spanAttributes: unknown,
+    spanAttributes: SpanAttributes,
     _links: unknown,
   ): SamplingResult {
     const options = this._client.getOptions();
@@ -82,6 +83,15 @@ export class SentrySampler implements Sampler {
       };
     }
 
+    const method = `${spanAttributes[SemanticAttributes.HTTP_METHOD]}`.toUpperCase();
+    if (method === 'OPTIONS' || method === 'HEAD') {
+      return {
+        decision: SamplingDecision.NOT_RECORD,
+        attributes,
+        traceState: traceState.set(SENTRY_TRACE_STATE_SAMPLED_NOT_RECORDING, '1'),
+      };
+    }
+
     // if the function returned 0 (or false), or if `tracesSampleRate` is 0, it's a sign the transaction should be dropped
     if (!sampleRate) {
       DEBUG_BUILD &&
diff --git a/packages/opentelemetry/test/trace.test.ts b/packages/opentelemetry/test/trace.test.ts
@@ -20,6 +20,7 @@ import {
 import type { Event, Scope } from '@sentry/types';
 import { getSamplingDecision, makeTraceState } from '../src/propagator';
 
+import { SemanticAttributes } from '@opentelemetry/semantic-conventions';
 import { continueTrace, startInactiveSpan, startSpan, startSpanManual } from '../src/trace';
 import type { AbstractSpan } from '../src/types';
 import { getDynamicSamplingContextFromSpan } from '../src/utils/dynamicSamplingContext';
@@ -1353,6 +1354,76 @@ describe('trace (sampling)', () => {
   });
 });
 
+describe('HTTP methods (sampling)', () => {
+  beforeEach(() => {
+    mockSdkInit({ enableTracing: true });
+  });
+
+  afterEach(() => {
+    cleanupOtel();
+  });
+
+  it('does sample when HTTP method is other than OPTIONS or HEAD', () => {
+    const spanGET = startSpanManual(
+      { name: 'test span', attributes: { [SemanticAttributes.HTTP_METHOD]: 'GET' } },
+      span => {
+        return span;
+      },
+    );
+    expect(spanIsSampled(spanGET)).toBe(true);
+    expect(getSamplingDecision(spanGET.spanContext())).toBe(true);
+
+    const spanPOST = startSpanManual(
+      { name: 'test span', attributes: { [SemanticAttributes.HTTP_METHOD]: 'POST' } },
+      span => {
+        return span;
+      },
+    );
+    expect(spanIsSampled(spanPOST)).toBe(true);
+    expect(getSamplingDecision(spanPOST.spanContext())).toBe(true);
+
+    const spanPUT = startSpanManual(
+      { name: 'test span', attributes: { [SemanticAttributes.HTTP_METHOD]: 'PUT' } },
+      span => {
+        return span;
+      },
+    );
+    expect(spanIsSampled(spanPUT)).toBe(true);
+    expect(getSamplingDecision(spanPUT.spanContext())).toBe(true);
+
+    const spanDELETE = startSpanManual(
+      { name: 'test span', attributes: { [SemanticAttributes.HTTP_METHOD]: 'DELETE' } },
+      span => {
+        return span;
+      },
+    );
+    expect(spanIsSampled(spanDELETE)).toBe(true);
+    expect(getSamplingDecision(spanDELETE.spanContext())).toBe(true);
+  });
+
+  it('does not sample when HTTP method is OPTIONS', () => {
+    const span = startSpanManual(
+      { name: 'test span', attributes: { [SemanticAttributes.HTTP_METHOD]: 'OPTIONS' } },
+      span => {
+        return span;
+      },
+    );
+    expect(spanIsSampled(span)).toBe(false);
+    expect(getSamplingDecision(span.spanContext())).toBe(false);
+  });
+
+  it('does not sample when HTTP method is HEAD', () => {
+    const span = startSpanManual(
+      { name: 'test span', attributes: { [SemanticAttributes.HTTP_METHOD]: 'HEAD' } },
+      span => {
+        return span;
+      },
+    );
+    expect(spanIsSampled(span)).toBe(false);
+    expect(getSamplingDecision(span.spanContext())).toBe(false);
+  });
+});
+
 describe('continueTrace', () => {
   beforeEach(() => {
     mockSdkInit({ enableTracing: true });
