ref(tracing): Use previous source when logging name changes (#5733)

AbhiPrasad · web-flow · commit 51bc032d6afa · 2022-09-15T11:50:04.000Z
Use previous source instead of current one when tracking name changes. To make sure this is always accurate, set the default source in the transaction constructor of `custom`.
diff --git a/packages/hub/src/exports.ts b/packages/hub/src/exports.ts
@@ -181,11 +181,5 @@ export function startTransaction(
   context: TransactionContext,
   customSamplingContext?: CustomSamplingContext,
 ): ReturnType<Hub['startTransaction']> {
-  return getCurrentHub().startTransaction(
-    {
-      metadata: { source: 'custom' },
-      ...context,
-    },
-    customSamplingContext,
-  );
+  return getCurrentHub().startTransaction({ ...context }, customSamplingContext);
 }
diff --git a/packages/hub/test/exports.test.ts b/packages/hub/test/exports.test.ts
@@ -10,7 +10,6 @@ import {
   setTag,
   setTags,
   setUser,
-  startTransaction,
   withScope,
 } from '../src/exports';
 
@@ -186,35 +185,6 @@ describe('Top Level API', () => {
     });
   });
 
-  describe('startTransaction', () => {
-    beforeEach(() => {
-      global.__SENTRY__ = {
-        hub: undefined,
-        extensions: {
-          startTransaction: (context: any) => ({
-            name: context.name,
-            // Spread rather than assign in case it's undefined
-            metadata: { ...context.metadata },
-          }),
-        },
-      };
-    });
-
-    it("sets source to `'custom'` if no source provided", () => {
-      const transaction = startTransaction({ name: 'dogpark' });
-
-      expect(transaction.name).toEqual('dogpark');
-      expect(transaction.metadata.source).toEqual('custom');
-    });
-
-    it('uses given `source` value', () => {
-      const transaction = startTransaction({ name: 'dogpark', metadata: { source: 'route' } });
-
-      expect(transaction.name).toEqual('dogpark');
-      expect(transaction.metadata.source).toEqual('route');
-    });
-  });
-
   test('Clear Scope', () => {
     const client: any = new TestClient({});
     getCurrentHub().withScope(() => {
diff --git a/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out-bad-tx-name/server.ts b/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out-bad-tx-name/server.ts
diff --git a/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out-bad-tx-name/test.ts b/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out-bad-tx-name/test.ts
diff --git a/packages/node-integration-tests/suites/express/tracing/test.ts b/packages/node-integration-tests/suites/express/tracing/test.ts
@@ -41,7 +41,7 @@ test('should set a correct transaction name for routes specified in RegEx', asyn
       changes: [
         {
           propagations: 0,
-          source: 'route',
+          source: 'url',
           timestamp: expect.any(Number),
         },
       ],
@@ -77,7 +77,7 @@ test.each([['array1'], ['array5']])(
         changes: [
           {
             propagations: 0,
-            source: 'route',
+            source: 'url',
             timestamp: expect.any(Number),
           },
         ],
@@ -121,7 +121,7 @@ test.each([
       changes: [
         {
           propagations: 0,
-          source: 'route',
+          source: 'url',
           timestamp: expect.any(Number),
         },
       ],
diff --git a/packages/node/test/integrations/http.test.ts b/packages/node/test/integrations/http.test.ts
@@ -119,7 +119,7 @@ describe('tracing', () => {
     const baggageHeader = request.getHeader('baggage') as string;
 
     expect(baggageHeader).toEqual(
-      'sentry-environment=production,sentry-release=1.0.0,' +
+      'sentry-environment=production,sentry-release=1.0.0,sentry-transaction=dogpark,' +
         'sentry-user_segment=segmentA,sentry-public_key=dogsarebadatkeepingsecrets,' +
         'sentry-trace_id=12312012123120121231201212312012,sentry-sample_rate=1',
     );
@@ -135,14 +135,14 @@ describe('tracing', () => {
 
     expect(baggageHeader).toEqual([
       'dog=great',
-      'sentry-environment=production,sentry-release=1.0.0,sentry-user_segment=segmentA,sentry-public_key=dogsarebadatkeepingsecrets,sentry-trace_id=12312012123120121231201212312012,sentry-sample_rate=1',
+      'sentry-environment=production,sentry-release=1.0.0,sentry-transaction=dogpark,sentry-user_segment=segmentA,sentry-public_key=dogsarebadatkeepingsecrets,sentry-trace_id=12312012123120121231201212312012,sentry-sample_rate=1',
     ]);
   });
 
   it('adds the transaction name to the the baggage header if a valid transaction source is set', async () => {
     nock('http://dogs.are.great').get('/').reply(200);
 
-    createTransactionOnScope({}, { metadata: { source: 'custom' } });
+    createTransactionOnScope({}, { metadata: { source: 'route' } });
 
     const request = http.get({ host: 'http://dogs.are.great/', headers: { baggage: 'dog=great' } });
     const baggageHeader = request.getHeader('baggage') as string;
@@ -153,6 +153,20 @@ describe('tracing', () => {
     ]);
   });
 
+  it('does not add the transaction name to the the baggage header if url transaction source is set', async () => {
+    nock('http://dogs.are.great').get('/').reply(200);
+
+    createTransactionOnScope({}, { metadata: { source: 'url' } });
+
+    const request = http.get({ host: 'http://dogs.are.great/', headers: { baggage: 'dog=great' } });
+    const baggageHeader = request.getHeader('baggage') as string;
+
+    expect(baggageHeader).toEqual([
+      'dog=great',
+      'sentry-environment=production,sentry-release=1.0.0,sentry-user_segment=segmentA,sentry-public_key=dogsarebadatkeepingsecrets,sentry-trace_id=12312012123120121231201212312012,sentry-sample_rate=1',
+    ]);
+  });
+
   it("doesn't attach the sentry-trace header to outgoing sentry requests", () => {
     nock('http://squirrelchasers.ingest.sentry.io').get('/api/12312012/store/').reply(200);
 
diff --git a/packages/tracing/src/transaction.ts b/packages/tracing/src/transaction.ts
@@ -44,6 +44,7 @@ export class Transaction extends SpanClass implements TransactionInterface {
     this._name = transactionContext.name || '';
 
     this.metadata = {
+      source: 'custom',
       ...transactionContext.metadata,
       spanMetadata: {},
       changes: [],
@@ -82,7 +83,8 @@ export class Transaction extends SpanClass implements TransactionInterface {
     // parameterized by virtue of having no parameters in its path
     if (name !== this.name || source !== this.metadata.source) {
       this.metadata.changes.push({
-        source,
+        // log previous source
+        source: this.metadata.source,
         timestamp: timestampInSeconds(),
         propagations: this.metadata.propagations,
       });
diff --git a/packages/tracing/test/browser/browsertracing.test.ts b/packages/tracing/test/browser/browsertracing.test.ts
@@ -228,7 +228,7 @@ describe('BrowserTracing', () => {
         expect(mockBeforeNavigation).toHaveBeenCalledTimes(1);
       });
 
-      it("doesn't set transaction name source if name is not changed", () => {
+      it('sets transaction name source to default `custom` if name is not changed', () => {
         const mockBeforeNavigation = jest.fn(ctx => ({
           ...ctx,
         }));
@@ -239,7 +239,7 @@ describe('BrowserTracing', () => {
         const transaction = getActiveTransaction(hub) as IdleTransaction;
         expect(transaction).toBeDefined();
         expect(transaction.name).toBe('a/path');
-        expect(transaction.metadata.source).toBeUndefined();
+        expect(transaction.metadata.source).toBe('custom');
 
         expect(mockBeforeNavigation).toHaveBeenCalledTimes(1);
       });
diff --git a/packages/tracing/test/span.test.ts b/packages/tracing/test/span.test.ts
@@ -440,26 +440,23 @@ describe('Span', () => {
         environment: 'production',
         sample_rate: '0.56',
         trace_id: expect.any(String),
+        transaction: 'tx',
       });
     });
 
     describe('Including transaction name in DSC', () => {
-      test.each([
-        ['is not included if transaction source is not set', undefined],
-        ['is not included if transaction source is url', 'url'],
-      ])('%s', (_: string, source) => {
+      test('is not included if transaction source is url', () => {
         const transaction = new Transaction(
           {
             name: 'tx',
             metadata: {
-              ...(source && { source: source as TransactionSource }),
+              source: 'url',
             },
           },
           hub,
         );
 
         const dsc = transaction.getDynamicSamplingContext()!;
-
         expect(dsc.transaction).toBeUndefined();
       });
 
@@ -485,25 +482,6 @@ describe('Span', () => {
   });
 
   describe('Transaction source', () => {
-    test('is not included by default', () => {
-      const spy = jest.spyOn(hub as any, 'captureEvent') as any;
-      const transaction = hub.startTransaction({ name: 'test', sampled: true });
-      expect(spy).toHaveBeenCalledTimes(0);
-
-      transaction.finish();
-
-      expect(spy).toHaveBeenCalledTimes(1);
-      expect(spy).toHaveBeenLastCalledWith(
-        expect.not.objectContaining({
-          transaction_info: {
-            source: expect.any(String),
-            changes: [],
-            propagations: 0,
-          },
-        }),
-      );
-    });
-
     test('is included when transaction metadata is set', () => {
       const spy = jest.spyOn(hub as any, 'captureEvent') as any;
       const transaction = hub.startTransaction({ name: 'test', sampled: true });
diff --git a/packages/tracing/test/transaction.test.ts b/packages/tracing/test/transaction.test.ts
@@ -9,11 +9,11 @@ describe('`Transaction` class', () => {
       expect(transaction.metadata.source).toEqual('route');
     });
 
-    it("doesn't set source in constructor if not provided", () => {
+    it("sets source to be `'custom'` in constructor if not provided", () => {
       const transaction = new Transaction({ name: 'dogpark' });
 
       expect(transaction.name).toEqual('dogpark');
-      expect(transaction.metadata.source).toBeUndefined();
+      expect(transaction.metadata.source).toBe('custom');
     });
 
     it("sets source to `'custom'` when assigning to `name` property", () => {
@@ -25,14 +25,14 @@ describe('`Transaction` class', () => {
     });
 
     it('updates transaction name changes with correct variables needed', () => {
-      const transaction = new Transaction({ name: 'dogpark' });
+      const transaction = new Transaction({ name: 'dogpark', metadata: { source: 'url' } });
       expect(transaction.metadata.changes).toEqual([]);
 
       transaction.name = 'ballpit';
 
       expect(transaction.metadata.changes).toEqual([
         {
-          source: 'custom',
+          source: 'url',
           timestamp: expect.any(Number),
           propagations: 0,
         },
@@ -42,7 +42,7 @@ describe('`Transaction` class', () => {
 
       expect(transaction.metadata.changes).toEqual([
         {
-          source: 'custom',
+          source: 'url',
           timestamp: expect.any(Number),
           propagations: 0,
         },
@@ -52,7 +52,7 @@ describe('`Transaction` class', () => {
 
       expect(transaction.metadata.changes).toEqual([
         {
-          source: 'custom',
+          source: 'url',
           timestamp: expect.any(Number),
           propagations: 0,
         },
@@ -68,7 +68,7 @@ describe('`Transaction` class', () => {
 
       expect(transaction.metadata.changes).toEqual([
         {
-          source: 'custom',
+          source: 'url',
           timestamp: expect.any(Number),
           propagations: 0,
         },
@@ -78,7 +78,7 @@ describe('`Transaction` class', () => {
           propagations: 3,
         },
         {
-          source: 'route',
+          source: 'custom',
           timestamp: expect.any(Number),
           propagations: 3,
         },
@@ -140,14 +140,14 @@ describe('`Transaction` class', () => {
       });
 
       it('updates transaction name changes with correct variables needed', () => {
-        const transaction = new Transaction({ name: 'dogpark' });
+        const transaction = new Transaction({ name: 'dogpark', metadata: { source: 'url' } });
         expect(transaction.metadata.changes).toEqual([]);
 
         transaction.name = 'ballpit';
 
         expect(transaction.metadata.changes).toEqual([
           {
-            source: 'custom',
+            source: 'url',
             timestamp: expect.any(Number),
             propagations: 0,
           },
@@ -157,7 +157,7 @@ describe('`Transaction` class', () => {
 
         expect(transaction.metadata.changes).toEqual([
           {
-            source: 'custom',
+            source: 'url',
             timestamp: expect.any(Number),
             propagations: 0,
           },
@@ -167,12 +167,12 @@ describe('`Transaction` class', () => {
 
         expect(transaction.metadata.changes).toEqual([
           {
-            source: 'custom',
+            source: 'url',
             timestamp: expect.any(Number),
             propagations: 0,
           },
           {
-            source: 'task',
+            source: 'custom',
             timestamp: expect.any(Number),
             propagations: 3,
           },
diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts
