From 7740df1421c571eaf69c8cdbb9a1d77ab13e62b8 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 3 Apr 2023 12:58:18 +0200 Subject: [PATCH 1/3] fix(ember): Ensure only one client is created With the new `addIntegration` API, we don't need to create multiple clients anymore, which also potentially broke replays. --- .../sentry-performance.ts | 93 +++++++++---------- .../tests/acceptance/sentry-replay-test.js | 20 ++++ packages/ember/tests/dummy/app/app.js | 2 + packages/ember/tests/dummy/app/router.js | 1 + .../ember/tests/dummy/app/routes/replay.js | 13 +++ .../tests/dummy/app/templates/application.hbs | 1 + .../tests/dummy/app/templates/replay.hbs | 1 + .../ember/tests/dummy/config/environment.js | 6 +- 8 files changed, 86 insertions(+), 51 deletions(-) create mode 100644 packages/ember/tests/acceptance/sentry-replay-test.js create mode 100644 packages/ember/tests/dummy/app/routes/replay.js create mode 100644 packages/ember/tests/dummy/app/templates/replay.hbs diff --git a/packages/ember/addon/instance-initializers/sentry-performance.ts b/packages/ember/addon/instance-initializers/sentry-performance.ts index 0001e6c5f97c..146f0273eeb1 100644 --- a/packages/ember/addon/instance-initializers/sentry-performance.ts +++ b/packages/ember/addon/instance-initializers/sentry-performance.ts @@ -390,57 +390,56 @@ export async function instrumentForPerformance(appInstance: ApplicationInstance) const idleTimeout = config.transitionTimeout || 5000; - const existingIntegrations = (sentryConfig['integrations'] || []) as Integration[]; - - sentryConfig['integrations'] = [ - ...existingIntegrations, - new BrowserTracing({ - routingInstrumentation: (customStartTransaction, startTransactionOnPageLoad) => { - const routerMain = appInstance.lookup('router:main'); - 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); - }, - idleTimeout, - ...browserTracingOptions, - }), - ]; - - class FakeBrowserTracingClass { - static id = 'BrowserTracing'; - public name = FakeBrowserTracingClass.id; - setupOnce() { - // noop - We're just faking this class for a lookup + const browserTracing = new BrowserTracing({ + routingInstrumentation: (customStartTransaction, startTransactionOnPageLoad) => { + const routerMain = appInstance.lookup('router:main'); + 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); + }, + idleTimeout, + ...browserTracingOptions, + }); + + if (macroCondition(isTesting())) { + class FakeBrowserTracingClass { + static id = 'BrowserTracing'; + public name = FakeBrowserTracingClass.id; + setupOnce() { + // noop - We're just faking this class for a lookup + } } - } - if ( - isTesting() && - Sentry.getCurrentHub()?.getIntegration( - // This is a temporary hack because the BrowserTracing integration cannot have a static `id` field for tree - // shaking reasons. However, `getIntegration` needs that field. - FakeBrowserTracingClass, - ) - ) { - // Initializers are called more than once in tests, causing the integrations to not be setup correctly. - return; + if ( + Sentry.getCurrentHub()?.getIntegration( + // This is a temporary hack because the BrowserTracing integration cannot have a static `id` field for tree + // shaking reasons. However, `getIntegration` needs that field. + FakeBrowserTracingClass, + ) + ) { + // Initializers are called more than once in tests, causing the integrations to not be setup correctly. + return; + } } - Sentry.init(sentryConfig); // Call init again to rebind client with new integration list in addition to the defaults + const client = Sentry.getCurrentHub().getClient(); + if (client && client.addIntegration) { + client.addIntegration(browserTracing); + } _instrumentEmberRunloop(config); _instrumentComponents(config); diff --git a/packages/ember/tests/acceptance/sentry-replay-test.js b/packages/ember/tests/acceptance/sentry-replay-test.js new file mode 100644 index 000000000000..f6c7272a7e62 --- /dev/null +++ b/packages/ember/tests/acceptance/sentry-replay-test.js @@ -0,0 +1,20 @@ +import { test, module } from 'qunit'; +import { setupApplicationTest } from 'ember-qunit'; +import { visit } from '@ember/test-helpers'; +import { setupSentryTest } from '../helpers/setup-sentry'; +import * as Sentry from '@sentry/ember'; + +module('Acceptance | Sentry Session Replay', function (hooks) { + setupApplicationTest(hooks); + setupSentryTest(hooks); + + test('Test replay', async function (assert) { + await visit('/replay'); + + const replay = Sentry.getCurrentHub().getIntegration(Sentry.Replay); + assert.ok(replay); + + assert.true(replay._replay.isEnabled()); + assert.false(replay._replay.isPaused()); + }); +}); diff --git a/packages/ember/tests/dummy/app/app.js b/packages/ember/tests/dummy/app/app.js index 54af0233c190..322931b8d45a 100644 --- a/packages/ember/tests/dummy/app/app.js +++ b/packages/ember/tests/dummy/app/app.js @@ -5,6 +5,8 @@ import config from './config/environment'; import * as Sentry from '@sentry/ember'; Sentry.init({ + replaysSessionSampleRate: 1, + replaysOnErrorSampleRate: 1, browserTracingOptions: { _experiments: { // This lead to some flaky tests, as that is sometimes logged diff --git a/packages/ember/tests/dummy/app/router.js b/packages/ember/tests/dummy/app/router.js index 5e5dd54a4db2..51b41107869e 100644 --- a/packages/ember/tests/dummy/app/router.js +++ b/packages/ember/tests/dummy/app/router.js @@ -8,6 +8,7 @@ export default class Router extends EmberRouter { Router.map(function () { this.route('tracing'); + this.route('replay'); this.route('slow-loading-route', function () { this.route('index', { path: '/' }); }); diff --git a/packages/ember/tests/dummy/app/routes/replay.js b/packages/ember/tests/dummy/app/routes/replay.js new file mode 100644 index 000000000000..ed3a95e9cee7 --- /dev/null +++ b/packages/ember/tests/dummy/app/routes/replay.js @@ -0,0 +1,13 @@ +import Route from '@ember/routing/route'; +import * as Sentry from '@sentry/ember'; + +export default class ReplayRoute extends Route { + async beforeModel() { + const { Replay } = Sentry; + + if (!Sentry.getCurrentHub().getIntegration(Replay)) { + const client = Sentry.getCurrentHub().getClient(); + client.addIntegration(new Replay()); + } + } +} diff --git a/packages/ember/tests/dummy/app/templates/application.hbs b/packages/ember/tests/dummy/app/templates/application.hbs index c924cade6da2..09e6d2fffbb3 100644 --- a/packages/ember/tests/dummy/app/templates/application.hbs +++ b/packages/ember/tests/dummy/app/templates/application.hbs @@ -11,6 +11,7 @@
Errors Tracing + Replay
{{outlet}} diff --git a/packages/ember/tests/dummy/app/templates/replay.hbs b/packages/ember/tests/dummy/app/templates/replay.hbs new file mode 100644 index 000000000000..effb884d8f5f --- /dev/null +++ b/packages/ember/tests/dummy/app/templates/replay.hbs @@ -0,0 +1 @@ +

Visiting this page starts Replay!

diff --git a/packages/ember/tests/dummy/config/environment.js b/packages/ember/tests/dummy/config/environment.js index 0d5f17a1c3e8..d80432e025b6 100644 --- a/packages/ember/tests/dummy/config/environment.js +++ b/packages/ember/tests/dummy/config/environment.js @@ -22,7 +22,8 @@ module.exports = function (environment) { ENV['@sentry/ember'] = { sentry: { tracesSampleRate: 1, - dsn: process.env.SENTRY_DSN, + // Include fake dsn so that instrumentation is enabled when running from cli + dsn: process.env.SENTRY_DSN || 'https://0@0.ingest.sentry.io/0', browserTracingOptions: { tracingOrigins: ['localhost', 'doesntexist.example'], }, @@ -50,9 +51,6 @@ module.exports = function (environment) { ENV.APP.rootElement = '#ember-testing'; ENV.APP.autoboot = false; - - // Include fake dsn so that instrumentation is enabled when running from cli - ENV['@sentry/ember'].sentry.dsn = 'https://0@0.ingest.sentry.io/0'; } if (environment === 'production') { From 692275677f8e6e2df9dee0ae8570f381841ba4ef Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 4 Apr 2023 10:22:45 +0200 Subject: [PATCH 2/3] replace hacky check --- .../instance-initializers/sentry-performance.ts | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/packages/ember/addon/instance-initializers/sentry-performance.ts b/packages/ember/addon/instance-initializers/sentry-performance.ts index 146f0273eeb1..33cd2ebee2a1 100644 --- a/packages/ember/addon/instance-initializers/sentry-performance.ts +++ b/packages/ember/addon/instance-initializers/sentry-performance.ts @@ -10,6 +10,7 @@ import { browserPerformanceTimeOrigin, GLOBAL_OBJ, timestampWithMs } from '@sent import { macroCondition, isTesting, getOwnConfig } from '@embroider/macros'; import { EmberSentryConfig, GlobalConfig, OwnConfig } from '../types'; import RouterService from '@ember/routing/router-service'; +import { BaseClient } from '@sentry/core'; function getSentryConfig() { const _global = GLOBAL_OBJ as typeof GLOBAL_OBJ & GlobalConfig; @@ -416,20 +417,12 @@ export async function instrumentForPerformance(appInstance: ApplicationInstance) }); if (macroCondition(isTesting())) { - class FakeBrowserTracingClass { - static id = 'BrowserTracing'; - public name = FakeBrowserTracingClass.id; - setupOnce() { - // noop - We're just faking this class for a lookup - } - } + const client = Sentry.getCurrentHub().getClient(); if ( - Sentry.getCurrentHub()?.getIntegration( - // This is a temporary hack because the BrowserTracing integration cannot have a static `id` field for tree - // shaking reasons. However, `getIntegration` needs that field. - FakeBrowserTracingClass, - ) + client && + (client as BaseClient).getIntegrationById && + (client as BaseClient).getIntegrationById('BrowserTracing') ) { // Initializers are called more than once in tests, causing the integrations to not be setup correctly. return; From 81bbfb540cf061c92c3e9e8e2fbf5a882304e463 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 4 Apr 2023 11:21:00 +0200 Subject: [PATCH 3/3] fix linting --- .../ember/addon/instance-initializers/sentry-performance.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/ember/addon/instance-initializers/sentry-performance.ts b/packages/ember/addon/instance-initializers/sentry-performance.ts index 33cd2ebee2a1..35725d503c77 100644 --- a/packages/ember/addon/instance-initializers/sentry-performance.ts +++ b/packages/ember/addon/instance-initializers/sentry-performance.ts @@ -3,14 +3,14 @@ import { run, _backburner, scheduleOnce } from '@ember/runloop'; import { subscribe } from '@ember/instrumentation'; import * as Sentry from '@sentry/browser'; import { ExtendedBackburner } from '@sentry/ember/runloop'; -import { Span, Transaction, Integration } from '@sentry/types'; +import { Span, Transaction } from '@sentry/types'; import { EmberRunQueues } from '@ember/runloop/-private/types'; import { getActiveTransaction } from '..'; import { browserPerformanceTimeOrigin, GLOBAL_OBJ, timestampWithMs } from '@sentry/utils'; import { macroCondition, isTesting, getOwnConfig } from '@embroider/macros'; import { EmberSentryConfig, GlobalConfig, OwnConfig } from '../types'; import RouterService from '@ember/routing/router-service'; -import { BaseClient } from '@sentry/core'; +import type { BaseClient } from '@sentry/core'; function getSentryConfig() { const _global = GLOBAL_OBJ as typeof GLOBAL_OBJ & GlobalConfig;