From cb45cbcf12866f655280c0cac468feca3a8c52ce Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Fri, 22 Mar 2024 09:36:03 -0400 Subject: [PATCH] feat(tracing): Remove `instrumentRoutingWithDefaults` (#11240) This functionality is no longer being used, so we can remove it and it's tests. Part of https://github.com/getsentry/sentry-javascript/issues/9885 since it makes it easier to port over browsertracing into browser package. --- .../tracing-internal/src/browser/router.ts | 72 ---------- .../test/browser/router.test.ts | 127 ------------------ 2 files changed, 199 deletions(-) delete mode 100644 packages/tracing-internal/src/browser/router.ts delete mode 100644 packages/tracing-internal/test/browser/router.test.ts diff --git a/packages/tracing-internal/src/browser/router.ts b/packages/tracing-internal/src/browser/router.ts deleted file mode 100644 index b94911046171..000000000000 --- a/packages/tracing-internal/src/browser/router.ts +++ /dev/null @@ -1,72 +0,0 @@ -import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, spanToJSON } from '@sentry/core'; -import type { Transaction, TransactionContext } from '@sentry/types'; -import { addHistoryInstrumentationHandler, browserPerformanceTimeOrigin, logger } from '@sentry/utils'; - -import { DEBUG_BUILD } from '../common/debug-build'; -import { WINDOW } from './types'; - -/** - * Default function implementing pageload and navigation transactions - */ -export function instrumentRoutingWithDefaults( - customStartTransaction: (context: TransactionContext) => T | undefined, - startTransactionOnPageLoad: boolean = true, - startTransactionOnLocationChange: boolean = true, -): void { - if (!WINDOW || !WINDOW.location) { - DEBUG_BUILD && logger.warn('Could not initialize routing instrumentation due to invalid location'); - return; - } - - let startingUrl: string | undefined = WINDOW.location.href; - - let activeTransaction: T | undefined; - if (startTransactionOnPageLoad) { - activeTransaction = customStartTransaction({ - name: WINDOW.location.pathname, - // pageload should always start at timeOrigin (and needs to be in s, not ms) - startTimestamp: browserPerformanceTimeOrigin ? browserPerformanceTimeOrigin / 1000 : undefined, - op: 'pageload', - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.pageload.browser', - }, - }); - } - - if (startTransactionOnLocationChange) { - addHistoryInstrumentationHandler(({ to, from }) => { - /** - * This early return is there to account for some cases where a navigation transaction starts right after - * long-running pageload. We make sure that if `from` is undefined and a valid `startingURL` exists, we don't - * create an uneccessary navigation transaction. - * - * This was hard to duplicate, but this behavior stopped as soon as this fix was applied. This issue might also - * only be caused in certain development environments where the usage of a hot module reloader is causing - * errors. - */ - if (from === undefined && startingUrl && startingUrl.indexOf(to) !== -1) { - startingUrl = undefined; - return; - } - - if (from !== to) { - startingUrl = undefined; - if (activeTransaction) { - DEBUG_BUILD && - logger.log(`[Tracing] Finishing current transaction with op: ${spanToJSON(activeTransaction).op}`); - // If there's an open transaction on the scope, we need to finish it before creating an new one. - activeTransaction.end(); - } - activeTransaction = customStartTransaction({ - name: WINDOW.location.pathname, - op: 'navigation', - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.browser', - }, - }); - } - }); - } -} diff --git a/packages/tracing-internal/test/browser/router.test.ts b/packages/tracing-internal/test/browser/router.test.ts deleted file mode 100644 index 12c44f9029c4..000000000000 --- a/packages/tracing-internal/test/browser/router.test.ts +++ /dev/null @@ -1,127 +0,0 @@ -import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '@sentry/core'; -import type { HandlerDataHistory } from '@sentry/types'; -import { JSDOM } from 'jsdom'; -import { conditionalTest } from '../../../node/test/utils'; - -import { instrumentRoutingWithDefaults } from '../../src/browser/router'; - -let mockChangeHistory: undefined | ((data: HandlerDataHistory) => void); -jest.mock('@sentry/utils', () => { - const actual = jest.requireActual('@sentry/utils'); - return { - ...actual, - addHistoryInstrumentationHandler: (callback: (data: HandlerDataHistory) => void): void => { - mockChangeHistory = callback; - }, - }; -}); - -conditionalTest({ min: 16 })('instrumentRoutingWithDefaults', () => { - const mockFinish = jest.fn(); - const customStartTransaction = jest.fn().mockReturnValue({ end: mockFinish }); - beforeEach(() => { - const dom = new JSDOM(); - // @ts-expect-error need to override global document - global.document = dom.window.document; - // @ts-expect-error need to override global document - global.window = dom.window; - // @ts-expect-error need to override global document - global.location = dom.window.location; - - customStartTransaction.mockClear(); - mockFinish.mockClear(); - }); - - it('does not start transactions if global location is undefined', () => { - // @ts-expect-error need to override global document - global.location = undefined; - instrumentRoutingWithDefaults(customStartTransaction); - expect(customStartTransaction).toHaveBeenCalledTimes(0); - }); - - it('starts a pageload transaction', () => { - instrumentRoutingWithDefaults(customStartTransaction); - expect(customStartTransaction).toHaveBeenCalledTimes(1); - expect(customStartTransaction).toHaveBeenLastCalledWith({ - name: 'blank', - op: 'pageload', - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.pageload.browser', - }, - startTimestamp: expect.any(Number), - }); - }); - - it('does not start a pageload transaction if startTransactionOnPageLoad is false', () => { - instrumentRoutingWithDefaults(customStartTransaction, false); - expect(customStartTransaction).toHaveBeenCalledTimes(0); - }); - - describe('navigation transaction', () => { - beforeEach(() => { - mockChangeHistory = undefined; - }); - - it('it is not created automatically', () => { - instrumentRoutingWithDefaults(customStartTransaction); - expect(customStartTransaction).not.toHaveBeenCalledWith(expect.objectContaining({ op: 'navigation' })); - }); - - it('is created on location change', () => { - instrumentRoutingWithDefaults(customStartTransaction); - expect(mockChangeHistory).toBeDefined(); - mockChangeHistory!({ to: 'here', from: 'there' }); - - expect(customStartTransaction).toHaveBeenCalledTimes(2); - expect(customStartTransaction).toHaveBeenLastCalledWith({ - name: 'blank', - op: 'navigation', - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.browser', - }, - }); - }); - - it('is not created if startTransactionOnLocationChange is false', () => { - instrumentRoutingWithDefaults(customStartTransaction, true, false); - expect(mockChangeHistory).toBeUndefined(); - - expect(customStartTransaction).toHaveBeenCalledTimes(1); - }); - - it('finishes the last active transaction', () => { - instrumentRoutingWithDefaults(customStartTransaction); - - expect(mockChangeHistory).toBeDefined(); - - expect(mockFinish).toHaveBeenCalledTimes(0); - mockChangeHistory!({ to: 'here', from: 'there' }); - expect(mockFinish).toHaveBeenCalledTimes(1); - }); - - it('will finish active transaction multiple times', () => { - instrumentRoutingWithDefaults(customStartTransaction); - - expect(mockChangeHistory).toBeDefined(); - - expect(mockFinish).toHaveBeenCalledTimes(0); - mockChangeHistory!({ to: 'here', from: 'there' }); - expect(mockFinish).toHaveBeenCalledTimes(1); - mockChangeHistory!({ to: 'over/there', from: 'here' }); - expect(mockFinish).toHaveBeenCalledTimes(2); - mockChangeHistory!({ to: 'nowhere', from: 'over/there' }); - expect(mockFinish).toHaveBeenCalledTimes(3); - }); - - it('not created if `from` is equal to `to`', () => { - instrumentRoutingWithDefaults(customStartTransaction); - expect(mockChangeHistory).toBeDefined(); - mockChangeHistory!({ to: 'first/path', from: 'first/path' }); - - expect(customStartTransaction).toHaveBeenCalledTimes(1); - expect(customStartTransaction).not.toHaveBeenLastCalledWith('navigation'); - }); - }); -});