ref(tracing): Remove transaction name and user_id from DSC (#5363)

Lms24 · web-flow · commit f372d842ba39 · 2022-07-06T00:18:46.000+02:00
This patch temporarily removes the `user_id` and `transaction` (name) fields from the dynamic sampling context, meaning they will no longer propagated with outgoing requests via the baggage Http header or sent to sentry via the `trace` envelope header.

We're taking this temporary measure to ensure that for the moment PII is not sent to third parties.
diff --git a/packages/core/test/lib/envelope.test.ts b/packages/core/test/lib/envelope.test.ts
@@ -44,8 +44,8 @@ describe('createEventEnvelope', () => {
               {
                 environment: 'prod',
                 release: '1.0.0',
-                transaction: 'TX',
-                user_id: 'bob',
+                // transaction: 'TX',
+                // user_id: 'bob',
                 user_segment: 'segmentA',
                 sample_rate: '0.95',
                 public_key: 'pubKey123',
@@ -59,8 +59,8 @@ describe('createEventEnvelope', () => {
         {
           environment: 'prod',
           release: '1.0.0',
-          transaction: 'TX',
-          user_id: 'bob',
+          // transaction: 'TX',
+          // user_id: 'bob',
           user_segment: 'segmentA',
           sample_rate: '0.95',
           public_key: 'pubKey123',
diff --git a/packages/integration-tests/suites/tracing/envelope-header-no-pii/test.ts b/packages/integration-tests/suites/tracing/envelope-header-no-pii/test.ts
@@ -5,7 +5,7 @@ import { sentryTest } from '../../../utils/fixtures';
 import { envelopeHeaderRequestParser, getFirstSentryEnvelopeRequest } from '../../../utils/helpers';
 
 sentryTest(
-  'should not send user_id in DSC data in trace envelope header if sendDefaultPii option is not set',
+  'should not send user_id and transaction in DSC data in trace envelope header (for now)',
   async ({ getLocalTestPath, page }) => {
     const url = await getLocalTestPath({ testDir: __dirname });
 
@@ -14,7 +14,6 @@ sentryTest(
     expect(envHeader.trace).toBeDefined();
     expect(envHeader.trace).toEqual({
       environment: 'production',
-      transaction: expect.stringContaining('index.html'),
       user_segment: 'segmentB',
       sample_rate: '1',
       trace_id: expect.any(String),
diff --git a/packages/integration-tests/suites/tracing/envelope-header/init.js b/packages/integration-tests/suites/tracing/envelope-header/init.js
@@ -8,7 +8,8 @@ Sentry.init({
   integrations: [new Integrations.BrowserTracing({ tracingOrigins: [/.*/] })],
   environment: 'production',
   tracesSampleRate: 1,
-  sendDefaultPii: true,
+  // TODO: We're rethinking the mechanism for including Pii data in DSC, hence commenting out sendDefaultPii for now
+  // sendDefaultPii: true,
   debug: true,
 });
 
diff --git a/packages/integration-tests/suites/tracing/envelope-header/test.ts b/packages/integration-tests/suites/tracing/envelope-header/test.ts
@@ -14,8 +14,8 @@ sentryTest(
     expect(envHeader.trace).toBeDefined();
     expect(envHeader.trace).toEqual({
       environment: 'production',
-      transaction: expect.stringContaining('index.html'),
-      user_id: 'user123',
+      // transaction: expect.stringContaining('index.html'),
+      // user_id: 'user123',
       user_segment: 'segmentB',
       sample_rate: '1',
       trace_id: expect.any(String),
diff --git a/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-assign/test.ts b/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-assign/test.ts
@@ -79,8 +79,10 @@ test('Should populate and propagate sentry baggage if sentry-trace header does n
     test_data: {
       host: 'somewhere.not.sentry',
       baggage: expect.stringContaining(
-        'sentry-environment=prod,sentry-release=1.0,sentry-transaction=GET%20%2Ftest%2Fexpress,' +
-          'sentry-public_key=public,sentry-trace_id=',
+        // 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=',
       ),
     },
   });
@@ -99,8 +101,10 @@ 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(
-        'sentry-environment=prod,sentry-release=1.0,sentry-transaction=GET%20%2Ftest%2Fexpress,' +
-          'sentry-public_key=public,sentry-trace_id=',
+        // 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=',
       ),
     },
   });
diff --git a/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out/test.ts b/packages/node-integration-tests/suites/express/sentry-trace/baggage-header-out/test.ts
@@ -13,22 +13,26 @@ test('should attach a `baggage` header to an outgoing request.', async () => {
     test_data: {
       host: 'somewhere.not.sentry',
       baggage:
-        'sentry-environment=prod,sentry-release=1.0,sentry-transaction=GET%20%2Ftest%2Fexpress,sentry-user_segment=SegmentA' +
+        // 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 in baggage if sendDefaultPii is not set', async () => {
+test('Does not include user_id and transaction name (for now)', async () => {
   const url = await runServer(__dirname, `${path.resolve(__dirname, '.')}/server.ts`);
 
   const response = (await getAPIResponse(new URL(`${url}/express`))) as TestAPIResponse;
+  const baggageString = response.test_data.baggage;
 
   expect(response).toBeDefined();
   expect(response).toMatchObject({
     test_data: {
       host: 'somewhere.not.sentry',
-      baggage: expect.not.stringContaining('sentry-user_id'),
     },
   });
+  expect(baggageString).not.toContain('sentry-user_id=');
+  expect(baggageString).not.toContain('sentry-transaction=');
 });
diff --git a/packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/test.ts b/packages/node-integration-tests/suites/express/sentry-trace/baggage-other-vendors-with-sentry-entries/test.ts
@@ -30,8 +30,10 @@ test('should ignore sentry-values in `baggage` header of a third party vendor an
     test_data: {
       host: 'somewhere.not.sentry',
       baggage: expect.stringContaining(
-        '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',
+        // 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',
       ),
     },
   });
diff --git a/packages/node-integration-tests/suites/express/sentry-trace/baggage-user-id/server.ts b/packages/node-integration-tests/suites/express/sentry-trace/baggage-user-id/server.ts
@@ -14,7 +14,8 @@ Sentry.init({
   environment: 'prod',
   integrations: [new Sentry.Integrations.Http({ tracing: true }), new Tracing.Integrations.Express({ app })],
   tracesSampleRate: 1.0,
-  sendDefaultPii: true,
+  // TODO: We're rethinking the mechanism for including Pii data in DSC, hence commenting out sendDefaultPii for now
+  // sendDefaultPii: true,
 });
 
 Sentry.setUser({ id: 'user123', segment: 'SegmentA' });
diff --git a/packages/node-integration-tests/suites/express/sentry-trace/baggage-user-id/test.ts b/packages/node-integration-tests/suites/express/sentry-trace/baggage-user-id/test.ts
@@ -3,7 +3,8 @@ import * as path from 'path';
 import { getAPIResponse, runServer } from '../../../../utils/index';
 import { TestAPIResponse } from '../server';
 
-test('Includes user_id in baggage if sendDefaultPii is set to true', async () => {
+// 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 () => {
   const url = await runServer(__dirname, `${path.resolve(__dirname, '.')}/server.ts`);
 
   const response = (await getAPIResponse(new URL(`${url}/express`))) as TestAPIResponse;
diff --git a/packages/node/test/integrations/http.test.ts b/packages/node/test/integrations/http.test.ts
@@ -24,7 +24,6 @@ describe('tracing', () => {
       integrations: [new HttpIntegration({ tracing: true })],
       release: '1.0.0',
       environment: 'production',
-      sendDefaultPii: true,
       ...customOptions,
     });
     const hub = new Hub(new NodeClient(options));
@@ -113,7 +112,9 @@ describe('tracing', () => {
     expect(baggageHeader).toBeDefined();
     expect(typeof baggageHeader).toEqual('string');
     expect(baggageHeader).toEqual(
-      'sentry-environment=production,sentry-release=1.0.0,sentry-transaction=dogpark,sentry-user_id=uid123,' +
+      // 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',
     );
@@ -130,24 +131,31 @@ describe('tracing', () => {
     expect(baggageHeader).toBeDefined();
     expect(typeof baggageHeader).toEqual('string');
     expect(baggageHeader).toEqual(
-      '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,' +
+      // 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',
     );
   });
 
-  it('does not add the user_id to the baggage header if sendDefaultPii is set to false', async () => {
+  // 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 () => {
     nock('http://dogs.are.great').get('/').reply(200);
 
-    createTransactionOnScope({ sendDefaultPii: false });
+    createTransactionOnScope();
 
     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(
-      'dog=great,sentry-environment=production,sentry-release=1.0.0,sentry-transaction=dogpark,' +
+      // 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,' +
         'sentry-user_segment=segmentA,sentry-public_key=dogsarebadatkeepingsecrets,' +
         'sentry-trace_id=12312012123120121231201212312012,sentry-sample_rate=1',
     );
diff --git a/packages/tracing/src/transaction.ts b/packages/tracing/src/transaction.ts
@@ -229,15 +229,19 @@ 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 { /* id: user_id, */ segment: user_segment } = (scope && scope.getUser()) || {};
 
     return createBaggage(
       dropUndefinedKeys({
         environment,
         release,
-        transaction: this.name,
-        ...(hub.shouldSendDefaultPii() && { user_id }),
+        // transaction: this.name,
+        // replace `someContidion` with whatever decision we come up with to guard PII in DSC
+        // ...(someCondition && { user_id }),
         user_segment,
         public_key,
         trace_id: this.traceId,
diff --git a/packages/tracing/test/browser/browsertracing.test.ts b/packages/tracing/test/browser/browsertracing.test.ts
@@ -509,7 +509,7 @@ describe('BrowserTracing', () => {
         expect(baggage[0]).toEqual({
           release: '1.0.0',
           environment: 'production',
-          transaction: 'blank',
+          // transaction: 'blank',
           public_key: 'pubKey',
           trace_id: expect.not.stringMatching('12312012123120121231201212312012'),
         });
diff --git a/packages/tracing/test/span.test.ts b/packages/tracing/test/span.test.ts
@@ -438,13 +438,12 @@ describe('Span', () => {
 
       const baggage = transaction.getBaggage();
 
-      // this is called twice because hub.shouldSendDefaultPii also calls getOptions()
-      expect(getOptionsSpy).toHaveBeenCalledTimes(2);
+      expect(getOptionsSpy).toHaveBeenCalledTimes(1);
       expect(baggage && isSentryBaggageEmpty(baggage)).toBe(false);
       expect(baggage && getSentryBaggageItems(baggage)).toStrictEqual({
         release: '1.0.1',
         environment: 'production',
-        transaction: 'tx',
+        // transaction: 'tx',
         sample_rate: '0.56',
         trace_id: expect.any(String),
       });
@@ -468,7 +467,7 @@ describe('Span', () => {
       expect(baggage && getSentryBaggageItems(baggage)).toStrictEqual({
         release: '1.0.1',
         environment: 'production',
-        transaction: 'tx',
+        // transaction: 'tx',
         sample_rate: '0.0000000000000145',
         trace_id: expect.any(String),
       });
diff --git a/packages/types/src/envelope.ts b/packages/types/src/envelope.ts
@@ -15,8 +15,8 @@ export type DynamicSamplingContext = {
   sample_rate?: string;
   release?: string;
   environment?: string;
-  transaction?: string;
-  user_id?: string;
+  // transaction?: string; // omitted for now
+  // user_id?: string; // omitted for now
   user_segment?: string;
 };
 
diff --git a/packages/types/src/options.ts b/packages/types/src/options.ts
@@ -153,17 +153,16 @@ export interface ClientOptions<TO extends BaseTransportOptions = BaseTransportOp
   tunnel?: string;
 
   /**
-   * Currently controls if the user id (e.g. set by `Sentry.setUser`) should
-   * be used for dynamic sampling. Only if this flag is set to `true`, the user id
-   * will be propagated to outgoing requests in the `baggage` Http header.
+   * Important: This option is currently unused and will only work in the next major version of the SDK
    *
-   * Note that in the next major version of this SDK, this option will not only
-   * control dynamic sampling data: As long as this flag is not set to `true`,
-   * the SDK will not send sensitive data by default.
+   * Controls if potentially sensitive data should be sent to Sentry by default.
+   * Note that this only applies to data that the SDK is sending by default
+   * but not data that was explicitly set (e.g. by calling `Sentry.setUser()`).
+   * Details about the implementation are TBD.
    *
    * Defaults to `false`.
    *
-   * @experimental (will be fully introduced in the next major version)
+   * @ignore
    */
   sendDefaultPii?: boolean;
 
