feat(core): Deprecate Span.transaction in favor of getRootSpan (#10134)

Lms24 · web-flow · commit 324e5bff64e0 · 2024-01-11T17:50:45.000+01:00
Deprecate the `transaction` field on the `Span` interface and
class. Instead, we'll store the span hierarchy in a different format
external to the span class. In node-experimental, we use a WeakMap
referencing the parent span as a data structure which we might need to
generally do in v8. This, however, is a breaking change, so for now we
deprecate the field but use it in the replacement utility function for
v7.
diff --git a/MIGRATION.md b/MIGRATION.md
@@ -152,6 +152,7 @@ In v8, the Span class is heavily reworked. The following properties & methods ar
 - `span.setTag()`: Use `span.setAttribute()` instead or set tags on the surrounding scope.
 - `span.setData()`: Use `span.setAttribute()` instead.
 - `span.instrumenter` This field was removed and will be replaced internally.
+- `span.transaction`: Use `getRootSpan` utility function instead.
 - `transaction.setContext()`: Set context on the surrounding scope instead.
 
 ## Deprecate `pushScope` & `popScope` in favor of `withScope`
diff --git a/packages/astro/src/server/meta.ts b/packages/astro/src/server/meta.ts
@@ -1,6 +1,7 @@
 import {
   getDynamicSamplingContextFromClient,
   getDynamicSamplingContextFromSpan,
+  getRootSpan,
   spanToTraceHeader,
 } from '@sentry/core';
 import type { Client, Scope, Span } from '@sentry/types';
@@ -32,12 +33,12 @@ export function getTracingMetaTags(
   client: Client | undefined,
 ): { sentryTrace: string; baggage?: string } {
   const { dsc, sampled, traceId } = scope.getPropagationContext();
-  const transaction = span?.transaction;
+  const rootSpan = span && getRootSpan(span);
 
   const sentryTrace = span ? spanToTraceHeader(span) : generateSentryTraceHeader(traceId, undefined, sampled);
 
-  const dynamicSamplingContext = transaction
-    ? getDynamicSamplingContextFromSpan(transaction)
+  const dynamicSamplingContext = rootSpan
+    ? getDynamicSamplingContextFromSpan(rootSpan)
     : dsc
       ? dsc
       : client
diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts
@@ -81,6 +81,7 @@ export {
   spanToJSON,
   spanIsSampled,
 } from './utils/spanUtils';
+export { getRootSpan } from './utils/getRootSpan';
 export { DEFAULT_ENVIRONMENT } from './constants';
 export { ModuleMetadata } from './integrations/metadata';
 export { RequestData } from './integrations/requestdata';
diff --git a/packages/core/src/scope.ts b/packages/core/src/scope.ts
@@ -334,6 +334,9 @@ export class Scope implements ScopeInterface {
     // Often, this span (if it exists at all) will be a transaction, but it's not guaranteed to be. Regardless, it will
     // have a pointer to the currently-active transaction.
     const span = this._span;
+    // Cannot replace with getRootSpan because getRootSpan returns a span, not a transaction
+    // Also, this method will be removed anyway.
+    // eslint-disable-next-line deprecation/deprecation
     return span && span.transaction;
   }
 
diff --git a/packages/core/src/server-runtime-client.ts b/packages/core/src/server-runtime-client.ts
@@ -26,6 +26,7 @@ import {
   getDynamicSamplingContextFromClient,
   getDynamicSamplingContextFromSpan,
 } from './tracing';
+import { getRootSpan } from './utils/getRootSpan';
 import { spanToTraceContext } from './utils/spanUtils';
 
 export interface ServerRuntimeClientOptions extends ClientOptions<BaseTransportOptions> {
@@ -262,7 +263,7 @@ export class ServerRuntimeClient<
     // eslint-disable-next-line deprecation/deprecation
     const span = scope.getSpan();
     if (span) {
-      const samplingContext = span.transaction ? getDynamicSamplingContextFromSpan(span) : undefined;
+      const samplingContext = getRootSpan(span) ? getDynamicSamplingContextFromSpan(span) : undefined;
       return [samplingContext, spanToTraceContext(span)];
     }
 
diff --git a/packages/core/src/tracing/dynamicSamplingContext.ts b/packages/core/src/tracing/dynamicSamplingContext.ts
@@ -3,6 +3,7 @@ import { dropUndefinedKeys } from '@sentry/utils';
 
 import { DEFAULT_ENVIRONMENT } from '../constants';
 import { getClient, getCurrentScope } from '../exports';
+import { getRootSpan } from '../utils/getRootSpan';
 import { spanIsSampled, spanToJSON } from '../utils/spanUtils';
 
 /**
@@ -54,9 +55,8 @@ export function getDynamicSamplingContextFromSpan(span: Span): Readonly<Partial<
   // passing emit=false here to only emit later once the DSC is actually populated
   const dsc = getDynamicSamplingContextFromClient(spanToJSON(span).trace_id || '', client, getCurrentScope());
 
-  // As long as we use `Transaction`s internally, this should be fine.
-  // TODO: We need to replace this with a `getRootSpan(span)` function though
-  const txn = span.transaction as TransactionWithV7FrozenDsc | undefined;
+  // TODO (v8): Remove v7FrozenDsc as a Transaction will no longer have _frozenDynamicSamplingContext
+  const txn = getRootSpan(span) as TransactionWithV7FrozenDsc | undefined;
   if (!txn) {
     return dsc;
   }
diff --git a/packages/core/src/tracing/span.ts b/packages/core/src/tracing/span.ts
@@ -16,6 +16,7 @@ import type {
 import { dropUndefinedKeys, logger, timestampInSeconds, uuid4 } from '@sentry/utils';
 
 import { DEBUG_BUILD } from '../debug-build';
+import { getRootSpan } from '../utils/getRootSpan';
 import {
   TRACE_FLAG_NONE,
   TRACE_FLAG_SAMPLED,
@@ -105,6 +106,7 @@ export class Span implements SpanInterface {
 
   /**
    * @inheritDoc
+   * @deprecated Use top level `Sentry.getRootSpan()` instead
    */
   public transaction?: Transaction;
 
@@ -304,12 +306,16 @@ export class Span implements SpanInterface {
       childSpan.spanRecorder.add(childSpan);
     }
 
-    childSpan.transaction = this.transaction;
+    const rootSpan = getRootSpan(this);
+    // TODO: still set span.transaction here until we have a more permanent solution
+    // Probably similarly to the weakmap we hold in node-experimental
+    // eslint-disable-next-line deprecation/deprecation
+    childSpan.transaction = rootSpan as Transaction;
 
-    if (DEBUG_BUILD && childSpan.transaction) {
+    if (DEBUG_BUILD && rootSpan) {
       const opStr = (spanContext && spanContext.op) || '< unknown op >';
       const nameStr = spanToJSON(childSpan).description || '< unknown name >';
-      const idStr = childSpan.transaction.spanContext().spanId;
+      const idStr = rootSpan.spanContext().spanId;
 
       const logMessage = `[Tracing] Starting '${opStr}' span on transaction '${nameStr}' (${idStr}).`;
       logger.log(logMessage);
@@ -416,11 +422,12 @@ export class Span implements SpanInterface {
 
   /** @inheritdoc */
   public end(endTimestamp?: SpanTimeInput): void {
+    const rootSpan = getRootSpan(this);
     if (
       DEBUG_BUILD &&
       // Don't call this for transactions
-      this.transaction &&
-      this.transaction.spanContext().spanId !== this._spanId
+      rootSpan &&
+      rootSpan.spanContext().spanId !== this._spanId
     ) {
       const logMessage = this._logMessage;
       if (logMessage) {
diff --git a/packages/core/src/tracing/transaction.ts b/packages/core/src/tracing/transaction.ts
@@ -66,6 +66,8 @@ export class Transaction extends SpanClass implements TransactionInterface {
     this._trimEnd = transactionContext.trimEnd;
 
     // this is because transactions are also spans, and spans have a transaction pointer
+    // TODO (v8): Replace this with another way to set the root span
+    // eslint-disable-next-line deprecation/deprecation
     this.transaction = this;
 
     // If Dynamic Sampling Context is provided during the creation of the transaction, we freeze it as it usually means
diff --git a/packages/core/src/utils/applyScopeDataToEvent.ts b/packages/core/src/utils/applyScopeDataToEvent.ts
@@ -1,6 +1,7 @@
 import type { Breadcrumb, Event, PropagationContext, ScopeData, Span } from '@sentry/types';
 import { arrayify } from '@sentry/utils';
 import { getDynamicSamplingContextFromSpan } from '../tracing/dynamicSamplingContext';
+import { getRootSpan } from './getRootSpan';
 import { spanToJSON, spanToTraceContext } from './spanUtils';
 
 /**
@@ -174,13 +175,13 @@ function applySdkMetadataToEvent(
 
 function applySpanToEvent(event: Event, span: Span): void {
   event.contexts = { trace: spanToTraceContext(span), ...event.contexts };
-  const transaction = span.transaction;
-  if (transaction) {
+  const rootSpan = getRootSpan(span);
+  if (rootSpan) {
     event.sdkProcessingMetadata = {
       dynamicSamplingContext: getDynamicSamplingContextFromSpan(span),
       ...event.sdkProcessingMetadata,
     };
-    const transactionName = spanToJSON(transaction).description;
+    const transactionName = spanToJSON(rootSpan).description;
     if (transactionName) {
       event.tags = { transaction: transactionName, ...event.tags };
     }
diff --git a/packages/core/src/utils/getRootSpan.ts b/packages/core/src/utils/getRootSpan.ts
@@ -0,0 +1,15 @@
+import type { Span } from '@sentry/types';
+
+/**
+ * Returns the root span of a given span.
+ *
+ * As long as we use `Transaction`s internally, the returned root span
+ * will be a `Transaction` but be aware that this might change in the future.
+ *
+ * If the given span has no root span or transaction, `undefined` is returned.
+ */
+export function getRootSpan(span: Span): Span | undefined {
+  // TODO (v8): Remove this check and just return span
+  // eslint-disable-next-line deprecation/deprecation
+  return span.transaction;
+}
diff --git a/packages/core/test/lib/tracing/dynamicSamplingContext.test.ts b/packages/core/test/lib/tracing/dynamicSamplingContext.test.ts
@@ -20,7 +20,7 @@ describe('getDynamicSamplingContextFromSpan', () => {
   });
 
   test('returns the DSC provided during transaction creation', () => {
-    // eslint-disable-next-line deprecation/deprecation
+    // eslint-disable-next-line deprecation/deprecation -- using old API on purpose
     const transaction = new Transaction({
       name: 'tx',
       metadata: { dynamicSamplingContext: { environment: 'myEnv' } },
@@ -68,7 +68,7 @@ describe('getDynamicSamplingContextFromSpan', () => {
   });
 
   test('returns a new DSC, if no DSC was provided during transaction creation (via new Txn and deprecated metadata)', () => {
-    // eslint-disable-next-line deprecation/deprecation
+    // eslint-disable-next-line deprecation/deprecation -- using old API on purpose
     const transaction = new Transaction({
       name: 'tx',
       metadata: {
@@ -92,7 +92,7 @@ describe('getDynamicSamplingContextFromSpan', () => {
 
   describe('Including transaction name in DSC', () => {
     test('is not included if transaction source is url', () => {
-      // eslint-disable-next-line deprecation/deprecation
+      // eslint-disable-next-line deprecation/deprecation -- using old API on purpose
       const transaction = new Transaction({
         name: 'tx',
         metadata: {
@@ -109,8 +109,7 @@ describe('getDynamicSamplingContextFromSpan', () => {
       ['is included if transaction source is parameterized route/url', 'route'],
       ['is included if transaction source is a custom name', 'custom'],
     ])('%s', (_: string, source) => {
-      // eslint-disable-next-line deprecation/deprecation
-      const transaction = new Transaction({
+      const transaction = startInactiveSpan({
         name: 'tx',
         metadata: {
           ...(source && { source: source as TransactionSource }),
@@ -120,7 +119,7 @@ describe('getDynamicSamplingContextFromSpan', () => {
       // Only setting the attribute manually because we're directly calling new Transaction()
       transaction?.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, source);
 
-      const dsc = getDynamicSamplingContextFromSpan(transaction);
+      const dsc = getDynamicSamplingContextFromSpan(transaction!);
 
       expect(dsc.transaction).toEqual('tx');
     });
diff --git a/packages/core/test/lib/utils/getRootSpan.ts b/packages/core/test/lib/utils/getRootSpan.ts
@@ -0,0 +1,36 @@
+import { Span, Transaction, getRootSpan } from '../../../src';
+
+describe('getRootSpan', () => {
+  it('returns the root span of a span (Span)', () => {
+    const root = new Span({ name: 'test' });
+    // @ts-expect-error this is highly illegal and shouldn't happen IRL
+    // eslint-disable-next-line deprecation/deprecation
+    root.transaction = root;
+
+    // eslint-disable-next-line deprecation/deprecation
+    const childSpan = root.startChild({ name: 'child' });
+    expect(getRootSpan(childSpan)).toBe(root);
+  });
+
+  it('returns the root span of a span (Transaction)', () => {
+    // eslint-disable-next-line deprecation/deprecation
+    const root = new Transaction({ name: 'test' });
+
+    // eslint-disable-next-line deprecation/deprecation
+    const childSpan = root.startChild({ name: 'child' });
+    expect(getRootSpan(childSpan)).toBe(root);
+  });
+
+  it('returns the span itself if it is a root span', () => {
+    // eslint-disable-next-line deprecation/deprecation
+    const span = new Transaction({ name: 'test' });
+
+    expect(getRootSpan(span)).toBe(span);
+  });
+
+  it('returns undefined if span has no root span', () => {
+    const span = new Span({ name: 'test' });
+
+    expect(getRootSpan(span)).toBe(undefined);
+  });
+});
diff --git a/packages/opentelemetry-node/src/propagator.ts b/packages/opentelemetry-node/src/propagator.ts
@@ -1,7 +1,7 @@
 import type { Baggage, Context, TextMapGetter, TextMapSetter } from '@opentelemetry/api';
 import { TraceFlags, isSpanContextValid, propagation, trace } from '@opentelemetry/api';
 import { W3CBaggagePropagator, isTracingSuppressed } from '@opentelemetry/core';
-import { getDynamicSamplingContextFromSpan, spanToTraceHeader } from '@sentry/core';
+import { getDynamicSamplingContextFromSpan, getRootSpan, spanToTraceHeader } from '@sentry/core';
 import {
   SENTRY_BAGGAGE_KEY_PREFIX,
   baggageHeaderToDynamicSamplingContext,
@@ -35,7 +35,7 @@ export class SentryPropagator extends W3CBaggagePropagator {
     if (span) {
       setter.set(carrier, SENTRY_TRACE_HEADER, spanToTraceHeader(span));
 
-      if (span.transaction) {
+      if (getRootSpan(span)) {
         const dynamicSamplingContext = getDynamicSamplingContextFromSpan(span);
         baggage = Object.entries(dynamicSamplingContext).reduce<Baggage>((b, [dscKey, dscValue]) => {
           if (dscValue) {
diff --git a/packages/opentelemetry-node/src/utils/spanMap.ts b/packages/opentelemetry-node/src/utils/spanMap.ts
@@ -1,3 +1,4 @@
+import { getRootSpan } from '@sentry/core';
 import type { Span as SentrySpan } from '@sentry/types';
 
 interface SpanMapEntry {
@@ -31,7 +32,7 @@ export function getSentrySpan(spanId: string): SentrySpan | undefined {
 export function setSentrySpan(spanId: string, sentrySpan: SentrySpan): void {
   let ref: SpanRefType = SPAN_REF_ROOT;
 
-  const rootSpanId = sentrySpan.transaction?.spanContext().spanId;
+  const rootSpanId = getRootSpan(sentrySpan)?.spanContext().spanId;
 
   if (rootSpanId && rootSpanId !== spanId) {
     const root = SPAN_MAP.get(rootSpanId);
diff --git a/packages/svelte/package.json b/packages/svelte/package.json
@@ -30,6 +30,7 @@
   },
   "dependencies": {
     "@sentry/browser": "7.93.0",
+    "@sentry/core": "7.93.0",
     "@sentry/types": "7.93.0",
     "@sentry/utils": "7.93.0",
     "magic-string": "^0.30.0"
diff --git a/packages/svelte/src/performance.ts b/packages/svelte/src/performance.ts
@@ -3,6 +3,7 @@ import type { Span, Transaction } from '@sentry/types';
 import { afterUpdate, beforeUpdate, onMount } from 'svelte';
 import { current_component } from 'svelte/internal';
 
+import { getRootSpan } from '@sentry/core';
 import { DEFAULT_COMPONENT_NAME, UI_SVELTE_INIT, UI_SVELTE_UPDATE } from './constants';
 import type { TrackComponentOptions } from './types';
 
@@ -74,7 +75,7 @@ function recordUpdateSpans(componentName: string, initSpan?: Span): void {
     // If we are initializing the component when the update span is started, we start it as child
     // of the init span. Else, we start it as a child of the transaction.
     const parentSpan =
-      initSpan && !initSpan.endTimestamp && initSpan.transaction === transaction ? initSpan : transaction;
+      initSpan && !initSpan.endTimestamp && getRootSpan(initSpan) === transaction ? initSpan : transaction;
 
     // eslint-disable-next-line deprecation/deprecation
     updateSpan = parentSpan.startChild({
diff --git a/packages/tracing-internal/src/browser/request.ts b/packages/tracing-internal/src/browser/request.ts
@@ -5,6 +5,7 @@ import {
   getCurrentScope,
   getDynamicSamplingContextFromClient,
   getDynamicSamplingContextFromSpan,
+  getRootSpan,
   hasTracingEnabled,
   spanToJSON,
   spanToTraceHeader,
@@ -298,7 +299,7 @@ export function xhrCallback(
 
   if (xhr.setRequestHeader && shouldAttachHeaders(sentryXhrData.url)) {
     if (span) {
-      const transaction = span && span.transaction;
+      const transaction = span && getRootSpan(span);
       const dynamicSamplingContext = transaction && getDynamicSamplingContextFromSpan(transaction);
       const sentryBaggageHeader = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext);
       setHeaderOnXhr(xhr, spanToTraceHeader(span), sentryBaggageHeader);
diff --git a/packages/tracing-internal/src/common/fetch.ts b/packages/tracing-internal/src/common/fetch.ts
@@ -4,6 +4,7 @@ import {
   getCurrentScope,
   getDynamicSamplingContextFromClient,
   getDynamicSamplingContextFromSpan,
+  getRootSpan,
   hasTracingEnabled,
   spanToTraceHeader,
 } from '@sentry/core';
@@ -134,7 +135,7 @@ export function addTracingHeadersToFetchRequest(
   // eslint-disable-next-line deprecation/deprecation
   const span = requestSpan || scope.getSpan();
 
-  const transaction = span && span.transaction;
+  const transaction = span && getRootSpan(span);
 
   const { traceId, sampled, dsc } = scope.getPropagationContext();
 
diff --git a/packages/types/src/span.ts b/packages/types/src/span.ts
@@ -216,6 +216,7 @@ export interface Span extends SpanContext {
 
   /**
    * The transaction containing this span
+   * @deprecated Use top level `Sentry.getRootSpan()` instead
    */
   transaction?: Transaction;
 

Original file line number	Diff line number	Diff line change
`@@ -334,6 +334,9 @@ export class Scope implements ScopeInterface {`
`334`	`334`	`// Often, this span (if it exists at all) will be a transaction, but it's not guaranteed to be. Regardless, it will`
`335`	`335`	`// have a pointer to the currently-active transaction.`
`336`	`336`	`const span = this._span;`
	`337`	`+ // Cannot replace with getRootSpan because getRootSpan returns a span, not a transaction`
	`338`	`+ // Also, this method will be removed anyway.`
	`339`	`+ // eslint-disable-next-line deprecation/deprecation`
`337`	`340`	`return span && span.transaction;`
`338`	`341`	`}`
`339`	`342`