fix(ember): Ensure unnecessary spans are avoided (#11846)

Extracted out from
#11827.

I added tests for loading & error states as well, to ensure this does
not regress.

Author: mydea

packages/ember/addon/index.ts

@@ -5,7 +5,7 @@ import { getOwnConfig, isDevelopingApp, macroCondition } from '@embroider/macros
 import { startSpan } from '@sentry/browser';
 import type { BrowserOptions } from '@sentry/browser';
 import * as Sentry from '@sentry/browser';
-import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, applySdkMetadata } from '@sentry/core';
+import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, applySdkMetadata } from '@sentry/core';
 import { GLOBAL_OBJ } from '@sentry/utils';
 import Ember from 'ember';
 
@@ -75,9 +75,11 @@ export const instrumentRoutePerformance = <T extends RouteConstructor>(BaseRoute
       {
         attributes: {
           [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: source,
+          [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.ui.ember',
         },
         op,
         name,
+        onlyIfParent: true,
       },
       () => {
         return fn(...args);

packages/ember/addon/instance-initializers/sentry-performance.ts

@@ -145,6 +145,12 @@ export function _instrumentEmberRouter(
 
   routerService.on('routeWillChange', (transition: Transition) => {
     const { fromRoute, toRoute } = getTransitionInformation(transition, routerService);
+
+    // We want to ignore loading && error routes
+    if (transitionIsIntermediate(transition)) {
+      return;
+    }
+
     activeRootSpan?.end();
 
     activeRootSpan = startBrowserTracingNavigationSpan(client, {
@@ -167,8 +173,8 @@ export function _instrumentEmberRouter(
     });
   });
 
-  routerService.on('routeDidChange', () => {
-    if (!transitionSpan || !activeRootSpan) {
+  routerService.on('routeDidChange', transition => {
+    if (!transitionSpan || !activeRootSpan || transitionIsIntermediate(transition)) {
       return;
     }
     transitionSpan.end();
@@ -492,3 +498,18 @@ function _instrumentNavigation(
 export default {
   initialize,
 };
+
+function transitionIsIntermediate(transition: Transition): boolean {
+  //  We want to use ignore, as this may actually be defined on new versions
+  // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+  // @ts-ignore This actually exists on newer versions
+  const isIntermediate: boolean | undefined = transition.isIntermediate;
+
+  if (typeof isIntermediate === 'boolean') {
+    return isIntermediate;
+  }
+
+  // For versions without this, we look if the route is a `.loading` or `.error` route
+  // This is not perfect and may false-positive in some cases, but it's the best we can do
+  return transition.to?.localName === 'loading' || transition.to?.localName === 'error';
+}

packages/ember/tests/acceptance/sentry-performance-test.ts

@@ -1,4 +1,4 @@
-import { click, visit } from '@ember/test-helpers';
+import { click, find, visit } from '@ember/test-helpers';
 import { setupApplicationTest } from 'ember-qunit';
 import { module, test } from 'qunit';
 
@@ -56,4 +56,45 @@ module('Acceptance | Sentry Performance', function (hooks) {
       },
     });
   });
+
+  test('Test page with loading state', async function (assert) {
+    await visit('/with-loading');
+
+    assertSentryTransactionCount(assert, 1);
+    assertSentryTransactions(assert, 0, {
+      spans: [
+        'ui.ember.transition | route:undefined -> route:with-loading.index',
+        'ui.ember.route.before_model | with-loading.index',
+        'ui.ember.route.model | with-loading.index',
+        'ui.ember.route.after_model | with-loading.index',
+        'ui.ember.route.setup_controller | with-loading.index',
+      ],
+      transaction: 'route:with-loading.index',
+      attributes: {
+        fromRoute: undefined,
+        toRoute: 'with-loading.index',
+      },
+    });
+  });
+
+  test('Test page with error state', async function (assert) {
+    await visit('/with-error');
+
+    // Ensure we are on error page
+    assert.ok(find('#test-page-load-error'));
+
+    assertSentryTransactionCount(assert, 1);
+    assertSentryTransactions(assert, 0, {
+      spans: [
+        'ui.ember.transition | route:undefined -> route:with-error.index',
+        'ui.ember.route.before_model | with-error.index',
+        'ui.ember.route.model | with-error.index',
+      ],
+      transaction: 'route:with-error.index',
+      attributes: {
+        fromRoute: undefined,
+        toRoute: 'with-error.index',
+      },
+    });
+  });
 });

packages/ember/tests/dummy/app/router.ts

@@ -15,4 +15,12 @@ Router.map(function () {
   this.route('slow-loading-route', function () {
     this.route('index', { path: '/' });
   });
+
+  this.route('with-loading', function () {
+    this.route('index', { path: '/' });
+  });
+
+  this.route('with-error', function () {
+    this.route('index', { path: '/' });
+  });
 });

packages/ember/tests/dummy/app/routes/with-error/error.ts

@@ -0,0 +1,11 @@
+import Route from '@ember/routing/route';
+
+export default class WithErrorErrorRoute extends Route {
+  public model(): void {
+    // Just swallow the error...
+  }
+
+  public setupController() {
+    // Just swallow the error...
+  }
+}

packages/ember/tests/dummy/app/routes/with-error/index.ts

@@ -0,0 +1,10 @@
+import Route from '@ember/routing/route';
+import { instrumentRoutePerformance } from '@sentry/ember';
+
+class WithErrorIndexRoute extends Route {
+  public model(): Promise<void> {
+    return Promise.reject('Test error');
+  }
+}
+
+export default instrumentRoutePerformance(WithErrorIndexRoute);

packages/ember/tests/dummy/app/routes/with-loading/index.ts

@@ -0,0 +1,12 @@
+import Route from '@ember/routing/route';
+import { instrumentRoutePerformance } from '@sentry/ember';
+
+import timeout from '../../helpers/utils';
+
+class WithLoadingIndexRoute extends Route {
+  public model(): Promise<void> {
+    return timeout(1000);
+  }
+}
+
+export default instrumentRoutePerformance(WithLoadingIndexRoute);

packages/ember/tests/dummy/app/templates/application.hbs

@@ -12,6 +12,8 @@
           <Link @route='index'>Errors</Link>
           <Link @route='tracing'>Tracing</Link>
           <Link @route='replay'>Replay</Link>
+          <Link @route='with-loading'>With Loading</Link>
+          <Link @route='with-error'>With Error</Link>
         </div>
         <div class="content-container">
           {{outlet}}

packages/ember/tests/dummy/app/templates/with-error.hbs

@@ -0,0 +1 @@
+{{outlet}}

packages/ember/tests/dummy/app/templates/with-error/error.hbs

@@ -0,0 +1 @@
+<div id='test-page-load-error'>Error when loading the page!</div>

packages/ember/tests/dummy/app/templates/with-error/index.hbs

@@ -0,0 +1 @@
+Page loaded!

packages/ember/tests/dummy/app/templates/with-loading.hbs

@@ -0,0 +1 @@
+{{outlet}}

packages/ember/tests/dummy/app/templates/with-loading/index.hbs

@@ -0,0 +1 @@
+Page loaded!

packages/ember/tests/dummy/app/templates/with-loading/loading.hbs

@@ -0,0 +1 @@
+Loading page...