From 6ee370ec35c3eeb10f84a0c91534fc8187f23695 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 28 Jul 2022 07:47:25 -0700 Subject: [PATCH 1/4] only start transactions if tracing is enabled --- packages/node/src/handlers.ts | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts index eedee94abf0f..74f96cb498c3 100644 --- a/packages/node/src/handlers.ts +++ b/packages/node/src/handlers.ts @@ -33,6 +33,24 @@ export function tracingHandler(): ( res: http.ServerResponse, next: (error?: any) => void, ): void { + const hub = getCurrentHub(); + const options = hub.getClient()?.getOptions(); + + if (!options) { + return next(); + } + + // TODO: This is the `hasTracingEnabled` check, but we're doing it manually snice `@sentry/tracing` isn't a + // dependency of `@sentry/node`. Long term, that function should probably move to `@sentry/hub. + if (!('tracesSampleRate' in options) && !('tracesSampler' in options)) { + __DEBUG_BUILD__ && + logger.warn( + 'Sentry `tracingHandler` is being used, but tracing is disabled. Please enable tracing by setting ' + + 'either `tracesSampleRate` or `tracesSampler` in your `Sentry.init()` options.', + ); + return next(); + } + // If there is a trace header set, we extract the data from it (parentSpanId, traceId, and sampling decision) const traceparentData = req.headers && isString(req.headers['sentry-trace']) && extractTraceparentData(req.headers['sentry-trace']); @@ -53,7 +71,7 @@ export function tracingHandler(): ( ); // We put the transaction on the scope so users can attach children to it - getCurrentHub().configureScope(scope => { + hub.configureScope(scope => { scope.setSpan(transaction); }); From e3e7b7623f3fc6957f43871dc5f76184c7c072a2 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 28 Jul 2022 07:49:06 -0700 Subject: [PATCH 2/4] don't trace `HEAD` or `OPTIONS` requests --- packages/node/src/handlers.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts index 74f96cb498c3..f1e2b1d8dfc3 100644 --- a/packages/node/src/handlers.ts +++ b/packages/node/src/handlers.ts @@ -36,7 +36,7 @@ export function tracingHandler(): ( const hub = getCurrentHub(); const options = hub.getClient()?.getOptions(); - if (!options) { + if (!options || req.method?.toUpperCase() === 'OPTIONS' || req.method?.toUpperCase() === 'HEAD') { return next(); } From c3ffd384781ed3a6a484bf4314b315b22b99effb Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Thu, 28 Jul 2022 08:01:31 -0700 Subject: [PATCH 3/4] add tests --- packages/node/test/handlers.test.ts | 31 ++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/packages/node/test/handlers.test.ts b/packages/node/test/handlers.test.ts index e06c1846d0c2..10d2181e3549 100644 --- a/packages/node/test/handlers.test.ts +++ b/packages/node/test/handlers.test.ts @@ -150,7 +150,7 @@ describe('tracingHandler', () => { const sentryTracingMiddleware = tracingHandler(); - let req: http.IncomingMessage, res: http.ServerResponse, next: () => undefined; + let hub: Hub, req: http.IncomingMessage, res: http.ServerResponse, next: () => undefined; function createNoOpSpy() { const noop = { noop: () => undefined }; // this is wrapped in an object so jest can spy on it @@ -158,6 +158,8 @@ describe('tracingHandler', () => { } beforeEach(() => { + hub = new Hub(new NodeClient(getDefaultNodeClientOptions({ tracesSampleRate: 1.0 }))); + jest.spyOn(sentryCore, 'getCurrentHub').mockReturnValue(hub); req = { headers, method, @@ -181,6 +183,33 @@ describe('tracingHandler', () => { expect(startTransaction).toHaveBeenCalled(); }); + it("doesn't create a transaction when handling a `HEAD` request", () => { + const startTransaction = jest.spyOn(sentryCore, 'startTransaction'); + req.method = 'HEAD'; + + sentryTracingMiddleware(req, res, next); + + expect(startTransaction).not.toHaveBeenCalled(); + }); + + it("doesn't create a transaction when handling an `OPTIONS` request", () => { + const startTransaction = jest.spyOn(sentryCore, 'startTransaction'); + req.method = 'OPTIONS'; + + sentryTracingMiddleware(req, res, next); + + expect(startTransaction).not.toHaveBeenCalled(); + }); + + it("doesn't create a transaction if tracing is disabled", () => { + delete hub.getClient()?.getOptions().tracesSampleRate; + const startTransaction = jest.spyOn(sentryCore, 'startTransaction'); + + sentryTracingMiddleware(req, res, next); + + expect(startTransaction).not.toHaveBeenCalled(); + }); + it("pulls parent's data from tracing header on the request", () => { req.headers = { 'sentry-trace': '12312012123120121231201212312012-1121201211212012-0' }; From 2af59d2d4f9abe7c1ac3bdfdfca908d7c584d922 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 28 Jul 2022 18:18:46 +0200 Subject: [PATCH 4/4] Update packages/node/src/handlers.ts --- packages/node/src/handlers.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts index f1e2b1d8dfc3..a5ed4b6b35b4 100644 --- a/packages/node/src/handlers.ts +++ b/packages/node/src/handlers.ts @@ -40,7 +40,7 @@ export function tracingHandler(): ( return next(); } - // TODO: This is the `hasTracingEnabled` check, but we're doing it manually snice `@sentry/tracing` isn't a + // TODO: This is the `hasTracingEnabled` check, but we're doing it manually since `@sentry/tracing` isn't a // dependency of `@sentry/node`. Long term, that function should probably move to `@sentry/hub. if (!('tracesSampleRate' in options) && !('tracesSampler' in options)) { __DEBUG_BUILD__ &&