From 8b5e39a4c0d1644c394a5d88502055b4bd91beb2 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 10 Feb 2025 09:33:56 +0000 Subject: [PATCH 1/2] Revert "fix(node): Ensure `httpIntegration` propagates traces (#15233)" --- .../http-no-tracing-no-spans/scenario.ts | 61 --------- .../requests/http-no-tracing-no-spans/test.ts | 95 ------------- packages/core/src/utils-hoist/baggage.ts | 2 +- packages/core/src/utils-hoist/index.ts | 1 - .../http/SentryHttpInstrumentation.ts | 128 ++---------------- 5 files changed, 11 insertions(+), 276 deletions(-) delete mode 100644 dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing-no-spans/scenario.ts delete mode 100644 dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing-no-spans/test.ts diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing-no-spans/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing-no-spans/scenario.ts deleted file mode 100644 index 77884dab80c7..000000000000 --- a/dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing-no-spans/scenario.ts +++ /dev/null @@ -1,61 +0,0 @@ -import { loggingTransport } from '@sentry-internal/node-integration-tests'; -import * as Sentry from '@sentry/node'; - -Sentry.init({ - dsn: 'https://public@dsn.ingest.sentry.io/1337', - release: '1.0', - tracePropagationTargets: [/\/v0/, 'v1'], - integrations: [Sentry.httpIntegration({ spans: false })], - transport: loggingTransport, - // Ensure this gets a correct hint - beforeBreadcrumb(breadcrumb, hint) { - breadcrumb.data = breadcrumb.data || {}; - const req = hint?.request as { path?: string }; - breadcrumb.data.ADDED_PATH = req?.path; - return breadcrumb; - }, -}); - -import * as http from 'http'; - -async function run(): Promise { - Sentry.addBreadcrumb({ message: 'manual breadcrumb' }); - - await makeHttpRequest(`${process.env.SERVER_URL}/api/v0`); - await makeHttpGet(`${process.env.SERVER_URL}/api/v1`); - await makeHttpRequest(`${process.env.SERVER_URL}/api/v2`); - await makeHttpRequest(`${process.env.SERVER_URL}/api/v3`); - - Sentry.captureException(new Error('foo')); -} - -// eslint-disable-next-line @typescript-eslint/no-floating-promises -run(); - -function makeHttpRequest(url: string): Promise { - return new Promise(resolve => { - http - .request(url, httpRes => { - httpRes.on('data', () => { - // we don't care about data - }); - httpRes.on('end', () => { - resolve(); - }); - }) - .end(); - }); -} - -function makeHttpGet(url: string): Promise { - return new Promise(resolve => { - http.get(url, httpRes => { - httpRes.on('data', () => { - // we don't care about data - }); - httpRes.on('end', () => { - resolve(); - }); - }); - }); -} diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing-no-spans/test.ts b/dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing-no-spans/test.ts deleted file mode 100644 index b85cc7913c2c..000000000000 --- a/dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing-no-spans/test.ts +++ /dev/null @@ -1,95 +0,0 @@ -import { createRunner } from '../../../../utils/runner'; -import { createTestServer } from '../../../../utils/server'; - -test('outgoing http requests are correctly instrumented with tracing & spans disabled', done => { - expect.assertions(11); - - createTestServer(done) - .get('/api/v0', headers => { - expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})$/)); - expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000'); - expect(headers['baggage']).toEqual(expect.any(String)); - }) - .get('/api/v1', headers => { - expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})$/)); - expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000'); - expect(headers['baggage']).toEqual(expect.any(String)); - }) - .get('/api/v2', headers => { - expect(headers['baggage']).toBeUndefined(); - expect(headers['sentry-trace']).toBeUndefined(); - }) - .get('/api/v3', headers => { - expect(headers['baggage']).toBeUndefined(); - expect(headers['sentry-trace']).toBeUndefined(); - }) - .start() - .then(([SERVER_URL, closeTestServer]) => { - createRunner(__dirname, 'scenario.ts') - .withEnv({ SERVER_URL }) - .ensureNoErrorOutput() - .expect({ - event: { - exception: { - values: [ - { - type: 'Error', - value: 'foo', - }, - ], - }, - breadcrumbs: [ - { - message: 'manual breadcrumb', - timestamp: expect.any(Number), - }, - { - category: 'http', - data: { - 'http.method': 'GET', - url: `${SERVER_URL}/api/v0`, - status_code: 200, - ADDED_PATH: '/api/v0', - }, - timestamp: expect.any(Number), - type: 'http', - }, - { - category: 'http', - data: { - 'http.method': 'GET', - url: `${SERVER_URL}/api/v1`, - status_code: 200, - ADDED_PATH: '/api/v1', - }, - timestamp: expect.any(Number), - type: 'http', - }, - { - category: 'http', - data: { - 'http.method': 'GET', - url: `${SERVER_URL}/api/v2`, - status_code: 200, - ADDED_PATH: '/api/v2', - }, - timestamp: expect.any(Number), - type: 'http', - }, - { - category: 'http', - data: { - 'http.method': 'GET', - url: `${SERVER_URL}/api/v3`, - status_code: 200, - ADDED_PATH: '/api/v3', - }, - timestamp: expect.any(Number), - type: 'http', - }, - ], - }, - }) - .start(closeTestServer); - }); -}); diff --git a/packages/core/src/utils-hoist/baggage.ts b/packages/core/src/utils-hoist/baggage.ts index 84d1078b7583..075dbf4389df 100644 --- a/packages/core/src/utils-hoist/baggage.ts +++ b/packages/core/src/utils-hoist/baggage.ts @@ -130,7 +130,7 @@ function baggageHeaderToObject(baggageHeader: string): Record { * @returns a baggage header string, or `undefined` if the object didn't have any values, since an empty baggage header * is not spec compliant. */ -export function objectToBaggageHeader(object: Record): string | undefined { +function objectToBaggageHeader(object: Record): string | undefined { if (Object.keys(object).length === 0) { // An empty baggage header is not spec compliant: We return undefined. return undefined; diff --git a/packages/core/src/utils-hoist/index.ts b/packages/core/src/utils-hoist/index.ts index 1b2032dbc4bc..a593b72e73ad 100644 --- a/packages/core/src/utils-hoist/index.ts +++ b/packages/core/src/utils-hoist/index.ts @@ -128,7 +128,6 @@ export { baggageHeaderToDynamicSamplingContext, dynamicSamplingContextToSentryBaggageHeader, parseBaggageHeader, - objectToBaggageHeader, } from './baggage'; export { getSanitizedUrlString, parseUrl, stripUrlQueryAndFragment } from './url'; diff --git a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts index 6d777a3822b2..ed76ac332567 100644 --- a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts +++ b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts @@ -1,31 +1,26 @@ +/* eslint-disable max-lines */ import type * as http from 'node:http'; import type { IncomingMessage, RequestOptions } from 'node:http'; import type * as https from 'node:https'; import type { EventEmitter } from 'node:stream'; -/* eslint-disable max-lines */ import { VERSION } from '@opentelemetry/core'; import type { InstrumentationConfig } from '@opentelemetry/instrumentation'; import { InstrumentationBase, InstrumentationNodeModuleDefinition } from '@opentelemetry/instrumentation'; import type { AggregationCounts, Client, RequestEventData, SanitizedRequestData, Scope } from '@sentry/core'; import { - LRUMap, addBreadcrumb, generateSpanId, getBreadcrumbLogLevelFromHttpStatusCode, getClient, getIsolationScope, getSanitizedUrlString, - getTraceData, httpRequestToRequestData, logger, - objectToBaggageHeader, - parseBaggageHeader, parseUrl, stripUrlQueryAndFragment, withIsolationScope, withScope, } from '@sentry/core'; -import { shouldPropagateTraceForUrl } from '@sentry/opentelemetry'; import { DEBUG_BUILD } from '../../debug-build'; import { getRequestUrl } from '../../utils/getRequestUrl'; import { getRequestInfo } from './vendor/getRequestInfo'; @@ -33,12 +28,6 @@ import { getRequestInfo } from './vendor/getRequestInfo'; type Http = typeof http; type Https = typeof https; -type RequestArgs = - // eslint-disable-next-line @typescript-eslint/ban-types - | [url: string | URL, options?: RequestOptions, callback?: Function] - // eslint-disable-next-line @typescript-eslint/ban-types - | [options: RequestOptions, callback?: Function]; - type SentryHttpInstrumentationOptions = InstrumentationConfig & { /** * Whether breadcrumbs should be recorded for requests. @@ -91,11 +80,8 @@ const MAX_BODY_BYTE_LENGTH = 1024 * 1024; * https://github.com/open-telemetry/opentelemetry-js/blob/f8ab5592ddea5cba0a3b33bf8d74f27872c0367f/experimental/packages/opentelemetry-instrumentation-http/src/http.ts */ export class SentryHttpInstrumentation extends InstrumentationBase { - private _propagationDecisionMap: LRUMap; - public constructor(config: SentryHttpInstrumentationOptions = {}) { super('@sentry/instrumentation-http', VERSION, config); - this._propagationDecisionMap = new LRUMap(100); } /** @inheritdoc */ @@ -222,32 +208,22 @@ export class SentryHttpInstrumentation extends InstrumentationBase; + const request = original.apply(this, args) as ReturnType; request.prependListener('response', (response: http.IncomingMessage) => { const _breadcrumbs = instrumentation.getConfig().breadcrumbs; @@ -481,44 +457,6 @@ function patchRequestToCaptureBody(req: IncomingMessage, isolationScope: Scope): } } -/** - * Mutates the passed in `options` and adds `sentry-trace` / `baggage` headers, if they are not already set. - */ -function addSentryHeadersToRequestOptions( - url: string, - options: RequestOptions, - propagationDecisionMap: LRUMap, -): void { - // Manually add the trace headers, if it applies - // Note: We do not use `propagation.inject()` here, because our propagator relies on an active span - // Which we do not have in this case - const tracePropagationTargets = getClient()?.getOptions().tracePropagationTargets; - const addedHeaders = shouldPropagateTraceForUrl(url, tracePropagationTargets, propagationDecisionMap) - ? getTraceData() - : undefined; - - if (!addedHeaders) { - return; - } - - if (!options.headers) { - options.headers = {}; - } - const headers = options.headers; - - const { 'sentry-trace': sentryTrace, baggage } = addedHeaders; - - // We do not want to overwrite existing header here, if it was already set - if (sentryTrace && !headers['sentry-trace']) { - headers['sentry-trace'] = sentryTrace; - } - - // For baggage, we make sure to merge this into a possibly existing header - if (baggage) { - headers['baggage'] = mergeBaggageHeaders(headers['baggage'], baggage); - } -} - /** * Starts a session and tracks it in the context of a given isolation scope. * When the passed response is finished, the session is put into a task and is @@ -593,49 +531,3 @@ const clientToRequestSessionAggregatesMap = new Map< Client, { [timestampRoundedToSeconds: string]: { exited: number; crashed: number; errored: number } } >(); - -function getAbsoluteUrl(origin: string, path: string = '/'): string { - try { - const url = new URL(path, origin); - return url.toString(); - } catch { - // fallback: Construct it on our own - const url = `${origin}`; - - if (url.endsWith('/') && path.startsWith('/')) { - return `${url}${path.slice(1)}`; - } - - if (!url.endsWith('/') && !path.startsWith('/')) { - return `${url}/${path.slice(1)}`; - } - - return `${url}${path}`; - } -} - -function mergeBaggageHeaders( - existing: string | string[] | number | undefined, - baggage: string, -): string | string[] | number | undefined { - if (!existing) { - return baggage; - } - - const existingBaggageEntries = parseBaggageHeader(existing); - const newBaggageEntries = parseBaggageHeader(baggage); - - if (!newBaggageEntries) { - return existing; - } - - // Existing entries take precedence, ensuring order remains stable for minimal changes - const mergedBaggageEntries = { ...existingBaggageEntries }; - Object.entries(newBaggageEntries).forEach(([key, value]) => { - if (!mergedBaggageEntries[key]) { - mergedBaggageEntries[key] = value; - } - }); - - return objectToBaggageHeader(mergedBaggageEntries); -} From a5f626f5dacb3f2a3d998d5df3b74525d858fdbe Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 10 Feb 2025 09:44:01 +0000 Subject: [PATCH 2/2] lint --- .../node/src/integrations/http/SentryHttpInstrumentation.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts index ed76ac332567..d645ac5c9ec2 100644 --- a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts +++ b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts @@ -99,7 +99,7 @@ export class SentryHttpInstrumentation extends InstrumentationBase http.ClientRequest, ) => (options: URL | http.RequestOptions | string, ...args: unknown[]) => http.ClientRequest {