From 9f606dc70acdc764119f52916a4008bb4285c528 Mon Sep 17 00:00:00 2001
From: Francesco Novy <francesco.novy@sentry.io>
Date: Tue, 30 Apr 2024 09:55:46 +0200
Subject: [PATCH] fix(ember): Ensure unnecessary spans are avoided

add test

fix(ember): Ensure spans & state is correctly handled
---
 packages/ember/addon/index.ts                 |  4 +-
 .../sentry-performance.ts                     | 25 ++++++++++-
 .../acceptance/sentry-performance-test.ts     | 43 ++++++++++++++++++-
 packages/ember/tests/dummy/app/router.ts      |  8 ++++
 .../dummy/app/routes/with-error/error.ts      | 11 +++++
 .../dummy/app/routes/with-error/index.ts      | 10 +++++
 .../dummy/app/routes/with-loading/index.ts    | 12 ++++++
 .../tests/dummy/app/templates/application.hbs |  2 +
 .../tests/dummy/app/templates/with-error.hbs  |  1 +
 .../dummy/app/templates/with-error/error.hbs  |  1 +
 .../dummy/app/templates/with-error/index.hbs  |  1 +
 .../dummy/app/templates/with-loading.hbs      |  1 +
 .../app/templates/with-loading/index.hbs      |  1 +
 .../app/templates/with-loading/loading.hbs    |  1 +
 14 files changed, 117 insertions(+), 4 deletions(-)
 create mode 100644 packages/ember/tests/dummy/app/routes/with-error/error.ts
 create mode 100644 packages/ember/tests/dummy/app/routes/with-error/index.ts
 create mode 100644 packages/ember/tests/dummy/app/routes/with-loading/index.ts
 create mode 100644 packages/ember/tests/dummy/app/templates/with-error.hbs
 create mode 100644 packages/ember/tests/dummy/app/templates/with-error/error.hbs
 create mode 100644 packages/ember/tests/dummy/app/templates/with-error/index.hbs
 create mode 100644 packages/ember/tests/dummy/app/templates/with-loading.hbs
 create mode 100644 packages/ember/tests/dummy/app/templates/with-loading/index.hbs
 create mode 100644 packages/ember/tests/dummy/app/templates/with-loading/loading.hbs

diff --git a/packages/ember/addon/index.ts b/packages/ember/addon/index.ts
index f17e4749ceb7..33ba495e5acc 100644
--- a/packages/ember/addon/index.ts
+++ b/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);
diff --git a/packages/ember/addon/instance-initializers/sentry-performance.ts b/packages/ember/addon/instance-initializers/sentry-performance.ts
index 7db6e386c192..f8d03a025217 100644
--- a/packages/ember/addon/instance-initializers/sentry-performance.ts
+++ b/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';
+}
diff --git a/packages/ember/tests/acceptance/sentry-performance-test.ts b/packages/ember/tests/acceptance/sentry-performance-test.ts
index db22ff063c21..7ab6a2bf3dd3 100644
--- a/packages/ember/tests/acceptance/sentry-performance-test.ts
+++ b/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',
+      },
+    });
+  });
 });
diff --git a/packages/ember/tests/dummy/app/router.ts b/packages/ember/tests/dummy/app/router.ts
index e13dec6d82c5..fde2b0dba15e 100644
--- a/packages/ember/tests/dummy/app/router.ts
+++ b/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: '/' });
+  });
 });
diff --git a/packages/ember/tests/dummy/app/routes/with-error/error.ts b/packages/ember/tests/dummy/app/routes/with-error/error.ts
new file mode 100644
index 000000000000..057afce5fb5e
--- /dev/null
+++ b/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...
+  }
+}
diff --git a/packages/ember/tests/dummy/app/routes/with-error/index.ts b/packages/ember/tests/dummy/app/routes/with-error/index.ts
new file mode 100644
index 000000000000..e33d0f528792
--- /dev/null
+++ b/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);
diff --git a/packages/ember/tests/dummy/app/routes/with-loading/index.ts b/packages/ember/tests/dummy/app/routes/with-loading/index.ts
new file mode 100644
index 000000000000..b9cc53907855
--- /dev/null
+++ b/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);
diff --git a/packages/ember/tests/dummy/app/templates/application.hbs b/packages/ember/tests/dummy/app/templates/application.hbs
index 09e6d2fffbb3..1e00e135f754 100644
--- a/packages/ember/tests/dummy/app/templates/application.hbs
+++ b/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}}
diff --git a/packages/ember/tests/dummy/app/templates/with-error.hbs b/packages/ember/tests/dummy/app/templates/with-error.hbs
new file mode 100644
index 000000000000..c24cd68950a9
--- /dev/null
+++ b/packages/ember/tests/dummy/app/templates/with-error.hbs
@@ -0,0 +1 @@
+{{outlet}}
diff --git a/packages/ember/tests/dummy/app/templates/with-error/error.hbs b/packages/ember/tests/dummy/app/templates/with-error/error.hbs
new file mode 100644
index 000000000000..91e84d524602
--- /dev/null
+++ b/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>
diff --git a/packages/ember/tests/dummy/app/templates/with-error/index.hbs b/packages/ember/tests/dummy/app/templates/with-error/index.hbs
new file mode 100644
index 000000000000..31906f8ff04d
--- /dev/null
+++ b/packages/ember/tests/dummy/app/templates/with-error/index.hbs
@@ -0,0 +1 @@
+Page loaded!
diff --git a/packages/ember/tests/dummy/app/templates/with-loading.hbs b/packages/ember/tests/dummy/app/templates/with-loading.hbs
new file mode 100644
index 000000000000..c24cd68950a9
--- /dev/null
+++ b/packages/ember/tests/dummy/app/templates/with-loading.hbs
@@ -0,0 +1 @@
+{{outlet}}
diff --git a/packages/ember/tests/dummy/app/templates/with-loading/index.hbs b/packages/ember/tests/dummy/app/templates/with-loading/index.hbs
new file mode 100644
index 000000000000..31906f8ff04d
--- /dev/null
+++ b/packages/ember/tests/dummy/app/templates/with-loading/index.hbs
@@ -0,0 +1 @@
+Page loaded!
diff --git a/packages/ember/tests/dummy/app/templates/with-loading/loading.hbs b/packages/ember/tests/dummy/app/templates/with-loading/loading.hbs
new file mode 100644
index 000000000000..1f9b5fa806bd
--- /dev/null
+++ b/packages/ember/tests/dummy/app/templates/with-loading/loading.hbs
@@ -0,0 +1 @@
+Loading page...