WIP http & node-fetch fixes

Author: mydea

dev-packages/node-integration-tests/suites/tracing-new/httpIntegration/spans/scenario.ts

@@ -0,0 +1,21 @@
+import '@sentry/tracing';
+
+import * as http from 'http';
+import * as Sentry from '@sentry/node';
+
+Sentry.init({
+  dsn: 'https://[email protected]/1337',
+  release: '1.0',
+  tracesSampleRate: 1.0,
+  integrations: [Sentry.httpIntegration({})],
+  debug: true,
+});
+
+// eslint-disable-next-line @typescript-eslint/no-floating-promises
+Sentry.startSpan({ name: 'test_transaction' }, async () => {
+  http.get('http://match-this-url.com/api/v0');
+  http.get('http://match-this-url.com/api/v1');
+
+  // Give it a tick to resolve...
+  await new Promise(resolve => setTimeout(resolve, 100));
+});

dev-packages/node-integration-tests/suites/tracing-new/httpIntegration/spans/test.ts

@@ -0,0 +1,40 @@
+import nock from 'nock';
+
+import { TestEnv, assertSentryTransaction } from '../../../../utils';
+
+test('should capture spans for outgoing http requests', async () => {
+  const match1 = nock('http://match-this-url.com').get('/api/v0').reply(200);
+  const match2 = nock('http://match-this-url.com').get('/api/v1').reply(200);
+
+  const env = await TestEnv.init(__dirname);
+  const envelope = await env.getEnvelopeRequest({ envelopeType: 'transaction' });
+
+  expect(match1.isDone()).toBe(true);
+  expect(match2.isDone()).toBe(true);
+
+  expect(envelope).toHaveLength(3);
+
+  assertSentryTransaction(envelope[2], {
+    transaction: 'test_transaction',
+    spans: [
+      {
+        description: 'GET http://match-this-url.com/api/v0',
+        op: 'http.client',
+        origin: 'auto.http.node.http',
+        status: 'ok',
+        tags: {
+          'http.status_code': '200',
+        },
+      },
+      {
+        description: 'GET http://match-this-url.com/api/v1',
+        op: 'http.client',
+        origin: 'auto.http.node.http',
+        status: 'ok',
+        tags: {
+          'http.status_code': '200',
+        },
+      },
+    ],
+  });
+});

dev-packages/node-integration-tests/suites/tracing-new/httpIntegration/spansDisabled/scenario.ts

@@ -0,0 +1,20 @@
+import '@sentry/tracing';
+
+import * as http from 'http';
+import * as Sentry from '@sentry/node';
+
+Sentry.init({
+  dsn: 'https://[email protected]/1337',
+  release: '1.0',
+  tracesSampleRate: 1.0,
+  integrations: [Sentry.httpIntegration({ tracing: false })],
+});
+
+// eslint-disable-next-line @typescript-eslint/no-floating-promises
+Sentry.startSpan({ name: 'test_transaction' }, async () => {
+  http.get('http://match-this-url.com/api/v0');
+  http.get('http://match-this-url.com/api/v1');
+
+  // Give it a tick to resolve...
+  await new Promise(resolve => setTimeout(resolve, 100));
+});

dev-packages/node-integration-tests/suites/tracing-new/httpIntegration/spansDisabled/test.ts

@@ -0,0 +1,21 @@
+import nock from 'nock';
+
+import { TestEnv, assertSentryTransaction } from '../../../../utils';
+
+test('should not capture spans for outgoing http requests if tracing is disabled', async () => {
+  const match1 = nock('http://match-this-url.com').get('/api/v0').reply(200);
+  const match2 = nock('http://match-this-url.com').get('/api/v1').reply(200);
+
+  const env = await TestEnv.init(__dirname);
+  const envelope = await env.getEnvelopeRequest({ envelopeType: 'transaction' });
+
+  expect(match1.isDone()).toBe(true);
+  expect(match2.isDone()).toBe(true);
+
+  expect(envelope).toHaveLength(3);
+
+  assertSentryTransaction(envelope[2], {
+    transaction: 'test_transaction',
+    spans: [],
+  });
+});

dev-packages/node-integration-tests/suites/tracing-new/httpIntegration/tracePropagationTargets/scenario.ts

@@ -0,0 +1,20 @@
+// eslint-disable-next-line @typescript-eslint/no-unused-vars
+import '@sentry/tracing';
+
+import * as http from 'http';
+import * as Sentry from '@sentry/node';
+
+Sentry.init({
+  dsn: 'https://[email protected]/1337',
+  release: '1.0',
+  tracesSampleRate: 1.0,
+  tracePropagationTargets: [/\/v0/, 'v1'],
+  integrations: [Sentry.httpIntegration({})],
+});
+
+Sentry.startSpan({ name: 'test_transaction' }, () => {
+  http.get('http://match-this-url.com/api/v0');
+  http.get('http://match-this-url.com/api/v1');
+  http.get('http://dont-match-this-url.com/api/v2');
+  http.get('http://dont-match-this-url.com/api/v3');
+});

dev-packages/node-integration-tests/suites/tracing-new/httpIntegration/tracePropagationTargets/test.ts

@@ -0,0 +1,42 @@
+import nock from 'nock';
+
+import { TestEnv, runScenario } from '../../../../utils';
+
+test('httpIntegration should instrument correct requests when tracePropagationTargets option is provided & tracing is enabled', async () => {
+  const match1 = nock('http://match-this-url.com')
+    .get('/api/v0')
+    .matchHeader('baggage', val => typeof val === 'string')
+    .matchHeader('sentry-trace', val => typeof val === 'string')
+    .reply(200);
+
+  const match2 = nock('http://match-this-url.com')
+    .get('/api/v1')
+    .matchHeader('baggage', val => typeof val === 'string')
+    .matchHeader('sentry-trace', val => typeof val === 'string')
+    .reply(200);
+
+  const match3 = nock('http://dont-match-this-url.com')
+    .get('/api/v2')
+    .matchHeader('baggage', val => val === undefined)
+    .matchHeader('sentry-trace', val => val === undefined)
+    .reply(200);
+
+  const match4 = nock('http://dont-match-this-url.com')
+    .get('/api/v3')
+    .matchHeader('baggage', val => val === undefined)
+    .matchHeader('sentry-trace', val => val === undefined)
+    .reply(200);
+
+  const env = await TestEnv.init(__dirname);
+  await runScenario(env.url);
+
+  env.server.close();
+  nock.cleanAll();
+
+  await new Promise(resolve => env.server.close(resolve));
+
+  expect(match1.isDone()).toBe(true);
+  expect(match2.isDone()).toBe(true);
+  expect(match3.isDone()).toBe(true);
+  expect(match4.isDone()).toBe(true);
+});

dev-packages/node-integration-tests/suites/tracing-new/httpIntegration/tracePropagationTargetsDisabled/scenario.ts

@@ -0,0 +1,19 @@
+// eslint-disable-next-line @typescript-eslint/no-unused-vars
+import '@sentry/tracing';
+
+import * as http from 'http';
+import * as Sentry from '@sentry/node';
+
+Sentry.init({
+  dsn: 'https://[email protected]/1337',
+  release: '1.0',
+  tracePropagationTargets: [/\/v0/, 'v1'],
+  integrations: [Sentry.httpIntegration({})],
+});
+
+Sentry.startSpan({ name: 'test_transaction' }, () => {
+  http.get('http://match-this-url.com/api/v0');
+  http.get('http://match-this-url.com/api/v1');
+  http.get('http://dont-match-this-url.com/api/v2');
+  http.get('http://dont-match-this-url.com/api/v3');
+});

dev-packages/node-integration-tests/suites/tracing-new/httpIntegration/tracePropagationTargetsDisabled/test.ts

@@ -0,0 +1,42 @@
+import nock from 'nock';
+
+import { TestEnv, runScenario } from '../../../../utils';
+
+test('httpIntegration should not instrument when tracing is enabled', async () => {
+  const match1 = nock('http://match-this-url.com')
+    .get('/api/v0')
+    .matchHeader('baggage', val => val === undefined)
+    .matchHeader('sentry-trace', val => val === undefined)
+    .reply(200);
+
+  const match2 = nock('http://match-this-url.com')
+    .get('/api/v1')
+    .matchHeader('baggage', val => val === undefined)
+    .matchHeader('sentry-trace', val => val === undefined)
+    .reply(200);
+
+  const match3 = nock('http://dont-match-this-url.com')
+    .get('/api/v2')
+    .matchHeader('baggage', val => val === undefined)
+    .matchHeader('sentry-trace', val => val === undefined)
+    .reply(200);
+
+  const match4 = nock('http://dont-match-this-url.com')
+    .get('/api/v3')
+    .matchHeader('baggage', val => val === undefined)
+    .matchHeader('sentry-trace', val => val === undefined)
+    .reply(200);
+
+  const env = await TestEnv.init(__dirname);
+  await runScenario(env.url);
+
+  env.server.close();
+  nock.cleanAll();
+
+  await new Promise(resolve => env.server.close(resolve));
+
+  expect(match1.isDone()).toBe(true);
+  expect(match2.isDone()).toBe(true);
+  expect(match3.isDone()).toBe(true);
+  expect(match4.isDone()).toBe(true);
+});

packages/node/src/integrations/http.ts

@@ -1,7 +1,8 @@
+/* eslint-disable max-lines */
 import type * as http from 'http';
 import type * as https from 'https';
 import type { Hub } from '@sentry/core';
-import { getIsolationScope, defineIntegration } from '@sentry/core';
+import { defineIntegration, getIsolationScope, hasTracingEnabled } from '@sentry/core';
 import {
   addBreadcrumb,
   getActiveSpan,
@@ -16,7 +17,7 @@ import {
   spanToTraceHeader,
 } from '@sentry/core';
 import type {
-  DynamicSamplingContext,
+  ClientOptions,
   EventProcessor,
   Integration,
   IntegrationFn,
@@ -26,6 +27,7 @@ import type {
 } from '@sentry/types';
 import {
   LRUMap,
+  dropUndefinedKeys,
   dynamicSamplingContextToSentryBaggageHeader,
   fill,
   generateSentryTraceHeader,
@@ -36,6 +38,7 @@ import {
 import type { NodeClient } from '../client';
 import { DEBUG_BUILD } from '../debug-build';
 import { NODE_VERSION } from '../nodeVersion';
+import type { NodeClientOptions } from '../types';
 import type { RequestMethod, RequestMethodArgs, RequestOptions } from './utils/http';
 import { cleanSpanDescription, extractRawUrl, extractUrl, normalizeRequestArgs } from './utils/http';
 
@@ -64,6 +67,12 @@ interface TracingOptions {
    * By default, spans will be created for all outgoing requests.
    */
   shouldCreateSpanForRequest?: (url: string) => boolean;
+
+  /**
+   * This option is just for compatibility with v7.
+   * In v8, this will be the default behavior.
+   */
+  enableIfHasTracingEnabled?: boolean;
 }
 
 interface HttpOptions {
@@ -80,11 +89,52 @@ interface HttpOptions {
   tracing?: TracingOptions | boolean;
 }
 
-const _httpIntegration = ((options?: HttpOptions) => {
+/* These are the newer options for `httpIntegration`. */
+interface HttpIntegrationOptions {
+  /**
+   * Whether breadcrumbs should be recorded for requests
+   * Defaults to true.
+   */
+  breadcrumbs?: boolean;
+
+  /**
+   * Whether tracing spans should be created for requests
+   * If not set, this will be enabled/disabled based on if tracing is enabled.
+   */
+  tracing?: boolean;
+
+  /**
+   * Function determining whether or not to create spans to track outgoing requests to the given URL.
+   * By default, spans will be created for all outgoing requests.
+   */
+  shouldCreateSpanForRequest?: (url: string) => boolean;
+}
+
+const _httpIntegration = ((options: HttpIntegrationOptions = {}) => {
+  const { breadcrumbs, tracing, shouldCreateSpanForRequest } = options;
+
+  const convertedOptions: HttpOptions = {
+    breadcrumbs,
+    tracing:
+      tracing === false
+        ? false
+        : dropUndefinedKeys({
+            // If tracing is forced to `true`, we don't want to set `enableIfHasTracingEnabled`
+            enableIfHasTracingEnabled: tracing === true ? undefined : true,
+            shouldCreateSpanForRequest,
+          }),
+  };
+
   // eslint-disable-next-line deprecation/deprecation
-  return new Http(options) as unknown as IntegrationFnResult;
+  return new Http(convertedOptions) as unknown as IntegrationFnResult;
 }) satisfies IntegrationFn;
 
+/**
+ * The http module integration instruments Node's internal http module. It creates breadcrumbs, spans for outgoing
+ * http requests, and attaches trace data when tracing is enabled via its `tracing` option.
+ *
+ * By default, this will always create breadcrumbs, and will create spans if tracing is enabled.
+ */
 export const httpIntegration = defineIntegration(_httpIntegration);
 
 /**
@@ -123,23 +173,26 @@ export class Http implements Integration {
     _addGlobalEventProcessor: (callback: EventProcessor) => void,
     setupOnceGetCurrentHub: () => Hub,
   ): void {
+    // eslint-disable-next-line deprecation/deprecation
+    const clientOptions = setupOnceGetCurrentHub().getClient<NodeClient>()?.getOptions();
+
+    // If `tracing` is not explicitly set, we default this based on whether or not tracing is enabled.
+    // But for compatibility, we only do that if `enableIfHasTracingEnabled` is set.
+    const shouldCreateSpans = _shouldCreateSpans(this._tracing, clientOptions);
+
     // No need to instrument if we don't want to track anything
-    if (!this._breadcrumbs && !this._tracing) {
+    if (!this._breadcrumbs && !shouldCreateSpans) {
       return;
     }
 
-    // eslint-disable-next-line deprecation/deprecation
-    const clientOptions = setupOnceGetCurrentHub().getClient<NodeClient>()?.getOptions();
-
     // Do not auto-instrument for other instrumenter
     if (clientOptions && clientOptions.instrumenter !== 'sentry') {
       DEBUG_BUILD && logger.log('HTTP Integration is skipped because of instrumenter configuration.');
       return;
     }
 
-    const shouldCreateSpanForRequest =
-      // eslint-disable-next-line deprecation/deprecation
-      this._tracing?.shouldCreateSpanForRequest || clientOptions?.shouldCreateSpanForRequest;
+    const shouldCreateSpanForRequest = _getShouldCreateSpanForRequest(shouldCreateSpans, this._tracing, clientOptions);
+
     // eslint-disable-next-line deprecation/deprecation
     const tracePropagationTargets = clientOptions?.tracePropagationTargets || this._tracing?.tracePropagationTargets;
 
@@ -407,3 +460,29 @@ function normalizeBaggageHeader(
   // 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[];
 }
+
+/** Exported for tests only. */
+export function _shouldCreateSpans(
+  tracingOptions: TracingOptions | undefined,
+  clientOptions: Partial<ClientOptions> | undefined,
+): boolean {
+  return tracingOptions === undefined
+    ? false
+    : tracingOptions.enableIfHasTracingEnabled
+      ? hasTracingEnabled(clientOptions)
+      : true;
+}
+
+/** Exported for tests only. */
+export function _getShouldCreateSpanForRequest(
+  shouldCreateSpans: boolean,
+  tracingOptions: TracingOptions | undefined,
+  clientOptions: Partial<NodeClientOptions> | undefined,
+): undefined | ((url: string) => boolean) {
+  const handler = shouldCreateSpans
+    ? // eslint-disable-next-line deprecation/deprecation
+      tracingOptions?.shouldCreateSpanForRequest || clientOptions?.shouldCreateSpanForRequest
+    : () => false;
+
+  return handler;
+}