From 29e5f2243a06fdf30c2b7da8fb3aafbd2e675382 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Wed, 26 Oct 2022 18:14:57 +0200 Subject: [PATCH 1/5] Add `shouldCreateSpanForRequest` --- packages/node/src/integrations/http.ts | 25 +++++++++++++++---------- packages/node/src/types.ts | 6 ++++++ 2 files changed, 21 insertions(+), 10 deletions(-) diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index 151fa384b9f9..171fee9c4b7b 100644 --- a/packages/node/src/integrations/http.ts +++ b/packages/node/src/integrations/http.ts @@ -1,5 +1,5 @@ import { getCurrentHub, Hub } from '@sentry/core'; -import { EventProcessor, Integration, Span, TracePropagationTargets } from '@sentry/types'; +import { EventProcessor, Integration, Span } from '@sentry/types'; import { dynamicSamplingContextToSentryBaggageHeader, fill, @@ -69,11 +69,7 @@ export class Http implements Integration { const clientOptions = setupOnceGetCurrentHub().getClient()?.getOptions() as NodeClientOptions | undefined; - const wrappedHandlerMaker = _createWrappedRequestMethodFactory( - this._breadcrumbs, - this._tracing, - clientOptions?.tracePropagationTargets, - ); + const wrappedHandlerMaker = _createWrappedRequestMethodFactory(this._breadcrumbs, this._tracing, clientOptions); // eslint-disable-next-line @typescript-eslint/no-var-requires const httpModule = require('http'); @@ -109,12 +105,21 @@ type WrappedRequestMethodFactory = (original: OriginalRequestMethod) => WrappedR function _createWrappedRequestMethodFactory( breadcrumbsEnabled: boolean, tracingEnabled: boolean, - tracePropagationTargets: TracePropagationTargets | undefined, + options: NodeClientOptions | undefined, ): WrappedRequestMethodFactory { // We're caching results so we dont have to recompute regexp everytime we create a request. const urlMap: Record = {}; + + const shouldCreateSpan = (url: string): boolean => { + if (options?.shouldCreateSpanForRequest === undefined) { + return true; + } + + return options.shouldCreateSpanForRequest(url); + }; + const shouldAttachTraceData = (url: string): boolean => { - if (tracePropagationTargets === undefined) { + if (options?.tracePropagationTargets === undefined) { return true; } @@ -122,7 +127,7 @@ function _createWrappedRequestMethodFactory( return urlMap[url]; } - urlMap[url] = tracePropagationTargets.some(tracePropagationTarget => + urlMap[url] = options.tracePropagationTargets.some(tracePropagationTarget => isMatchingPattern(url, tracePropagationTarget), ); @@ -148,7 +153,7 @@ function _createWrappedRequestMethodFactory( const scope = getCurrentHub().getScope(); - if (scope && tracingEnabled) { + if (scope && tracingEnabled && shouldCreateSpan(requestUrl)) { parentSpan = scope.getSpan(); if (parentSpan) { diff --git a/packages/node/src/types.ts b/packages/node/src/types.ts index 4d890d8e10b8..a45e8ae2de13 100644 --- a/packages/node/src/types.ts +++ b/packages/node/src/types.ts @@ -19,6 +19,12 @@ export interface BaseNodeOptions { */ tracePropagationTargets?: TracePropagationTargets; + /** + * This function will be called before creating a span for a request with the given url. + * Return false if you don't want a span for the given url. + */ + shouldCreateSpanForRequest?(url: string): boolean; + /** Callback that is executed when a fatal global error occurs. */ onFatalError?(error: Error): void; } From 5525f152db4bac11c7c77dcea03becc0739dc233 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Mon, 31 Oct 2022 13:52:45 +0100 Subject: [PATCH 2/5] Add test --- packages/node/test/integrations/http.test.ts | 28 ++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/packages/node/test/integrations/http.test.ts b/packages/node/test/integrations/http.test.ts index 94bc4edf6c30..4f5205f0b596 100644 --- a/packages/node/test/integrations/http.test.ts +++ b/packages/node/test/integrations/http.test.ts @@ -221,8 +221,36 @@ describe('tracing', () => { addExtensionMethods(); const transaction = hub.startTransaction({ name: 'dogpark' }); hub.getScope()?.setSpan(transaction); + return transaction; } + it("doesn't create span if shouldCreateSpanForRequest returns false", () => { + const url = 'http://dogs.are.great/api/v1/index/'; + nock(url).get(/.*/).reply(200); + + const httpIntegration = new HttpIntegration({ tracing: true }); + + const hub = createHub({ shouldCreateSpanForRequest: () => false }); + + httpIntegration.setupOnce( + () => undefined, + () => hub, + ); + + const transaction = createTransactionAndPutOnScope(hub); + const spans = (transaction as unknown as Span).spanRecorder?.spans as Span[]; + + const request = http.get(url); + + // There should be no http spans + const httpSpans = spans.filter(span => span.op?.startsWith('http')); + expect(httpSpans.length).toBe(0); + + // And headers are not attached without span creation + expect(request.getHeader('sentry-trace')).toBeUndefined(); + expect(request.getHeader('baggage')).toBeUndefined(); + }); + it.each([ ['http://dogs.are.great/api/v1/index/', [/.*/]], ['http://dogs.are.great/api/v1/index/', [/\/api/]], From 098487f4a78df34b44e3b1f3fa9860a1433462e5 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Mon, 31 Oct 2022 13:53:21 +0100 Subject: [PATCH 3/5] Update jsdoc from PR review Co-authored-by: Katie Byers --- packages/node/src/types.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/node/src/types.ts b/packages/node/src/types.ts index a45e8ae2de13..59c4c9af7ec7 100644 --- a/packages/node/src/types.ts +++ b/packages/node/src/types.ts @@ -20,8 +20,8 @@ export interface BaseNodeOptions { tracePropagationTargets?: TracePropagationTargets; /** - * This function will be called before creating a span for a request with the given url. - * Return false if you don't want a span for the given url. + * Function determining whether or not to create spans to track outgoing requests to the given URL. + * By default, spans will be created for all outgoing requests. */ shouldCreateSpanForRequest?(url: string): boolean; From 6895e31abf32bf7ef432da5b28bae4a675d608d5 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Mon, 31 Oct 2022 14:21:22 +0100 Subject: [PATCH 4/5] Cache results from `shouldCreateSpanForRequest` --- packages/node/src/integrations/http.ts | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index 171fee9c4b7b..dec726200fa1 100644 --- a/packages/node/src/integrations/http.ts +++ b/packages/node/src/integrations/http.ts @@ -107,15 +107,22 @@ function _createWrappedRequestMethodFactory( tracingEnabled: boolean, options: NodeClientOptions | undefined, ): WrappedRequestMethodFactory { - // We're caching results so we dont have to recompute regexp everytime we create a request. - const urlMap: Record = {}; + // We're caching results so we don't have to recompute regexp every time we create a request. + const createSpanUrlMap: Record = {}; + const headersUrlMap: Record = {}; const shouldCreateSpan = (url: string): boolean => { if (options?.shouldCreateSpanForRequest === undefined) { return true; } - return options.shouldCreateSpanForRequest(url); + if (createSpanUrlMap[url]) { + return createSpanUrlMap[url]; + } + + createSpanUrlMap[url] = options.shouldCreateSpanForRequest(url); + + return createSpanUrlMap[url]; }; const shouldAttachTraceData = (url: string): boolean => { @@ -123,15 +130,15 @@ function _createWrappedRequestMethodFactory( return true; } - if (urlMap[url]) { - return urlMap[url]; + if (headersUrlMap[url]) { + return headersUrlMap[url]; } - urlMap[url] = options.tracePropagationTargets.some(tracePropagationTarget => + headersUrlMap[url] = options.tracePropagationTargets.some(tracePropagationTarget => isMatchingPattern(url, tracePropagationTarget), ); - return urlMap[url]; + return headersUrlMap[url]; }; return function wrappedRequestMethodFactory(originalRequestMethod: OriginalRequestMethod): WrappedRequestMethod { From 3393293b485cc493f70b0e13daf29ea680df7c1b Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Mon, 31 Oct 2022 14:25:58 +0100 Subject: [PATCH 5/5] Use `NodeClient` so options are typed --- packages/node/src/integrations/http.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index dec726200fa1..96f6c3f9dbd7 100644 --- a/packages/node/src/integrations/http.ts +++ b/packages/node/src/integrations/http.ts @@ -10,6 +10,7 @@ import { import * as http from 'http'; import * as https from 'https'; +import { NodeClient } from '../client'; import { NodeClientOptions } from '../types'; import { cleanSpanDescription, @@ -67,8 +68,7 @@ export class Http implements Integration { return; } - const clientOptions = setupOnceGetCurrentHub().getClient()?.getOptions() as NodeClientOptions | undefined; - + const clientOptions = setupOnceGetCurrentHub().getClient()?.getOptions(); const wrappedHandlerMaker = _createWrappedRequestMethodFactory(this._breadcrumbs, this._tracing, clientOptions); // eslint-disable-next-line @typescript-eslint/no-var-requires