fix(sampling): Remove stray sampling data tags (#3197)

lobsterkatie · web-flow · commit 5a084de87211 · 2021-02-01T10:20:40.000-08:00
diff --git a/packages/core/src/request.ts b/packages/core/src/request.ts
@@ -51,14 +51,18 @@ export function sessionToSentryRequest(session: Session, api: API): SentryReques
 
 /** Creates a SentryRequest from an event. */
 export function eventToSentryRequest(event: Event, api: API): SentryRequest {
-  // since JS has no Object.prototype.pop()
-  const { __sentry_samplingMethod: samplingMethod, __sentry_sampleRate: sampleRate, ...otherTags } = event.tags || {};
-  event.tags = otherTags;
-
   const sdkInfo = getSdkMetadataForEnvelopeHeader(api);
   const eventType = event.type || 'event';
   const useEnvelope = eventType === 'transaction';
 
+  const { transactionSampling, ...metadata } = event.debug_meta || {};
+  const { method: samplingMethod, rate: sampleRate } = transactionSampling || {};
+  if (Object.keys(metadata).length === 0) {
+    delete event.debug_meta;
+  } else {
+    event.debug_meta = metadata;
+  }
+
   const req: SentryRequest = {
     body: JSON.stringify(sdkInfo ? enhanceEventWithSdkInfo(event, api.metadata.sdk) : event),
     type: eventType,
diff --git a/packages/core/test/lib/request.test.ts b/packages/core/test/lib/request.test.ts
@@ -1,21 +1,30 @@
-import { Event, TransactionSamplingMethod } from '@sentry/types';
+import { DebugMeta, Event, SentryRequest, TransactionSamplingMethod } from '@sentry/types';
 
 import { API } from '../../src/api';
 import { eventToSentryRequest } from '../../src/request';
 
 describe('eventToSentryRequest', () => {
-  let api: API;
+  function parseEnvelopeRequest(request: SentryRequest): any {
+    const [envelopeHeaderString, itemHeaderString, eventString] = request.body.split('\n');
+
+    return {
+      envelopeHeader: JSON.parse(envelopeHeaderString),
+      itemHeader: JSON.parse(itemHeaderString),
+      event: JSON.parse(eventString),
+    };
+  }
+
+  const api = new API('https://dogsarebadatkeepingsecrets@squirrelchasers.ingest.sentry.io/12312012', {
+    sdk: {
+      integrations: ['AWSLambda'],
+      name: 'sentry.javascript.browser',
+      version: `12.31.12`,
+      packages: [{ name: 'npm:@sentry/browser', version: `12.31.12` }],
+    },
+  });
   let event: Event;
 
   beforeEach(() => {
-    api = new API('https://dogsarebadatkeepingsecrets@squirrelchasers.ingest.sentry.io/12312012', {
-      sdk: {
-        integrations: ['AWSLambda'],
-        name: 'sentry.javascript.browser',
-        version: `12.31.12`,
-        packages: [{ name: 'npm:@sentry/browser', version: `6.6.6` }],
-      },
-    });
     event = {
       contexts: { trace: { trace_id: '1231201211212012', span_id: '12261980', op: 'pageload' } },
       environment: 'dogpark',
@@ -28,68 +37,63 @@ describe('eventToSentryRequest', () => {
     };
   });
 
-  [
-    { method: TransactionSamplingMethod.Rate, rate: '0.1121', dog: 'Charlie' },
-    { method: TransactionSamplingMethod.Sampler, rate: '0.1231', dog: 'Maisey' },
-    { method: TransactionSamplingMethod.Inheritance, dog: 'Cory' },
-    { method: TransactionSamplingMethod.Explicit, dog: 'Bodhi' },
-
-    // this shouldn't ever happen (at least the method should always be included in tags), but good to know that things
-    // won't blow up if it does
-    { dog: 'Lucy' },
-  ].forEach(({ method, rate, dog }) => {
-    it(`adds transaction sampling information to item header - ${method}, ${rate}, ${dog}`, () => {
-      // TODO kmclb - once tag types are loosened, don't need to cast to string here
-      event.tags = { __sentry_samplingMethod: String(method), __sentry_sampleRate: String(rate), dog };
-
-      const result = eventToSentryRequest(event, api);
-
-      const [envelopeHeaderString, itemHeaderString, eventString] = result.body.split('\n');
-
-      const envelope = {
-        envelopeHeader: JSON.parse(envelopeHeaderString),
-        itemHeader: JSON.parse(itemHeaderString),
-        event: JSON.parse(eventString),
-      };
-
-      // the right stuff is added to the item header
-      expect(envelope.itemHeader).toEqual({
-        type: 'transaction',
-        // TODO kmclb - once tag types are loosened, don't need to cast to string here
-        sample_rates: [{ id: String(method), rate: String(rate) }],
-      });
-
-      // show that it pops the right tags and leaves the rest alone
-      expect('__sentry_samplingMethod' in envelope.event.tags).toBe(false);
-      expect('__sentry_sampleRate' in envelope.event.tags).toBe(false);
-      expect('dog' in envelope.event.tags).toBe(true);
-    });
+  it(`adds transaction sampling information to item header`, () => {
+    event.debug_meta = { transactionSampling: { method: TransactionSamplingMethod.Rate, rate: 0.1121 } };
+
+    const result = eventToSentryRequest(event, api);
+    const envelope = parseEnvelopeRequest(result);
+
+    expect(envelope.itemHeader).toEqual(
+      expect.objectContaining({
+        sample_rates: [{ id: TransactionSamplingMethod.Rate, rate: 0.1121 }],
+      }),
+    );
   });
 
-  it('adds sdk info to envelope header', () => {
+  it('removes transaction sampling information (and only that) from debug_meta', () => {
+    event.debug_meta = {
+      transactionSampling: { method: TransactionSamplingMethod.Sampler, rate: 0.1121 },
+      dog: 'Charlie',
+    } as DebugMeta;
+
     const result = eventToSentryRequest(event, api);
+    const envelope = parseEnvelopeRequest(result);
 
-    const envelopeHeaderString = result.body.split('\n')[0];
-    const parsedHeader = JSON.parse(envelopeHeaderString);
+    expect('transactionSampling' in envelope.event.debug_meta).toBe(false);
+    expect('dog' in envelope.event.debug_meta).toBe(true);
+  });
 
-    expect(parsedHeader).toEqual(
+  it('removes debug_meta entirely if it ends up empty', () => {
+    event.debug_meta = {
+      transactionSampling: { method: TransactionSamplingMethod.Rate, rate: 0.1121 },
+    } as DebugMeta;
+
+    const result = eventToSentryRequest(event, api);
+    const envelope = parseEnvelopeRequest(result);
+
+    expect('debug_meta' in envelope.event).toBe(false);
+  });
+
+  it('adds sdk info to envelope header', () => {
+    const result = eventToSentryRequest(event, api);
+    const envelope = parseEnvelopeRequest(result);
+
+    expect(envelope.envelopeHeader).toEqual(
       expect.objectContaining({ sdk: { name: 'sentry.javascript.browser', version: '12.31.12' } }),
     );
   });
 
   it('adds sdk info to event body', () => {
     const result = eventToSentryRequest(event, api);
+    const envelope = parseEnvelopeRequest(result);
 
-    const eventString = result.body.split('\n')[2];
-    const parsedEvent = JSON.parse(eventString);
-
-    expect(parsedEvent).toEqual(
+    expect(envelope.event).toEqual(
       expect.objectContaining({
         sdk: {
           integrations: ['AWSLambda'],
           name: 'sentry.javascript.browser',
           version: `12.31.12`,
-          packages: [{ name: 'npm:@sentry/browser', version: `6.6.6` }],
+          packages: [{ name: 'npm:@sentry/browser', version: `12.31.12` }],
         },
       }),
     );
@@ -99,22 +103,21 @@ describe('eventToSentryRequest', () => {
     event.sdk = {
       integrations: ['Clojure'],
       name: 'foo',
-      packages: [{ name: 'npm:@sentry/clj', version: `6.6.6` }],
+      packages: [{ name: 'npm:@sentry/clj', version: `12.31.12` }],
       version: '1337',
     };
-    const result = eventToSentryRequest(event, api);
 
-    const eventString = result.body.split('\n')[2];
-    const parsedEvent = JSON.parse(eventString);
+    const result = eventToSentryRequest(event, api);
+    const envelope = parseEnvelopeRequest(result);
 
-    expect(parsedEvent).toEqual(
+    expect(envelope.event).toEqual(
       expect.objectContaining({
         sdk: {
           integrations: ['Clojure', 'AWSLambda'],
           name: 'foo',
           packages: [
-            { name: 'npm:@sentry/clj', version: `6.6.6` },
-            { name: 'npm:@sentry/browser', version: `6.6.6` },
+            { name: 'npm:@sentry/clj', version: `12.31.12` },
+            { name: 'npm:@sentry/browser', version: `12.31.12` },
           ],
           version: '1337',
         },
diff --git a/packages/tracing/src/hubextensions.ts b/packages/tracing/src/hubextensions.ts
@@ -45,7 +45,9 @@ function sample<T extends Transaction>(hub: Hub, transaction: T, samplingContext
 
   // if the user has forced a sampling decision by passing a `sampled` value in their transaction context, go with that
   if (transaction.sampled !== undefined) {
-    transaction.tags = { ...transaction.tags, __sentry_samplingMethod: TransactionSamplingMethod.Explicit };
+    transaction.setMetadata({
+      transactionSampling: { method: TransactionSamplingMethod.Explicit },
+    });
     return transaction;
   }
 
@@ -54,25 +56,27 @@ function sample<T extends Transaction>(hub: Hub, transaction: T, samplingContext
   let sampleRate;
   if (typeof options.tracesSampler === 'function') {
     sampleRate = options.tracesSampler(samplingContext);
-    // cast the rate to a number first in case it's a boolean
-    transaction.tags = {
-      ...transaction.tags,
-      __sentry_samplingMethod: TransactionSamplingMethod.Sampler,
-      // TODO kmclb - once tag types are loosened, don't need to cast to string here
-      __sentry_sampleRate: String(Number(sampleRate)),
-    };
+    transaction.setMetadata({
+      transactionSampling: {
+        method: TransactionSamplingMethod.Sampler,
+        // cast to number in case it's a boolean
+        rate: Number(sampleRate),
+      },
+    });
   } else if (samplingContext.parentSampled !== undefined) {
     sampleRate = samplingContext.parentSampled;
-    transaction.tags = { ...transaction.tags, __sentry_samplingMethod: TransactionSamplingMethod.Inheritance };
+    transaction.setMetadata({
+      transactionSampling: { method: TransactionSamplingMethod.Inheritance },
+    });
   } else {
     sampleRate = options.tracesSampleRate;
-    // cast the rate to a number first in case it's a boolean
-    transaction.tags = {
-      ...transaction.tags,
-      __sentry_samplingMethod: TransactionSamplingMethod.Rate,
-      // TODO kmclb - once tag types are loosened, don't need to cast to string here
-      __sentry_sampleRate: String(Number(sampleRate)),
-    };
+    transaction.setMetadata({
+      transactionSampling: {
+        method: TransactionSamplingMethod.Rate,
+        // cast to number in case it's a boolean
+        rate: Number(sampleRate),
+      },
+    });
   }
 
   // Since this is coming from the user (or from a function provided by the user), who knows what we might get. (The
diff --git a/packages/tracing/src/transaction.ts b/packages/tracing/src/transaction.ts
@@ -4,9 +4,16 @@ import { dropUndefinedKeys, isInstanceOf, logger } from '@sentry/utils';
 
 import { Span as SpanClass, SpanRecorder } from './span';
 
+interface TransactionMetadata {
+  transactionSampling?: { [key: string]: string | number };
+}
+
 /** JSDoc */
 export class Transaction extends SpanClass implements TransactionInterface {
   public name: string;
+
+  private _metadata: TransactionMetadata = {};
+
   private _measurements: Measurements = {};
 
   /**
@@ -64,6 +71,14 @@ export class Transaction extends SpanClass implements TransactionInterface {
     this._measurements = { ...measurements };
   }
 
+  /**
+   * Set metadata for this transaction.
+   * @hidden
+   */
+  public setMetadata(newMetadata: TransactionMetadata): void {
+    this._metadata = { ...this._metadata, ...newMetadata };
+  }
+
   /**
    * @inheritDoc
    */
@@ -108,6 +123,7 @@ export class Transaction extends SpanClass implements TransactionInterface {
       timestamp: this.endTimestamp,
       transaction: this.name,
       type: 'transaction',
+      debug_meta: this._metadata,
     };
 
     const hasMeasurements = Object.keys(this._measurements).length > 0;
diff --git a/packages/tracing/test/hub.test.ts b/packages/tracing/test/hub.test.ts
@@ -2,17 +2,20 @@
 import { BrowserClient } from '@sentry/browser';
 import { Hub } from '@sentry/hub';
 import * as hubModule from '@sentry/hub';
+import { TransactionSamplingMethod } from '@sentry/types';
 import * as utilsModule from '@sentry/utils'; // for mocking
 import { logger } from '@sentry/utils';
 
 import { BrowserTracing } from '../src/browser/browsertracing';
 import { addExtensionMethods } from '../src/hubextensions';
+import { Transaction } from '../src/transaction';
 import { extractTraceparentData, TRACEPARENT_REGEXP } from '../src/utils';
 import { addDOMPropertiesToGlobal, getSymbolObjectKeyByName } from './testutils';
 
 addExtensionMethods();
 
 const mathRandom = jest.spyOn(Math, 'random');
+jest.spyOn(Transaction.prototype, 'setMetadata');
 
 // we have to add things into the real global object (rather than mocking the return value of getGlobalObject) because
 // there are modules which call getGlobalObject as they load, which is seemingly too early for jest to intervene
@@ -26,7 +29,7 @@ describe('Hub', () => {
   });
 
   afterEach(() => {
-    jest.restoreAllMocks();
+    jest.clearAllMocks();
     jest.useRealTimers();
   });
 
@@ -184,35 +187,39 @@ describe('Hub', () => {
       it('should record sampling method when sampling decision is explicitly set', () => {
         const tracesSampler = jest.fn().mockReturnValue(0.1121);
         const hub = new Hub(new BrowserClient({ tracesSampler }));
-        const transaction = hub.startTransaction({ name: 'dogpark', sampled: true });
+        hub.startTransaction({ name: 'dogpark', sampled: true });
 
-        expect(transaction.tags).toEqual(expect.objectContaining({ __sentry_samplingMethod: 'explicitly_set' }));
+        expect(Transaction.prototype.setMetadata).toHaveBeenCalledWith({
+          transactionSampling: { method: TransactionSamplingMethod.Explicit },
+        });
       });
 
       it('should record sampling method and rate when sampling decision comes from tracesSampler', () => {
         const tracesSampler = jest.fn().mockReturnValue(0.1121);
         const hub = new Hub(new BrowserClient({ tracesSampler }));
-        const transaction = hub.startTransaction({ name: 'dogpark' });
+        hub.startTransaction({ name: 'dogpark' });
 
-        expect(transaction.tags).toEqual(
-          expect.objectContaining({ __sentry_samplingMethod: 'client_sampler', __sentry_sampleRate: '0.1121' }),
-        );
+        expect(Transaction.prototype.setMetadata).toHaveBeenCalledWith({
+          transactionSampling: { method: TransactionSamplingMethod.Sampler, rate: 0.1121 },
+        });
       });
 
       it('should record sampling method when sampling decision is inherited', () => {
         const hub = new Hub(new BrowserClient({ tracesSampleRate: 0.1121 }));
-        const transaction = hub.startTransaction({ name: 'dogpark', parentSampled: true });
+        hub.startTransaction({ name: 'dogpark', parentSampled: true });
 
-        expect(transaction.tags).toEqual(expect.objectContaining({ __sentry_samplingMethod: 'inheritance' }));
+        expect(Transaction.prototype.setMetadata).toHaveBeenCalledWith({
+          transactionSampling: { method: TransactionSamplingMethod.Inheritance },
+        });
       });
 
       it('should record sampling method and rate when sampling decision comes from traceSampleRate', () => {
         const hub = new Hub(new BrowserClient({ tracesSampleRate: 0.1121 }));
-        const transaction = hub.startTransaction({ name: 'dogpark' });
+        hub.startTransaction({ name: 'dogpark' });
 
-        expect(transaction.tags).toEqual(
-          expect.objectContaining({ __sentry_samplingMethod: 'client_rate', __sentry_sampleRate: '0.1121' }),
-        );
+        expect(Transaction.prototype.setMetadata).toHaveBeenCalledWith({
+          transactionSampling: { method: TransactionSamplingMethod.Rate, rate: 0.1121 },
+        });
       });
     });
 
diff --git a/packages/types/src/debugMeta.ts b/packages/types/src/debugMeta.ts
@@ -3,6 +3,7 @@
  **/
 export interface DebugMeta {
   images?: Array<DebugImage>;
+  transactionSampling?: { rate?: number; method?: string };
 }
 
 /**

Original file line number	Diff line number	Diff line change
`@@ -3,6 +3,7 @@`
`3`	`3`	`**/`
`4`	`4`	`export interface DebugMeta {`
`5`	`5`	`images?: Array<DebugImage>;`
	`6`	`+ transactionSampling?: { rate?: number; method?: string };`
`6`	`7`	`}`
`7`	`8`
`8`	`9`	`/**`