From 466f076c84e0895007300957211fe49ac81297f9 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 6 May 2025 13:38:09 +0200 Subject: [PATCH 1/3] feat(node): Drop `http.server` spans with 404 status by default This can be configured via `dropSpansForIncomingRequestStatusCodes` option in `httpIntegration()`. --- .../tracing/instrument-filterStatusCode.mjs | 14 ++++ .../tracing/scenario-filterStatusCode.mjs | 25 +++++++ .../suites/express-v5/tracing/test.ts | 75 ++++++++++++++----- .../tracing/instrument-filterStatusCode.mjs | 14 ++++ .../tracing/scenario-filterStatusCode.mjs | 25 +++++++ .../suites/express/tracing/test.ts | 75 ++++++++++++++----- packages/node/src/integrations/http/index.ts | 26 +++++++ 7 files changed, 217 insertions(+), 37 deletions(-) create mode 100644 dev-packages/node-integration-tests/suites/express-v5/tracing/instrument-filterStatusCode.mjs create mode 100644 dev-packages/node-integration-tests/suites/express-v5/tracing/scenario-filterStatusCode.mjs create mode 100644 dev-packages/node-integration-tests/suites/express/tracing/instrument-filterStatusCode.mjs create mode 100644 dev-packages/node-integration-tests/suites/express/tracing/scenario-filterStatusCode.mjs diff --git a/dev-packages/node-integration-tests/suites/express-v5/tracing/instrument-filterStatusCode.mjs b/dev-packages/node-integration-tests/suites/express-v5/tracing/instrument-filterStatusCode.mjs new file mode 100644 index 000000000000..47528f0b5664 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/express-v5/tracing/instrument-filterStatusCode.mjs @@ -0,0 +1,14 @@ +import * as Sentry from '@sentry/node'; +import { loggingTransport } from '@sentry-internal/node-integration-tests'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + tracesSampleRate: 1.0, + transport: loggingTransport, + integrations: [ + Sentry.httpIntegration({ + dropSpansForIncomingRequestStatusCodes: [499, /3\d{2}/], + }), + ], +}); diff --git a/dev-packages/node-integration-tests/suites/express-v5/tracing/scenario-filterStatusCode.mjs b/dev-packages/node-integration-tests/suites/express-v5/tracing/scenario-filterStatusCode.mjs new file mode 100644 index 000000000000..f2e20014f48f --- /dev/null +++ b/dev-packages/node-integration-tests/suites/express-v5/tracing/scenario-filterStatusCode.mjs @@ -0,0 +1,25 @@ +import * as Sentry from '@sentry/node'; +import { startExpressServerAndSendPortToRunner } from '@sentry-internal/node-integration-tests'; +import express from 'express'; + +const app = express(); + +app.get('/', (_req, res) => { + res.send({ response: 'response 0' }); +}); + +app.get('/499', (_req, res) => { + res.status(499).send({ response: 'response 499' }); +}); + +app.get('/300', (_req, res) => { + res.status(300).send({ response: 'response 300' }); +}); + +app.get('/399', (_req, res) => { + res.status(399).send({ response: 'response 399' }); +}); + +Sentry.setupExpressErrorHandler(app); + +startExpressServerAndSendPortToRunner(app); diff --git a/dev-packages/node-integration-tests/suites/express-v5/tracing/test.ts b/dev-packages/node-integration-tests/suites/express-v5/tracing/test.ts index 0b76192b6113..4618f801a087 100644 --- a/dev-packages/node-integration-tests/suites/express-v5/tracing/test.ts +++ b/dev-packages/node-integration-tests/suites/express-v5/tracing/test.ts @@ -77,7 +77,7 @@ describe('express v5 tracing', () => { await runner.completed(); }); - test('handles root page correctly', async () => { + test('handles root route correctly', async () => { const runner = createRunner() .expect({ transaction: { @@ -89,30 +89,17 @@ describe('express v5 tracing', () => { await runner.completed(); }); - test('handles 404 page correctly', async () => { + test('ignores 404 routes by default', async () => { const runner = createRunner() .expect({ + // No transaction is sent for the 404 route transaction: { - transaction: 'GET /does-not-exist', - contexts: { - trace: { - span_id: expect.stringMatching(/[a-f0-9]{16}/), - trace_id: expect.stringMatching(/[a-f0-9]{32}/), - data: { - 'http.response.status_code': 404, - url: expect.stringMatching(/\/does-not-exist$/), - 'http.method': 'GET', - 'http.url': expect.stringMatching(/\/does-not-exist$/), - 'http.target': '/does-not-exist', - }, - op: 'http.server', - status: 'not_found', - }, - }, + transaction: 'GET /', }, }) .start(); runner.makeRequest('get', '/does-not-exist', { expectError: true }); + runner.makeRequest('get', '/'); await runner.completed(); }); @@ -290,4 +277,56 @@ describe('express v5 tracing', () => { }); }); }); + + describe('filter status codes', () => { + createEsmAndCjsTests( + __dirname, + 'scenario-filterStatusCode.mjs', + 'instrument-filterStatusCode.mjs', + (createRunner, test) => { + // We opt-out of the default 404 filtering in order to test how 404 spans are handled + test('handles 404 route correctly', async () => { + const runner = createRunner() + .expect({ + transaction: { + transaction: 'GET /does-not-exist', + contexts: { + trace: { + span_id: expect.stringMatching(/[a-f0-9]{16}/), + trace_id: expect.stringMatching(/[a-f0-9]{32}/), + data: { + 'http.response.status_code': 404, + url: expect.stringMatching(/\/does-not-exist$/), + 'http.method': 'GET', + 'http.url': expect.stringMatching(/\/does-not-exist$/), + 'http.target': '/does-not-exist', + }, + op: 'http.server', + status: 'not_found', + }, + }, + }, + }) + .start(); + runner.makeRequest('get', '/does-not-exist', { expectError: true }); + await runner.completed(); + }); + + test('filters defined status codes', async () => { + const runner = createRunner() + .expect({ + transaction: { + transaction: 'GET /', + }, + }) + .start(); + await runner.makeRequest('get', '/499', { expectError: true }); + await runner.makeRequest('get', '/300', { expectError: true }); + await runner.makeRequest('get', '/399', { expectError: true }); + await runner.makeRequest('get', '/'); + await runner.completed(); + }); + }, + ); + }); }); diff --git a/dev-packages/node-integration-tests/suites/express/tracing/instrument-filterStatusCode.mjs b/dev-packages/node-integration-tests/suites/express/tracing/instrument-filterStatusCode.mjs new file mode 100644 index 000000000000..47528f0b5664 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/express/tracing/instrument-filterStatusCode.mjs @@ -0,0 +1,14 @@ +import * as Sentry from '@sentry/node'; +import { loggingTransport } from '@sentry-internal/node-integration-tests'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + tracesSampleRate: 1.0, + transport: loggingTransport, + integrations: [ + Sentry.httpIntegration({ + dropSpansForIncomingRequestStatusCodes: [499, /3\d{2}/], + }), + ], +}); diff --git a/dev-packages/node-integration-tests/suites/express/tracing/scenario-filterStatusCode.mjs b/dev-packages/node-integration-tests/suites/express/tracing/scenario-filterStatusCode.mjs new file mode 100644 index 000000000000..f2e20014f48f --- /dev/null +++ b/dev-packages/node-integration-tests/suites/express/tracing/scenario-filterStatusCode.mjs @@ -0,0 +1,25 @@ +import * as Sentry from '@sentry/node'; +import { startExpressServerAndSendPortToRunner } from '@sentry-internal/node-integration-tests'; +import express from 'express'; + +const app = express(); + +app.get('/', (_req, res) => { + res.send({ response: 'response 0' }); +}); + +app.get('/499', (_req, res) => { + res.status(499).send({ response: 'response 499' }); +}); + +app.get('/300', (_req, res) => { + res.status(300).send({ response: 'response 300' }); +}); + +app.get('/399', (_req, res) => { + res.status(399).send({ response: 'response 399' }); +}); + +Sentry.setupExpressErrorHandler(app); + +startExpressServerAndSendPortToRunner(app); diff --git a/dev-packages/node-integration-tests/suites/express/tracing/test.ts b/dev-packages/node-integration-tests/suites/express/tracing/test.ts index d40f8b9b16c3..63706c0e5cb2 100644 --- a/dev-packages/node-integration-tests/suites/express/tracing/test.ts +++ b/dev-packages/node-integration-tests/suites/express/tracing/test.ts @@ -90,33 +90,17 @@ describe('express tracing', () => { await runner.completed(); }); - test('handles 404 page correctly', async () => { + test('ignores 404 routes by default', async () => { const runner = createRunner() .expect({ + // No transaction is sent for the 404 route transaction: { - // FIXME: This is wrong :( transaction: 'GET /', - contexts: { - trace: { - span_id: expect.stringMatching(/[a-f0-9]{16}/), - trace_id: expect.stringMatching(/[a-f0-9]{32}/), - data: { - 'http.response.status_code': 404, - url: expect.stringMatching(/\/does-not-exist$/), - 'http.method': 'GET', - // FIXME: This is wrong :( - 'http.route': '/', - 'http.url': expect.stringMatching(/\/does-not-exist$/), - 'http.target': '/does-not-exist', - }, - op: 'http.server', - status: 'not_found', - }, - }, }, }) .start(); runner.makeRequest('get', '/does-not-exist', { expectError: true }); + runner.makeRequest('get', '/'); await runner.completed(); }); @@ -324,4 +308,57 @@ describe('express tracing', () => { }); }); }); + + describe('filter status codes', () => { + createEsmAndCjsTests( + __dirname, + 'scenario-filterStatusCode.mjs', + 'instrument-filterStatusCode.mjs', + (createRunner, test) => { + // We opt-out of the default 404 filtering in order to test how 404 spans are handled + test('handles 404 route correctly', async () => { + const runner = createRunner() + .expect({ + transaction: { + // FIXME: This is incorrect, sadly :( + transaction: 'GET /', + contexts: { + trace: { + span_id: expect.stringMatching(/[a-f0-9]{16}/), + trace_id: expect.stringMatching(/[a-f0-9]{32}/), + data: { + 'http.response.status_code': 404, + url: expect.stringMatching(/\/does-not-exist$/), + 'http.method': 'GET', + 'http.url': expect.stringMatching(/\/does-not-exist$/), + 'http.target': '/does-not-exist', + }, + op: 'http.server', + status: 'not_found', + }, + }, + }, + }) + .start(); + runner.makeRequest('get', '/does-not-exist', { expectError: true }); + await runner.completed(); + }); + + test('filters defined status codes', async () => { + const runner = createRunner() + .expect({ + transaction: { + transaction: 'GET /', + }, + }) + .start(); + await runner.makeRequest('get', '/499', { expectError: true }); + await runner.makeRequest('get', '/300', { expectError: true }); + await runner.makeRequest('get', '/399', { expectError: true }); + await runner.makeRequest('get', '/'); + await runner.completed(); + }); + }, + ); + }); }); diff --git a/packages/node/src/integrations/http/index.ts b/packages/node/src/integrations/http/index.ts index 9fe4792e12a3..5bbd7d1a8c53 100644 --- a/packages/node/src/integrations/http/index.ts +++ b/packages/node/src/integrations/http/index.ts @@ -73,6 +73,14 @@ interface HttpOptions { */ ignoreIncomingRequests?: (urlPath: string, request: IncomingMessage) => boolean; + /** + * Do not capture spans for incoming HTTP requests with the given status codes. + * By default, spans with 404 status code are ignored. + * + * @default `[404]` + */ + dropSpansForIncomingRequestStatusCodes?: (number | RegExp)[]; + /** * Do not capture the request body for incoming HTTP requests to URLs where the given callback returns `true`. * This can be useful for long running requests where the body is not needed and we want to avoid capturing it. @@ -148,6 +156,8 @@ export function _shouldInstrumentSpans(options: HttpOptions, clientOptions: Part * It creates breadcrumbs and spans for outgoing HTTP requests which will be attached to the currently active span. */ export const httpIntegration = defineIntegration((options: HttpOptions = {}) => { + const dropSpansForIncomingRequestStatusCodes = options.dropSpansForIncomingRequestStatusCodes ?? [404]; + return { name: INTEGRATION_NAME, setupOnce() { @@ -180,6 +190,22 @@ export const httpIntegration = defineIntegration((options: HttpOptions = {}) => instrumentOtelHttp(instrumentationConfig); } }, + processEvent(event) { + // Drop transaction if it has a status code that should be ignored + if (event.type === 'transaction') { + const statusCode = event.contexts?.trace?.data?.['http.response.status_code']; + if ( + typeof statusCode === 'number' && + dropSpansForIncomingRequestStatusCodes.some(code => + typeof code === 'number' ? code === statusCode : code.test(statusCode.toString()), + ) + ) { + return null; + } + } + + return event; + }, }; }); From 5ae3c07455a1444c059af1b9b4630465a67add07 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 7 May 2025 10:47:23 +0200 Subject: [PATCH 2/3] use ranges instead of regex --- .../tracing/instrument-filterStatusCode.mjs | 2 +- .../tracing/instrument-filterStatusCode.mjs | 2 +- packages/node/src/integrations/http/index.ts | 14 ++++++++++---- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/express-v5/tracing/instrument-filterStatusCode.mjs b/dev-packages/node-integration-tests/suites/express-v5/tracing/instrument-filterStatusCode.mjs index 47528f0b5664..31473a90df73 100644 --- a/dev-packages/node-integration-tests/suites/express-v5/tracing/instrument-filterStatusCode.mjs +++ b/dev-packages/node-integration-tests/suites/express-v5/tracing/instrument-filterStatusCode.mjs @@ -8,7 +8,7 @@ Sentry.init({ transport: loggingTransport, integrations: [ Sentry.httpIntegration({ - dropSpansForIncomingRequestStatusCodes: [499, /3\d{2}/], + dropSpansForIncomingRequestStatusCodes: [499, [300, 399]], }), ], }); diff --git a/dev-packages/node-integration-tests/suites/express/tracing/instrument-filterStatusCode.mjs b/dev-packages/node-integration-tests/suites/express/tracing/instrument-filterStatusCode.mjs index 47528f0b5664..31473a90df73 100644 --- a/dev-packages/node-integration-tests/suites/express/tracing/instrument-filterStatusCode.mjs +++ b/dev-packages/node-integration-tests/suites/express/tracing/instrument-filterStatusCode.mjs @@ -8,7 +8,7 @@ Sentry.init({ transport: loggingTransport, integrations: [ Sentry.httpIntegration({ - dropSpansForIncomingRequestStatusCodes: [499, /3\d{2}/], + dropSpansForIncomingRequestStatusCodes: [499, [300, 399]], }), ], }); diff --git a/packages/node/src/integrations/http/index.ts b/packages/node/src/integrations/http/index.ts index 5bbd7d1a8c53..d25d19a86c8c 100644 --- a/packages/node/src/integrations/http/index.ts +++ b/packages/node/src/integrations/http/index.ts @@ -76,10 +76,11 @@ interface HttpOptions { /** * Do not capture spans for incoming HTTP requests with the given status codes. * By default, spans with 404 status code are ignored. + * Expects an array of status codes or a range of status codes, e.g. [[300,399], 404] would ignore 3xx and 404 status codes. * * @default `[404]` */ - dropSpansForIncomingRequestStatusCodes?: (number | RegExp)[]; + dropSpansForIncomingRequestStatusCodes?: (number | [number, number])[]; /** * Do not capture the request body for incoming HTTP requests to URLs where the given callback returns `true`. @@ -196,9 +197,14 @@ export const httpIntegration = defineIntegration((options: HttpOptions = {}) => const statusCode = event.contexts?.trace?.data?.['http.response.status_code']; if ( typeof statusCode === 'number' && - dropSpansForIncomingRequestStatusCodes.some(code => - typeof code === 'number' ? code === statusCode : code.test(statusCode.toString()), - ) + dropSpansForIncomingRequestStatusCodes.some(code => { + if (typeof code === 'number') { + return code === statusCode; + } + + const [min, max] = code; + return statusCode >= min && statusCode <= max; + }) ) { return null; } From 94ec0f29b43cf9a23761d0a41df7c090c94cdb97 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 7 May 2025 11:17:13 +0200 Subject: [PATCH 3/3] fix tests --- .../tests/generation-functions.test.ts | 17 ----------------- .../app/route-handlers/[param]/route.ts | 2 +- .../nextjs-app-dir/tests/route-handlers.test.ts | 2 +- .../test-applications/node-express/src/app.ts | 4 ++-- .../node-express/tests/trpc.test.ts | 6 +++--- .../test-applications/node-hapi/src/app.js | 2 +- .../node-hapi/tests/errors.test.ts | 2 +- 7 files changed, 9 insertions(+), 26 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/nextjs-14/tests/generation-functions.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-14/tests/generation-functions.test.ts index 346928c44ebc..084824824225 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-14/tests/generation-functions.test.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-14/tests/generation-functions.test.ts @@ -125,20 +125,3 @@ test('Should send a transaction event with correct status for a generateMetadata expect((await transactionPromise).contexts?.trace?.status).toBe('ok'); }); - -test('Should send a transaction event with correct status for a generateMetadata() function invocation with notfound()', async ({ - page, -}) => { - const testTitle = 'notfound-foobar'; - - const transactionPromise = waitForTransaction('nextjs-14', async transactionEvent => { - return ( - transactionEvent.contexts?.trace?.data?.['http.target'] === - `/generation-functions/with-notfound?metadataTitle=${testTitle}` - ); - }); - - await page.goto(`/generation-functions/with-notfound?metadataTitle=${testTitle}`); - - expect((await transactionPromise).contexts?.trace?.status).toBe('not_found'); -}); diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/route-handlers/[param]/route.ts b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/route-handlers/[param]/route.ts index 386b8c6e117f..df5361852508 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/route-handlers/[param]/route.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/app/route-handlers/[param]/route.ts @@ -5,5 +5,5 @@ export async function GET() { } export async function POST() { - return NextResponse.json({ name: 'John Doe' }, { status: 404 }); + return NextResponse.json({ name: 'John Doe' }, { status: 403 }); } diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/route-handlers.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/route-handlers.test.ts index 648ee81839ac..02f2b6dc4f24 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/route-handlers.test.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/route-handlers.test.ts @@ -28,7 +28,7 @@ test('Should create a transaction for route handlers and correctly set span stat const routehandlerTransaction = await routehandlerTransactionPromise; - expect(routehandlerTransaction.contexts?.trace?.status).toBe('not_found'); + expect(routehandlerTransaction.contexts?.trace?.status).toBe('permission_denied'); expect(routehandlerTransaction.contexts?.trace?.op).toBe('http.server'); }); diff --git a/dev-packages/e2e-tests/test-applications/node-express/src/app.ts b/dev-packages/e2e-tests/test-applications/node-express/src/app.ts index 6b320e26eb8a..e2211aa7e634 100644 --- a/dev-packages/e2e-tests/test-applications/node-express/src/app.ts +++ b/dev-packages/e2e-tests/test-applications/node-express/src/app.ts @@ -123,8 +123,8 @@ export const appRouter = t.router({ .mutation(() => { throw new Error('I crashed in a trpc handler'); }), - dontFindSomething: procedure.mutation(() => { - throw new TRPCError({ code: 'NOT_FOUND', cause: new Error('Page not found') }); + unauthorized: procedure.mutation(() => { + throw new TRPCError({ code: 'UNAUTHORIZED', cause: new Error('Unauthorized') }); }), }); diff --git a/dev-packages/e2e-tests/test-applications/node-express/tests/trpc.test.ts b/dev-packages/e2e-tests/test-applications/node-express/tests/trpc.test.ts index 633306ae713a..3cb458f81175 100644 --- a/dev-packages/e2e-tests/test-applications/node-express/tests/trpc.test.ts +++ b/dev-packages/e2e-tests/test-applications/node-express/tests/trpc.test.ts @@ -109,12 +109,12 @@ test('Should record transaction and error for a trpc handler that returns a stat const transactionEventPromise = waitForTransaction('node-express', transactionEvent => { return ( transactionEvent.transaction === 'POST /trpc' && - !!transactionEvent.spans?.find(span => span.description === 'trpc/dontFindSomething') + !!transactionEvent.spans?.find(span => span.description === 'trpc/unauthorized') ); }); const errorEventPromise = waitForError('node-express', errorEvent => { - return !!errorEvent?.exception?.values?.some(exception => exception.value?.includes('Page not found')); + return !!errorEvent?.exception?.values?.some(exception => exception.value?.includes('Unauthorized')); }); const trpcClient = createTRPCProxyClient({ @@ -125,7 +125,7 @@ test('Should record transaction and error for a trpc handler that returns a stat ], }); - await expect(trpcClient.dontFindSomething.mutate()).rejects.toBeDefined(); + await expect(trpcClient.unauthorized.mutate()).rejects.toBeDefined(); await expect(transactionEventPromise).resolves.toBeDefined(); await expect(errorEventPromise).resolves.toBeDefined(); diff --git a/dev-packages/e2e-tests/test-applications/node-hapi/src/app.js b/dev-packages/e2e-tests/test-applications/node-hapi/src/app.js index ae803aa3edf7..8b68e8412aba 100644 --- a/dev-packages/e2e-tests/test-applications/node-hapi/src/app.js +++ b/dev-packages/e2e-tests/test-applications/node-hapi/src/app.js @@ -102,7 +102,7 @@ const init = async () => { const path = request.route.path; if (path.includes('boom-4xx')) { - throw Boom.notFound('4xx not found (onPreResponse)'); + throw Boom.badRequest('4xx bad request (onPreResponse)'); } else if (path.includes('boom-5xx')) { throw Boom.gatewayTimeout('5xx not implemented (onPreResponse)'); } else if (path.includes('JS-error-onPreResponse')) { diff --git a/dev-packages/e2e-tests/test-applications/node-hapi/tests/errors.test.ts b/dev-packages/e2e-tests/test-applications/node-hapi/tests/errors.test.ts index 6531b83baa8e..4f2fba52aa95 100644 --- a/dev-packages/e2e-tests/test-applications/node-hapi/tests/errors.test.ts +++ b/dev-packages/e2e-tests/test-applications/node-hapi/tests/errors.test.ts @@ -77,7 +77,7 @@ test('Does not send errors to Sentry if boom throws in "onPreResponse" after JS const response4xx = await fetch(`${baseURL}/test-failure-boom-4xx`); const response5xx = await fetch(`${baseURL}/test-failure-boom-5xx`); - expect(response4xx.status).toBe(404); + expect(response4xx.status).toBe(400); expect(response5xx.status).toBe(504); const transactionEvent4xx = await transactionEventPromise4xx;