From a7d42a2ec0c33011ec20fbec903fda9639e9e16c Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 3 Apr 2024 15:14:44 +0200 Subject: [PATCH 1/4] ref(v8): Remove `metadata` on transaction Instead, we freeze the DSC explicitly onto the transaction now. Apply suggestions from code review Co-authored-by: Lukas Stracke fix comments fix linting --- .../rollup-utils/plugins/bundlePlugins.mjs | 4 +- .../src/tracing/dynamicSamplingContext.ts | 51 ++++++----- packages/core/src/tracing/trace.ts | 25 ++---- packages/core/src/tracing/transaction.ts | 87 ++++++------------- .../tracing/dynamicSamplingContext.test.ts | 15 ++-- .../core/test/lib/tracing/transaction.test.ts | 57 +----------- packages/types/src/transaction.ts | 11 --- 7 files changed, 70 insertions(+), 180 deletions(-) diff --git a/dev-packages/rollup-utils/plugins/bundlePlugins.mjs b/dev-packages/rollup-utils/plugins/bundlePlugins.mjs index 38c7811f3d1a..2404e3fc2d38 100644 --- 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 index 04510683a1b9..2f69e9db6a2e 100644 --- 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 | undefined; +}; + +/** + * Freeze the given DSC on the given span. + */ +export function freezeDscOnSpan(span: Span, dsc: Partial): 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> | undefined; - - private _metadata: Partial; - /** * 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): 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 index f38d3f3e26de..f68e5e0e75a1 100644 --- 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 index 511c26f4b191..12e631760b3c 100644 --- a/packages/core/test/lib/tracing/transaction.test.ts +++ b/packages/core/test/lib/tracing/transaction.test.ts @@ -1,10 +1,4 @@ -import { - SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, - SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, - SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, - Transaction, - spanToJSON, -} from '../../../src'; +import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, Transaction, spanToJSON } from '../../../src'; describe('transaction', () => { describe('name', () => { @@ -27,53 +21,4 @@ describe('transaction', () => { }); /* eslint-enable deprecation/deprecation */ }); - - describe('metadata', () => { - /* eslint-disable deprecation/deprecation */ - it('works with defaults', () => { - const transaction = new Transaction({ name: 'span name' }); - expect(transaction.metadata).toEqual({}); - }); - - it('allows to set metadata in constructor', () => { - const transaction = new Transaction({ name: 'span name', metadata: { request: {} } }); - expect(transaction.metadata).toEqual({ - request: {}, - }); - }); - - it('allows to set source & sample rate data in constructor', () => { - const transaction = new Transaction({ - name: 'span name', - metadata: { request: {} }, - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', - [SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 0.5, - }, - }); - - expect(transaction.metadata).toEqual({ - sampleRate: 0.5, - request: {}, - }); - - expect(spanToJSON(transaction).data).toEqual({ - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'manual', - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', - [SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 0.5, - }); - }); - - it('allows to update metadata via setMetadata', () => { - const transaction = new Transaction({ name: 'span name', metadata: {} }); - - transaction.setMetadata({ request: {} }); - - expect(transaction.metadata).toEqual({ - request: {}, - }); - }); - - /* eslint-enable deprecation/deprecation */ - }); }); diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts index 219097e14981..96cec744b9e0 100644 --- a/packages/types/src/transaction.ts +++ b/packages/types/src/transaction.ts @@ -25,12 +25,6 @@ export interface TransactionArguments extends SentrySpanArguments { * If this transaction has a parent, the parent's sampling decision */ parentSampled?: boolean | undefined; - - /** - * Metadata associated with the transaction, for internal SDK use. - * @deprecated Use attributes or store data on the scope instead. - */ - metadata?: Partial; } /** @@ -58,12 +52,7 @@ export interface TraceparentData { */ export interface Transaction extends Omit, Span { /** - * Metadata about the transaction. - * @deprecated Use attributes or store data on the scope instead. - */ - metadata: TransactionMetadata; - /** * Set observed measurement for this transaction. * * @param name Name of the measurement From ab7171e8f83b3c74859c68b167e4553583824d67 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 3 Apr 2024 16:21:33 +0200 Subject: [PATCH 2/4] fixes --- packages/core/src/tracing/transaction.ts | 24 ------------------------ packages/types/src/transaction.ts | 6 ------ 2 files changed, 30 deletions(-) diff --git a/packages/core/src/tracing/transaction.ts b/packages/core/src/tracing/transaction.ts index c6d9b98946a1..b8b7bbe9fb17 100644 --- a/packages/core/src/tracing/transaction.ts +++ b/packages/core/src/tracing/transaction.ts @@ -91,30 +91,6 @@ 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. */ diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts index 96cec744b9e0..4cebbf954c95 100644 --- a/packages/types/src/transaction.ts +++ b/packages/types/src/transaction.ts @@ -62,12 +62,6 @@ export interface Transaction extends Omit): void; } /** From 8bb175353dc77f53e0de9c75f3887679d788e73f Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 3 Apr 2024 16:30:40 +0200 Subject: [PATCH 3/4] remove `TransactionMetadata` type --- packages/types/src/index.ts | 1 - packages/types/src/transaction.ts | 21 --------------------- 2 files changed, 22 deletions(-) diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 871f41b82625..9928f5d14d0a 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -111,7 +111,6 @@ export type { TraceparentData, Transaction, TransactionArguments, - TransactionMetadata, TransactionSource, } from './transaction'; export type { diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts index 4cebbf954c95..a8081719714b 100644 --- a/packages/types/src/transaction.ts +++ b/packages/types/src/transaction.ts @@ -99,27 +99,6 @@ export interface SamplingContext extends CustomSamplingContext { request?: ExtractedNodeRequestData; } -export interface TransactionMetadata { - /** - * The sample rate used when sampling this transaction. - * @deprecated Use `SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE` attribute instead. - */ - sampleRate?: number; - - /** - * The Dynamic Sampling Context of a transaction. If provided during transaction creation, its Dynamic Sampling - * Context Will be frozen - */ - dynamicSamplingContext?: Partial; - - /** For transactions tracing server-side request handling, the request being tracked. */ - request?: PolymorphicRequest; - - /** For transactions tracing server-side request handling, the path of the request being tracked. */ - /** TODO: If we rm -rf `instrumentServer`, this can go, too */ - requestPath?: string; -} - /** * Contains information about how the name of the transaction was determined. This will be used by the server to decide * whether or not to scrub identifiers from the transaction name, or replace the entire name with a placeholder. From 00a8f25d17ef8a3aeb742eef50d8c1c59b2cf6b4 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 3 Apr 2024 16:55:13 +0200 Subject: [PATCH 4/4] fix linting --- packages/types/src/transaction.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts index a8081719714b..ce98124969ff 100644 --- a/packages/types/src/transaction.ts +++ b/packages/types/src/transaction.ts @@ -1,7 +1,5 @@ -import type { DynamicSamplingContext } from './envelope'; import type { MeasurementUnit } from './measurement'; import type { ExtractedNodeRequestData, WorkerLocation } from './misc'; -import type { PolymorphicRequest } from './polymorphics'; import type { SentrySpanArguments, Span } from './span'; /**