feat(node): Use @opentelemetry/instrumentation-undici for fetch tracing (#13485)

timfish · web-flow · commit facaae451e1b · 2024-09-09T10:28:37.000+02:00
This PR migrates the `nativeNodeFetchIntegration` to use `@opentelemetry/instrumentation-undici` instead of `opentelemetry-instrumentation-fetch-node`. The instrumentation is still exported as `nativeNodeFetchIntegration` and is named `NodeFetch` to ensure backwards compatibility and the tests pass ~~without changes~~. Note: One `nextjs-14` e2e test did need a change due to the new/differing attribute names. It's worth noting that `@opentelemetry/instrumentation-undici` [uses different attributes](open-telemetry/opentelemetry-js-contrib#2417 (comment)) from the latest semantic convention version vs what we are using and what's used by `opentelemetry-instrumentation-fetch-node`. It looks like the [http instrumentation is migrating to these too](open-telemetry/opentelemetry-js#4940) so some of the changes in this PR will ensure that the http instrumentation continues to work after these updates.
diff --git a/.github/dependabot.yml b/.github/dependabot.yml
@@ -20,7 +20,6 @@ updates:
       - dependency-name: "@sentry/esbuild-plugin"
       - dependency-name: "@opentelemetry/*"
       - dependency-name: "@prisma/instrumentation"
-      - dependency-name: "opentelemetry-instrumentation-fetch-node"
     versioning-strategy: increase
     commit-message:
       prefix: feat
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
@@ -15,7 +15,7 @@ test('Should send a transaction with a fetch span', async ({ page }) => {
   expect(transactionEvent.spans).toContainEqual(
     expect.objectContaining({
       data: expect.objectContaining({
-        'http.method': 'GET',
+        'http.request.method': 'GET',
         'sentry.op': 'http.client',
         'sentry.origin': 'auto.http.otel.node_fetch',
       }),
diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled-no-active-span/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-sampled-no-active-span/scenario.ts
@@ -11,8 +11,6 @@ Sentry.init({
 });
 
 async function run(): Promise<void> {
-  // Since fetch is lazy loaded, we need to wait a bit until it's fully instrumented
-  await new Promise(resolve => setTimeout(resolve, 100));
   await fetch(`${process.env.SERVER_URL}/api/v0`).then(res => res.text());
   await fetch(`${process.env.SERVER_URL}/api/v1`).then(res => res.text());
   await fetch(`${process.env.SERVER_URL}/api/v2`).then(res => res.text());
diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-unsampled/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-unsampled/scenario.ts
@@ -13,8 +13,6 @@ Sentry.init({
 async function run(): Promise<void> {
   // Wrap in span that is not sampled
   await Sentry.startSpan({ name: 'outer' }, async () => {
-    // Since fetch is lazy loaded, we need to wait a bit until it's fully instrumented
-    await new Promise(resolve => setTimeout(resolve, 100));
     await fetch(`${process.env.SERVER_URL}/api/v0`).then(res => res.text());
     await fetch(`${process.env.SERVER_URL}/api/v1`).then(res => res.text());
     await fetch(`${process.env.SERVER_URL}/api/v2`).then(res => res.text());
diff --git a/packages/bun/src/integrations/bunserver.ts b/packages/bun/src/integrations/bunserver.ts
@@ -1,4 +1,5 @@
 import {
+  SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD,
   SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
   SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
   captureException,
@@ -65,7 +66,7 @@ function instrumentBunServeOptions(serveOptions: Parameters<typeof Bun.serve>[0]
         const parsedUrl = parseUrl(request.url);
         const attributes: SpanAttributes = {
           [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.bun.serve',
-          'http.request.method': request.method || 'GET',
+          [SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD]: request.method || 'GET',
           [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url',
         };
         if (parsedUrl.search) {
diff --git a/packages/cloudflare/src/request.ts b/packages/cloudflare/src/request.ts
@@ -1,9 +1,11 @@
 import type { ExecutionContext, IncomingRequestCfProperties } from '@cloudflare/workers-types';
 
 import {
+  SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD,
   SEMANTIC_ATTRIBUTE_SENTRY_OP,
   SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
   SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
+  SEMANTIC_ATTRIBUTE_URL_FULL,
   captureException,
   continueTrace,
   flush,
@@ -45,8 +47,8 @@ export function wrapRequestHandler(
       [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.cloudflare',
       [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url',
       [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'http.server',
-      ['http.request.method']: request.method,
-      ['url.full']: request.url,
+      [SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD]: request.method,
+      [SEMANTIC_ATTRIBUTE_URL_FULL]: request.url,
     };
 
     const contentLength = request.headers.get('content-length');
diff --git a/packages/core/src/semanticAttributes.ts b/packages/core/src/semanticAttributes.ts
@@ -41,3 +41,7 @@ export const SEMANTIC_ATTRIBUTE_CACHE_HIT = 'cache.hit';
 export const SEMANTIC_ATTRIBUTE_CACHE_KEY = 'cache.key';
 
 export const SEMANTIC_ATTRIBUTE_CACHE_ITEM_SIZE = 'cache.item_size';
+
+/** TODO: Remove these once we update to latest semantic conventions */
+export const SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD = 'http.request.method';
+export const SEMANTIC_ATTRIBUTE_URL_FULL = 'url.full';
diff --git a/packages/node/package.json b/packages/node/package.json
@@ -86,6 +86,7 @@
     "@opentelemetry/instrumentation-nestjs-core": "0.40.0",
     "@opentelemetry/instrumentation-pg": "0.44.0",
     "@opentelemetry/instrumentation-redis-4": "0.42.0",
+    "@opentelemetry/instrumentation-undici": "0.5.0",
     "@opentelemetry/resources": "^1.25.1",
     "@opentelemetry/sdk-trace-base": "^1.25.1",
     "@opentelemetry/semantic-conventions": "^1.25.1",
@@ -99,9 +100,6 @@
   "devDependencies": {
     "@types/node": "^14.18.0"
   },
-  "optionalDependencies": {
-    "opentelemetry-instrumentation-fetch-node": "1.2.3"
-  },
   "scripts": {
     "build": "run-p build:transpile build:types",
     "build:dev": "yarn build",
diff --git a/packages/node/src/integrations/node-fetch.ts b/packages/node/src/integrations/node-fetch.ts
@@ -1,32 +1,9 @@
-import type { Span } from '@opentelemetry/api';
-import { trace } from '@opentelemetry/api';
-import { context, propagation } from '@opentelemetry/api';
-import { addBreadcrumb, defineIntegration, getCurrentScope, hasTracingEnabled } from '@sentry/core';
-import {
-  addOpenTelemetryInstrumentation,
-  generateSpanContextForPropagationContext,
-  getPropagationContextFromSpan,
-} from '@sentry/opentelemetry';
+import type { UndiciRequest, UndiciResponse } from '@opentelemetry/instrumentation-undici';
+import { UndiciInstrumentation } from '@opentelemetry/instrumentation-undici';
+import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, addBreadcrumb, defineIntegration } from '@sentry/core';
+import { addOpenTelemetryInstrumentation } from '@sentry/opentelemetry';
 import type { IntegrationFn, SanitizedRequestData } from '@sentry/types';
-import { getSanitizedUrlString, logger, parseUrl } from '@sentry/utils';
-import { DEBUG_BUILD } from '../debug-build';
-import { NODE_MAJOR } from '../nodeVersion';
-
-import type { FetchInstrumentation } from 'opentelemetry-instrumentation-fetch-node';
-
-import { addOriginToSpan } from '../utils/addOriginToSpan';
-
-interface FetchRequest {
-  method: string;
-  origin: string;
-  path: string;
-  headers: string | string[];
-}
-
-interface FetchResponse {
-  headers: Buffer[];
-  statusCode: number;
-}
+import { getSanitizedUrlString, parseUrl } from '@sentry/utils';
 
 interface NodeFetchOptions {
   /**
@@ -46,106 +23,38 @@ const _nativeNodeFetchIntegration = ((options: NodeFetchOptions = {}) => {
   const _breadcrumbs = typeof options.breadcrumbs === 'undefined' ? true : options.breadcrumbs;
   const _ignoreOutgoingRequests = options.ignoreOutgoingRequests;
 
-  async function getInstrumentation(): Promise<FetchInstrumentation | void> {
-    // Only add NodeFetch if Node >= 18, as previous versions do not support it
-    if (NODE_MAJOR < 18) {
-      DEBUG_BUILD && logger.log('NodeFetch is not supported on Node < 18, skipping instrumentation...');
-      return;
-    }
-
-    try {
-      const pkg = await import('opentelemetry-instrumentation-fetch-node');
-      const { FetchInstrumentation } = pkg;
-
-      class SentryNodeFetchInstrumentation extends FetchInstrumentation {
-        // We extend this method so we have access to request _and_ response for the breadcrumb
-        public onHeaders({ request, response }: { request: FetchRequest; response: FetchResponse }): void {
-          if (_breadcrumbs) {
-            _addRequestBreadcrumb(request, response);
-          }
-
-          return super.onHeaders({ request, response });
-        }
-      }
-
-      return new SentryNodeFetchInstrumentation({
-        ignoreRequestHook: (request: FetchRequest) => {
+  return {
+    name: 'NodeFetch',
+    setupOnce() {
+      const instrumentation = new UndiciInstrumentation({
+        requireParentforSpans: false,
+        ignoreRequestHook: request => {
           const url = getAbsoluteUrl(request.origin, request.path);
-          const tracingDisabled = !hasTracingEnabled();
           const shouldIgnore = _ignoreOutgoingRequests && url && _ignoreOutgoingRequests(url);
 
-          if (shouldIgnore) {
-            return true;
-          }
-
-          // If tracing is disabled, we still want to propagate traces
-          // So we do that manually here, matching what the instrumentation does otherwise
-          if (tracingDisabled) {
-            const ctx = context.active();
-            const addedHeaders: Record<string, string> = {};
-
-            // We generate a virtual span context from the active one,
-            // Where we attach the URL to the trace state, so the propagator can pick it up
-            const activeSpan = trace.getSpan(ctx);
-            const propagationContext = activeSpan
-              ? getPropagationContextFromSpan(activeSpan)
-              : getCurrentScope().getPropagationContext();
-
-            const spanContext = generateSpanContextForPropagationContext(propagationContext);
-            // We know that in practice we'll _always_ haven a traceState here
-            spanContext.traceState = spanContext.traceState?.set('sentry.url', url);
-            const ctxWithUrlTraceState = trace.setSpanContext(ctx, spanContext);
-
-            propagation.inject(ctxWithUrlTraceState, addedHeaders);
-
-            const requestHeaders = request.headers;
-            if (Array.isArray(requestHeaders)) {
-              Object.entries(addedHeaders).forEach(headers => requestHeaders.push(...headers));
-            } else {
-              request.headers += Object.entries(addedHeaders)
-                .map(([k, v]) => `${k}: ${v}\r\n`)
-                .join('');
-            }
-
-            // Prevent starting a span for this request
-            return true;
-          }
-
-          return false;
+          return !!shouldIgnore;
         },
-        onRequest: ({ span }: { span: Span }) => {
-          _updateSpan(span);
+        startSpanHook: () => {
+          return {
+            [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.otel.node_fetch',
+          };
+        },
+        responseHook: (_, { request, response }) => {
+          if (_breadcrumbs) {
+            addRequestBreadcrumb(request, response);
+          }
         },
-        // eslint-disable-next-line @typescript-eslint/no-explicit-any
-      } as any);
-    } catch (error) {
-      // Could not load instrumentation
-      DEBUG_BUILD && logger.log('Error while loading NodeFetch instrumentation: \n', error);
-    }
-  }
-
-  return {
-    name: 'NodeFetch',
-    setupOnce() {
-      // eslint-disable-next-line @typescript-eslint/no-floating-promises
-      getInstrumentation().then(instrumentation => {
-        if (instrumentation) {
-          addOpenTelemetryInstrumentation(instrumentation);
-        }
       });
+
+      addOpenTelemetryInstrumentation(instrumentation);
     },
   };
 }) satisfies IntegrationFn;
 
 export const nativeNodeFetchIntegration = defineIntegration(_nativeNodeFetchIntegration);
 
-/** Update the span with data we need. */
-function _updateSpan(span: Span): void {
-  addOriginToSpan(span, 'auto.http.otel.node_fetch');
-}
-
 /** Add a breadcrumb for outgoing requests. */
-function _addRequestBreadcrumb(request: FetchRequest, response: FetchResponse): void {
+function addRequestBreadcrumb(request: UndiciRequest, response: UndiciResponse): void {
   const data = getBreadcrumbData(request);
 
   addBreadcrumb(
@@ -165,7 +74,7 @@ function _addRequestBreadcrumb(request: FetchRequest, response: FetchResponse):
   );
 }
 
-function getBreadcrumbData(request: FetchRequest): Partial<SanitizedRequestData> {
+function getBreadcrumbData(request: UndiciRequest): Partial<SanitizedRequestData> {
   try {
     const url = new URL(request.path, request.origin);
     const parsedUrl = parseUrl(url.toString());
diff --git a/packages/opentelemetry/src/propagator.ts b/packages/opentelemetry/src/propagator.ts
@@ -5,6 +5,7 @@ import { propagation, trace } from '@opentelemetry/api';
 import { W3CBaggagePropagator, isTracingSuppressed } from '@opentelemetry/core';
 import { SEMATTRS_HTTP_URL } from '@opentelemetry/semantic-conventions';
 import type { continueTrace } from '@sentry/core';
+import { SEMANTIC_ATTRIBUTE_URL_FULL } from '@sentry/core';
 import { hasTracingEnabled } from '@sentry/core';
 import { getRootSpan } from '@sentry/core';
 import { spanToJSON } from '@sentry/core';
@@ -292,7 +293,8 @@ function getExistingBaggage(carrier: unknown): string | undefined {
  * 2. Else, if the active span has no URL attribute (e.g. it is unsampled), we check a special trace state (which we set in our sampler).
  */
 function getCurrentURL(span: Span): string | undefined {
-  const urlAttribute = spanToJSON(span).data?.[SEMATTRS_HTTP_URL];
+  const spanData = spanToJSON(span).data;
+  const urlAttribute = spanData?.[SEMATTRS_HTTP_URL] || spanData?.[SEMANTIC_ATTRIBUTE_URL_FULL];
   if (urlAttribute) {
     return urlAttribute;
   }
diff --git a/packages/opentelemetry/src/sampler.ts b/packages/opentelemetry/src/sampler.ts
@@ -5,8 +5,10 @@ import { TraceState } from '@opentelemetry/core';
 import type { Sampler, SamplingResult } from '@opentelemetry/sdk-trace-base';
 import { SamplingDecision } from '@opentelemetry/sdk-trace-base';
 import {
+  SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD,
   SEMANTIC_ATTRIBUTE_SENTRY_OP,
   SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE,
+  SEMANTIC_ATTRIBUTE_URL_FULL,
   hasTracingEnabled,
   sampleSpan,
 } from '@sentry/core';
@@ -54,7 +56,7 @@ export class SentrySampler implements Sampler {
     // but we want to leave downstream sampling decisions up to the server
     if (
       spanKind === SpanKind.CLIENT &&
-      spanAttributes[SEMATTRS_HTTP_METHOD] &&
+      (spanAttributes[SEMATTRS_HTTP_METHOD] || spanAttributes[SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD]) &&
       (!parentSpan || parentContext?.isRemote)
     ) {
       return wrapSamplingDecision({ decision: undefined, context, spanAttributes });
@@ -196,7 +198,7 @@ function getBaseTraceState(context: Context, spanAttributes: SpanAttributes): Tr
   let traceState = parentContext?.traceState || new TraceState();
 
   // We always keep the URL on the trace state, so we can access it in the propagator
-  const url = spanAttributes[SEMATTRS_HTTP_URL];
+  const url = spanAttributes[SEMATTRS_HTTP_URL] || spanAttributes[SEMANTIC_ATTRIBUTE_URL_FULL];
   if (url && typeof url === 'string') {
     traceState = traceState.set(SENTRY_TRACE_STATE_URL, url);
   }
diff --git a/packages/opentelemetry/src/utils/isSentryRequest.ts b/packages/opentelemetry/src/utils/isSentryRequest.ts
@@ -1,5 +1,5 @@
 import { SEMATTRS_HTTP_URL } from '@opentelemetry/semantic-conventions';
-import { getClient, isSentryRequestUrl } from '@sentry/core';
+import { SEMANTIC_ATTRIBUTE_URL_FULL, getClient, isSentryRequestUrl } from '@sentry/core';
 
 import type { AbstractSpan } from '../types';
 import { spanHasAttributes } from './spanTypes';
@@ -16,7 +16,7 @@ export function isSentryRequestSpan(span: AbstractSpan): boolean {
 
   const { attributes } = span;
 
-  const httpUrl = attributes[SEMATTRS_HTTP_URL];
+  const httpUrl = attributes[SEMATTRS_HTTP_URL] || attributes[SEMANTIC_ATTRIBUTE_URL_FULL];
 
   if (!httpUrl) {
     return false;
diff --git a/packages/opentelemetry/src/utils/parseSpanDescription.ts b/packages/opentelemetry/src/utils/parseSpanDescription.ts
@@ -14,7 +14,12 @@ import {
 import type { SpanAttributes, TransactionSource } from '@sentry/types';
 import { getSanitizedUrlString, parseUrl, stripUrlQueryAndFragment } from '@sentry/utils';
 
-import { SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from '@sentry/core';
+import {
+  SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD,
+  SEMANTIC_ATTRIBUTE_SENTRY_OP,
+  SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
+  SEMANTIC_ATTRIBUTE_URL_FULL,
+} from '@sentry/core';
 import { SEMANTIC_ATTRIBUTE_SENTRY_GRAPHQL_OPERATION } from '../semanticAttributes';
 import type { AbstractSpan } from '../types';
 import { getSpanKind } from './getSpanKind';
@@ -45,10 +50,7 @@ export function inferSpanData(name: string, attributes: SpanAttributes, kind: Sp
   }
 
   // if http.method exists, this is an http request span
-  //
-  // TODO: Referencing `http.request.method` is a temporary workaround until the semantic
-  // conventions export an attribute key for it.
-  const httpMethod = attributes['http.request.method'] || attributes[SEMATTRS_HTTP_METHOD];
+  const httpMethod = attributes[SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD] || attributes[SEMATTRS_HTTP_METHOD];
   if (httpMethod) {
     return descriptionForHttpMethod({ attributes, name, kind }, httpMethod);
   }
@@ -213,7 +215,7 @@ export function getSanitizedUrl(
   // This is the relative path of the URL, e.g. /sub
   const httpTarget = attributes[SEMATTRS_HTTP_TARGET];
   // This is the full URL, including host & query params etc., e.g. https://example.com/sub?foo=bar
-  const httpUrl = attributes[SEMATTRS_HTTP_URL];
+  const httpUrl = attributes[SEMATTRS_HTTP_URL] || attributes[SEMANTIC_ATTRIBUTE_URL_FULL];
   // This is the normalized route name - may not always be available!
   const httpRoute = attributes[SEMATTRS_HTTP_ROUTE];
 
diff --git a/yarn.lock b/yarn.lock
