From 63f1af96f1a86683359ea2af1f5d3df8f9f556f7 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 16 Jul 2024 10:58:00 +0200 Subject: [PATCH 1/5] fix(node): Ensure correct URL is passed to `ignoreIncomingRequests` callback --- .../node-integration-tests/src/index.ts | 10 +++- .../server-ignoreIncomingRequests.js | 38 +++++++++++++ .../server-ignoreOutgoingRequests.js | 42 ++++++++++++++ .../suites/tracing/httpIntegration/test.ts | 55 +++++++++++++++++++ packages/node/src/integrations/http.ts | 5 +- 5 files changed, 145 insertions(+), 5 deletions(-) create mode 100644 dev-packages/node-integration-tests/suites/tracing/httpIntegration/server-ignoreIncomingRequests.js create mode 100644 dev-packages/node-integration-tests/suites/tracing/httpIntegration/server-ignoreOutgoingRequests.js diff --git a/dev-packages/node-integration-tests/src/index.ts b/dev-packages/node-integration-tests/src/index.ts index 08afc11fe7ea..4bd0a9ccce25 100644 --- a/dev-packages/node-integration-tests/src/index.ts +++ b/dev-packages/node-integration-tests/src/index.ts @@ -20,13 +20,17 @@ export function loggingTransport(_options: BaseTransportOptions): Transport { /** * Starts an express server and sends the port to the runner + * @param app Express app + * @param port Port to start the app on. USE WITH CAUTION! By default a random port will be chosen. + * Setting this port to something specific is useful for local debugging but dangerous for + * CI/CD environments where port collisions can cause flakes! */ -export function startExpressServerAndSendPortToRunner(app: Express): void { - const server = app.listen(0, () => { +export function startExpressServerAndSendPortToRunner(app: Express, port: number | undefined = undefined): void { + const server = app.listen(port || 0, () => { const address = server.address() as AddressInfo; // eslint-disable-next-line no-console - console.log(`{"port":${address.port}}`); + console.log(`{"port":${port || address.port}}`); }); } diff --git a/dev-packages/node-integration-tests/suites/tracing/httpIntegration/server-ignoreIncomingRequests.js b/dev-packages/node-integration-tests/suites/tracing/httpIntegration/server-ignoreIncomingRequests.js new file mode 100644 index 000000000000..f1e5d9870fcf --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/httpIntegration/server-ignoreIncomingRequests.js @@ -0,0 +1,38 @@ +const { loggingTransport } = require('@sentry-internal/node-integration-tests'); +const Sentry = require('@sentry/node'); + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + tracesSampleRate: 1.0, + transport: loggingTransport, + + integrations: [ + Sentry.httpIntegration({ + ignoreIncomingRequests: url => { + return url.includes('/liveness'); + }, + }), + ], +}); + +// express must be required after Sentry is initialized +const express = require('express'); +const cors = require('cors'); +const { startExpressServerAndSendPortToRunner } = require('@sentry-internal/node-integration-tests'); + +const app = express(); + +app.use(cors()); + +app.get('/test', (_req, res) => { + res.send({ response: 'response 1' }); +}); + +app.get('/liveness', (_req, res) => { + res.send({ response: 'liveness' }); +}); + +Sentry.setupExpressErrorHandler(app); + +startExpressServerAndSendPortToRunner(app); diff --git a/dev-packages/node-integration-tests/suites/tracing/httpIntegration/server-ignoreOutgoingRequests.js b/dev-packages/node-integration-tests/suites/tracing/httpIntegration/server-ignoreOutgoingRequests.js new file mode 100644 index 000000000000..ce520c999259 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/httpIntegration/server-ignoreOutgoingRequests.js @@ -0,0 +1,42 @@ +const { loggingTransport } = require('@sentry-internal/node-integration-tests'); +const Sentry = require('@sentry/node'); +const http = require('http'); + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + tracesSampleRate: 1.0, + transport: loggingTransport, + + integrations: [ + Sentry.httpIntegration({ + ignoreOutgoingRequests: url => { + return url.includes('example.com'); + }, + }), + ], +}); + +// express must be required after Sentry is initialized +const express = require('express'); +const cors = require('cors'); +const { startExpressServerAndSendPortToRunner } = require('@sentry-internal/node-integration-tests'); + +const app = express(); + +app.use(cors()); + +app.get('/test', (_req, response) => { + http + .request('http://example.com/', res => { + res.on('data', () => {}); + res.on('end', () => { + response.send({ response: 'done' }); + }); + }) + .end(); +}); + +Sentry.setupExpressErrorHandler(app); + +startExpressServerAndSendPortToRunner(app); diff --git a/dev-packages/node-integration-tests/suites/tracing/httpIntegration/test.ts b/dev-packages/node-integration-tests/suites/tracing/httpIntegration/test.ts index 6be5d36e2ee3..aebe0dd676ba 100644 --- a/dev-packages/node-integration-tests/suites/tracing/httpIntegration/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/httpIntegration/test.ts @@ -75,4 +75,59 @@ describe('httpIntegration', () => { .start(done) .makeRequest('get', '/test'); }); + + test("doesn't create a root span for incoming requests ignored via `ignoreIncomingRequests`", done => { + const runner = createRunner(__dirname, 'server-ignoreIncomingRequests.js') + .expect({ + transaction: { + contexts: { + trace: { + span_id: expect.any(String), + trace_id: expect.any(String), + data: { + url: expect.stringMatching(/\/test$/), + 'http.response.status_code': 200, + }, + op: 'http.server', + status: 'ok', + }, + }, + transaction: 'GET /test', + }, + }) + .start(done); + + runner.makeRequest('get', '/liveness'); // should be ignored + runner.makeRequest('get', '/test'); + }); + + test("doesn't create child spans for outgoing requests ignored via `ignoreOutgoingRequests`", done => { + const runner = createRunner(__dirname, 'server-ignoreOutgoingRequests.js') + .expect({ + transaction: { + contexts: { + trace: { + span_id: expect.any(String), + trace_id: expect.any(String), + data: { + url: expect.stringMatching(/\/test$/), + 'http.response.status_code': 200, + }, + op: 'http.server', + status: 'ok', + }, + }, + transaction: 'GET /test', + spans: [ + expect.objectContaining({ op: 'middleware.express', description: 'query' }), + expect.objectContaining({ op: 'middleware.express', description: 'expressInit' }), + expect.objectContaining({ op: 'middleware.express', description: 'corsMiddleware' }), + expect.objectContaining({ op: 'request_handler.express', description: '/test' }), + ], + }, + }) + .start(done); + + runner.makeRequest('get', '/test'); + }); }); diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index 418fa8aa7853..4ac916fc22f8 100644 --- a/packages/node/src/integrations/http.ts +++ b/packages/node/src/integrations/http.ts @@ -15,6 +15,7 @@ import { import { getClient } from '@sentry/opentelemetry'; import type { IntegrationFn, SanitizedRequestData } from '@sentry/types'; +import type { RequestOptions } from 'node:https'; import { getSanitizedUrlString, parseUrl, stripUrlQueryAndFragment } from '@sentry/utils'; import type { NodeClient } from '../sdk/client'; import { setIsolationScope } from '../sdk/scope'; @@ -103,7 +104,7 @@ export const instrumentHttp = Object.assign( }, ignoreIncomingRequestHook: request => { - const url = getRequestUrl(request); + const url = request.url; const method = request.method?.toUpperCase(); // We do not capture OPTIONS/HEAD requests as transactions @@ -112,7 +113,7 @@ export const instrumentHttp = Object.assign( } const _ignoreIncomingRequests = _httpOptions.ignoreIncomingRequests; - if (_ignoreIncomingRequests && _ignoreIncomingRequests(url)) { + if (url && _ignoreIncomingRequests && _ignoreIncomingRequests(url)) { return true; } From 7cbfd34246fdd8c5dc59ca164f3f5c371993b569 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 16 Jul 2024 11:11:56 +0200 Subject: [PATCH 2/5] lint --- packages/node/src/integrations/http.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index 4ac916fc22f8..f2eea9dac877 100644 --- a/packages/node/src/integrations/http.ts +++ b/packages/node/src/integrations/http.ts @@ -15,7 +15,6 @@ import { import { getClient } from '@sentry/opentelemetry'; import type { IntegrationFn, SanitizedRequestData } from '@sentry/types'; -import type { RequestOptions } from 'node:https'; import { getSanitizedUrlString, parseUrl, stripUrlQueryAndFragment } from '@sentry/utils'; import type { NodeClient } from '../sdk/client'; import { setIsolationScope } from '../sdk/scope'; From 93487171df86f0befc955882b2519759e3306532 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 16 Jul 2024 13:05:31 +0200 Subject: [PATCH 3/5] change variable name s/url/urlPath, update JSDoc --- .../server-ignoreIncomingRequests.js | 1 + packages/node/src/integrations/http.ts | 14 +++++++++++--- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/tracing/httpIntegration/server-ignoreIncomingRequests.js b/dev-packages/node-integration-tests/suites/tracing/httpIntegration/server-ignoreIncomingRequests.js index f1e5d9870fcf..c2ad3382a660 100644 --- a/dev-packages/node-integration-tests/suites/tracing/httpIntegration/server-ignoreIncomingRequests.js +++ b/dev-packages/node-integration-tests/suites/tracing/httpIntegration/server-ignoreIncomingRequests.js @@ -10,6 +10,7 @@ Sentry.init({ integrations: [ Sentry.httpIntegration({ ignoreIncomingRequests: url => { + console.log('url', url); return url.includes('/liveness'); }, }), diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index f2eea9dac877..3af75c1b32e2 100644 --- a/packages/node/src/integrations/http.ts +++ b/packages/node/src/integrations/http.ts @@ -34,14 +34,20 @@ interface HttpOptions { /** * Do not capture spans or breadcrumbs for outgoing HTTP requests to URLs where the given callback returns `true`. * This controls both span & breadcrumb creation - spans will be non recording if tracing is disabled. + * + * The `url` param contains the entire URL, including query string (if any), protocol, host, etc. of the outgoing request. + * For example: `'https://someService.com/users/details?id=123'` */ ignoreOutgoingRequests?: (url: string) => boolean; /** * Do not capture spans or breadcrumbs for incoming HTTP requests to URLs where the given callback returns `true`. * This controls both span & breadcrumb creation - spans will be non recording if tracing is disabled. + * + * The `url` param consists of the URL path and query string (if any) of the incoming request. + * For example: `'/users/details?id=123'` */ - ignoreIncomingRequests?: (url: string) => boolean; + ignoreIncomingRequests?: (urlPath: string) => boolean; /** * Additional instrumentation options that are passed to the underlying HttpInstrumentation. @@ -103,7 +109,9 @@ export const instrumentHttp = Object.assign( }, ignoreIncomingRequestHook: request => { - const url = request.url; + // request.url is the only property that holds any information about the url + // it only consists of the URL path and query string (if any) + const urlPath = request.url; const method = request.method?.toUpperCase(); // We do not capture OPTIONS/HEAD requests as transactions @@ -112,7 +120,7 @@ export const instrumentHttp = Object.assign( } const _ignoreIncomingRequests = _httpOptions.ignoreIncomingRequests; - if (url && _ignoreIncomingRequests && _ignoreIncomingRequests(url)) { + if (urlPath && _ignoreIncomingRequests && _ignoreIncomingRequests(urlPath)) { return true; } From 4b5d587829b986918544a01d6a74722d08957a76 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 16 Jul 2024 13:06:37 +0200 Subject: [PATCH 4/5] rm console.log --- .../tracing/httpIntegration/server-ignoreIncomingRequests.js | 1 - 1 file changed, 1 deletion(-) diff --git a/dev-packages/node-integration-tests/suites/tracing/httpIntegration/server-ignoreIncomingRequests.js b/dev-packages/node-integration-tests/suites/tracing/httpIntegration/server-ignoreIncomingRequests.js index c2ad3382a660..f1e5d9870fcf 100644 --- a/dev-packages/node-integration-tests/suites/tracing/httpIntegration/server-ignoreIncomingRequests.js +++ b/dev-packages/node-integration-tests/suites/tracing/httpIntegration/server-ignoreIncomingRequests.js @@ -10,7 +10,6 @@ Sentry.init({ integrations: [ Sentry.httpIntegration({ ignoreIncomingRequests: url => { - console.log('url', url); return url.includes('/liveness'); }, }), From 757120e66fb104cc686845d1df8d5314b820751c Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 16 Jul 2024 13:37:11 +0200 Subject: [PATCH 5/5] Update packages/node/src/integrations/http.ts Co-authored-by: Francesco Novy --- packages/node/src/integrations/http.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index 3af75c1b32e2..632b6023e7a3 100644 --- a/packages/node/src/integrations/http.ts +++ b/packages/node/src/integrations/http.ts @@ -44,7 +44,7 @@ interface HttpOptions { * Do not capture spans or breadcrumbs for incoming HTTP requests to URLs where the given callback returns `true`. * This controls both span & breadcrumb creation - spans will be non recording if tracing is disabled. * - * The `url` param consists of the URL path and query string (if any) of the incoming request. + * The `urlPath` param consists of the URL path and query string (if any) of the incoming request. * For example: `'/users/details?id=123'` */ ignoreIncomingRequests?: (urlPath: string) => boolean;