From 105dcf17867069dfa89ecfdf97a6b766e8326aa0 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 5 Apr 2023 13:44:03 +0200 Subject: [PATCH] feat(sveltekit): Add partial instrumentation for client-side fetch (#7626) Add partial instrumentation to the client-side `fetch` passed to the universal `load` functions. It enables distributed traces of fetch calls happening **inside** a `load` function. Limitation: `fetch` requests made by SvelteKit (e.g. to call server-only load functions) are **not** touched by this instrumentation because we cannot access the Kit-internal fetch function at this time --- packages/node/src/integrations/http.ts | 17 +- packages/sveltekit/src/client/load.ts | 204 ++++++++++- packages/sveltekit/test/client/load.test.ts | 344 ++++++++++++++++-- .../tracing-internal/src/browser/index.ts | 6 +- .../tracing-internal/src/browser/request.ts | 15 +- packages/tracing-internal/src/index.ts | 1 + packages/types/src/index.ts | 2 +- packages/types/src/request.ts | 12 + packages/utils/src/instrument.ts | 5 +- packages/utils/src/url.ts | 38 +- packages/utils/test/url.test.ts | 50 ++- 11 files changed, 631 insertions(+), 63 deletions(-) diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index 058393bc0767..c7239d924af3 100644 --- a/packages/node/src/integrations/http.ts +++ b/packages/node/src/integrations/http.ts @@ -1,6 +1,6 @@ import type { Hub } from '@sentry/core'; import { getCurrentHub } from '@sentry/core'; -import type { EventProcessor, Integration, Span, TracePropagationTargets } from '@sentry/types'; +import type { EventProcessor, Integration, SanitizedRequestData, Span, TracePropagationTargets } from '@sentry/types'; import { dynamicSamplingContextToSentryBaggageHeader, fill, logger, stringMatchesSomePattern } from '@sentry/utils'; import type * as http from 'http'; import type * as https from 'https'; @@ -122,16 +122,6 @@ type OriginalRequestMethod = RequestMethod; type WrappedRequestMethod = RequestMethod; type WrappedRequestMethodFactory = (original: OriginalRequestMethod) => WrappedRequestMethod; -/** - * See https://develop.sentry.dev/sdk/data-handling/#structuring-data - */ -type RequestSpanData = { - url: string; - method: string; - 'http.fragment'?: string; - 'http.query'?: string; -}; - /** * Function which creates a function which creates wrapped versions of internal `request` and `get` calls within `http` * and `https` modules. (NB: Not a typo - this is a creator^2!) @@ -197,7 +187,7 @@ function _createWrappedRequestMethodFactory( const scope = getCurrentHub().getScope(); - const requestSpanData: RequestSpanData = { + const requestSpanData: SanitizedRequestData = { url: requestUrl, method: requestOptions.method || 'GET', }; @@ -304,7 +294,7 @@ function _createWrappedRequestMethodFactory( */ function addRequestBreadcrumb( event: string, - requestSpanData: RequestSpanData, + requestSpanData: SanitizedRequestData, req: http.ClientRequest, res?: http.IncomingMessage, ): void { @@ -316,7 +306,6 @@ function addRequestBreadcrumb( { category: 'http', data: { - method: req.method, status_code: res && res.statusCode, ...requestSpanData, }, diff --git a/packages/sveltekit/src/client/load.ts b/packages/sveltekit/src/client/load.ts index a154816b3dba..6db4dae09840 100644 --- a/packages/sveltekit/src/client/load.ts +++ b/packages/sveltekit/src/client/load.ts @@ -1,6 +1,17 @@ -import { trace } from '@sentry/core'; +import { addTracingHeadersToFetchRequest } from '@sentry-internal/tracing'; +import type { BaseClient } from '@sentry/core'; +import { getCurrentHub, trace } from '@sentry/core'; +import type { Breadcrumbs, BrowserTracing } from '@sentry/svelte'; import { captureException } from '@sentry/svelte'; -import { addExceptionMechanism, objectify } from '@sentry/utils'; +import type { ClientOptions, SanitizedRequestData } from '@sentry/types'; +import { + addExceptionMechanism, + getSanitizedUrlString, + objectify, + parseFetchArgs, + parseUrl, + stringMatchesSomePattern, +} from '@sentry/utils'; import type { LoadEvent } from '@sveltejs/kit'; import { isRedirect } from '../common/utils'; @@ -34,7 +45,17 @@ function sendErrorToSentry(e: unknown): unknown { } /** - * @inheritdoc + * Wrap load function with Sentry. This wrapper will + * + * - catch errors happening during the execution of `load` + * - create a load span if performance monitoring is enabled + * - attach tracing Http headers to `fech` requests if performance monitoring is enabled to get connected traces. + * - add a fetch breadcrumb for every `fetch` request + * + * Note that tracing Http headers are only attached if the url matches the specified `tracePropagationTargets` + * entries to avoid CORS errors. + * + * @param origLoad SvelteKit user defined load function */ // The liberal generic typing of `T` is necessary because we cannot let T extend `Load`. // This function needs to tell TS that it returns exactly the type that it was called with @@ -47,6 +68,11 @@ export function wrapLoadWithSentry any>(origLoad: T) // Type casting here because `T` cannot extend `Load` (see comment above function signature) const event = args[0] as LoadEvent; + const patchedEvent = { + ...event, + fetch: instrumentSvelteKitFetch(event.fetch), + }; + const routeId = event.route.id; return trace( { @@ -57,9 +83,179 @@ export function wrapLoadWithSentry any>(origLoad: T) source: routeId ? 'route' : 'url', }, }, - () => wrappingTarget.apply(thisArg, args), + () => wrappingTarget.apply(thisArg, [patchedEvent]), sendErrorToSentry, ); }, }); } + +type SvelteKitFetch = LoadEvent['fetch']; + +/** + * Instruments SvelteKit's client `fetch` implementation which is passed to the client-side universal `load` functions. + * + * We need to instrument this in addition to the native fetch we instrument in BrowserTracing because SvelteKit + * stores the native fetch implementation before our SDK is initialized. + * + * see: https://github.com/sveltejs/kit/blob/master/packages/kit/src/runtime/client/fetcher.js + * + * This instrumentation takes the fetch-related options from `BrowserTracing` to determine if we should + * instrument fetch for perfomance monitoring, create a span for or attach our tracing headers to the given request. + * + * To dertermine if breadcrumbs should be recorded, this instrumentation relies on the availability of and the options + * set in the `BreadCrumbs` integration. + * + * @param originalFetch SvelteKit's original fetch implemenetation + * + * @returns a proxy of SvelteKit's fetch implementation + */ +function instrumentSvelteKitFetch(originalFetch: SvelteKitFetch): SvelteKitFetch { + const client = getCurrentHub().getClient() as BaseClient; + + const browserTracingIntegration = + client.getIntegrationById && (client.getIntegrationById('BrowserTracing') as BrowserTracing | undefined); + const breadcrumbsIntegration = + client.getIntegrationById && (client.getIntegrationById('Breadcrumbs') as Breadcrumbs | undefined); + + const browserTracingOptions = browserTracingIntegration && browserTracingIntegration.options; + + const shouldTraceFetch = browserTracingOptions && browserTracingOptions.traceFetch; + const shouldAddFetchBreadcrumb = breadcrumbsIntegration && breadcrumbsIntegration.options.fetch; + + /* Identical check as in BrowserTracing, just that we also need to verify that BrowserTracing is actually installed */ + const shouldCreateSpan = + browserTracingOptions && typeof browserTracingOptions.shouldCreateSpanForRequest === 'function' + ? browserTracingOptions.shouldCreateSpanForRequest + : (_: string) => shouldTraceFetch; + + /* Identical check as in BrowserTracing, just that we also need to verify that BrowserTracing is actually installed */ + const shouldAttachHeaders: (url: string) => boolean = url => { + return ( + !!shouldTraceFetch && + stringMatchesSomePattern(url, browserTracingOptions.tracePropagationTargets || ['localhost', /^\//]) + ); + }; + + return new Proxy(originalFetch, { + apply: (wrappingTarget, thisArg, args: Parameters) => { + const [input, init] = args; + const { url: rawUrl, method } = parseFetchArgs(args); + + // TODO: extract this to a util function (and use it in breadcrumbs integration as well) + if (rawUrl.match(/sentry_key/)) { + // We don't create spans or breadcrumbs for fetch requests that contain `sentry_key` (internal sentry requests) + return wrappingTarget.apply(thisArg, args); + } + + const urlObject = parseUrl(rawUrl); + + const requestData: SanitizedRequestData = { + url: getSanitizedUrlString(urlObject), + method, + ...(urlObject.search && { 'http.query': urlObject.search.substring(1) }), + ...(urlObject.hash && { 'http.hash': urlObject.hash.substring(1) }), + }; + + const patchedInit: RequestInit = { ...init }; + const activeSpan = getCurrentHub().getScope().getSpan(); + const activeTransaction = activeSpan && activeSpan.transaction; + + const createSpan = activeTransaction && shouldCreateSpan(rawUrl); + const attachHeaders = createSpan && activeTransaction && shouldAttachHeaders(rawUrl); + + // only attach headers if we should create a span + if (attachHeaders) { + const dsc = activeTransaction.getDynamicSamplingContext(); + + const headers = addTracingHeadersToFetchRequest( + input as string | Request, + dsc, + activeSpan, + patchedInit as { + headers: + | { + [key: string]: string[] | string | undefined; + } + | Request['headers']; + }, + ) as HeadersInit; + + patchedInit.headers = headers; + } + let fetchPromise: Promise; + + const patchedFetchArgs = [input, patchedInit]; + + if (createSpan) { + fetchPromise = trace( + { + name: `${requestData.method} ${requestData.url}`, // this will become the description of the span + op: 'http.client', + data: requestData, + }, + span => { + const promise: Promise = wrappingTarget.apply(thisArg, patchedFetchArgs); + if (span) { + promise.then(res => span.setHttpStatus(res.status)).catch(_ => span.setStatus('internal_error')); + } + return promise; + }, + ); + } else { + fetchPromise = wrappingTarget.apply(thisArg, patchedFetchArgs); + } + + if (shouldAddFetchBreadcrumb) { + addFetchBreadcrumb(fetchPromise, requestData, args); + } + + return fetchPromise; + }, + }); +} + +/* Adds a breadcrumb for the given fetch result */ +function addFetchBreadcrumb( + fetchResult: Promise, + requestData: SanitizedRequestData, + args: Parameters, +): void { + const breadcrumbStartTimestamp = Date.now(); + fetchResult.then( + response => { + getCurrentHub().addBreadcrumb( + { + type: 'http', + category: 'fetch', + data: { + ...requestData, + status_code: response.status, + }, + }, + { + input: args, + response, + startTimestamp: breadcrumbStartTimestamp, + endTimestamp: Date.now(), + }, + ); + }, + error => { + getCurrentHub().addBreadcrumb( + { + type: 'http', + category: 'fetch', + level: 'error', + data: requestData, + }, + { + input: args, + data: error, + startTimestamp: breadcrumbStartTimestamp, + endTimestamp: Date.now(), + }, + ); + }, + ); +} diff --git a/packages/sveltekit/test/client/load.test.ts b/packages/sveltekit/test/client/load.test.ts index ed5193e81e47..22c13157e9db 100644 --- a/packages/sveltekit/test/client/load.test.ts +++ b/packages/sveltekit/test/client/load.test.ts @@ -1,10 +1,17 @@ import { addTracingExtensions, Scope } from '@sentry/svelte'; +import { baggageHeaderToDynamicSamplingContext } from '@sentry/utils'; import type { Load } from '@sveltejs/kit'; import { redirect } from '@sveltejs/kit'; import { vi } from 'vitest'; import { wrapLoadWithSentry } from '../../src/client/load'; +const SENTRY_TRACE_HEADER = '1234567890abcdef1234567890abcdef-1234567890abcdef-1'; +const BAGGAGE_HEADER = + 'sentry-environment=production,sentry-release=1.0.0,sentry-transaction=dogpark,' + + 'sentry-user_segment=segmentA,sentry-public_key=dogsarebadatkeepingsecrets,' + + 'sentry-trace_id=1234567890abcdef1234567890abcdef,sentry-sample_rate=1'; + const mockCaptureException = vi.fn(); let mockScope = new Scope(); @@ -22,6 +29,29 @@ vi.mock('@sentry/svelte', async () => { const mockTrace = vi.fn(); +const mockedBrowserTracing = { + options: { + tracePropagationTargets: ['example.com', /^\\/], + traceFetch: true, + shouldCreateSpanForRequest: undefined as undefined | (() => boolean), + }, +}; + +const mockedBreadcrumbs = { + options: { + fetch: true, + }, +}; + +const mockedGetIntegrationById = vi.fn(id => { + if (id === 'BrowserTracing') { + return mockedBrowserTracing; + } else if (id === 'Breadcrumbs') { + return mockedBreadcrumbs; + } + return undefined; +}); + vi.mock('@sentry/core', async () => { const original = (await vi.importActual('@sentry/core')) as any; return { @@ -30,10 +60,37 @@ vi.mock('@sentry/core', async () => { mockTrace(...args); return original.trace(...args); }, + getCurrentHub: () => { + return { + getClient: () => { + return { + getIntegrationById: mockedGetIntegrationById, + }; + }, + getScope: () => { + return { + getSpan: () => { + return { + transaction: { + getDynamicSamplingContext: () => { + return baggageHeaderToDynamicSamplingContext(BAGGAGE_HEADER); + }, + }, + toTraceparent: () => { + return SENTRY_TRACE_HEADER; + }, + }; + }, + }; + }, + addBreadcrumb: mockedAddBreadcrumb, + }; + }, }; }); const mockAddExceptionMechanism = vi.fn(); +const mockedAddBreadcrumb = vi.fn(); vi.mock('@sentry/utils', async () => { const original = (await vi.importActual('@sentry/utils')) as any; @@ -47,6 +104,8 @@ function getById(_id?: string) { throw new Error('error'); } +const mockedSveltekitFetch = vi.fn().mockReturnValue(Promise.resolve({ status: 200 })); + const MOCK_LOAD_ARGS: any = { params: { id: '123' }, route: { @@ -57,21 +116,18 @@ const MOCK_LOAD_ARGS: any = { headers: { get: (key: string) => { if (key === 'sentry-trace') { - return '1234567890abcdef1234567890abcdef-1234567890abcdef-1'; + return SENTRY_TRACE_HEADER; } if (key === 'baggage') { - return ( - 'sentry-environment=production,sentry-release=1.0.0,sentry-transaction=dogpark,' + - 'sentry-user_segment=segmentA,sentry-public_key=dogsarebadatkeepingsecrets,' + - 'sentry-trace_id=1234567890abcdef1234567890abcdef,sentry-sample_rate=1' - ); + return BAGGAGE_HEADER; } return null; }, }, }, + fetch: mockedSveltekitFetch, }; beforeAll(() => { @@ -83,6 +139,9 @@ describe('wrapLoadWithSentry', () => { mockCaptureException.mockClear(); mockAddExceptionMechanism.mockClear(); mockTrace.mockClear(); + mockedGetIntegrationById.mockClear(); + mockedSveltekitFetch.mockClear(); + mockedAddBreadcrumb.mockClear(); mockScope = new Scope(); }); @@ -112,29 +171,260 @@ describe('wrapLoadWithSentry', () => { expect(mockCaptureException).not.toHaveBeenCalled(); }); - it('calls trace function', async () => { - async function load({ params }: Parameters[0]): Promise> { - return { - post: params.id, - }; - } + describe('calls trace function', async () => { + it('creates a load span', async () => { + async function load({ params }: Parameters[0]): Promise> { + return { + post: params.id, + }; + } - const wrappedLoad = wrapLoadWithSentry(load); - await wrappedLoad(MOCK_LOAD_ARGS); - - expect(mockTrace).toHaveBeenCalledTimes(1); - expect(mockTrace).toHaveBeenCalledWith( - { - op: 'function.sveltekit.load', - name: '/users/[id]', - status: 'ok', - metadata: { - source: 'route', + const wrappedLoad = wrapLoadWithSentry(load); + await wrappedLoad(MOCK_LOAD_ARGS); + + expect(mockTrace).toHaveBeenCalledTimes(1); + expect(mockTrace).toHaveBeenCalledWith( + { + op: 'function.sveltekit.load', + name: '/users/[id]', + status: 'ok', + metadata: { + source: 'route', + }, }, - }, - expect.any(Function), - expect.any(Function), - ); + expect.any(Function), + expect.any(Function), + ); + }); + + describe.each([ + [ + 'fetch call with fragment and params', + ['example.com/api/users/?id=123#testfragment'], + { + op: 'http.client', + name: 'GET example.com/api/users/', + data: { + method: 'GET', + url: 'example.com/api/users/', + 'http.hash': 'testfragment', + 'http.query': 'id=123', + }, + }, + ], + [ + 'fetch call with options object', + ['example.com/api/users/?id=123#testfragment', { method: 'POST' }], + { + op: 'http.client', + name: 'POST example.com/api/users/', + data: { + method: 'POST', + url: 'example.com/api/users/', + 'http.hash': 'testfragment', + 'http.query': 'id=123', + }, + }, + ], + [ + 'fetch call with custom headers in options ', + ['example.com/api/users/?id=123#testfragment', { method: 'POST', headers: { 'x-my-header': 'some value' } }], + { + op: 'http.client', + name: 'POST example.com/api/users/', + data: { + method: 'POST', + url: 'example.com/api/users/', + 'http.hash': 'testfragment', + 'http.query': 'id=123', + }, + }, + ], + [ + 'fetch call with a Request object ', + [{ url: '/api/users?id=123', headers: { 'x-my-header': 'value' } } as unknown as Request], + { + op: 'http.client', + name: 'GET /api/users', + data: { + method: 'GET', + url: '/api/users', + 'http.query': 'id=123', + }, + }, + ], + ])('instruments fetch (%s)', (_, originalFetchArgs, spanCtx) => { + beforeEach(() => { + mockedBrowserTracing.options = { + tracePropagationTargets: ['example.com', /^\//], + traceFetch: true, + shouldCreateSpanForRequest: undefined, + }; + }); + + const load = async ({ params, fetch }) => { + await fetch(...originalFetchArgs); + return { + post: params.id, + }; + }; + + it('creates a fetch span and attaches tracing headers by default when event.fetch was called', async () => { + const wrappedLoad = wrapLoadWithSentry(load); + await wrappedLoad(MOCK_LOAD_ARGS); + + expect(mockTrace).toHaveBeenCalledTimes(2); + expect(mockTrace).toHaveBeenNthCalledWith( + 1, + { + op: 'function.sveltekit.load', + name: '/users/[id]', + status: 'ok', + metadata: { + source: 'route', + }, + }, + expect.any(Function), + expect.any(Function), + ); + expect(mockTrace).toHaveBeenNthCalledWith(2, spanCtx, expect.any(Function)); + + const hasSecondArg = originalFetchArgs.length > 1; + const expectedFetchArgs = [ + originalFetchArgs[0], + { + ...(hasSecondArg && (originalFetchArgs[1] as RequestInit)), + headers: { + // @ts-ignore that's fine + ...(hasSecondArg && (originalFetchArgs[1].headers as RequestInit['headers'])), + baggage: expect.any(String), + 'sentry-trace': expect.any(String), + }, + }, + ]; + + expect(mockedSveltekitFetch).toHaveBeenCalledWith(...expectedFetchArgs); + }); + + it("only creates a span but doesn't propagate headers if traceProgagationTargets don't match", async () => { + const previousPropagationTargets = mockedBrowserTracing.options.tracePropagationTargets; + mockedBrowserTracing.options.tracePropagationTargets = []; + + const wrappedLoad = wrapLoadWithSentry(load); + await wrappedLoad(MOCK_LOAD_ARGS); + + expect(mockTrace).toHaveBeenCalledTimes(2); + expect(mockTrace).toHaveBeenNthCalledWith( + 1, + { + op: 'function.sveltekit.load', + name: '/users/[id]', + status: 'ok', + metadata: { + source: 'route', + }, + }, + expect.any(Function), + expect.any(Function), + ); + expect(mockTrace).toHaveBeenNthCalledWith(2, spanCtx, expect.any(Function)); + + expect(mockedSveltekitFetch).toHaveBeenCalledWith( + ...[originalFetchArgs[0], originalFetchArgs.length === 2 ? originalFetchArgs[1] : {}], + ); + + mockedBrowserTracing.options.tracePropagationTargets = previousPropagationTargets; + }); + + it("doesn't create a span nor propagate headers, if `Browsertracing.options.traceFetch` is false", async () => { + mockedBrowserTracing.options.traceFetch = false; + + const wrappedLoad = wrapLoadWithSentry(load); + await wrappedLoad(MOCK_LOAD_ARGS); + + expect(mockTrace).toHaveBeenCalledTimes(1); + expect(mockTrace).toHaveBeenCalledWith( + { + op: 'function.sveltekit.load', + name: '/users/[id]', + status: 'ok', + metadata: { + source: 'route', + }, + }, + expect.any(Function), + expect.any(Function), + ); + + expect(mockedSveltekitFetch).toHaveBeenCalledWith( + ...[originalFetchArgs[0], originalFetchArgs.length === 2 ? originalFetchArgs[1] : {}], + ); + + mockedBrowserTracing.options.traceFetch = true; + }); + + it("doesn't create a span nor propagate headers, if `shouldCreateSpanForRequest` returns false", async () => { + mockedBrowserTracing.options.shouldCreateSpanForRequest = () => false; + + const wrappedLoad = wrapLoadWithSentry(load); + await wrappedLoad(MOCK_LOAD_ARGS); + + expect(mockTrace).toHaveBeenCalledTimes(1); + expect(mockTrace).toHaveBeenCalledWith( + { + op: 'function.sveltekit.load', + name: '/users/[id]', + status: 'ok', + metadata: { + source: 'route', + }, + }, + expect.any(Function), + expect.any(Function), + ); + + expect(mockedSveltekitFetch).toHaveBeenCalledWith( + ...[originalFetchArgs[0], originalFetchArgs.length === 2 ? originalFetchArgs[1] : {}], + ); + + mockedBrowserTracing.options.shouldCreateSpanForRequest = () => true; + }); + + it('adds a breadcrumb for the fetch call', async () => { + const wrappedLoad = wrapLoadWithSentry(load); + await wrappedLoad(MOCK_LOAD_ARGS); + + expect(mockedAddBreadcrumb).toHaveBeenCalledWith( + { + category: 'fetch', + data: { + ...spanCtx.data, + status_code: 200, + }, + type: 'http', + }, + { + endTimestamp: expect.any(Number), + input: [...originalFetchArgs], + response: { + status: 200, + }, + startTimestamp: expect.any(Number), + }, + ); + }); + + it("doesn't add a breadcrumb if fetch breadcrumbs are deactivated in the integration", async () => { + mockedBreadcrumbs.options.fetch = false; + + const wrappedLoad = wrapLoadWithSentry(load); + await wrappedLoad(MOCK_LOAD_ARGS); + + expect(mockedAddBreadcrumb).not.toHaveBeenCalled(); + + mockedBreadcrumbs.options.fetch = true; + }); + }); }); it('adds an exception mechanism', async () => { diff --git a/packages/tracing-internal/src/browser/index.ts b/packages/tracing-internal/src/browser/index.ts index 3ed465eea6ca..4cca5a19d9cf 100644 --- a/packages/tracing-internal/src/browser/index.ts +++ b/packages/tracing-internal/src/browser/index.ts @@ -3,4 +3,8 @@ export * from '../exports'; export type { RequestInstrumentationOptions } from './request'; export { BrowserTracing, BROWSER_TRACING_INTEGRATION_ID } from './browsertracing'; -export { instrumentOutgoingRequests, defaultRequestInstrumentationOptions } from './request'; +export { + instrumentOutgoingRequests, + defaultRequestInstrumentationOptions, + addTracingHeadersToFetchRequest, +} from './request'; diff --git a/packages/tracing-internal/src/browser/request.ts b/packages/tracing-internal/src/browser/request.ts index e1e1c9aaf7fb..bce017af48f4 100644 --- a/packages/tracing-internal/src/browser/request.ts +++ b/packages/tracing-internal/src/browser/request.ts @@ -98,7 +98,7 @@ type PolymorphicRequestHeaders = // eslint-disable-next-line @typescript-eslint/no-explicit-any [key: string]: any; append: (key: string, value: string) => void; - get: (key: string) => string; + get: (key: string) => string | null | undefined; }; export const defaultRequestInstrumentationOptions: RequestInstrumentationOptions = { @@ -221,8 +221,11 @@ export function fetchCallback( } } -function addTracingHeadersToFetchRequest( - request: string | Request, +/** + * Adds sentry-trace and baggage headers to the various forms of fetch headers + */ +export function addTracingHeadersToFetchRequest( + request: string | unknown, // unknown is actually type Request but we can't export DOM types from this package, dynamicSamplingContext: Partial, span: Span, options: { @@ -230,7 +233,7 @@ function addTracingHeadersToFetchRequest( | { [key: string]: string[] | string | undefined; } - | Request['headers']; + | PolymorphicRequestHeaders; }, ): PolymorphicRequestHeaders { const sentryBaggageHeader = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext); @@ -247,7 +250,7 @@ function addTracingHeadersToFetchRequest( newHeaders.append('sentry-trace', sentryTraceHeader); if (sentryBaggageHeader) { - // If the same header is appended miultiple times the browser will merge the values into a single request header. + // If the same header is appended multiple times the browser will merge the values into a single request header. // Its therefore safe to simply push a "baggage" entry, even though there might already be another baggage header. newHeaders.append(BAGGAGE_HEADER_NAME, sentryBaggageHeader); } @@ -262,7 +265,7 @@ function addTracingHeadersToFetchRequest( newHeaders.push([BAGGAGE_HEADER_NAME, sentryBaggageHeader]); } - return newHeaders; + return newHeaders as PolymorphicRequestHeaders; } else { const existingBaggageHeader = 'baggage' in headers ? headers.baggage : undefined; const newBaggageHeaders: string[] = []; diff --git a/packages/tracing-internal/src/index.ts b/packages/tracing-internal/src/index.ts index da9b94aac666..8742ec7c3ea5 100644 --- a/packages/tracing-internal/src/index.ts +++ b/packages/tracing-internal/src/index.ts @@ -17,6 +17,7 @@ export { BROWSER_TRACING_INTEGRATION_ID, instrumentOutgoingRequests, defaultRequestInstrumentationOptions, + addTracingHeadersToFetchRequest, } from './browser'; export type { RequestInstrumentationOptions } from './browser'; diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index e62fe6390ac8..7768a73fb6da 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -48,7 +48,7 @@ export type { ClientOptions, Options } from './options'; export type { Package } from './package'; export type { PolymorphicEvent, PolymorphicRequest } from './polymorphics'; export type { ReplayEvent, ReplayRecordingData, ReplayRecordingMode } from './replay'; -export type { QueryParams, Request } from './request'; +export type { QueryParams, Request, SanitizedRequestData } from './request'; export type { Runtime } from './runtime'; export type { CaptureContext, Scope, ScopeContext } from './scope'; export type { SdkInfo } from './sdkinfo'; diff --git a/packages/types/src/request.ts b/packages/types/src/request.ts index c06b29525a84..0c5677a35fdd 100644 --- a/packages/types/src/request.ts +++ b/packages/types/src/request.ts @@ -10,3 +10,15 @@ export interface Request { } export type QueryParams = string | { [key: string]: string } | Array<[string, string]>; + +/** + * Request data that is considered safe for `span.data` on `http.client` spans + * and for `http` breadcrumbs + * See https://develop.sentry.dev/sdk/data-handling/#structuring-data + */ +export type SanitizedRequestData = { + url: string; + method: string; + 'http.fragment'?: string; + 'http.query'?: string; +}; diff --git a/packages/utils/src/instrument.ts b/packages/utils/src/instrument.ts index f2ced4540d5f..d8779afee1b5 100644 --- a/packages/utils/src/instrument.ts +++ b/packages/utils/src/instrument.ts @@ -210,9 +210,8 @@ function getUrlFromResource(resource: FetchResource): string { } /** - * Exported only for tests. - * @hidden - * */ + * Parses the fetch arguments to find the used Http method and the url of the request + */ export function parseFetchArgs(fetchArgs: unknown[]): { method: string; url: string } { if (fetchArgs.length === 0) { return { method: 'GET', url: '' }; diff --git a/packages/utils/src/url.ts b/packages/utils/src/url.ts index 8dce48e7428b..71ce23e63a57 100644 --- a/packages/utils/src/url.ts +++ b/packages/utils/src/url.ts @@ -1,3 +1,12 @@ +type PartialURL = { + host?: string; + path?: string; + protocol?: string; + relative?: string; + search?: string; + hash?: string; +}; + /** * Parses string form of URL into an object * // borrowed from https://tools.ietf.org/html/rfc3986#appendix-B @@ -5,12 +14,7 @@ * // environments where DOM might not be available * @returns parsed URL object */ -export function parseUrl(url: string): { - host?: string; - path?: string; - protocol?: string; - relative?: string; -} { +export function parseUrl(url: string): PartialURL { if (!url) { return {}; } @@ -28,6 +32,8 @@ export function parseUrl(url: string): { host: match[4], path: match[5], protocol: match[2], + search: query, + hash: fragment, relative: match[5] + query + fragment, // everything minus origin }; } @@ -50,3 +56,23 @@ export function getNumberOfUrlSegments(url: string): number { // split at '/' or at '\/' to split regex urls correctly return url.split(/\\?\//).filter(s => s.length > 0 && s !== ',').length; } + +/** + * Takes a URL object and returns a sanitized string which is safe to use as span description + * see: https://develop.sentry.dev/sdk/data-handling/#structuring-data + */ +export function getSanitizedUrlString(url: PartialURL): string { + const { protocol, host, path } = url; + + const filteredHost = + (host && + host + // Always filter out authority + .replace(/^.*@/, '[filtered]:[filtered]@') + // Don't show standard :80 (http) and :443 (https) ports to reduce the noise + .replace(':80', '') + .replace(':443', '')) || + ''; + + return `${protocol ? `${protocol}://` : ''}${filteredHost}${path}`; +} diff --git a/packages/utils/test/url.test.ts b/packages/utils/test/url.test.ts index e356662b9b29..9caad36f572b 100644 --- a/packages/utils/test/url.test.ts +++ b/packages/utils/test/url.test.ts @@ -1,4 +1,4 @@ -import { getNumberOfUrlSegments, stripUrlQueryAndFragment } from '../src/url'; +import { getNumberOfUrlSegments, getSanitizedUrlString, parseUrl, stripUrlQueryAndFragment } from '../src/url'; describe('stripQueryStringAndFragment', () => { const urlString = 'http://dogs.are.great:1231/yay/'; @@ -31,3 +31,51 @@ describe('getNumberOfUrlSegments', () => { expect(getNumberOfUrlSegments(input)).toEqual(output); }); }); + +describe('getSanitizedUrlString', () => { + it.each([ + ['regular url', 'https://somedomain.com', 'https://somedomain.com'], + ['regular url with a path', 'https://somedomain.com/path/to/happiness', 'https://somedomain.com/path/to/happiness'], + [ + 'url with standard http port 80', + 'http://somedomain.com:80/path/to/happiness', + 'http://somedomain.com/path/to/happiness', + ], + [ + 'url with standard https port 443', + 'https://somedomain.com:443/path/to/happiness', + 'https://somedomain.com/path/to/happiness', + ], + [ + 'url with non-standard port', + 'https://somedomain.com:4200/path/to/happiness', + 'https://somedomain.com:4200/path/to/happiness', + ], + [ + 'url with query params', + 'https://somedomain.com:4200/path/to/happiness?auhtToken=abc123¶m2=bar', + 'https://somedomain.com:4200/path/to/happiness', + ], + [ + 'url with a fragment', + 'https://somedomain.com/path/to/happiness#somewildfragment123', + 'https://somedomain.com/path/to/happiness', + ], + [ + 'url with a fragment and query params', + 'https://somedomain.com/path/to/happiness#somewildfragment123?auhtToken=abc123¶m2=bar', + 'https://somedomain.com/path/to/happiness', + ], + [ + 'url with authorization', + 'https://username:password@somedomain.com', + 'https://[filtered]:[filtered]@somedomain.com', + ], + ['same-origin url', '/api/v4/users?id=123', '/api/v4/users'], + ['url without a protocol', 'example.com', 'example.com'], + ['url without a protocol with a path', 'example.com/sub/path?id=123', 'example.com/sub/path'], + ])('returns a sanitized URL for a %s', (_, rawUrl: string, sanitizedURL: string) => { + const urlObject = parseUrl(rawUrl); + expect(getSanitizedUrlString(urlObject)).toEqual(sanitizedURL); + }); +});