Skip to content

fix(browser): Move browserTracingIntegration code to setup hook #16386

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.

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

Merged
merged 3 commits into from
May 26, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import * as Sentry from '@sentry/browser';

window.Sentry = Sentry;

Sentry.init({
dsn: 'https://public@dsn.ingest.sentry.io/1337',
integrations: [Sentry.browserTracingIntegration()],
tracePropagationTargets: ['http://sentry-test-site.example'],
tracesSampleRate: 1,
autoSessionTracking: false,
});

// fetch directly after init
fetch('http://sentry-test-site.example/0');
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import { expect } from '@playwright/test';
import { sentryTest } from '../../../../utils/fixtures';
import {
envelopeRequestParser,
shouldSkipTracingTest,
waitForTransactionRequestOnUrl,
} from '../../../../utils/helpers';

sentryTest('should create spans for fetch requests called directly after init', async ({ getLocalTestUrl, page }) => {
if (shouldSkipTracingTest()) {
sentryTest.skip();
}

await page.route('http://sentry-test-site.example/*', route => route.fulfill({ body: 'ok' }));

const url = await getLocalTestUrl({ testDir: __dirname });

const req = await waitForTransactionRequestOnUrl(page, url);
const tracingEvent = envelopeRequestParser(req);

const requestSpans = tracingEvent.spans?.filter(({ op }) => op === 'http.client');

expect(requestSpans).toHaveLength(1);

expect(requestSpans![0]).toMatchObject({
description: 'GET http://sentry-test-site.example/0',
parent_span_id: tracingEvent.contexts?.trace?.span_id,
span_id: expect.stringMatching(/[a-f0-9]{16}/),
start_timestamp: expect.any(Number),
timestamp: expect.any(Number),
trace_id: tracingEvent.contexts?.trace?.trace_id,
data: {
'http.method': 'GET',
'http.url': 'http://sentry-test-site.example/0',
url: 'http://sentry-test-site.example/0',
'server.address': 'sentry-test-site.example',
type: 'fetch',
},
});
});
75 changes: 34 additions & 41 deletions packages/browser/src/tracing/browserTracingIntegration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import type { Client, IntegrationFn, Span, StartSpanOptions, TransactionSource,
import {
addNonEnumerableProperty,
browserPerformanceTimeOrigin,
consoleSandbox,
generateTraceId,
getClient,
getCurrentScope,
Expand Down Expand Up @@ -233,8 +232,6 @@ const DEFAULT_BROWSER_TRACING_OPTIONS: BrowserTracingOptions = {
...defaultRequestInstrumentationOptions,
};

let _hasBeenInitialized = false;

/**
* The Browser Tracing integration automatically instruments browser pageload/navigation
* actions as transactions, and captures requests, metrics and errors as spans.
Expand All @@ -245,23 +242,17 @@ let _hasBeenInitialized = false;
* We explicitly export the proper type here, as this has to be extended in some cases.
*/
export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptions> = {}) => {
if (_hasBeenInitialized) {
consoleSandbox(() => {
// eslint-disable-next-line no-console
console.warn('Multiple browserTracingIntegration instances are not supported.');
});
}

_hasBeenInitialized = true;
const latestRoute: RouteInfo = {
name: undefined,
source: undefined,
};

/**
* This is just a small wrapper that makes `document` optional.
* We want to be extra-safe and always check that this exists, to ensure weird environments do not blow up.
*/
const optionalWindowDocument = WINDOW.document as (typeof WINDOW)['document'] | undefined;

registerSpanErrorInstrumentation();

const {
enableInp,
enableLongTask,
Expand All @@ -287,31 +278,7 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio
..._options,
};

const _collectWebVitals = startTrackingWebVitals({ recordClsStandaloneSpans: enableStandaloneClsSpans || false });

if (enableInp) {
startTrackingINP();
}

if (
enableLongAnimationFrame &&
GLOBAL_OBJ.PerformanceObserver &&
PerformanceObserver.supportedEntryTypes &&
PerformanceObserver.supportedEntryTypes.includes('long-animation-frame')
) {
startTrackingLongAnimationFrames();
} else if (enableLongTask) {
startTrackingLongTasks();
}

if (enableInteractions) {
startTrackingInteractions();
}

const latestRoute: RouteInfo = {
name: undefined,
source: undefined,
};
let _collectWebVitals: undefined | (() => void);

/** Create routing idle transaction. */
function _createRouteSpan(client: Client, startSpanOptions: StartSpanOptions): void {
Expand Down Expand Up @@ -340,7 +307,9 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio
// should wait for finish signal if it's a pageload transaction
disableAutoFinish: isPageloadTransaction,
beforeSpanEnd: span => {
_collectWebVitals();
// This will generally always be defined here, because it is set in `setup()` of the integration
// but technically, it is optional, so we guard here to be extra safe
_collectWebVitals?.();
addPerformanceEntries(span, { recordClsOnPageloadSpan: !enableStandaloneClsSpans });
setActiveIdleSpan(client, undefined);

Expand Down Expand Up @@ -378,8 +347,29 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio

return {
name: BROWSER_TRACING_INTEGRATION_ID,
afterAllSetup(client) {
let startingUrl: string | undefined = getLocationHref();
setup(client) {
registerSpanErrorInstrumentation();

_collectWebVitals = startTrackingWebVitals({ recordClsStandaloneSpans: enableStandaloneClsSpans || false });

if (enableInp) {
startTrackingINP();
}

if (
enableLongAnimationFrame &&
GLOBAL_OBJ.PerformanceObserver &&
PerformanceObserver.supportedEntryTypes &&
PerformanceObserver.supportedEntryTypes.includes('long-animation-frame')
) {
startTrackingLongAnimationFrames();
} else if (enableLongTask) {
startTrackingLongTasks();
}

if (enableInteractions) {
startTrackingInteractions();
}

function maybeEndActiveSpan(): void {
const activeSpan = getActiveIdleSpan(client);
Expand Down Expand Up @@ -440,6 +430,9 @@ export const browserTracingIntegration = ((_options: Partial<BrowserTracingOptio
...startSpanOptions,
});
});
},
afterAllSetup(client) {
let startingUrl: string | undefined = getLocationHref();

if (linkPreviousTrace !== 'off') {
linkTraces(client, { linkPreviousTrace, consistentTraceSampling });
Expand Down
31 changes: 27 additions & 4 deletions packages/core/test/lib/integration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,12 @@ class MockIntegration implements Integration {
// Only for testing - tag to keep separate instances straight when testing deduplication
public tag?: string;

public setupOnce = vi.fn(() => {});

public constructor(name: string, tag?: string) {
this.name = name;
this.tag = tag;
}

public setupOnce(): void {
// noop
}
}

type TestCase = [
Expand Down Expand Up @@ -74,6 +72,31 @@ describe('getIntegrationsToSetup', () => {
});
expect(integrations.map(i => i.name)).toEqual(expected);
});

test('it uses passed integration over default intergation', () => {
const integrationDefault = new MockIntegration('ChaseSquirrels');
const integration1 = new MockIntegration('ChaseSquirrels');

const integrations = getIntegrationsToSetup({
defaultIntegrations: [integrationDefault],
integrations: [integration1],
});

expect(integrations).toEqual([integration1]);
});

test('it uses last passed integration only', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah nice, I think this is good behaviour and probably better than taking first one. Just in case users have some kind of dynamic behaviour that builds the integrations array.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jup, totally, that was what we had already anyhow 👍 the test just shows that!

const integrationDefault = new MockIntegration('ChaseSquirrels');
const integration1 = new MockIntegration('ChaseSquirrels');
const integration2 = new MockIntegration('ChaseSquirrels');

const integrations = getIntegrationsToSetup({
defaultIntegrations: [integrationDefault],
integrations: [integration1, integration2],
});

expect(integrations).toEqual([integration2]);
});
});

describe('deduping', () => {
Expand Down
4 changes: 3 additions & 1 deletion packages/react/src/reactrouterv6-compat-utils.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,9 @@ export function createReactRouterV6CompatibleTracingIntegration(

return {
...integration,
setup() {
setup(client) {
integration.setup(client);

_useEffect = useEffect;
_useLocation = useLocation;
_useNavigationType = useNavigationType;
Expand Down
Loading