From 4e2d0175690d9fd1a8ad21d085752a525967fe64 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 21 May 2024 13:26:05 +0200 Subject: [PATCH 1/2] fix(node): Add request to hint for native node fetch breadcrumb --- packages/node/src/integrations/node-fetch.ts | 24 ++++++++++++-------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/packages/node/src/integrations/node-fetch.ts b/packages/node/src/integrations/node-fetch.ts index 6ade560a4835..f56f06949415 100644 --- a/packages/node/src/integrations/node-fetch.ts +++ b/packages/node/src/integrations/node-fetch.ts @@ -44,11 +44,11 @@ const _nativeNodeFetchIntegration = ((options: NodeFetchOptions = {}) => { const url = request.origin; return _ignoreOutgoingRequests && url && _ignoreOutgoingRequests(url); }, - onRequest: ({ span }: { span: Span }) => { + onRequest: ({ span, request }: { span: Span; request: unknown }) => { _updateSpan(span); if (_breadcrumbs) { - _addRequestBreadcrumb(span); + _addRequestBreadcrumb(span, request); } }, // eslint-disable-next-line @typescript-eslint/no-explicit-any @@ -80,17 +80,23 @@ function _updateSpan(span: Span): void { } /** Add a breadcrumb for outgoing requests. */ -function _addRequestBreadcrumb(span: Span): void { +function _addRequestBreadcrumb(span: Span, request: unknown): void { if (getSpanKind(span) !== SpanKind.CLIENT) { return; } const data = getRequestSpanData(span); - addBreadcrumb({ - category: 'http', - data: { - ...data, + addBreadcrumb( + { + category: 'http', + data: { + ...data, + }, + type: 'http', }, - type: 'http', - }); + { + event: 'response', + request, + }, + ); } From a5558ec5f1cf46c1bda196927896ed10809c9ef1 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 21 May 2024 15:25:50 +0200 Subject: [PATCH 2/2] fix breadcrumbs --- .../requests/fetch-breadcrumbs/scenario.ts | 33 ++++++++ .../requests/fetch-breadcrumbs/test.ts | 79 +++++++++++++++++++ .../requests/http-breadcrumbs/scenario.ts | 48 +++++++++++ .../tracing/requests/http-breadcrumbs/test.ts | 76 ++++++++++++++++++ packages/node/src/integrations/http.ts | 65 +++++++++++---- packages/node/src/integrations/node-fetch.ts | 71 +++++++++++++---- 6 files changed, 340 insertions(+), 32 deletions(-) create mode 100644 dev-packages/node-integration-tests/suites/tracing/requests/fetch-breadcrumbs/scenario.ts create mode 100644 dev-packages/node-integration-tests/suites/tracing/requests/fetch-breadcrumbs/test.ts create mode 100644 dev-packages/node-integration-tests/suites/tracing/requests/http-breadcrumbs/scenario.ts create mode 100644 dev-packages/node-integration-tests/suites/tracing/requests/http-breadcrumbs/test.ts diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-breadcrumbs/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-breadcrumbs/scenario.ts new file mode 100644 index 000000000000..eff91b2cd3e4 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-breadcrumbs/scenario.ts @@ -0,0 +1,33 @@ +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: [], + 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; + }, +}); + +async function run(): Promise { + Sentry.addBreadcrumb({ message: 'manual breadcrumb' }); + + // 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()); + await fetch(`${process.env.SERVER_URL}/api/v3`).then(res => res.text()); + + Sentry.captureException(new Error('foo')); +} + +// eslint-disable-next-line @typescript-eslint/no-floating-promises +run(); diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/fetch-breadcrumbs/test.ts b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-breadcrumbs/test.ts new file mode 100644 index 000000000000..f410f8209110 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/requests/fetch-breadcrumbs/test.ts @@ -0,0 +1,79 @@ +import { conditionalTest } from '../../../../utils'; +import { createRunner } from '../../../../utils/runner'; +import { createTestServer } from '../../../../utils/server'; + +conditionalTest({ min: 18 })('outgoing fetch', () => { + test('outgoing fetch requests create breadcrumbs', done => { + createTestServer(done) + .start() + .then(SERVER_URL => { + createRunner(__dirname, 'scenario.ts') + .withEnv({ SERVER_URL }) + .ensureNoErrorOutput() + .ignore('session', 'sessions') + .expect({ + event: { + breadcrumbs: [ + { + message: 'manual breadcrumb', + timestamp: expect.any(Number), + }, + { + category: 'http', + data: { + 'http.method': 'GET', + url: `${SERVER_URL}/api/v0`, + status_code: 404, + ADDED_PATH: '/api/v0', + }, + timestamp: expect.any(Number), + type: 'http', + }, + { + category: 'http', + data: { + 'http.method': 'GET', + url: `${SERVER_URL}/api/v1`, + status_code: 404, + ADDED_PATH: '/api/v1', + }, + timestamp: expect.any(Number), + type: 'http', + }, + { + category: 'http', + data: { + 'http.method': 'GET', + url: `${SERVER_URL}/api/v2`, + status_code: 404, + ADDED_PATH: '/api/v2', + }, + timestamp: expect.any(Number), + type: 'http', + }, + { + category: 'http', + data: { + 'http.method': 'GET', + url: `${SERVER_URL}/api/v3`, + status_code: 404, + ADDED_PATH: '/api/v3', + }, + timestamp: expect.any(Number), + type: 'http', + }, + ], + exception: { + values: [ + { + type: 'Error', + value: 'foo', + }, + ], + }, + }, + }) + .start(done); + }); + }); +}); diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/http-breadcrumbs/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/requests/http-breadcrumbs/scenario.ts new file mode 100644 index 000000000000..d7fb81d22e1f --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/requests/http-breadcrumbs/scenario.ts @@ -0,0 +1,48 @@ +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: [], + 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 makeHttpRequest(`${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(); + }); +} diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/http-breadcrumbs/test.ts b/dev-packages/node-integration-tests/suites/tracing/requests/http-breadcrumbs/test.ts new file mode 100644 index 000000000000..e6f44c7953f8 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/requests/http-breadcrumbs/test.ts @@ -0,0 +1,76 @@ +import { createRunner } from '../../../../utils/runner'; +import { createTestServer } from '../../../../utils/server'; + +test('outgoing http requests create breadcrumbs', done => { + createTestServer(done) + .start() + .then(SERVER_URL => { + createRunner(__dirname, 'scenario.ts') + .withEnv({ SERVER_URL }) + .ensureNoErrorOutput() + .ignore('session', 'sessions') + .expect({ + event: { + breadcrumbs: [ + { + message: 'manual breadcrumb', + timestamp: expect.any(Number), + }, + { + category: 'http', + data: { + 'http.method': 'GET', + url: `${SERVER_URL}/api/v0`, + status_code: 404, + ADDED_PATH: '/api/v0', + }, + timestamp: expect.any(Number), + type: 'http', + }, + { + category: 'http', + data: { + 'http.method': 'GET', + url: `${SERVER_URL}/api/v1`, + status_code: 404, + ADDED_PATH: '/api/v1', + }, + timestamp: expect.any(Number), + type: 'http', + }, + { + category: 'http', + data: { + 'http.method': 'GET', + url: `${SERVER_URL}/api/v2`, + status_code: 404, + ADDED_PATH: '/api/v2', + }, + timestamp: expect.any(Number), + type: 'http', + }, + { + category: 'http', + data: { + 'http.method': 'GET', + url: `${SERVER_URL}/api/v3`, + status_code: 404, + ADDED_PATH: '/api/v3', + }, + timestamp: expect.any(Number), + type: 'http', + }, + ], + exception: { + values: [ + { + type: 'Error', + value: 'foo', + }, + ], + }, + }, + }) + .start(done); + }); +}); diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index f3e4ea5dd48d..6854751fdcac 100644 --- a/packages/node/src/integrations/http.ts +++ b/packages/node/src/integrations/http.ts @@ -1,6 +1,5 @@ -import type { ClientRequest, IncomingMessage, ServerResponse } from 'node:http'; +import type { ClientRequest, ServerResponse } from 'node:http'; import type { Span } from '@opentelemetry/api'; -import { SpanKind } from '@opentelemetry/api'; import { HttpInstrumentation } from '@opentelemetry/instrumentation-http'; import { addOpenTelemetryInstrumentation } from '@sentry/opentelemetry'; @@ -13,10 +12,10 @@ import { isSentryRequestUrl, setCapturedScopesOnSpan, } from '@sentry/core'; -import { getClient, getRequestSpanData, getSpanKind } from '@sentry/opentelemetry'; -import type { IntegrationFn } from '@sentry/types'; +import { getClient } from '@sentry/opentelemetry'; +import type { IntegrationFn, SanitizedRequestData } from '@sentry/types'; -import { stripUrlQueryAndFragment } from '@sentry/utils'; +import { getSanitizedUrlString, parseUrl, stripUrlQueryAndFragment } from '@sentry/utils'; import type { NodeClient } from '../sdk/client'; import { setIsolationScope } from '../sdk/scope'; import type { HTTPModuleRequestIncomingMessage } from '../transports/http-module'; @@ -127,11 +126,7 @@ const _httpIntegration = ((options: HttpOptions = {}) => { isolationScope.setTransactionName(bestEffortTransactionName); }, - responseHook: (span, res) => { - if (_breadcrumbs) { - _addRequestBreadcrumb(span, res); - } - + responseHook: () => { const client = getClient(); if (client && client.getOptions().autoSessionTracking) { setImmediate(() => { @@ -139,6 +134,15 @@ const _httpIntegration = ((options: HttpOptions = {}) => { }); } }, + applyCustomAttributesOnSpan: ( + _span: Span, + request: ClientRequest | HTTPModuleRequestIncomingMessage, + response: HTTPModuleRequestIncomingMessage | ServerResponse, + ) => { + if (_breadcrumbs) { + _addRequestBreadcrumb(request, response); + } + }, }), ); }, @@ -152,12 +156,16 @@ const _httpIntegration = ((options: HttpOptions = {}) => { export const httpIntegration = defineIntegration(_httpIntegration); /** Add a breadcrumb for outgoing requests. */ -function _addRequestBreadcrumb(span: Span, response: HTTPModuleRequestIncomingMessage | ServerResponse): void { - if (getSpanKind(span) !== SpanKind.CLIENT) { +function _addRequestBreadcrumb( + request: ClientRequest | HTTPModuleRequestIncomingMessage, + response: HTTPModuleRequestIncomingMessage | ServerResponse, +): void { + // Only generate breadcrumbs for outgoing requests + if (!_isClientRequest(request)) { return; } - const data = getRequestSpanData(span); + const data = getBreadcrumbData(request); addBreadcrumb( { category: 'http', @@ -169,19 +177,42 @@ function _addRequestBreadcrumb(span: Span, response: HTTPModuleRequestIncomingMe }, { event: 'response', - // TODO FN: Do we need access to `request` here? - // If we do, we'll have to use the `applyCustomAttributesOnSpan` hook instead, - // but this has worse context semantics than request/responseHook. + request, response, }, ); } +function getBreadcrumbData(request: ClientRequest): Partial { + try { + // `request.host` does not contain the port, but the host header does + const host = request.getHeader('host') || request.host; + const url = new URL(request.path, `${request.protocol}//${host}`); + const parsedUrl = parseUrl(url.toString()); + + const data: Partial = { + url: getSanitizedUrlString(parsedUrl), + 'http.method': request.method || 'GET', + }; + + if (parsedUrl.search) { + data['http.query'] = parsedUrl.search; + } + if (parsedUrl.hash) { + data['http.fragment'] = parsedUrl.hash; + } + + return data; + } catch { + return {}; + } +} + /** * Determines if @param req is a ClientRequest, meaning the request was created within the express app * and it's an outgoing request. * Checking for properties instead of using `instanceOf` to avoid importing the request classes. */ -function _isClientRequest(req: ClientRequest | IncomingMessage): req is ClientRequest { +function _isClientRequest(req: ClientRequest | HTTPModuleRequestIncomingMessage): req is ClientRequest { return 'outputData' in req && 'outputSize' in req && !('client' in req) && !('statusCode' in req); } diff --git a/packages/node/src/integrations/node-fetch.ts b/packages/node/src/integrations/node-fetch.ts index f56f06949415..6150615d1bae 100644 --- a/packages/node/src/integrations/node-fetch.ts +++ b/packages/node/src/integrations/node-fetch.ts @@ -1,10 +1,8 @@ import type { Span } from '@opentelemetry/api'; -import { SpanKind } from '@opentelemetry/api'; import { addBreadcrumb, defineIntegration } from '@sentry/core'; import { addOpenTelemetryInstrumentation } from '@sentry/opentelemetry'; -import { getRequestSpanData, getSpanKind } from '@sentry/opentelemetry'; -import type { IntegrationFn } from '@sentry/types'; -import { logger } from '@sentry/utils'; +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'; @@ -12,6 +10,18 @@ import type { FetchInstrumentation } from 'opentelemetry-instrumentation-fetch-n import { addOriginToSpan } from '../utils/addOriginToSpan'; +interface FetchRequest { + method: string; + origin: string; + path: string; + headers: string | string[]; +} + +interface FetchResponse { + headers: Buffer[]; + statusCode: number; +} + interface NodeFetchOptions { /** * Whether breadcrumbs should be recorded for requests. @@ -39,17 +49,26 @@ const _nativeNodeFetchIntegration = ((options: NodeFetchOptions = {}) => { try { const pkg = await import('opentelemetry-instrumentation-fetch-node'); - return new pkg.FetchInstrumentation({ + 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: { origin?: string }) => { const url = request.origin; return _ignoreOutgoingRequests && url && _ignoreOutgoingRequests(url); }, - onRequest: ({ span, request }: { span: Span; request: unknown }) => { + onRequest: ({ span }: { span: Span }) => { _updateSpan(span); - - if (_breadcrumbs) { - _addRequestBreadcrumb(span, request); - } }, // eslint-disable-next-line @typescript-eslint/no-explicit-any } as any); @@ -80,16 +99,14 @@ function _updateSpan(span: Span): void { } /** Add a breadcrumb for outgoing requests. */ -function _addRequestBreadcrumb(span: Span, request: unknown): void { - if (getSpanKind(span) !== SpanKind.CLIENT) { - return; - } +function _addRequestBreadcrumb(request: FetchRequest, response: FetchResponse): void { + const data = getBreadcrumbData(request); - const data = getRequestSpanData(span); addBreadcrumb( { category: 'http', data: { + status_code: response.statusCode, ...data, }, type: 'http', @@ -97,6 +114,30 @@ function _addRequestBreadcrumb(span: Span, request: unknown): void { { event: 'response', request, + response, }, ); } + +function getBreadcrumbData(request: FetchRequest): Partial { + try { + const url = new URL(request.path, request.origin); + const parsedUrl = parseUrl(url.toString()); + + const data: Partial = { + url: getSanitizedUrlString(parsedUrl), + 'http.method': request.method || 'GET', + }; + + if (parsedUrl.search) { + data['http.query'] = parsedUrl.search; + } + if (parsedUrl.hash) { + data['http.fragment'] = parsedUrl.hash; + } + + return data; + } catch { + return {}; + } +}