fix(core): Set sentry.source attribute to custom when calling span.updateName on SentrySpan (#14251)

Lms24 · onurtemizkan · commit 93058fc0ca89 · 2024-11-22T10:28:01.000Z
Fix a "regression" introduced in v8 where we no longer updated
the source of the span when calling `span.updateName`. We had this
behaviour in our v7 `Transaction` class (IIRC via
`transaction.setName(name, source)` but most likely forgot to port it to
the `Span` class as the `event.transaction_info.source` field is only
relevant for transactions (i.e. root spans in today's SDK lingo).
diff --git a/.size-limit.js b/.size-limit.js
@@ -139,7 +139,7 @@ module.exports = [
     path: 'packages/vue/build/esm/index.js',
     import: createImport('init', 'browserTracingIntegration'),
     gzip: true,
-    limit: '38 KB',
+    limit: '38.5 KB',
   },
   // Svelte SDK (ESM)
   {
diff --git a/dev-packages/browser-integration-tests/suites/public-api/startSpan/basic/test.ts b/dev-packages/browser-integration-tests/suites/public-api/startSpan/basic/test.ts
@@ -1,24 +1,41 @@
 import { expect } from '@playwright/test';
 
+import {
+  SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
+  SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE,
+  SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
+} from '@sentry/browser';
 import { sentryTest } from '../../../../utils/fixtures';
 import {
   envelopeRequestParser,
   shouldSkipTracingTest,
   waitForTransactionRequestOnUrl,
 } from '../../../../utils/helpers';
 
-sentryTest('should send a transaction in an envelope', async ({ getLocalTestPath, page }) => {
-  if (shouldSkipTracingTest()) {
-    sentryTest.skip();
-  }
-
-  const url = await getLocalTestPath({ testDir: __dirname });
-  const req = await waitForTransactionRequestOnUrl(page, url);
-  const transaction = envelopeRequestParser(req);
-
-  expect(transaction.transaction).toBe('parent_span');
-  expect(transaction.spans).toBeDefined();
-});
+sentryTest(
+  'sends a transaction in an envelope with manual origin and custom source',
+  async ({ getLocalTestPath, page }) => {
+    if (shouldSkipTracingTest()) {
+      sentryTest.skip();
+    }
+
+    const url = await getLocalTestPath({ testDir: __dirname });
+    const req = await waitForTransactionRequestOnUrl(page, url);
+    const transaction = envelopeRequestParser(req);
+
+    const attributes = transaction.contexts?.trace?.data;
+    expect(attributes).toEqual({
+      [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'manual',
+      [SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1,
+      [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom',
+    });
+
+    expect(transaction.transaction_info?.source).toBe('custom');
+
+    expect(transaction.transaction).toBe('parent_span');
+    expect(transaction.spans).toBeDefined();
+  },
+);
 
 sentryTest('should report finished spans as children of the root transaction', async ({ getLocalTestPath, page }) => {
   if (shouldSkipTracingTest()) {
diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-update-txn-name/init.js b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-update-txn-name/init.js
@@ -0,0 +1,10 @@
+import * as Sentry from '@sentry/browser';
+
+window.Sentry = Sentry;
+window._testBaseTimestamp = performance.timeOrigin / 1000;
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  integrations: [Sentry.browserTracingIntegration()],
+  tracesSampleRate: 1,
+});
diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-update-txn-name/subject.js b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-update-txn-name/subject.js
@@ -0,0 +1,3 @@
+const activeSpan = Sentry.getActiveSpan();
+const rootSpan = activeSpan && Sentry.getRootSpan(activeSpan);
+rootSpan?.updateName('new name');
diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-update-txn-name/test.ts b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload-update-txn-name/test.ts
@@ -0,0 +1,36 @@
+import { expect } from '@playwright/test';
+import type { Event } from '@sentry/types';
+
+import {
+  SEMANTIC_ATTRIBUTE_SENTRY_OP,
+  SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
+  SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE,
+  SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
+} from '@sentry/browser';
+import { sentryTest } from '../../../../utils/fixtures';
+import { getFirstSentryEnvelopeRequest, shouldSkipTracingTest } from '../../../../utils/helpers';
+
+sentryTest('sets the source to custom when updating the transaction name', async ({ getLocalTestPath, page }) => {
+  if (shouldSkipTracingTest()) {
+    sentryTest.skip();
+  }
+
+  const url = await getLocalTestPath({ testDir: __dirname });
+
+  const eventData = await getFirstSentryEnvelopeRequest<Event>(page, url);
+
+  const traceContextData = eventData.contexts?.trace?.data;
+
+  expect(traceContextData).toMatchObject({
+    [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.pageload.browser',
+    [SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1,
+    [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom',
+    [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'pageload',
+  });
+
+  expect(eventData.transaction).toBe('new name');
+
+  expect(eventData.contexts?.trace?.op).toBe('pageload');
+  expect(eventData.spans?.length).toBeGreaterThan(0);
+  expect(eventData.transaction_info?.source).toEqual('custom');
+});
diff --git a/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload/test.ts b/dev-packages/browser-integration-tests/suites/tracing/browserTracingIntegration/pageload/test.ts
@@ -1,10 +1,16 @@
 import { expect } from '@playwright/test';
 import type { Event } from '@sentry/types';
 
+import {
+  SEMANTIC_ATTRIBUTE_SENTRY_OP,
+  SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
+  SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE,
+  SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
+} from '@sentry/browser';
 import { sentryTest } from '../../../../utils/fixtures';
 import { getFirstSentryEnvelopeRequest, shouldSkipTracingTest } from '../../../../utils/helpers';
 
-sentryTest('should create a pageload transaction', async ({ getLocalTestPath, page }) => {
+sentryTest('creates a pageload transaction with url as source', async ({ getLocalTestPath, page }) => {
   if (shouldSkipTracingTest()) {
     sentryTest.skip();
   }
@@ -16,8 +22,17 @@ sentryTest('should create a pageload transaction', async ({ getLocalTestPath, pa
 
   const { start_timestamp: startTimestamp } = eventData;
 
+  const traceContextData = eventData.contexts?.trace?.data;
+
   expect(startTimestamp).toBeCloseTo(timeOrigin, 1);
 
+  expect(traceContextData).toMatchObject({
+    [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.pageload.browser',
+    [SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1,
+    [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url',
+    [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'pageload',
+  });
+
   expect(eventData.contexts?.trace?.op).toBe('pageload');
   expect(eventData.spans?.length).toBeGreaterThan(0);
   expect(eventData.transaction_info?.source).toEqual('url');
diff --git a/packages/core/src/tracing/sentrySpan.ts b/packages/core/src/tracing/sentrySpan.ts
@@ -193,6 +193,7 @@ export class SentrySpan implements Span {
    */
   public updateName(name: string): this {
     this._name = name;
+    this.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'custom');
     return this;
   }
 
diff --git a/packages/core/test/lib/tracing/sentrySpan.test.ts b/packages/core/test/lib/tracing/sentrySpan.test.ts
@@ -1,5 +1,5 @@
 import { timestampInSeconds } from '@sentry/utils';
-import { setCurrentClient } from '../../../src';
+import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, setCurrentClient } from '../../../src';
 import { SentrySpan } from '../../../src/tracing/sentrySpan';
 import { SPAN_STATUS_ERROR } from '../../../src/tracing/spanstatus';
 import { TRACE_FLAG_NONE, TRACE_FLAG_SAMPLED, spanToJSON } from '../../../src/utils/spanUtils';
@@ -20,6 +20,19 @@ describe('SentrySpan', () => {
 
       expect(spanToJSON(span).description).toEqual('new name');
     });
+
+    it('sets the source to custom when calling updateName', () => {
+      const span = new SentrySpan({
+        name: 'original name',
+        attributes: { [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url' },
+      });
+
+      span.updateName('new name');
+
+      const spanJson = spanToJSON(span);
+      expect(spanJson.description).toEqual('new name');
+      expect(spanJson.data?.[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]).toEqual('custom');
+    });
   });
 
   describe('setters', () => {
diff --git a/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts b/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts
@@ -61,6 +61,7 @@ export function appRouterInstrumentNavigation(client: Client): void {
   WINDOW.addEventListener('popstate', () => {
     if (currentNavigationSpan && currentNavigationSpan.isRecording()) {
       currentNavigationSpan.updateName(WINDOW.location.pathname);
+      currentNavigationSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'url');
     } else {
       currentNavigationSpan = startBrowserTracingNavigationSpan(client, {
         name: WINDOW.location.pathname,
@@ -105,9 +106,11 @@ export function appRouterInstrumentNavigation(client: Client): void {
 
               if (routerFunctionName === 'push') {
                 span?.updateName(transactionNameifyRouterArgument(argArray[0]));
+                span?.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'url');
                 span?.setAttribute('navigation.type', 'router.push');
               } else if (routerFunctionName === 'replace') {
                 span?.updateName(transactionNameifyRouterArgument(argArray[0]));
+                span?.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'url');
                 span?.setAttribute('navigation.type', 'router.replace');
               } else if (routerFunctionName === 'back') {
                 span?.setAttribute('navigation.type', 'router.back');
diff --git a/packages/solid/src/solidrouter.ts b/packages/solid/src/solidrouter.ts
@@ -89,6 +89,7 @@ function withSentryRouterRoot(Root: Component<RouteSectionProps>): Component<Rou
         // everything else was already instrumented correctly in `useBeforeLeave`
         if (op === 'navigation' && description === '-1') {
           rootSpan.updateName(name);
+          rootSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'url');
         }
       }
     });

Original file line number	Diff line number	Diff line change
`@@ -139,7 +139,7 @@ module.exports = [`
`139`	`139`	`path: 'packages/vue/build/esm/index.js',`
`140`	`140`	`import: createImport('init', 'browserTracingIntegration'),`
`141`	`141`	`gzip: true,`
`142`		`- limit: '38 KB',`
	`142`	`+ limit: '38.5 KB',`
`143`	`143`	`},`
`144`	`144`	`// Svelte SDK (ESM)`
`145`	`145`	`{`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+const activeSpan = Sentry.getActiveSpan();`
	`2`	`+const rootSpan = activeSpan && Sentry.getRootSpan(activeSpan);`
	`3`	`+rootSpan?.updateName('new name');`
Original file line number	Diff line number	Diff line change
`@@ -193,6 +193,7 @@ export class SentrySpan implements Span {`
`193`	`193`	`*/`
`194`	`194`	`public updateName(name: string): this {`
`195`	`195`	`this._name = name;`
	`196`	`+ this.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'custom');`
`196`	`197`	`return this;`
`197`	`198`	`}`
`198`	`199`
Original file line number	Diff line number	Diff line change
`@@ -89,6 +89,7 @@ function withSentryRouterRoot(Root: Component<RouteSectionProps>): Component<Rou`
`89`	`89`	// everything else was already instrumented correctly in `useBeforeLeave`
`90`	`90`	`if (op === 'navigation' && description === '-1') {`
`91`	`91`	`rootSpan.updateName(name);`
	`92`	`+ rootSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'url');`
`92`	`93`	`}`
`93`	`94`	`}`
`94`	`95`	`});`