ref(v8): Remove metadata on transaction

mydea · mydea · commit 21ba125ce0af · 2024-04-03T15:14:44.000+02:00
Instead, we freeze the DSC explicitly onto the transaction now.

Apply suggestions from code review

Co-authored-by: Lukas Stracke &lt;lukas.stracke@sentry.io&gt;
fix comments

fix linting
diff --git a/dev-packages/rollup-utils/plugins/bundlePlugins.mjs b/dev-packages/rollup-utils/plugins/bundlePlugins.mjs
@@ -121,9 +121,7 @@ export function makeTerserPlugin() {
           // These are used by instrument.ts in utils for identifying HTML elements & events
           '_sentryCaptured',
           '_sentryId',
-          // For v7 backwards-compatibility we need to access txn._frozenDynamicSamplingContext
-          // TODO (v8): Remove this reserved word
-          '_frozenDynamicSamplingContext',
+          '_frozenDsc',
           // These are used to keep span & scope relationships
           '_sentryRootSpan',
           '_sentryChildSpans',
diff --git a/packages/core/src/tracing/dynamicSamplingContext.ts b/packages/core/src/tracing/dynamicSamplingContext.ts
@@ -1,11 +1,29 @@
-import type { Client, DynamicSamplingContext, Span, Transaction } from '@sentry/types';
-import { dropUndefinedKeys } from '@sentry/utils';
+import type { Client, DynamicSamplingContext, Span } from '@sentry/types';
+import { addNonEnumerableProperty, dropUndefinedKeys } from '@sentry/utils';
 
 import { DEFAULT_ENVIRONMENT } from '../constants';
 import { getClient } from '../currentScopes';
-import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '../semanticAttributes';
+import { SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '../semanticAttributes';
 import { getRootSpan, spanIsSampled, spanToJSON } from '../utils/spanUtils';
 
+/**
+ * If you change this value, also update the terser plugin config to
+ * avoid minification of the object property!
+ */
+const FROZEN_DSC_FIELD = '_frozenDsc';
+
+type SpanWithMaybeDsc = Span & {
+  [FROZEN_DSC_FIELD]?: Partial<DynamicSamplingContext> | undefined;
+};
+
+/**
+ * Freeze the given DSC on the given span.
+ */
+export function freezeDscOnSpan(span: Span, dsc: Partial<DynamicSamplingContext>): void {
+  const spanWithMaybeDsc = span as SpanWithMaybeDsc;
+  addNonEnumerableProperty(spanWithMaybeDsc, FROZEN_DSC_FIELD, dsc);
+}
+
 /**
  * Creates a dynamic sampling context from a client.
  *
@@ -28,11 +46,6 @@ export function getDynamicSamplingContextFromClient(trace_id: string, client: Cl
   return dsc;
 }
 
-/**
- * A Span with a frozen dynamic sampling context.
- */
-type TransactionWithV7FrozenDsc = Transaction & { _frozenDynamicSamplingContext?: DynamicSamplingContext };
-
 /**
  * Creates a dynamic sampling context from a span (and client and scope)
  *
@@ -48,32 +61,26 @@ export function getDynamicSamplingContextFromSpan(span: Span): Readonly<Partial<
 
   const dsc = getDynamicSamplingContextFromClient(spanToJSON(span).trace_id || '', client);
 
-  // TODO (v8): Remove v7FrozenDsc as a Transaction will no longer have _frozenDynamicSamplingContext
   const rootSpan = getRootSpan(span);
   if (!rootSpan) {
     return dsc;
   }
 
-  // TODO (v8): Remove v7FrozenDsc as a Transaction will no longer have _frozenDynamicSamplingContext
-  // For now we need to avoid breaking users who directly created a txn with a DSC, where this field is still set.
-  // @see Transaction class constructor
-  const v7FrozenDsc = rootSpan && (rootSpan as TransactionWithV7FrozenDsc)._frozenDynamicSamplingContext;
-  if (v7FrozenDsc) {
-    return v7FrozenDsc;
+  const frozenDsc = (rootSpan as SpanWithMaybeDsc)[FROZEN_DSC_FIELD];
+  if (frozenDsc) {
+    return frozenDsc;
   }
 
-  // TODO (v8): Replace txn.metadata with txn.attributes[]
-  // We can't do this yet because attributes aren't always set yet.
-  // eslint-disable-next-line deprecation/deprecation
-  const { sampleRate: maybeSampleRate } = (rootSpan as TransactionWithV7FrozenDsc).metadata || {};
+  const jsonSpan = spanToJSON(rootSpan);
+  const attributes = jsonSpan.data || {};
+  const maybeSampleRate = attributes[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE];
+
   if (maybeSampleRate != null) {
     dsc.sample_rate = `${maybeSampleRate}`;
   }
 
   // We don't want to have a transaction name in the DSC if the source is "url" because URLs might contain PII
-  const jsonSpan = spanToJSON(rootSpan);
-
-  const source = (jsonSpan.data || {})[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE];
+  const source = attributes[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE];
 
   // after JSON conversion, txn.name becomes jsonSpan.description
   if (source && source !== 'url') {
diff --git a/packages/core/src/tracing/trace.ts b/packages/core/src/tracing/trace.ts
@@ -17,7 +17,7 @@ import {
   spanTimeInputToSeconds,
   spanToJSON,
 } from '../utils/spanUtils';
-import { getDynamicSamplingContextFromSpan } from './dynamicSamplingContext';
+import { freezeDscOnSpan, getDynamicSamplingContextFromSpan } from './dynamicSamplingContext';
 import { sampleSpan } from './sampling';
 import { SentryNonRecordingSpan } from './sentryNonRecordingSpan';
 import type { SentrySpan } from './sentrySpan';
@@ -237,12 +237,9 @@ function createChildSpanOrTransaction({
       parentSpanId,
       parentSampled: sampled,
       ...spanContext,
-      metadata: {
-        dynamicSamplingContext: dsc,
-        // eslint-disable-next-line deprecation/deprecation
-        ...spanContext.metadata,
-      },
     });
+
+    freezeDscOnSpan(span, dsc);
   } else {
     const { traceId, dsc, parentSpanId, sampled } = {
       ...isolationScope.getPropagationContext(),
@@ -254,19 +251,11 @@ function createChildSpanOrTransaction({
       parentSpanId,
       parentSampled: sampled,
       ...spanContext,
-      metadata: {
-        dynamicSamplingContext: dsc,
-        // eslint-disable-next-line deprecation/deprecation
-        ...spanContext.metadata,
-      },
     });
-  }
 
-  // TODO v8: Technically `startTransaction` can return undefined, which is not reflected by the types
-  // This happens if tracing extensions have not been added
-  // In this case, we just want to return a non-recording span
-  if (!span) {
-    return new SentryNonRecordingSpan();
+    if (dsc) {
+      freezeDscOnSpan(span, dsc);
+    }
   }
 
   setCapturedScopesOnSpan(span, scope, isolationScope);
diff --git a/packages/core/src/tracing/transaction.ts b/packages/core/src/tracing/transaction.ts
@@ -1,6 +1,5 @@
 import type {
   Contexts,
-  DynamicSamplingContext,
   Hub,
   MeasurementUnit,
   Measurements,
@@ -9,15 +8,14 @@ import type {
   Transaction as TransactionInterface,
   TransactionArguments,
   TransactionEvent,
-  TransactionMetadata,
   TransactionSource,
 } from '@sentry/types';
 import { dropUndefinedKeys, logger } from '@sentry/utils';
 
 import { DEBUG_BUILD } from '../debug-build';
 import { getCurrentHub } from '../hub';
 import { getMetricSummaryJsonForSpan } from '../metrics/metric-summary';
-import { SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '../semanticAttributes';
+import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '../semanticAttributes';
 import { getSpanDescendants, spanTimeInputToSeconds, spanToJSON, spanToTraceContext } from '../utils/spanUtils';
 import { getDynamicSamplingContextFromSpan } from './dynamicSamplingContext';
 import { SentrySpan } from './sentrySpan';
@@ -38,11 +36,6 @@ export class Transaction extends SentrySpan implements TransactionInterface {
 
   private _trimEnd?: boolean | undefined;
 
-  // DO NOT yet remove this property, it is used in a hack for v7 backwards compatibility.
-  private _frozenDynamicSamplingContext: Readonly<Partial<DynamicSamplingContext>> | undefined;
-
-  private _metadata: Partial<TransactionMetadata>;
-
   /**
    * This constructor should never be called manually.
    * @internal
@@ -61,56 +54,14 @@ export class Transaction extends SentrySpan implements TransactionInterface {
 
     this._name = transactionContext.name || '';
 
-    this._metadata = {
-      // eslint-disable-next-line deprecation/deprecation
-      ...transactionContext.metadata,
-    };
-
     this._trimEnd = transactionContext.trimEnd;
 
     this._attributes = {
       [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'custom',
       ...this._attributes,
     };
-
-    // If Dynamic Sampling Context is provided during the creation of the transaction, we freeze it as it usually means
-    // there is incoming Dynamic Sampling Context. (Either through an incoming request, a baggage meta-tag, or other means)
-    const incomingDynamicSamplingContext = this._metadata.dynamicSamplingContext;
-    if (incomingDynamicSamplingContext) {
-      // We shallow copy this in case anything writes to the original reference of the passed in `dynamicSamplingContext`
-      this._frozenDynamicSamplingContext = { ...incomingDynamicSamplingContext };
-    }
   }
 
-  // This sadly conflicts with the getter/setter ordering :(
-
-  /**
-   * Get the metadata for this transaction.
-   * @deprecated Use `spanGetMetadata(transaction)` instead.
-   */
-  public get metadata(): TransactionMetadata {
-    // We merge attributes in for backwards compatibility
-    return {
-      // Legacy metadata
-      ...this._metadata,
-
-      // From attributes
-      ...(this._attributes[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE] && {
-        sampleRate: this._attributes[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE] as TransactionMetadata['sampleRate'],
-      }),
-    };
-  }
-
-  /**
-   * Update the metadata for this transaction.
-   * @deprecated Use `spanGetMetadata(transaction)` instead.
-   */
-  public set metadata(metadata: TransactionMetadata) {
-    this._metadata = metadata;
-  }
-
-  /* eslint-enable @typescript-eslint/member-ordering */
-
   /** @inheritdoc */
   public updateName(name: string): this {
     this._name = name;
@@ -127,14 +78,6 @@ export class Transaction extends SentrySpan implements TransactionInterface {
     this._measurements[name] = { value, unit };
   }
 
-  /**
-   * Store metadata on this transaction.
-   * @deprecated Use attributes or store data on the scope instead.
-   */
-  public setMetadata(newMetadata: Partial<TransactionMetadata>): void {
-    this._metadata = { ...this._metadata, ...newMetadata };
-  }
-
   /**
    * @inheritDoc
    */
@@ -148,6 +91,30 @@ export class Transaction extends SentrySpan implements TransactionInterface {
     return this._hub.captureEvent(transaction);
   }
 
+  /**
+   * @inheritDoc
+   */
+  public toContext(): TransactionArguments {
+    // eslint-disable-next-line deprecation/deprecation
+    const spanContext = super.toContext();
+
+    return dropUndefinedKeys({
+      ...spanContext,
+      name: this._name,
+      trimEnd: this._trimEnd,
+    });
+  }
+
+  /**
+   * Override the current hub with a new one.
+   * Used if you want another hub to finish the transaction.
+   *
+   * @internal
+   */
+  public setHub(hub: Hub): void {
+    this._hub = hub;
+  }
+
   /**
    * Finish the transaction & prepare the event to send to Sentry.
    */
@@ -198,9 +165,6 @@ export class Transaction extends SentrySpan implements TransactionInterface {
 
     const { scope: capturedSpanScope, isolationScope: capturedSpanIsolationScope } = getCapturedScopesOnSpan(this);
 
-    // eslint-disable-next-line deprecation/deprecation
-    const { metadata } = this;
-
     const source = this._attributes['sentry.source'] as TransactionSource | undefined;
 
     const transaction: TransactionEvent = {
@@ -215,7 +179,6 @@ export class Transaction extends SentrySpan implements TransactionInterface {
       transaction: this._name,
       type: 'transaction',
       sdkProcessingMetadata: {
-        ...metadata,
         capturedSpanScope,
         capturedSpanIsolationScope,
         ...dropUndefinedKeys({
diff --git a/packages/core/test/lib/tracing/dynamicSamplingContext.test.ts b/packages/core/test/lib/tracing/dynamicSamplingContext.test.ts
@@ -10,6 +10,7 @@ import {
   getDynamicSamplingContextFromSpan,
   startInactiveSpan,
 } from '../../../src/tracing';
+import { freezeDscOnSpan } from '../../../src/tracing/dynamicSamplingContext';
 import { TestClient, getDefaultTestClientOptions } from '../../mocks/client';
 
 describe('getDynamicSamplingContextFromSpan', () => {
@@ -25,13 +26,15 @@ describe('getDynamicSamplingContextFromSpan', () => {
     jest.resetAllMocks();
   });
 
-  test('returns the DSC provided during transaction creation', () => {
+  test('uses frozen DSC from span', () => {
     // eslint-disable-next-line deprecation/deprecation -- using old API on purpose
     const transaction = new Transaction({
       name: 'tx',
-      metadata: { dynamicSamplingContext: { environment: 'myEnv' } },
+      sampled: true,
     });
 
+    freezeDscOnSpan(transaction, { environment: 'myEnv' });
+
     const dynamicSamplingContext = getDynamicSamplingContextFromSpan(transaction);
 
     expect(dynamicSamplingContext).toStrictEqual({ environment: 'myEnv' });
@@ -77,10 +80,8 @@ describe('getDynamicSamplingContextFromSpan', () => {
     // eslint-disable-next-line deprecation/deprecation -- using old API on purpose
     const transaction = new Transaction({
       name: 'tx',
-      metadata: {
-        sampleRate: 0.56,
-      },
       attributes: {
+        [SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 0.56,
         [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route',
       },
       sampled: true,
@@ -103,11 +104,9 @@ describe('getDynamicSamplingContextFromSpan', () => {
       // eslint-disable-next-line deprecation/deprecation -- using old API on purpose
       const transaction = new Transaction({
         name: 'tx',
-        metadata: {
-          sampleRate: 0.56,
-        },
         attributes: {
           [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url',
+          [SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 0.56,
         },
       });
 
diff --git a/packages/core/test/lib/tracing/transaction.test.ts b/packages/core/test/lib/tracing/transaction.test.ts
diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts
