fix(vue): Don't overwrite custom transaction names of pageload transa…

…ctions (#6060) As brought to our attention in #6048, our pageload transaction start change (which makes the transaction start earlier) introduced in #5983 caused custom transaction names to be overwritten when `beforeNavigate` is used to update the transaction name. This patch fixes this problem by checking for the transaction source before (not) updating the current transaction name with the resolved route name once the router's `beforeEach` hook was called. Furthermore, it adds a test to cover this case. See #6048 (comment) for the motivation for creating this PR.
getsentry · Oct 27, 2022 · b5497c7 · b5497c7
1 parent 0a22a61
commit b5497c7
Show file tree

Hide file tree

Showing 2 changed files with 54 additions and 3 deletions.
diff --git a/packages/vue/src/router.ts b/packages/vue/src/router.ts
@@ -92,7 +92,9 @@ export function vueRouterInstrumentation(router: VueRouter): VueRouterInstrument
       if (startTransactionOnPageLoad && isPageLoadNavigation) {
         const pageloadTransaction = getActiveTransaction();
         if (pageloadTransaction) {
-          pageloadTransaction.setName(transactionName, transactionSource);
+          if (pageloadTransaction.metadata.source !== 'custom') {
+            pageloadTransaction.setName(transactionName, transactionSource);
+          }
           pageloadTransaction.setData('params', data.params);
           pageloadTransaction.setData('query', data.query);
         }

diff --git a/packages/vue/test/router.test.ts b/packages/vue/test/router.test.ts
@@ -123,10 +123,11 @@ describe('vueRouterInstrumentation()', () => {
     ['initialPageloadRoute', 'unmatchedRoute', '/e8733846-20ac-488c-9871-a5cbcb647294', 'url'],
   ])(
     'should return instrumentation that instruments VueRouter.beforeEach(%s, %s) for pageloads',
-    (fromKey, toKey, _transactionName, _transactionSource) => {
+    (fromKey, toKey, transactionName, transactionSource) => {
       const mockedTxn = {
         setName: jest.fn(),
         setData: jest.fn(),
+        metadata: {},
       };
       const customMockStartTxn = { ...mockStartTransaction }.mockImplementation(_ => {
         return mockedTxn;
@@ -160,14 +161,62 @@ describe('vueRouterInstrumentation()', () => {
       beforeEachCallback(to, from, mockNext);
       expect(mockVueRouter.beforeEach).toHaveBeenCalledTimes(1);
 
-      expect(mockedTxn.setName).toHaveBeenCalledWith(_transactionName, _transactionSource);
+      expect(mockedTxn.setName).toHaveBeenCalledWith(transactionName, transactionSource);
       expect(mockedTxn.setData).toHaveBeenNthCalledWith(1, 'params', to.params);
       expect(mockedTxn.setData).toHaveBeenNthCalledWith(2, 'query', to.query);
 
       expect(mockNext).toHaveBeenCalledTimes(1);
     },
   );
 
+  it("doesn't overwrite a pageload transaction name it was set to custom before the router resolved the route", () => {
+    const mockedTxn = {
+      setName: jest.fn(),
+      setData: jest.fn(),
+      name: '',
+      metadata: {
+        source: 'url',
+      },
+    };
+    const customMockStartTxn = { ...mockStartTransaction }.mockImplementation(_ => {
+      return mockedTxn;
+    });
+    jest.spyOn(vueTracing, 'getActiveTransaction').mockImplementation(() => mockedTxn as unknown as Transaction);
+
+    // create instrumentation
+    const instrument = vueRouterInstrumentation(mockVueRouter);
+
+    // instrument
+    instrument(customMockStartTxn, true, true);
+
+    // check for transaction start
+    expect(customMockStartTxn).toHaveBeenCalledTimes(1);
+    expect(customMockStartTxn).toHaveBeenCalledWith({
+      name: '/',
+      metadata: {
+        source: 'url',
+      },
+      op: 'pageload',
+      tags: {
+        'routing.instrumentation': 'vue-router',
+      },
+    });
+
+    // now we give the transaction a custom name, thereby simulating what would
+    // happen when users use the `beforeNavigate` hook
+    mockedTxn.name = 'customTxnName';
+    mockedTxn.metadata.source = 'custom';
+
+    const beforeEachCallback = mockVueRouter.beforeEach.mock.calls[0][0];
+    beforeEachCallback(testRoutes['normalRoute1'], testRoutes['initialPageloadRoute'], mockNext);
+
+    expect(mockVueRouter.beforeEach).toHaveBeenCalledTimes(1);
+
+    expect(mockedTxn.setName).not.toHaveBeenCalled();
+    expect(mockedTxn.metadata.source).toEqual('custom');
+    expect(mockedTxn.name).toEqual('customTxnName');
+  });
+
   test.each([
     [undefined, 1],
     [false, 0],