From da495db61a88d3548ea915242c9d12251d159c5a Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Thu, 6 Jul 2023 08:59:12 -0400 Subject: [PATCH] feat(tracing): Add tracing without performance to browser and client Sveltekit (#8458) Adds tracing without performance support to 1. fetch requests in browser 2. xhr requests in browser 3. Sveltekit fetch monkeypatching (pulled into this PR because it also uses `addTracingHeadersToFetchRequest` --- packages/sveltekit/src/client/load.ts | 26 +-- packages/sveltekit/test/client/load.test.ts | 12 +- .../tracing-internal/src/browser/request.ts | 190 +++++++++++------- .../test/browser/request.test.ts | 8 +- 4 files changed, 137 insertions(+), 99 deletions(-) diff --git a/packages/sveltekit/src/client/load.ts b/packages/sveltekit/src/client/load.ts index e56d33b2e23c..8907994e8649 100644 --- a/packages/sveltekit/src/client/load.ts +++ b/packages/sveltekit/src/client/load.ts @@ -129,6 +129,8 @@ function instrumentSvelteKitFetch(originalFetch: SvelteKitFetch): SvelteKitFetch return originalFetch; } + const options = client.getOptions(); + const browserTracingIntegration = client.getIntegrationById('BrowserTracing') as BrowserTracing | undefined; const breadcrumbsIntegration = client.getIntegrationById('Breadcrumbs') as Breadcrumbs | undefined; @@ -147,7 +149,10 @@ function instrumentSvelteKitFetch(originalFetch: SvelteKitFetch): SvelteKitFetch const shouldAttachHeaders: (url: string) => boolean = url => { return ( !!shouldTraceFetch && - stringMatchesSomePattern(url, browserTracingOptions.tracePropagationTargets || ['localhost', /^\//]) + stringMatchesSomePattern( + url, + options.tracePropagationTargets || browserTracingOptions.tracePropagationTargets || ['localhost', /^\//], + ) ); }; @@ -177,20 +182,15 @@ function instrumentSvelteKitFetch(originalFetch: SvelteKitFetch): SvelteKitFetch }; 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 hub = getCurrentHub(); + const scope = hub.getScope(); + const client = hub.getClient(); + if (client && shouldAttachHeaders(rawUrl)) { const headers = addTracingHeadersToFetchRequest( input as string | Request, - dsc, - activeSpan, + client, + scope, patchedInit as { headers: | { @@ -207,7 +207,7 @@ function instrumentSvelteKitFetch(originalFetch: SvelteKitFetch): SvelteKitFetch const patchedFetchArgs = [input, patchedInit]; - if (createSpan) { + if (shouldCreateSpan(rawUrl)) { fetchPromise = trace( { name: `${method} ${requestData.url}`, // this will become the description of the span diff --git a/packages/sveltekit/test/client/load.test.ts b/packages/sveltekit/test/client/load.test.ts index 07608bb9845a..6373ea1ff571 100644 --- a/packages/sveltekit/test/client/load.test.ts +++ b/packages/sveltekit/test/client/load.test.ts @@ -61,6 +61,7 @@ const mockedGetIntegrationById = vi.fn(id => { const mockedGetClient = vi.fn(() => { return { getIntegrationById: mockedGetIntegrationById, + getOptions: () => ({}), }; }); @@ -77,6 +78,11 @@ vi.mock('@sentry/core', async () => { getClient: mockedGetClient, getScope: () => { return { + getPropagationContext: () => ({ + traceId: '1234567890abcdef1234567890abcdef', + spanId: '1234567890abcdef', + sampled: false, + }), getSpan: () => { return { transaction: { @@ -371,7 +377,7 @@ describe('wrapLoadWithSentry', () => { mockedBrowserTracing.options.traceFetch = true; }); - it("doesn't create a span nor propagate headers, if `shouldCreateSpanForRequest` returns false", async () => { + it("doesn't create a span if `shouldCreateSpanForRequest` returns false", async () => { mockedBrowserTracing.options.shouldCreateSpanForRequest = () => false; const wrappedLoad = wrapLoadWithSentry(load); @@ -391,10 +397,6 @@ describe('wrapLoadWithSentry', () => { expect.any(Function), ); - expect(mockedSveltekitFetch).toHaveBeenCalledWith( - ...[originalFetchArgs[0], originalFetchArgs.length === 2 ? originalFetchArgs[1] : {}], - ); - mockedBrowserTracing.options.shouldCreateSpanForRequest = () => true; }); diff --git a/packages/tracing-internal/src/browser/request.ts b/packages/tracing-internal/src/browser/request.ts index 9a5bbb330b6b..071b2146bb16 100644 --- a/packages/tracing-internal/src/browser/request.ts +++ b/packages/tracing-internal/src/browser/request.ts @@ -1,11 +1,12 @@ /* eslint-disable max-lines */ -import { getCurrentHub, hasTracingEnabled } from '@sentry/core'; -import type { DynamicSamplingContext, Span } from '@sentry/types'; +import { getCurrentHub, getDynamicSamplingContextFromClient, hasTracingEnabled } from '@sentry/core'; +import type { Client, Scope, Span } from '@sentry/types'; import { addInstrumentationHandler, BAGGAGE_HEADER_NAME, browserPerformanceTimeOrigin, dynamicSamplingContextToSentryBaggageHeader, + generateSentryTraceHeader, isInstanceOf, SENTRY_XHR_DATA_KEY, stringMatchesSomePattern, @@ -219,12 +220,14 @@ export function fetchCallback( shouldCreateSpan: (url: string) => boolean, shouldAttachHeaders: (url: string) => boolean, spans: Record, -): Span | void { - if (!hasTracingEnabled() || !(handlerData.fetchData && shouldCreateSpan(handlerData.fetchData.url))) { - return; +): Span | undefined { + if (!hasTracingEnabled() || !handlerData.fetchData) { + return undefined; } - if (handlerData.endTimestamp) { + const shouldCreateSpanResult = shouldCreateSpan(handlerData.fetchData.url); + + if (handlerData.endTimestamp && shouldCreateSpanResult) { const spanId = handlerData.fetchData.__span; if (!spanId) return; @@ -251,27 +254,35 @@ export function fetchCallback( // eslint-disable-next-line @typescript-eslint/no-dynamic-delete delete spans[spanId]; } - return; + return undefined; } - const currentSpan = getCurrentHub().getScope().getSpan(); - const activeTransaction = currentSpan && currentSpan.transaction; - - if (currentSpan && activeTransaction) { - const { method, url } = handlerData.fetchData; - const span = currentSpan.startChild({ - data: { - url, - type: 'fetch', - 'http.method': method, - }, - description: `${method} ${url}`, - op: 'http.client', - }); - + const hub = getCurrentHub(); + const scope = hub.getScope(); + const client = hub.getClient(); + const parentSpan = scope.getSpan(); + + const { method, url } = handlerData.fetchData; + + const span = + shouldCreateSpanResult && parentSpan + ? parentSpan.startChild({ + data: { + url, + type: 'fetch', + 'http.method': method, + }, + description: `${method} ${url}`, + op: 'http.client', + }) + : undefined; + + if (span) { handlerData.fetchData.__span = span.spanId; spans[span.spanId] = span; + } + if (shouldAttachHeaders(handlerData.fetchData.url) && client) { const request: string | Request = handlerData.args[0]; // In case the user hasn't set the second argument of a fetch call we default it to `{}`. @@ -280,16 +291,11 @@ export function fetchCallback( // eslint-disable-next-line @typescript-eslint/no-explicit-any const options: { [key: string]: any } = handlerData.args[1]; - if (shouldAttachHeaders(handlerData.fetchData.url)) { - options.headers = addTracingHeadersToFetchRequest( - request, - activeTransaction.getDynamicSamplingContext(), - span, - options, - ); - } - return span; + // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access + options.headers = addTracingHeadersToFetchRequest(request, client, scope, options); } + + return span; } /** @@ -297,8 +303,8 @@ export function fetchCallback( */ 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, + client: Client, + scope: Scope, options: { headers?: | { @@ -306,9 +312,21 @@ export function addTracingHeadersToFetchRequest( } | PolymorphicRequestHeaders; }, -): PolymorphicRequestHeaders { +): PolymorphicRequestHeaders | undefined { + const span = scope.getSpan(); + + const transaction = span && span.transaction; + + const { traceId, sampled, dsc } = scope.getPropagationContext(); + + const sentryTraceHeader = span ? span.toTraceparent() : generateSentryTraceHeader(traceId, undefined, sampled); + const dynamicSamplingContext = transaction + ? transaction.getDynamicSamplingContext() + : dsc + ? dsc + : getDynamicSamplingContextFromClient(traceId, client, scope); + const sentryBaggageHeader = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext); - const sentryTraceHeader = span.toTraceparent(); const headers = typeof Request !== 'undefined' && isInstanceOf(request, Request) ? (request as Request).headers : options.headers; @@ -364,25 +382,24 @@ export function addTracingHeadersToFetchRequest( * * @returns Span if a span was created, otherwise void. */ +// eslint-disable-next-line complexity export function xhrCallback( handlerData: XHRData, shouldCreateSpan: (url: string) => boolean, shouldAttachHeaders: (url: string) => boolean, spans: Record, -): Span | void { +): Span | undefined { const xhr = handlerData.xhr; const sentryXhrData = xhr && xhr[SENTRY_XHR_DATA_KEY]; - if ( - !hasTracingEnabled() || - (xhr && xhr.__sentry_own_request__) || - !(xhr && sentryXhrData && shouldCreateSpan(sentryXhrData.url)) - ) { - return; + if (!hasTracingEnabled() || (xhr && xhr.__sentry_own_request__) || !xhr || !sentryXhrData) { + return undefined; } + const shouldCreateSpanResult = shouldCreateSpan(sentryXhrData.url); + // check first if the request has finished and is tracked by an existing span which should now end - if (handlerData.endTimestamp) { + if (handlerData.endTimestamp && shouldCreateSpanResult) { const spanId = xhr.__sentry_xhr_span_id__; if (!spanId) return; @@ -394,45 +411,68 @@ export function xhrCallback( // eslint-disable-next-line @typescript-eslint/no-dynamic-delete delete spans[spanId]; } - return; + return undefined; } - const currentSpan = getCurrentHub().getScope().getSpan(); - const activeTransaction = currentSpan && currentSpan.transaction; - - if (currentSpan && activeTransaction) { - const span = currentSpan.startChild({ - data: { - ...sentryXhrData.data, - type: 'xhr', - 'http.method': sentryXhrData.method, - url: sentryXhrData.url, - }, - description: `${sentryXhrData.method} ${sentryXhrData.url}`, - op: 'http.client', - }); - + const hub = getCurrentHub(); + const scope = hub.getScope(); + const parentSpan = scope.getSpan(); + + const span = + shouldCreateSpanResult && parentSpan + ? parentSpan.startChild({ + data: { + ...sentryXhrData.data, + type: 'xhr', + 'http.method': sentryXhrData.method, + url: sentryXhrData.url, + }, + description: `${sentryXhrData.method} ${sentryXhrData.url}`, + op: 'http.client', + }) + : undefined; + + if (span) { xhr.__sentry_xhr_span_id__ = span.spanId; spans[xhr.__sentry_xhr_span_id__] = span; + } - if (xhr.setRequestHeader && shouldAttachHeaders(sentryXhrData.url)) { - try { - xhr.setRequestHeader('sentry-trace', span.toTraceparent()); + if (xhr.setRequestHeader && shouldAttachHeaders(sentryXhrData.url)) { + if (span) { + const transaction = span && span.transaction; + const dynamicSamplingContext = transaction && transaction.getDynamicSamplingContext(); + const sentryBaggageHeader = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext); + setHeaderOnXhr(xhr, span.toTraceparent(), sentryBaggageHeader); + } else { + const client = hub.getClient(); + const { traceId, sampled, dsc } = scope.getPropagationContext(); + const sentryTraceHeader = generateSentryTraceHeader(traceId, undefined, sampled); + const dynamicSamplingContext = + dsc || (client ? getDynamicSamplingContextFromClient(traceId, client, scope) : undefined); + const sentryBaggageHeader = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext); + setHeaderOnXhr(xhr, sentryTraceHeader, sentryBaggageHeader); + } + } - const dynamicSamplingContext = activeTransaction.getDynamicSamplingContext(); - const sentryBaggageHeader = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext); + return span; +} - if (sentryBaggageHeader) { - // From MDN: "If this method is called several times with the same header, the values are merged into one single request header." - // We can therefore simply set a baggage header without checking what was there before - // https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/setRequestHeader - xhr.setRequestHeader(BAGGAGE_HEADER_NAME, sentryBaggageHeader); - } - } catch (_) { - // Error: InvalidStateError: Failed to execute 'setRequestHeader' on 'XMLHttpRequest': The object's state must be OPENED. - } +function setHeaderOnXhr( + xhr: NonNullable, + sentryTraceHeader: string, + sentryBaggageHeader: string | undefined, +): void { + try { + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + xhr.setRequestHeader!('sentry-trace', sentryTraceHeader); + if (sentryBaggageHeader) { + // From MDN: "If this method is called several times with the same header, the values are merged into one single request header." + // We can therefore simply set a baggage header without checking what was there before + // https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/setRequestHeader + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + xhr.setRequestHeader!(BAGGAGE_HEADER_NAME, sentryBaggageHeader); } - - return span; + } catch (_) { + // Error: InvalidStateError: Failed to execute 'setRequestHeader' on 'XMLHttpRequest': The object's state must be OPENED. } } diff --git a/packages/tracing-internal/test/browser/request.test.ts b/packages/tracing-internal/test/browser/request.test.ts index c0c544506a09..9c8307e97fd7 100644 --- a/packages/tracing-internal/test/browser/request.test.ts +++ b/packages/tracing-internal/test/browser/request.test.ts @@ -98,9 +98,7 @@ describe('callbacks', () => { // each case is [shouldCreateSpanReturnValue, shouldAttachHeadersReturnValue, expectedSpan, expectedHeaderKeys] [true, true, expect.objectContaining(fetchSpan), ['sentry-trace', 'baggage']], [true, false, expect.objectContaining(fetchSpan), []], - // If there's no span then there's no parent span id to stick into a header, so no headers, even if there's a - // `tracingOrigins` match - [false, true, undefined, []], + [false, true, undefined, ['sentry-trace', 'baggage']], [false, false, undefined, []], ])( 'span creation/header attachment interaction - shouldCreateSpan: %s, shouldAttachHeaders: %s', @@ -284,9 +282,7 @@ describe('callbacks', () => { // each case is [shouldCreateSpanReturnValue, shouldAttachHeadersReturnValue, expectedSpan, expectedHeaderKeys] [true, true, expect.objectContaining(xhrSpan), ['sentry-trace', 'baggage']], [true, false, expect.objectContaining(xhrSpan), []], - // If there's no span then there's no parent span id to stick into a header, so no headers, even if there's a - // `tracingOrigins` match - [false, true, undefined, []], + [false, true, undefined, ['sentry-trace', 'baggage']], [false, false, undefined, []], ])( 'span creation/header attachment interaction - shouldCreateSpan: %s, shouldAttachHeaders: %s',