Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(tracing): Add tracing without performance to browser and client Sveltekit #8458

Merged
merged 4 commits into from
Jul 6, 2023
Merged

feat(tracing): Add tracing without performance to browser and client Sveltekit #8458

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
605825b
feat(tracing): Add tracing without performance to browser and client
AbhiPrasad Jul 5, 2023
91bcd21
xhr callback
AbhiPrasad Jul 5, 2023
63e38d1
test fix
AbhiPrasad Jul 5, 2023
71fbf9b
Merge branch 'develop' into abhi-twp-browser
AbhiPrasad Jul 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 13 additions & 13 deletions packages/sveltekit/src/client/load.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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', /^\//],
)
);
};

Expand Down Expand Up @@ -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:
| {
Expand All @@ -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
Expand Down
12 changes: 7 additions & 5 deletions packages/sveltekit/test/client/load.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ const mockedGetIntegrationById = vi.fn(id => {
const mockedGetClient = vi.fn(() => {
return {
getIntegrationById: mockedGetIntegrationById,
getOptions: () => ({}),
};
});

Expand All @@ -77,6 +78,11 @@ vi.mock('@sentry/core', async () => {
getClient: mockedGetClient,
getScope: () => {
return {
getPropagationContext: () => ({
traceId: '1234567890abcdef1234567890abcdef',
spanId: '1234567890abcdef',
sampled: false,
}),
getSpan: () => {
return {
transaction: {
Expand Down Expand Up @@ -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);
Expand All @@ -391,10 +397,6 @@ describe('wrapLoadWithSentry', () => {
expect.any(Function),
);

expect(mockedSveltekitFetch).toHaveBeenCalledWith(
...[originalFetchArgs[0], originalFetchArgs.length === 2 ? originalFetchArgs[1] : {}],
);

mockedBrowserTracing.options.shouldCreateSpanForRequest = () => true;
});

Expand Down
190 changes: 115 additions & 75 deletions packages/tracing-internal/src/browser/request.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -219,12 +220,14 @@ export function fetchCallback(
shouldCreateSpan: (url: string) => boolean,
shouldAttachHeaders: (url: string) => boolean,
spans: Record<string, Span>,
): 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;

Expand All @@ -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 `{}`.
Expand All @@ -280,35 +291,42 @@ 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;
}

/**
* 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<DynamicSamplingContext>,
span: Span,
client: Client,
scope: Scope,
options: {
headers?:
| {
[key: string]: string[] | string | undefined;
}
| 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;
Expand Down Expand Up @@ -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<string, Span>,
): 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;

Expand All @@ -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<XHRData['xhr']>,
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.
}
}
8 changes: 2 additions & 6 deletions packages/tracing-internal/test/browser/request.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -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',
Expand Down