From 87b4bf088f4d55f454fba76084933c66307ce13a Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 26 Jan 2024 12:22:48 -0500 Subject: [PATCH 1/3] ref(ember): Use new `browserTracingIntegration()` under the hood --- .../sentry-performance.ts | 124 ++++++++---------- packages/ember/addon/types.ts | 2 +- packages/ember/tests/helpers/setup-sentry.ts | 22 ---- packages/ember/tests/helpers/utils.ts | 4 +- 4 files changed, 60 insertions(+), 92 deletions(-) diff --git a/packages/ember/addon/instance-initializers/sentry-performance.ts b/packages/ember/addon/instance-initializers/sentry-performance.ts index acabe5334cad..3de0504ba3d8 100644 --- a/packages/ember/addon/instance-initializers/sentry-performance.ts +++ b/packages/ember/addon/instance-initializers/sentry-performance.ts @@ -8,18 +8,13 @@ import type { EmberRunQueues } from '@ember/runloop/-private/types'; import { getOwnConfig, isTesting, macroCondition } from '@embroider/macros'; import * as Sentry from '@sentry/browser'; import type { ExtendedBackburner } from '@sentry/ember/runloop'; -import type { Span, Transaction } from '@sentry/types'; +import type { Span } from '@sentry/types'; import { GLOBAL_OBJ, browserPerformanceTimeOrigin, timestampInSeconds } from '@sentry/utils'; import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from '@sentry/core'; import type { BrowserClient } from '..'; import { getActiveSpan, startInactiveSpan } from '..'; -import type { EmberRouterMain, EmberSentryConfig, GlobalConfig, OwnConfig, StartTransactionFunction } from '../types'; - -type SentryTestRouterService = RouterService & { - _startTransaction?: StartTransactionFunction; - _sentryInstrumented?: boolean; -}; +import type { EmberRouterMain, EmberSentryConfig, GlobalConfig, OwnConfig } from '../types'; function getSentryConfig(): EmberSentryConfig { const _global = GLOBAL_OBJ as typeof GLOBAL_OBJ & GlobalConfig; @@ -98,26 +93,17 @@ export function _instrumentEmberRouter( routerService: RouterService, routerMain: EmberRouterMain, config: EmberSentryConfig, - startTransaction: StartTransactionFunction, - startTransactionOnPageLoad?: boolean, -): { - startTransaction: StartTransactionFunction; -} { +): void { const { disableRunloopPerformance } = config; const location = routerMain.location; - let activeTransaction: Transaction | undefined; + let activeRootSpan: Span | undefined; let transitionSpan: Span | undefined; const url = getLocationURL(location); - if (macroCondition(isTesting())) { - (routerService as SentryTestRouterService)._sentryInstrumented = true; - (routerService as SentryTestRouterService)._startTransaction = startTransaction; - } - - if (startTransactionOnPageLoad && url) { + if (url) { const routeInfo = routerService.recognize(url); - activeTransaction = startTransaction({ + Sentry.startBrowserTracingNavigationSpan({ name: `route:${routeInfo.name}`, op: 'pageload', origin: 'auto.pageload.ember', @@ -127,20 +113,22 @@ export function _instrumentEmberRouter( 'routing.instrumentation': '@sentry/ember', }, }); + activeRootSpan = getActiveSpan(); } const finishActiveTransaction = (_: unknown, nextInstance: unknown): void => { if (nextInstance) { return; } - activeTransaction?.end(); + activeRootSpan?.end(); getBackburner().off('end', finishActiveTransaction); }; routerService.on('routeWillChange', (transition: Transition) => { const { fromRoute, toRoute } = getTransitionInformation(transition, routerService); - activeTransaction?.end(); - activeTransaction = startTransaction({ + activeRootSpan?.end(); + + Sentry.startBrowserTracingNavigationSpan({ name: `route:${toRoute}`, op: 'navigation', origin: 'auto.navigation.ember', @@ -150,6 +138,9 @@ export function _instrumentEmberRouter( 'routing.instrumentation': '@sentry/ember', }, }); + + activeRootSpan = getActiveSpan(); + transitionSpan = startInactiveSpan({ attributes: { [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.ui.ember', @@ -160,22 +151,18 @@ export function _instrumentEmberRouter( }); routerService.on('routeDidChange', () => { - if (!transitionSpan || !activeTransaction) { + if (!transitionSpan || !activeRootSpan) { return; } transitionSpan.end(); if (disableRunloopPerformance) { - activeTransaction.end(); + activeRootSpan.end(); return; } getBackburner().on('end', finishActiveTransaction); }); - - return { - startTransaction, - }; } function _instrumentEmberRunloop(config: EmberSentryConfig): void { @@ -411,61 +398,64 @@ export async function instrumentForPerformance(appInstance: ApplicationInstance) // Maintaining backwards compatibility with config.browserTracingOptions, but passing it with Sentry options is preferred. const browserTracingOptions = config.browserTracingOptions || config.sentry.browserTracingOptions || {}; - const { BrowserTracing } = await import('@sentry/browser'); + const { browserTracingIntegration } = await import('@sentry/browser'); const idleTimeout = config.transitionTimeout || 5000; - const browserTracing = new BrowserTracing({ - routingInstrumentation: (customStartTransaction, startTransactionOnPageLoad) => { - // eslint-disable-next-line ember/no-private-routing-service - const routerMain = appInstance.lookup('router:main') as EmberRouterMain; - let routerService = appInstance.lookup('service:router') as RouterService & { - externalRouter?: RouterService; - _hasMountedSentryPerformanceRouting?: boolean; - }; - - if (routerService.externalRouter) { - // Using ember-engines-router-service in an engine. - routerService = routerService.externalRouter; - } - if (routerService._hasMountedSentryPerformanceRouting) { - // Routing listens to route changes on the main router, and should not be initialized multiple times per page. - return; - } - if (!routerService.recognize) { - // Router is missing critical functionality to limit cardinality of the transaction names. - return; - } - routerService._hasMountedSentryPerformanceRouting = true; - _instrumentEmberRouter(routerService, routerMain, config, customStartTransaction, startTransactionOnPageLoad); - }, + const browserTracing = browserTracingIntegration({ idleTimeout, ...browserTracingOptions, }); - if (macroCondition(isTesting())) { - const client = Sentry.getClient(); - - if ( - client && - (client as BrowserClient).getIntegrationByName && - (client as BrowserClient).getIntegrationByName('BrowserTracing') - ) { - // Initializers are called more than once in tests, causing the integrations to not be setup correctly. - return; - } - } + Sentry.disableDefaultBrowserTracingNavigationSpan(); + Sentry.disableDefaultBrowserTracingPageLoadSpan(); + + const client = Sentry.getClient(); + + const isAlreadyInitialized = macroCondition(isTesting()) ? !!client?.getIntegrationByName('BrowserTracing') : false; - const client = Sentry.getClient(); if (client && client.addIntegration) { client.addIntegration(browserTracing); } + // We _always_ call this, as it triggers the page load & navigation spans + _instrumentNavigation(appInstance, config); + + // Skip instrumenting the stuff below again in tests, as these are not reset between tests + if (isAlreadyInitialized) { + return; + } + _instrumentEmberRunloop(config); _instrumentComponents(config); _instrumentInitialLoad(config); } +function _instrumentNavigation(appInstance: ApplicationInstance, config: EmberSentryConfig): void { + // eslint-disable-next-line ember/no-private-routing-service + const routerMain = appInstance.lookup('router:main') as EmberRouterMain; + let routerService = appInstance.lookup('service:router') as RouterService & { + externalRouter?: RouterService; + _hasMountedSentryPerformanceRouting?: boolean; + }; + + if (routerService.externalRouter) { + // Using ember-engines-router-service in an engine. + routerService = routerService.externalRouter; + } + if (routerService._hasMountedSentryPerformanceRouting) { + // Routing listens to route changes on the main router, and should not be initialized multiple times per page. + return; + } + if (!routerService.recognize) { + // Router is missing critical functionality to limit cardinality of the transaction names. + return; + } + + routerService._hasMountedSentryPerformanceRouting = true; + _instrumentEmberRouter(routerService, routerMain, config); +} + export default { initialize, }; diff --git a/packages/ember/addon/types.ts b/packages/ember/addon/types.ts index 787eecc7e4cf..868d6265f62d 100644 --- a/packages/ember/addon/types.ts +++ b/packages/ember/addon/types.ts @@ -31,7 +31,7 @@ export interface EmberRouterMain { rootURL: string; }; } - +/** @deprecated This will be removed in v8. */ export type StartTransactionFunction = (context: TransactionContext) => Transaction | undefined; export type GlobalConfig = { diff --git a/packages/ember/tests/helpers/setup-sentry.ts b/packages/ember/tests/helpers/setup-sentry.ts index d8bb513dcd00..db439d226dc2 100644 --- a/packages/ember/tests/helpers/setup-sentry.ts +++ b/packages/ember/tests/helpers/setup-sentry.ts @@ -1,14 +1,7 @@ -import type RouterService from '@ember/routing/router-service'; import type { TestContext } from '@ember/test-helpers'; import { resetOnerror, setupOnerror } from '@ember/test-helpers'; -import { _instrumentEmberRouter } from '@sentry/ember/instance-initializers/sentry-performance'; -import type { EmberRouterMain, EmberSentryConfig, StartTransactionFunction } from '@sentry/ember/types'; import sinon from 'sinon'; -// Keep a reference to the original startTransaction as the application gets re-initialized and setup for -// the integration doesn't occur again after the first time. -let _routerStartTransaction: StartTransactionFunction | undefined; - export type SentryTestContext = TestContext & { errorMessages: string[]; fetchStub: sinon.SinonStub; @@ -16,11 +9,6 @@ export type SentryTestContext = TestContext & { _windowOnError: OnErrorEventHandler; }; -type SentryRouterService = RouterService & { - _startTransaction: StartTransactionFunction; - _sentryInstrumented?: boolean; -}; - export function setupSentryTest(hooks: NestedHooks): void { hooks.beforeEach(async function (this: SentryTestContext) { await window._sentryPerformanceLoad; @@ -28,16 +16,6 @@ export function setupSentryTest(hooks: NestedHooks): void { const errorMessages: string[] = []; this.errorMessages = errorMessages; - // eslint-disable-next-line ember/no-private-routing-service - const routerMain = this.owner.lookup('router:main') as EmberRouterMain; - const routerService = this.owner.lookup('service:router') as SentryRouterService; - - if (routerService._sentryInstrumented) { - _routerStartTransaction = routerService._startTransaction; - } else if (_routerStartTransaction) { - _instrumentEmberRouter(routerService, routerMain, {} as EmberSentryConfig, _routerStartTransaction); - } - /** * Stub out fetch function to assert on Sentry calls. */ diff --git a/packages/ember/tests/helpers/utils.ts b/packages/ember/tests/helpers/utils.ts index a14088a2329e..16a34bde340c 100644 --- a/packages/ember/tests/helpers/utils.ts +++ b/packages/ember/tests/helpers/utils.ts @@ -58,8 +58,8 @@ export function assertSentryTransactions( const sentryTestEvents = getTestSentryTransactions(); const event = sentryTestEvents[callNumber]; - assert.ok(event); - assert.ok(event.spans); + assert.ok(event, 'event exists'); + assert.ok(event.spans, 'event has spans'); const spans = event.spans || []; From 8fb1500a70ae09b85948fff158aff89b250c47ab Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 30 Jan 2024 11:21:53 +0100 Subject: [PATCH 2/3] fix --- .../instance-initializers/sentry-performance.ts | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/packages/ember/addon/instance-initializers/sentry-performance.ts b/packages/ember/addon/instance-initializers/sentry-performance.ts index 3de0504ba3d8..a7a9dbfc9532 100644 --- a/packages/ember/addon/instance-initializers/sentry-performance.ts +++ b/packages/ember/addon/instance-initializers/sentry-performance.ts @@ -99,11 +99,13 @@ export function _instrumentEmberRouter( let activeRootSpan: Span | undefined; let transitionSpan: Span | undefined; + // Maintaining backwards compatibility with config.browserTracingOptions, but passing it with Sentry options is preferred. + const browserTracingOptions = config.browserTracingOptions || config.sentry.browserTracingOptions || {}; const url = getLocationURL(location); - if (url) { + if (url && browserTracingOptions.startTransactionOnPageLoad !== false) { const routeInfo = routerService.recognize(url); - Sentry.startBrowserTracingNavigationSpan({ + Sentry.startBrowserTracingPageLoadSpan({ name: `route:${routeInfo.name}`, op: 'pageload', origin: 'auto.pageload.ember', @@ -124,6 +126,10 @@ export function _instrumentEmberRouter( getBackburner().off('end', finishActiveTransaction); }; + if (browserTracingOptions.startTransactionOnLocationChange === false) { + return; + } + routerService.on('routeWillChange', (transition: Transition) => { const { fromRoute, toRoute } = getTransitionInformation(transition, routerService); activeRootSpan?.end(); @@ -405,11 +411,10 @@ export async function instrumentForPerformance(appInstance: ApplicationInstance) const browserTracing = browserTracingIntegration({ idleTimeout, ...browserTracingOptions, + instrumentNavigation: false, + instrumentPageLoad: false, }); - Sentry.disableDefaultBrowserTracingNavigationSpan(); - Sentry.disableDefaultBrowserTracingPageLoadSpan(); - const client = Sentry.getClient(); const isAlreadyInitialized = macroCondition(isTesting()) ? !!client?.getIntegrationByName('BrowserTracing') : false; From 828896b053512d1ad305abe0f79c3ddaca2a35af Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 30 Jan 2024 15:16:46 +0100 Subject: [PATCH 3/3] fix for changes --- .../addon/instance-initializers/sentry-performance.ts | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/packages/ember/addon/instance-initializers/sentry-performance.ts b/packages/ember/addon/instance-initializers/sentry-performance.ts index a7a9dbfc9532..c86e280d167b 100644 --- a/packages/ember/addon/instance-initializers/sentry-performance.ts +++ b/packages/ember/addon/instance-initializers/sentry-performance.ts @@ -103,9 +103,15 @@ export function _instrumentEmberRouter( const browserTracingOptions = config.browserTracingOptions || config.sentry.browserTracingOptions || {}; const url = getLocationURL(location); + const client = Sentry.getClient(); + + if (!client) { + return; + } + if (url && browserTracingOptions.startTransactionOnPageLoad !== false) { const routeInfo = routerService.recognize(url); - Sentry.startBrowserTracingPageLoadSpan({ + Sentry.startBrowserTracingPageLoadSpan(client, { name: `route:${routeInfo.name}`, op: 'pageload', origin: 'auto.pageload.ember', @@ -134,7 +140,7 @@ export function _instrumentEmberRouter( const { fromRoute, toRoute } = getTransitionInformation(transition, routerService); activeRootSpan?.end(); - Sentry.startBrowserTracingNavigationSpan({ + Sentry.startBrowserTracingNavigationSpan(client, { name: `route:${toRoute}`, op: 'navigation', origin: 'auto.navigation.ember',