Skip to content

ref(v8): Remove metadata on transaction #11397

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Apr 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions dev-packages/rollup-utils/plugins/bundlePlugins.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
51 changes: 29 additions & 22 deletions packages/core/src/tracing/dynamicSamplingContext.ts
Original file line number Diff line number Diff line change
@@ -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.
*
Expand All @@ -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)
*
Expand All @@ -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') {
Expand Down
25 changes: 7 additions & 18 deletions packages/core/src/tracing/trace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import {
spanTimeInputToSeconds,
spanToJSON,
} from '../utils/spanUtils';
import { getDynamicSamplingContextFromSpan } from './dynamicSamplingContext';
import { freezeDscOnSpan, getDynamicSamplingContextFromSpan } from './dynamicSamplingContext';
import { logSpanStart } from './logSpans';
import { sampleSpan } from './sampling';
import { SentryNonRecordingSpan } from './sentryNonRecordingSpan';
Expand Down Expand Up @@ -245,12 +245,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(),
Expand All @@ -262,23 +259,15 @@ function createChildSpanOrTransaction({
parentSpanId,
parentSampled: sampled,
...spanContext,
metadata: {
dynamicSamplingContext: dsc,
// eslint-disable-next-line deprecation/deprecation
...spanContext.metadata,
},
});

if (dsc) {
freezeDscOnSpan(span, dsc);
}
Comment on lines +264 to +266
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to check here but I think this is already how it works: Freezing only happens where we're continuing a trace (i.e. if a propagationContext with a DSC exists). In case it doesn't (i.e. we're head of trace || no DSC on propCtx), we don't freeze the DSC at all at the moment, correct? Meaning, we'd populate it for outgoing requests headers or when the span/txn is is processed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, dsc will only be set on the propagation context if we had an incoming trace, otherwise this will be undefined and we don't freeze it!

}

logSpanStart(span);

// 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();
}

setCapturedScopesOnSpan(span, scope, isolationScope);

return span;
Expand Down
63 changes: 1 addition & 62 deletions packages/core/src/tracing/transaction.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import type {
Contexts,
DynamicSamplingContext,
Hub,
MeasurementUnit,
Measurements,
Expand All @@ -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';
Expand All @@ -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
Expand All @@ -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;
Expand All @@ -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
*/
Expand Down Expand Up @@ -198,9 +141,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 = {
Expand All @@ -215,7 +155,6 @@ export class Transaction extends SentrySpan implements TransactionInterface {
transaction: this._name,
type: 'transaction',
sdkProcessingMetadata: {
...metadata,
capturedSpanScope,
capturedSpanIsolationScope,
...dropUndefinedKeys({
Expand Down
15 changes: 7 additions & 8 deletions packages/core/test/lib/tracing/dynamicSamplingContext.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
getDynamicSamplingContextFromSpan,
startInactiveSpan,
} from '../../../src/tracing';
import { freezeDscOnSpan } from '../../../src/tracing/dynamicSamplingContext';
import { TestClient, getDefaultTestClientOptions } from '../../mocks/client';

describe('getDynamicSamplingContextFromSpan', () => {
Expand All @@ -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' });
Expand Down Expand Up @@ -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,
Expand All @@ -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,
},
});

Expand Down
57 changes: 1 addition & 56 deletions packages/core/test/lib/tracing/transaction.test.ts
Original file line number Diff line number Diff line change
@@ -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', () => {
Expand All @@ -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 */
});
});
1 change: 0 additions & 1 deletion packages/types/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,6 @@ export type {
TraceparentData,
Transaction,
TransactionArguments,
TransactionMetadata,
TransactionSource,
} from './transaction';
export type {
Expand Down
Loading