From 28a397fc4e6d61dc6045a76a747195c050d11d6e Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 4 Jul 2024 14:34:25 +0200 Subject: [PATCH] feat(node): Allow to pass instrumentation config to `httpIntegration` --- .../suites/express/tracing/test.ts | 2 +- .../httpIntegration/server-experimental.js | 38 ++++++++++ .../suites/tracing/httpIntegration/server.js | 58 +++++++++++++++ .../suites/tracing/httpIntegration/test.ts | 73 +++++++++++++++++++ packages/nextjs/src/server/httpIntegration.ts | 14 +--- packages/node/src/integrations/http.ts | 31 +++++++- packages/remix/src/utils/integrations/http.ts | 14 +--- 7 files changed, 201 insertions(+), 29 deletions(-) create mode 100644 dev-packages/node-integration-tests/suites/tracing/httpIntegration/server-experimental.js create mode 100644 dev-packages/node-integration-tests/suites/tracing/httpIntegration/server.js create mode 100644 dev-packages/node-integration-tests/suites/tracing/httpIntegration/test.ts 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 c05849f443ce..e590520838c2 100644 --- a/dev-packages/node-integration-tests/suites/express/tracing/test.ts +++ b/dev-packages/node-integration-tests/suites/express/tracing/test.ts @@ -1,6 +1,6 @@ import { cleanupChildProcesses, createRunner } from '../../../utils/runner'; -describe('express tracing experimental', () => { +describe('express tracing', () => { afterAll(() => { cleanupChildProcesses(); }); diff --git a/dev-packages/node-integration-tests/suites/tracing/httpIntegration/server-experimental.js b/dev-packages/node-integration-tests/suites/tracing/httpIntegration/server-experimental.js new file mode 100644 index 000000000000..9b4e62766f4e --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/httpIntegration/server-experimental.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', + // disable attaching headers to /test/* endpoints + tracePropagationTargets: [/^(?!.*test).*$/], + tracesSampleRate: 1.0, + transport: loggingTransport, + + integrations: [ + Sentry.httpIntegration({ + instrumentation: { + _experimentalConfig: { + serverName: 'sentry-test-server-name', + }, + }, + }), + ], +}); + +// 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' }); +}); + +Sentry.setupExpressErrorHandler(app); + +startExpressServerAndSendPortToRunner(app); diff --git a/dev-packages/node-integration-tests/suites/tracing/httpIntegration/server.js b/dev-packages/node-integration-tests/suites/tracing/httpIntegration/server.js new file mode 100644 index 000000000000..d10c24db500d --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/httpIntegration/server.js @@ -0,0 +1,58 @@ +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', + // disable attaching headers to /test/* endpoints + tracePropagationTargets: [/^(?!.*test).*$/], + tracesSampleRate: 1.0, + transport: loggingTransport, + + integrations: [ + Sentry.httpIntegration({ + instrumentation: { + requestHook: (span, req) => { + span.setAttribute('attr1', 'yes'); + Sentry.setExtra('requestHookCalled', { + url: req.url, + method: req.method, + }); + }, + responseHook: (span, res) => { + span.setAttribute('attr2', 'yes'); + Sentry.setExtra('responseHookCalled', { + url: res.req.url, + method: res.req.method, + }); + }, + applyCustomAttributesOnSpan: (span, req, res) => { + span.setAttribute('attr3', 'yes'); + Sentry.setExtra('applyCustomAttributesOnSpanCalled', { + reqUrl: req.url, + reqMethod: req.method, + resUrl: res.req.url, + resMethod: res.req.method, + }); + }, + }, + }), + ], +}); + +// 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' }); +}); + +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 new file mode 100644 index 000000000000..f8a3f22b6ed0 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/httpIntegration/test.ts @@ -0,0 +1,73 @@ +import { cleanupChildProcesses, createRunner } from '../../../utils/runner'; + +describe('httpIntegration', () => { + afterAll(() => { + cleanupChildProcesses(); + }); + + test('allows to pass instrumentation options to integration', done => { + createRunner(__dirname, 'server.js') + .ignore('session', 'sessions') + .expect({ + transaction: { + contexts: { + trace: { + span_id: expect.any(String), + trace_id: expect.any(String), + data: { + url: expect.stringMatching(/\/test$/), + 'http.response.status_code': 200, + attr1: 'yes', + attr2: 'yes', + attr3: 'yes', + }, + op: 'http.server', + status: 'ok', + }, + }, + extra: { + requestHookCalled: { + url: expect.stringMatching(/\/test$/), + method: 'GET', + }, + responseHookCalled: { + url: expect.stringMatching(/\/test$/), + method: 'GET', + }, + applyCustomAttributesOnSpanCalled: { + reqUrl: expect.stringMatching(/\/test$/), + reqMethod: 'GET', + resUrl: expect.stringMatching(/\/test$/), + resMethod: 'GET', + }, + }, + }, + }) + .start(done) + .makeRequest('get', '/test'); + }); + + test('allows to pass experimental config through to integration', done => { + createRunner(__dirname, 'server-experimental.js') + .ignore('session', 'sessions') + .expect({ + transaction: { + contexts: { + trace: { + span_id: expect.any(String), + trace_id: expect.any(String), + data: { + url: expect.stringMatching(/\/test$/), + 'http.response.status_code': 200, + 'http.server_name': 'sentry-test-server-name', + }, + op: 'http.server', + status: 'ok', + }, + }, + }, + }) + .start(done) + .makeRequest('get', '/test'); + }); +}); diff --git a/packages/nextjs/src/server/httpIntegration.ts b/packages/nextjs/src/server/httpIntegration.ts index 4fdc615deb92..49f7318826c9 100644 --- a/packages/nextjs/src/server/httpIntegration.ts +++ b/packages/nextjs/src/server/httpIntegration.ts @@ -22,19 +22,7 @@ class CustomNextjsHttpIntegration extends HttpInstrumentation { } } -interface HttpOptions { - /** - * Whether breadcrumbs should be recorded for requests. - * Defaults to true - */ - breadcrumbs?: boolean; - - /** - * 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. - */ - ignoreOutgoingRequests?: (url: string) => boolean; -} +type HttpOptions = Parameters[0]; /** * The http integration instruments Node's internal http and https modules. diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index c135c32816a2..418fa8aa7853 100644 --- a/packages/node/src/integrations/http.ts +++ b/packages/node/src/integrations/http.ts @@ -43,6 +43,25 @@ interface HttpOptions { */ ignoreIncomingRequests?: (url: string) => boolean; + /** + * Additional instrumentation options that are passed to the underlying HttpInstrumentation. + */ + instrumentation?: { + requestHook?: (span: Span, req: ClientRequest | HTTPModuleRequestIncomingMessage) => void; + responseHook?: (span: Span, response: HTTPModuleRequestIncomingMessage | ServerResponse) => void; + applyCustomAttributesOnSpan?: ( + span: Span, + request: ClientRequest | HTTPModuleRequestIncomingMessage, + response: HTTPModuleRequestIncomingMessage | ServerResponse, + ) => void; + + /** + * You can pass any configuration through to the underlying instrumention. + * Note that there are no semver guarantees for this! + */ + _experimentalConfig?: ConstructorParameters[0]; + }; + /** Allows to pass a custom version of HttpInstrumentation. We use this for Next.js. */ _instrumentation?: typeof HttpInstrumentation; } @@ -63,6 +82,7 @@ export const instrumentHttp = Object.assign( const _InstrumentationClass = _httpOptions._instrumentation || HttpInstrumentation; _httpInstrumentation = new _InstrumentationClass({ + ..._httpOptions.instrumentation?._experimentalConfig, ignoreOutgoingRequestHook: request => { const url = getRequestUrl(request); @@ -107,6 +127,7 @@ export const instrumentHttp = Object.assign( // both, incoming requests and "client" requests made within the app trigger the requestHook // we only want to isolate and further annotate incoming requests (IncomingMessage) if (_isClientRequest(req)) { + _httpOptions.instrumentation?.requestHook?.(span, req); return; } @@ -134,17 +155,21 @@ export const instrumentHttp = Object.assign( const bestEffortTransactionName = `${httpMethod} ${httpTarget}`; isolationScope.setTransactionName(bestEffortTransactionName); + + _httpOptions.instrumentation?.requestHook?.(span, req); }, - responseHook: () => { + responseHook: (span, res) => { const client = getClient(); if (client && client.getOptions().autoSessionTracking) { setImmediate(() => { client['_captureRequestSession'](); }); } + + _httpOptions.instrumentation?.responseHook?.(span, res); }, applyCustomAttributesOnSpan: ( - _span: Span, + span: Span, request: ClientRequest | HTTPModuleRequestIncomingMessage, response: HTTPModuleRequestIncomingMessage | ServerResponse, ) => { @@ -152,6 +177,8 @@ export const instrumentHttp = Object.assign( if (_breadcrumbs) { _addRequestBreadcrumb(request, response); } + + _httpOptions.instrumentation?.applyCustomAttributesOnSpan?.(span, request, response); }, }); diff --git a/packages/remix/src/utils/integrations/http.ts b/packages/remix/src/utils/integrations/http.ts index 7c4b80f44fe7..d3a7ae03e351 100644 --- a/packages/remix/src/utils/integrations/http.ts +++ b/packages/remix/src/utils/integrations/http.ts @@ -16,19 +16,7 @@ class RemixHttpIntegration extends HttpInstrumentation { } } -interface HttpOptions { - /** - * Whether breadcrumbs should be recorded for requests. - * Defaults to true - */ - breadcrumbs?: boolean; - - /** - * 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. - */ - ignoreOutgoingRequests?: (url: string) => boolean; -} +type HttpOptions = Parameters[0]; /** * The http integration instruments Node's internal http and https modules.