feat(node): Add tracing without performance to Node http integration

getsentry · Jul 10, 2023 · 2950ca6 · 2950ca6
1 parent 86ffdf4
commit 2950ca6
Show file tree

Hide file tree

Showing 15 changed files with 225 additions and 108 deletions.
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
@@ -661,7 +661,7 @@ jobs:
     needs: [job_get_metadata, job_build]
     if: needs.job_get_metadata.outputs.changed_node == 'true' || github.event_name != 'pull_request'
     runs-on: ubuntu-20.04
-    timeout-minutes: 10
+    timeout-minutes: 15
     strategy:
       fail-fast: false
       matrix:

diff --git a/packages/nextjs/test/integration/sentry.client.config.js b/packages/nextjs/test/integration/sentry.client.config.js
@@ -3,7 +3,7 @@ import { Integrations } from '@sentry/tracing';
 
 Sentry.init({
   dsn: 'https://public@dsn.ingest.sentry.io/1337',
-  tracesSampleRate: 1,
+  tracesSampler: () => true,
   debug: process.env.SDK_DEBUG,
 
   integrations: [

diff --git a/packages/nextjs/test/integration/sentry.edge.config.js b/packages/nextjs/test/integration/sentry.edge.config.js
@@ -2,6 +2,7 @@ import * as Sentry from '@sentry/nextjs';
 
 Sentry.init({
   dsn: 'https://public@dsn.ingest.sentry.io/1337',
-  tracesSampleRate: 1,
+  tracesSampleRate: 1.0,
+  tracePropagationTargets: ['http://example.com'],
   debug: process.env.SDK_DEBUG,
 });
diff --git a/packages/nextjs/test/integration/sentry.server.config.js b/packages/nextjs/test/integration/sentry.server.config.js
@@ -2,7 +2,8 @@ import * as Sentry from '@sentry/nextjs';
 
 Sentry.init({
   dsn: 'https://public@dsn.ingest.sentry.io/1337',
-  tracesSampleRate: 1,
+  tracesSampleRate: 1.0,
+  tracePropagationTargets: ['http://example.com'],
   debug: process.env.SDK_DEBUG,
 
   integrations: defaults => [

diff --git a/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out/server.ts b/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out/server.ts
@@ -12,6 +12,7 @@ Sentry.init({
   dsn: 'https://public@dsn.ingest.sentry.io/1337',
   release: '1.0',
   environment: 'prod',
+  tracePropagationTargets: [/^(?!.*express).*$/],
   // eslint-disable-next-line deprecation/deprecation
   integrations: [new Sentry.Integrations.Http({ tracing: true }), new Tracing.Integrations.Express({ app })],
   tracesSampleRate: 1.0,

diff --git a/...ion-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/server.ts b/...ion-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/server.ts
@@ -12,6 +12,8 @@ Sentry.init({
   dsn: 'https://public@dsn.ingest.sentry.io/1337',
   release: '1.0',
   environment: 'prod',
+  // disable requests to /express
+  tracePropagationTargets: [/^(?!.*express).*$/],
   // eslint-disable-next-line deprecation/deprecation
   integrations: [new Sentry.Integrations.Http({ tracing: true }), new Tracing.Integrations.Express({ app })],
   tracesSampleRate: 1.0,

diff --git a/packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors/server.ts b/packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors/server.ts
@@ -12,6 +12,8 @@ Sentry.init({
   dsn: 'https://public@dsn.ingest.sentry.io/1337',
   release: '1.0',
   environment: 'prod',
+  // disable requests to /express
+  tracePropagationTargets: [/^(?!.*express).*$/],
   // eslint-disable-next-line deprecation/deprecation
   integrations: [new Sentry.Integrations.Http({ tracing: true }), new Tracing.Integrations.Express({ app })],
   tracesSampleRate: 1.0,

diff --git a/...ges/node-integration-tests/suites/express/sentry-trace/baggage-transaction-name/server.ts b/...ges/node-integration-tests/suites/express/sentry-trace/baggage-transaction-name/server.ts
@@ -12,6 +12,8 @@ Sentry.init({
   dsn: 'https://public@dsn.ingest.sentry.io/1337',
   release: '1.0',
   environment: 'prod',
+  // disable requests to /express
+  tracePropagationTargets: [/^(?!.*express).*$/],
   // eslint-disable-next-line deprecation/deprecation
   integrations: [new Sentry.Integrations.Http({ tracing: true }), new Tracing.Integrations.Express({ app })],
   tracesSampleRate: 1.0,

diff --git a/packages/node-integration-tests/suites/express/sentry-trace/server.ts b/packages/node-integration-tests/suites/express/sentry-trace/server.ts
@@ -12,6 +12,7 @@ Sentry.init({
   dsn: 'https://public@dsn.ingest.sentry.io/1337',
   release: '1.0',
   environment: 'prod',
+  tracePropagationTargets: [/^(?!.*express).*$/],
   // eslint-disable-next-line deprecation/deprecation
   integrations: [new Sentry.Integrations.Http({ tracing: true }), new Tracing.Integrations.Express({ app })],
   tracesSampleRate: 1.0,

diff --git a/packages/node-integration-tests/suites/express/tracing/server.ts b/packages/node-integration-tests/suites/express/tracing/server.ts
@@ -7,6 +7,8 @@ const app = express();
 Sentry.init({
   dsn: 'https://public@dsn.ingest.sentry.io/1337',
   release: '1.0',
+  // disable attaching headers to /test/* endpoints
+  tracePropagationTargets: [/^(?!.*test).*$/],
   integrations: [new Sentry.Integrations.Http({ tracing: true }), new Sentry.Integrations.Express({ app })],
   tracesSampleRate: 1.0,
 });

diff --git a/packages/node-integration-tests/utils/index.ts b/packages/node-integration-tests/utils/index.ts
@@ -202,11 +202,11 @@ export class TestEnv {
    */
   public async getAPIResponse(
     url?: string,
-    headers?: Record<string, string>,
+    headers: Record<string, string> = {},
     endServer: boolean = true,
   ): Promise<unknown> {
     try {
-      const { data } = await axios.get(url || this.url, { headers: headers || {} });
+      const { data } = await axios.get(url || this.url, { headers });
       return data;
     } finally {
       await Sentry.flush();

diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts
@@ -1,14 +1,26 @@
 import type { Hub } from '@sentry/core';
-import { getCurrentHub } from '@sentry/core';
-import type { EventProcessor, Integration, SanitizedRequestData, Span, TracePropagationTargets } from '@sentry/types';
-import { dynamicSamplingContextToSentryBaggageHeader, fill, logger, stringMatchesSomePattern } from '@sentry/utils';
+import { getCurrentHub, getDynamicSamplingContextFromClient } from '@sentry/core';
+import type {
+  DynamicSamplingContext,
+  EventProcessor,
+  Integration,
+  SanitizedRequestData,
+  TracePropagationTargets,
+} from '@sentry/types';
+import {
+  dynamicSamplingContextToSentryBaggageHeader,
+  fill,
+  generateSentryTraceHeader,
+  logger,
+  stringMatchesSomePattern,
+} from '@sentry/utils';
 import type * as http from 'http';
 import type * as https from 'https';
 import { LRUMap } from 'lru_map';
 
 import type { NodeClient } from '../client';
 import { NODE_VERSION } from '../nodeVersion';
-import type { RequestMethod, RequestMethodArgs } from './utils/http';
+import type { RequestMethod, RequestMethodArgs, RequestOptions } from './utils/http';
 import { cleanSpanDescription, extractRawUrl, extractUrl, isSentryRequest, normalizeRequestArgs } from './utils/http';
 
 interface TracingOptions {
@@ -178,6 +190,36 @@ function _createWrappedRequestMethodFactory(
     return decision;
   };
 
+  /**
+   * Captures Breadcrumb based on provided request/response pair
+   */
+  function addRequestBreadcrumb(
+    event: string,
+    requestSpanData: SanitizedRequestData,
+    req: http.ClientRequest,
+    res?: http.IncomingMessage,
+  ): void {
+    if (!getCurrentHub().getIntegration(Http)) {
+      return;
+    }
+
+    getCurrentHub().addBreadcrumb(
+      {
+        category: 'http',
+        data: {
+          status_code: res && res.statusCode,
+          ...requestSpanData,
+        },
+        type: 'http',
+      },
+      {
+        event,
+        request: req,
+        response: res,
+      },
+    );
+  }
+
   return function wrappedRequestMethodFactory(originalRequestMethod: OriginalRequestMethod): WrappedRequestMethod {
     return function wrappedMethod(this: unknown, ...args: RequestMethodArgs): http.ClientRequest {
       const requestArgs = normalizeRequestArgs(httpModule, args);
@@ -191,74 +233,38 @@ function _createWrappedRequestMethodFactory(
         return originalRequestMethod.apply(httpModule, requestArgs);
       }
 
-      let requestSpan: Span | undefined;
-      const parentSpan = getCurrentHub().getScope().getSpan();
-
-      const method = requestOptions.method || 'GET';
-      const requestSpanData: SanitizedRequestData = {
-        url: requestUrl,
-        'http.method': method,
-      };
-      if (requestOptions.hash) {
-        // strip leading "#"
-        requestSpanData['http.fragment'] = requestOptions.hash.substring(1);
-      }
-      if (requestOptions.search) {
-        // strip leading "?"
-        requestSpanData['http.query'] = requestOptions.search.substring(1);
-      }
+      const hub = getCurrentHub();
+      const scope = hub.getScope();
+      const parentSpan = scope.getSpan();
+
+      const data = getRequestSpanData(requestUrl, requestOptions);
 
-      if (tracingOptions && shouldCreateSpan(rawRequestUrl)) {
-        if (parentSpan) {
-          requestSpan = parentSpan.startChild({
-            description: `${method} ${requestSpanData.url}`,
+      const requestSpan = shouldCreateSpan(rawRequestUrl)
+        ? parentSpan?.startChild({
             op: 'http.client',
-            data: requestSpanData,
-          });
-
-          if (shouldAttachTraceData(rawRequestUrl)) {
-            const sentryTraceHeader = requestSpan.toTraceparent();
-            __DEBUG_BUILD__ &&
-              logger.log(
-                `[Tracing] Adding sentry-trace header ${sentryTraceHeader} to outgoing request to "${requestUrl}": `,
-              );
-
-            requestOptions.headers = {
-              ...requestOptions.headers,
-              'sentry-trace': sentryTraceHeader,
-            };
-
-            if (parentSpan.transaction) {
-              const dynamicSamplingContext = parentSpan.transaction.getDynamicSamplingContext();
-              const sentryBaggageHeader = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext);
-
-              let newBaggageHeaderField;
-              if (!requestOptions.headers || !requestOptions.headers.baggage) {
-                newBaggageHeaderField = sentryBaggageHeader;
-              } else if (!sentryBaggageHeader) {
-                newBaggageHeaderField = requestOptions.headers.baggage;
-              } else if (Array.isArray(requestOptions.headers.baggage)) {
-                newBaggageHeaderField = [...requestOptions.headers.baggage, sentryBaggageHeader];
-              } else {
-                // Type-cast explanation:
-                // Technically this the following could be of type `(number | string)[]` but for the sake of simplicity
-                // we say this is undefined behaviour, since it would not be baggage spec conform if the user did this.
-                newBaggageHeaderField = [requestOptions.headers.baggage, sentryBaggageHeader] as string[];
-              }
-
-              requestOptions.headers = {
-                ...requestOptions.headers,
-                // Setting a hader to `undefined` will crash in node so we only set the baggage header when it's defined
-                ...(newBaggageHeaderField && { baggage: newBaggageHeaderField }),
-              };
-            }
-          } else {
-            __DEBUG_BUILD__ &&
-              logger.log(
-                `[Tracing] Not adding sentry-trace header to outgoing request (${requestUrl}) due to mismatching tracePropagationTargets option.`,
-              );
-          }
+            description: `${data['http.method']} ${data.url}`,
+            data,
+          })
+        : undefined;
+
+      if (shouldAttachTraceData(rawRequestUrl)) {
+        if (requestSpan) {
+          const sentryTraceHeader = requestSpan.toTraceparent();
+          const dynamicSamplingContext = requestSpan?.transaction?.getDynamicSamplingContext();
+          addHeadersToRequestOptions(requestOptions, requestUrl, sentryTraceHeader, dynamicSamplingContext);
+        } else {
+          const client = hub.getClient();
+          const { traceId, sampled, dsc } = scope.getPropagationContext();
+          const sentryTraceHeader = generateSentryTraceHeader(traceId, undefined, sampled);
+          const dynamicSamplingContext =
+            dsc || (client ? getDynamicSamplingContextFromClient(traceId, client, scope) : undefined);
+          addHeadersToRequestOptions(requestOptions, requestUrl, sentryTraceHeader, dynamicSamplingContext);
         }
+      } else {
+        __DEBUG_BUILD__ &&
+          logger.log(
+            `[Tracing] Not adding sentry-trace header to outgoing request (${requestUrl}) due to mismatching tracePropagationTargets option.`,
+          );
       }
 
       // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
@@ -268,7 +274,7 @@ function _createWrappedRequestMethodFactory(
           // eslint-disable-next-line @typescript-eslint/no-this-alias
           const req = this;
           if (breadcrumbsEnabled) {
-            addRequestBreadcrumb('response', requestSpanData, req, res);
+            addRequestBreadcrumb('response', data, req, res);
           }
           if (requestSpan) {
             if (res.statusCode) {
@@ -283,7 +289,7 @@ function _createWrappedRequestMethodFactory(
           const req = this;
 
           if (breadcrumbsEnabled) {
-            addRequestBreadcrumb('error', requestSpanData, req);
+            addRequestBreadcrumb('error', data, req);
           }
           if (requestSpan) {
             requestSpan.setHttpStatus(500);
@@ -295,32 +301,55 @@ function _createWrappedRequestMethodFactory(
   };
 }
 
-/**
- * Captures Breadcrumb based on provided request/response pair
- */
-function addRequestBreadcrumb(
-  event: string,
-  requestSpanData: SanitizedRequestData,
-  req: http.ClientRequest,
-  res?: http.IncomingMessage,
+function addHeadersToRequestOptions(
+  requestOptions: RequestOptions,
+  requestUrl: string,
+  sentryTraceHeader: string,
+  dynamicSamplingContext: Partial<DynamicSamplingContext> | undefined,
 ): void {
-  if (!getCurrentHub().getIntegration(Http)) {
-    return;
+  __DEBUG_BUILD__ &&
+    logger.log(`[Tracing] Adding sentry-trace header ${sentryTraceHeader} to outgoing request to "${requestUrl}": `);
+  const sentryBaggage = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext);
+  const sentryBaggageHeader = normalizeBaggageHeader(requestOptions, sentryBaggage);
+  requestOptions.headers = {
+    ...requestOptions.headers,
+    'sentry-trace': sentryTraceHeader,
+    // Setting a header to `undefined` will crash in node so we only set the baggage header when it's defined
+    ...(sentryBaggageHeader && { baggage: sentryBaggageHeader }),
+  };
+}
+
+function getRequestSpanData(requestUrl: string, requestOptions: RequestOptions): SanitizedRequestData {
+  const method = requestOptions.method || 'GET';
+  const data: SanitizedRequestData = {
+    url: requestUrl,
+    'http.method': method,
+  };
+  if (requestOptions.hash) {
+    // strip leading "#"
+    data['http.fragment'] = requestOptions.hash.substring(1);
+  }
+  if (requestOptions.search) {
+    // strip leading "?"
+    data['http.query'] = requestOptions.search.substring(1);
+  }
+  return data;
+}
+
+function normalizeBaggageHeader(
+  requestOptions: RequestOptions,
+  sentryBaggageHeader: string | undefined,
+): string | string[] | undefined {
+  if (!requestOptions.headers || !requestOptions.headers.baggage) {
+    return sentryBaggageHeader;
+  } else if (!sentryBaggageHeader) {
+    return requestOptions.headers.baggage as string | string[];
+  } else if (Array.isArray(requestOptions.headers.baggage)) {
+    return [...requestOptions.headers.baggage, sentryBaggageHeader];
   }
 
-  getCurrentHub().addBreadcrumb(
-    {
-      category: 'http',
-      data: {
-        status_code: res && res.statusCode,
-        ...requestSpanData,
-      },
-      type: 'http',
-    },
-    {
-      event,
-      request: req,
-      response: res,
-    },
-  );
+  // Type-cast explanation:
+  // Technically this the following could be of type `(number | string)[]` but for the sake of simplicity
+  // we say this is undefined behaviour, since it would not be baggage spec conform if the user did this.
+  return [requestOptions.headers.baggage, sentryBaggageHeader] as string[];
 }