ref(tracing): Include transaction in DSC if transaction source is not an unparameterized URL (#5392)

This patch re-introduces the `transaction` field in the Dynamic Sampling Context (DSC). However, its presence is now determined by the [transaction source](https://develop.sentry.dev/sdk/event-payloads/transaction/#transaction-annotations) which was introduced in #5367.

As of this we we add the `transaction` field back, if the source indicates that the transaction name is not an unparameterized URL (meaning, the source is set and it is not `url`). 

Additionally, the PR (once again) adjusts our unit and integration tests to reflect this change. Repurposed some DSC<=>`sendDefaultPii` tests that we previously skipped to now cover the transaction<=>transaction source dependence. Did some cleanup of commented out old code and explanations that no longer apply.

Remove he `'unknown'` field from the `TransactionSource` type because it is only used by Relay and SDKs shouldn't set it.

Author: Lms24

packages/core/test/lib/envelope.test.ts

@@ -44,8 +44,7 @@ describe('createEventEnvelope', () => {
               {
                 environment: 'prod',
                 release: '1.0.0',
-                // transaction: 'TX',
-                // user_id: 'bob',
+                transaction: 'TX',
                 user_segment: 'segmentA',
                 sample_rate: '0.95',
                 public_key: 'pubKey123',
@@ -59,8 +58,7 @@ describe('createEventEnvelope', () => {
         {
           environment: 'prod',
           release: '1.0.0',
-          // transaction: 'TX',
-          // user_id: 'bob',
+          transaction: 'TX',
           user_segment: 'segmentA',
           sample_rate: '0.95',
           public_key: 'pubKey123',

packages/integration-tests/suites/tracing/envelope-header-no-pii/init.js renamed to packages/integration-tests/suites/tracing/envelope-header-transaction-name/init.js

@@ -14,4 +14,5 @@ Sentry.init({
 Sentry.configureScope(scope => {
   scope.setUser({ id: 'user123', segment: 'segmentB' });
   scope.setTransactionName('testTransactionDSC');
+  scope.getTransaction().setMetadata({ source: 'custom' });
 });

packages/integration-tests/suites/tracing/envelope-header-no-pii/test.ts renamed to packages/integration-tests/suites/tracing/envelope-header-transaction-name/test.ts

@@ -5,7 +5,7 @@ import { sentryTest } from '../../../utils/fixtures';
 import { envelopeHeaderRequestParser, getFirstSentryEnvelopeRequest } from '../../../utils/helpers';
 
 sentryTest(
-  'should not send user_id and transaction in DSC data in trace envelope header (for now)',
+  'should only include transaction name if source is better than an unparameterized URL',
   async ({ getLocalTestPath, page }) => {
     const url = await getLocalTestPath({ testDir: __dirname });
 
@@ -16,6 +16,7 @@ sentryTest(
       environment: 'production',
       user_segment: 'segmentB',
       sample_rate: '1',
+      transaction: expect.stringContaining('/index.html'),
       trace_id: expect.any(String),
       public_key: 'public',
     });

packages/integration-tests/suites/tracing/envelope-header/init.js

@@ -8,8 +8,6 @@ Sentry.init({
   integrations: [new Integrations.BrowserTracing({ tracingOrigins: [/.*/] })],
   environment: 'production',
   tracesSampleRate: 1,
-  // TODO: We're rethinking the mechanism for including Pii data in DSC, hence commenting out sendDefaultPii for now
-  // sendDefaultPii: true,
   debug: true,
 });
 

packages/integration-tests/suites/tracing/envelope-header/test.ts

@@ -11,11 +11,13 @@ sentryTest(
 
     const envHeader = await getFirstSentryEnvelopeRequest<EventEnvelopeHeaders>(page, url, envelopeHeaderRequestParser);
 
+    // In this test, we don't expect trace.transaction to be present because without a custom routing instrumentation
+    // we for now don't have parameterization. This might change in the future but for now the only way of having
+    // transaction in DSC with the default BrowserTracing integration is when the transaction name is set manually.
+    // This scenario is covered in another integration test (envelope-header-transaction-name).
     expect(envHeader.trace).toBeDefined();
     expect(envHeader.trace).toEqual({
       environment: 'production',
-      // transaction: expect.stringContaining('index.html'),
-      // user_id: 'user123',
       user_segment: 'segmentB',
       sample_rate: '1',
       trace_id: expect.any(String),

packages/node-integration-tests/suites/express/sentry-trace/baggage-header-assign/test.ts

@@ -78,10 +78,8 @@ test('Should populate and propagate sentry baggage if sentry-trace header does n
   expect(response).toMatchObject({
     test_data: {
       host: 'somewhere.not.sentry',
+      // TraceId changes, hence we only expect that the string contains the traceid key
       baggage: expect.stringContaining(
-        // Commented out as long as transaction and user_id are not part of DSC
-        // 'sentry-environment=prod,sentry-release=1.0,sentry-transaction=GET%20%2Ftest%2Fexpress,' +
-        // 'sentry-public_key=public,sentry-trace_id=',
         'sentry-environment=prod,sentry-release=1.0,sentry-public_key=public,sentry-trace_id=',
       ),
     },
@@ -101,9 +99,6 @@ test('Should populate Sentry and ignore 3rd party content if sentry-trace header
       host: 'somewhere.not.sentry',
       // TraceId changes, hence we only expect that the string contains the traceid key
       baggage: expect.stringContaining(
-        // Commented out as long as transaction and user_id are not part of DSC
-        // 'sentry-environment=prod,sentry-release=1.0,sentry-transaction=GET%20%2Ftest%2Fexpress,' +
-        // 'sentry-public_key=public,sentry-trace_id=',
         'sentry-environment=prod,sentry-release=1.0,sentry-public_key=public,sentry-trace_id=',
       ),
     },

packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out/test.ts

@@ -13,15 +13,13 @@ test('should attach a `baggage` header to an outgoing request.', async () => {
     test_data: {
       host: 'somewhere.not.sentry',
       baggage:
-        // Commented out as long as transaction and user_id are not part of DSC
-        // 'sentry-environment=prod,sentry-release=1.0,sentry-transaction=GET%20%2Ftest%2Fexpress,sentry-user_segment=SegmentA' +
         'sentry-environment=prod,sentry-release=1.0,sentry-user_segment=SegmentA' +
         ',sentry-public_key=public,sentry-trace_id=86f39e84263a4de99c326acab3bfe3bd,sentry-sample_rate=1',
     },
   });
 });
 
-test('Does not include user_id and transaction name (for now)', async () => {
+test('Does not include transaction name if transaction source is not set', async () => {
   const url = await runServer(__dirname, `${path.resolve(__dirname, '.')}/server.ts`);
 
   const response = (await getAPIResponse(new URL(`${url}/express`))) as TestAPIResponse;

packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/test.ts

@@ -30,9 +30,6 @@ test('should ignore sentry-values in `baggage` header of a third party vendor an
     test_data: {
       host: 'somewhere.not.sentry',
       baggage: expect.stringContaining(
-        // Commented out as long as transaction and user_id are not part of DSC
-        // 'other=vendor,foo=bar,third=party,last=item,sentry-environment=prod,sentry-release=1.0,' +
-        // 'sentry-transaction=GET%20%2Ftest%2Fexpress,sentry-public_key=public',
         'other=vendor,foo=bar,third=party,last=item,sentry-environment=prod,sentry-release=1.0,sentry-public_key=public',
       ),
     },

packages/node-integration-tests/suites/express/sentry-trace/baggage-user-id/server.ts renamed to packages/node-integration-tests/suites/express/sentry-trace/baggage-transaction-name/server.ts

@@ -29,6 +29,7 @@ app.get('/test/express', (_req, res) => {
   const transaction = Sentry.getCurrentHub().getScope()?.getTransaction();
   if (transaction) {
     transaction.traceId = '86f39e84263a4de99c326acab3bfe3bd';
+    transaction.setMetadata({ source: 'route' });
   }
   const headers = http.get('http://somewhere.not.sentry/').getHeaders();
 

packages/node-integration-tests/suites/express/sentry-trace/baggage-user-id/test.ts renamed to packages/node-integration-tests/suites/express/sentry-trace/baggage-transaction-name/test.ts

@@ -3,8 +3,7 @@ import * as path from 'path';
 import { getAPIResponse, runServer } from '../../../../utils/index';
 import { TestAPIResponse } from '../server';
 
-// TODO: Skipping this test because right now we're rethinking the mechanism for including such data
-test.skip('Includes user_id in baggage if <optionTBA> is set to true', async () => {
+test('Includes transaction in baggage if the transaction name is parameterized', async () => {
   const url = await runServer(__dirname, `${path.resolve(__dirname, '.')}/server.ts`);
 
   const response = (await getAPIResponse(new URL(`${url}/express`))) as TestAPIResponse;
@@ -13,7 +12,7 @@ test.skip('Includes user_id in baggage if <optionTBA> is set to true', async ()
   expect(response).toMatchObject({
     test_data: {
       host: 'somewhere.not.sentry',
-      baggage: expect.stringContaining('sentry-user_id=user123'),
+      baggage: expect.stringContaining('sentry-transaction=GET%20%2Ftest%2Fexpress'),
     },
   });
 });

packages/node/test/integrations/http.test.ts

@@ -2,6 +2,7 @@ import * as sentryCore from '@sentry/core';
 import * as hubModule from '@sentry/hub';
 import { Hub } from '@sentry/hub';
 import { addExtensionMethods, Span, TRACEPARENT_REGEXP, Transaction } from '@sentry/tracing';
+import { TransactionContext } from '@sentry/types';
 import { parseSemver } from '@sentry/utils';
 import * as http from 'http';
 import * as https from 'https';
@@ -17,7 +18,10 @@ import { getDefaultNodeClientOptions } from '../helper/node-client-options';
 const NODE_VERSION = parseSemver(process.versions.node);
 
 describe('tracing', () => {
-  function createTransactionOnScope(customOptions: Partial<NodeClientOptions> = {}) {
+  function createTransactionOnScope(
+    customOptions: Partial<NodeClientOptions> = {},
+    customContext?: Partial<TransactionContext>,
+  ) {
     const options = getDefaultNodeClientOptions({
       dsn: 'https://[email protected]/12312012',
       tracesSampleRate: 1.0,
@@ -44,7 +48,9 @@ describe('tracing', () => {
     const transaction = hub.startTransaction({
       name: 'dogpark',
       traceId: '12312012123120121231201212312012',
+      ...customContext,
     });
+
     hub.getScope()?.setSpan(transaction);
 
     return transaction;
@@ -112,8 +118,6 @@ describe('tracing', () => {
     expect(baggageHeader).toBeDefined();
     expect(typeof baggageHeader).toEqual('string');
     expect(baggageHeader).toEqual(
-      // Commented out as long as transaction and user_id are not part of DSC
-      // 'sentry-environment=production,sentry-release=1.0.0,sentry-transaction=dogpark,sentry-user_id=uid123,' +
       'sentry-environment=production,sentry-release=1.0.0,' +
         'sentry-user_segment=segmentA,sentry-public_key=dogsarebadatkeepingsecrets,' +
         'sentry-trace_id=12312012123120121231201212312012,sentry-sample_rate=1',
@@ -131,31 +135,24 @@ describe('tracing', () => {
     expect(baggageHeader).toBeDefined();
     expect(typeof baggageHeader).toEqual('string');
     expect(baggageHeader).toEqual(
-      // Commented out as long as transaction and user_id are not part of DSC
-      // 'dog=great,sentry-environment=production,sentry-release=1.0.0,sentry-transaction=dogpark,' +
-      // 'sentry-user_id=uid123,sentry-user_segment=segmentA,sentry-public_key=dogsarebadatkeepingsecrets,' +
-      // 'sentry-trace_id=12312012123120121231201212312012,sentry-sample_rate=1',
       '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',
     );
   });
 
-  // TODO: Skipping this test because right now we're rethinking the mechanism for including such data
-  it.skip('does not add the user_id to the baggage header if <optionTBA> is set to false', async () => {
+  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();
+    createTransactionOnScope({}, { metadata: { source: 'custom' } });
 
     const request = http.get({ host: 'http://dogs.are.great/', headers: { baggage: 'dog=great' } });
     const baggageHeader = request.getHeader('baggage') as string;
 
     expect(baggageHeader).toBeDefined();
     expect(typeof baggageHeader).toEqual('string');
     expect(baggageHeader).toEqual(
-      // Commented out as long as transaction and user_id are not part of DSC
-      // 'dog=great,sentry-environment=production,sentry-release=1.0.0,sentry-transaction=dogpark,' +
-      'dog=great,sentry-environment=production,sentry-release=1.0.0,' +
+      'dog=great,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',
     );

packages/tracing/src/transaction.ts

@@ -235,19 +235,17 @@ export class Transaction extends SpanClass implements TransactionInterface {
         ? rate.toLocaleString('fullwide', { useGrouping: false, maximumFractionDigits: 16 })
         : undefined;
 
-    // For now we're not sending the transaction name and user_id due to PII concerns
-    // commenting it out for now because we'll probably need it in the future
-
     const scope = hub.getScope();
-    const { /* id: user_id, */ segment: user_segment } = (scope && scope.getUser()) || {};
+    const { segment: user_segment } = (scope && scope.getUser()) || {};
+
+    const source = this.metadata.source;
+    const transaction = source && source !== 'url' ? this.name : undefined;
 
     return createBaggage(
       dropUndefinedKeys({
         environment,
         release,
-        // transaction: this.name,
-        // replace `someContidion` with whatever decision we come up with to guard PII in DSC
-        // ...(someCondition && { user_id }),
+        transaction,
         user_segment,
         public_key,
         trace_id: this.traceId,

packages/tracing/test/browser/browsertracing.test.ts

@@ -509,7 +509,6 @@ describe('BrowserTracing', () => {
         expect(baggage[0]).toEqual({
           release: '1.0.0',
           environment: 'production',
-          // transaction: 'blank',
           public_key: 'pubKey',
           trace_id: expect.not.stringMatching('12312012123120121231201212312012'),
         });

packages/tracing/test/span.test.ts

@@ -1,6 +1,6 @@
 import { BrowserClient } from '@sentry/browser';
 import { Hub, makeMain, Scope } from '@sentry/hub';
-import { BaseTransportOptions, ClientOptions } from '@sentry/types';
+import { BaseTransportOptions, ClientOptions, TransactionSource } from '@sentry/types';
 import { createBaggage, getSentryBaggageItems, getThirdPartyBaggage, isSentryBaggageEmpty } from '@sentry/utils';
 
 import { Span, Transaction } from '../src';
@@ -443,7 +443,6 @@ describe('Span', () => {
       expect(baggage && getSentryBaggageItems(baggage)).toStrictEqual({
         release: '1.0.1',
         environment: 'production',
-        // transaction: 'tx',
         sample_rate: '0.56',
         trace_id: expect.any(String),
       });
@@ -473,6 +472,46 @@ describe('Span', () => {
       });
       expect(baggage && getThirdPartyBaggage(baggage)).toStrictEqual('');
     });
+
+    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) => {
+        const transaction = new Transaction(
+          {
+            name: 'tx',
+            metadata: {
+              ...(source && { source: source as TransactionSource }),
+            },
+          },
+          hub,
+        );
+
+        const dsc = getSentryBaggageItems(transaction.getBaggage());
+
+        expect(dsc.transaction).toBeUndefined();
+      });
+
+      test.each([
+        ['is included if transaction source is paremeterized route/url', 'route'],
+        ['is included if transaction source is a custom name', 'custom'],
+      ])('%s', (_: string, source) => {
+        const transaction = new Transaction(
+          {
+            name: 'tx',
+            metadata: {
+              ...(source && { source: source as TransactionSource }),
+            },
+          },
+          hub,
+        );
+
+        const dsc = getSentryBaggageItems(transaction.getBaggage());
+
+        expect(dsc.transaction).toEqual('tx');
+      });
+    });
   });
 
   describe('Transaction source', () => {

packages/types/src/envelope.ts

@@ -15,8 +15,7 @@ export type DynamicSamplingContext = {
   sample_rate?: string;
   release?: string;
   environment?: string;
-  // transaction?: string; // omitted for now
-  // user_id?: string; // omitted for now
+  transaction?: string;
   user_segment?: string;
 };
 

packages/types/src/transaction.ts

@@ -162,8 +162,6 @@ export type TransactionSource =
   | 'route'
   /** Name of the view handling the request */
   | 'view'
-  /** This is the default value set by Relay for legacy SDKs. */
-  | 'unknown'
   /** Named after a software component, such as a function or class name. */
   | 'component'
   /** Name of a background task (e.g. a Celery task) */