From 0d61c0c47d13c9bf49eb32d9e430c1aa5c21dac4 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Mon, 25 Mar 2024 12:17:24 -0400 Subject: [PATCH 1/2] feat(v8/core): Remove span.sampled --- packages/core/src/tracing/sampling.ts | 54 ++++++++----------------- packages/core/src/tracing/sentrySpan.ts | 16 -------- packages/core/src/tracing/trace.ts | 13 ++++-- packages/types/src/transaction.ts | 6 --- 4 files changed, 27 insertions(+), 62 deletions(-) diff --git a/packages/core/src/tracing/sampling.ts b/packages/core/src/tracing/sampling.ts index 1670d38b740d..ce269e55a27d 100644 --- a/packages/core/src/tracing/sampling.ts +++ b/packages/core/src/tracing/sampling.ts @@ -1,11 +1,8 @@ -import type { Options, SamplingContext } from '@sentry/types'; +import type { Options, SamplingContext, TransactionContext } from '@sentry/types'; import { isNaN, logger } from '@sentry/utils'; import { DEBUG_BUILD } from '../debug-build'; -import { SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE } from '../semanticAttributes'; import { hasTracingEnabled } from '../utils/hasTracingEnabled'; -import { spanToJSON } from '../utils/spanUtils'; -import type { Transaction } from './transaction'; /** * Makes a sampling decision for the given transaction and stores it on the transaction. @@ -16,24 +13,21 @@ import type { Transaction } from './transaction'; * This method muttes the given `transaction` and will set the `sampled` value on it. * It returns the same transaction, for convenience. */ -export function sampleTransaction( - transaction: T, +export function sampleTransaction( + transactionContext: TransactionContext, options: Pick, samplingContext: SamplingContext, -): T { +): [sampled: boolean, sampleRate?: number] { // nothing to do if tracing is not enabled if (!hasTracingEnabled(options)) { - // eslint-disable-next-line deprecation/deprecation - transaction.sampled = false; - return transaction; + return [false]; } - // if the user has forced a sampling decision by passing a `sampled` value in their transaction context, go with that - // eslint-disable-next-line deprecation/deprecation - if (transaction.sampled !== undefined) { - // eslint-disable-next-line deprecation/deprecation - transaction.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, Number(transaction.sampled)); - return transaction; + const transactionContextSampled = transactionContext.sampled; + // if the user has forced a sampling decision by passing a `sampled` value in + // their transaction context, go with that. + if (transactionContextSampled !== undefined) { + return [transactionContextSampled, Number(transactionContextSampled)]; } // we would have bailed already if neither `tracesSampler` nor `tracesSampleRate` nor `enableTracing` were defined, so one of these should @@ -41,25 +35,20 @@ export function sampleTransaction( let sampleRate; if (typeof options.tracesSampler === 'function') { sampleRate = options.tracesSampler(samplingContext); - transaction.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, Number(sampleRate)); } else if (samplingContext.parentSampled !== undefined) { sampleRate = samplingContext.parentSampled; } else if (typeof options.tracesSampleRate !== 'undefined') { sampleRate = options.tracesSampleRate; - transaction.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, Number(sampleRate)); } else { // When `enableTracing === true`, we use a sample rate of 100% sampleRate = 1; - transaction.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, sampleRate); } // Since this is coming from the user (or from a function provided by the user), who knows what we might get. (The // only valid values are booleans or numbers between 0 and 1.) if (!isValidSampleRate(sampleRate)) { DEBUG_BUILD && logger.warn('[Tracing] Discarding transaction because of invalid sample rate.'); - // eslint-disable-next-line deprecation/deprecation - transaction.sampled = false; - return transaction; + return [false]; } // if the function returned 0 (or false), or if `tracesSampleRate` is 0, it's a sign the transaction should be dropped @@ -72,40 +61,31 @@ export function sampleTransaction( : 'a negative sampling decision was inherited or tracesSampleRate is set to 0' }`, ); - // eslint-disable-next-line deprecation/deprecation - transaction.sampled = false; - return transaction; + return [false, Number(sampleRate)]; } // Now we roll the dice. Math.random is inclusive of 0, but not of 1, so strict < is safe here. In case sampleRate is // a boolean, the < comparison will cause it to be automatically cast to 1 if it's true and 0 if it's false. - // eslint-disable-next-line deprecation/deprecation - transaction.sampled = Math.random() < (sampleRate as number | boolean); + const shouldSample = Math.random() < sampleRate; // if we're not going to keep it, we're done - // eslint-disable-next-line deprecation/deprecation - if (!transaction.sampled) { + if (!shouldSample) { DEBUG_BUILD && logger.log( `[Tracing] Discarding transaction because it's not included in the random sample (sampling rate = ${Number( sampleRate, )})`, ); - return transaction; + return [false, Number(sampleRate)]; } - if (DEBUG_BUILD) { - const { op, description } = spanToJSON(transaction); - logger.log(`[Tracing] starting ${op} transaction - ${description}`); - } - - return transaction; + return [true, Number(sampleRate)]; } /** * Checks the given sample rate to make sure it is valid type and value (a boolean, or a number between 0 and 1). */ -function isValidSampleRate(rate: unknown): boolean { +function isValidSampleRate(rate: unknown): rate is number | boolean { // we need to check NaN explicitly because it's of type 'number' and therefore wouldn't get caught by this typecheck if (isNaN(rate) || !(typeof rate === 'number' || typeof rate === 'boolean')) { DEBUG_BUILD && diff --git a/packages/core/src/tracing/sentrySpan.ts b/packages/core/src/tracing/sentrySpan.ts index 68570997131d..2eceafafa953 100644 --- a/packages/core/src/tracing/sentrySpan.ts +++ b/packages/core/src/tracing/sentrySpan.ts @@ -97,22 +97,6 @@ export class SentrySpan implements Span { // This rule conflicts with another eslint rule :( /* eslint-disable @typescript-eslint/member-ordering */ - /** - * Was this span chosen to be sent as part of the sample? - * @deprecated Use `isRecording()` instead. - */ - public get sampled(): boolean | undefined { - return this._sampled; - } - - /** - * Was this span chosen to be sent as part of the sample? - * @deprecated You cannot update the sampling decision of a span after span creation. - */ - public set sampled(sampled: boolean | undefined) { - this._sampled = sampled; - } - /** * Attributes for the span. * @deprecated Use `spanToJSON(span).atttributes` instead. diff --git a/packages/core/src/tracing/trace.ts b/packages/core/src/tracing/trace.ts index 2548d94fd60e..b28b739217fd 100644 --- a/packages/core/src/tracing/trace.ts +++ b/packages/core/src/tracing/trace.ts @@ -14,6 +14,7 @@ import { getMainCarrier } from '../asyncContext'; import { getClient, getCurrentScope, getIsolationScope, withScope } from '../currentScopes'; import { getAsyncContextStrategy, getCurrentHub } from '../hub'; +import { SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE } from '../semanticAttributes'; import { handleCallbackErrors } from '../utils/handleCallbackErrors'; import { hasTracingEnabled } from '../utils/hasTracingEnabled'; import { @@ -325,9 +326,7 @@ function _startTransaction(transactionContext: TransactionContext): Transaction const client = getClient(); const options: Partial = (client && client.getOptions()) || {}; - // eslint-disable-next-line deprecation/deprecation - let transaction = new Transaction(transactionContext, getCurrentHub()); - transaction = sampleTransaction(transaction, options, { + const [sampled, sampleRate] = sampleTransaction(transactionContext, options, { name: transactionContext.name, parentSampled: transactionContext.parentSampled, transactionContext, @@ -337,8 +336,16 @@ function _startTransaction(transactionContext: TransactionContext): Transaction ...transactionContext.attributes, }, }); + + // eslint-disable-next-line deprecation/deprecation + const transaction = new Transaction({ ...transactionContext, sampled }, getCurrentHub()); + if (sampleRate) { + transaction.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, sampleRate); + } + if (client) { client.emit('spanStart', transaction); } + return transaction; } diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts index 533401455488..b1db54c45d28 100644 --- a/packages/types/src/transaction.ts +++ b/packages/types/src/transaction.ts @@ -58,12 +58,6 @@ export interface TraceparentData { * Transaction "Class", inherits Span only has `setName` */ export interface Transaction extends Omit, Span { - /** - * Was this transaction chosen to be sent as part of the sample? - * @deprecated Use `spanIsSampled(transaction)` instead. - */ - sampled?: boolean | undefined; - /** * @inheritDoc */ From ed8d136eb182859b7b6d7adc64cd3007eb8d1640 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Mon, 25 Mar 2024 13:01:01 -0400 Subject: [PATCH 2/2] better undefined check --- packages/core/src/tracing/trace.ts | 2 +- packages/core/test/lib/tracing/trace.test.ts | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/core/src/tracing/trace.ts b/packages/core/src/tracing/trace.ts index b28b739217fd..0c853b10b412 100644 --- a/packages/core/src/tracing/trace.ts +++ b/packages/core/src/tracing/trace.ts @@ -339,7 +339,7 @@ function _startTransaction(transactionContext: TransactionContext): Transaction // eslint-disable-next-line deprecation/deprecation const transaction = new Transaction({ ...transactionContext, sampled }, getCurrentHub()); - if (sampleRate) { + if (sampleRate !== undefined) { transaction.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, sampleRate); } diff --git a/packages/core/test/lib/tracing/trace.test.ts b/packages/core/test/lib/tracing/trace.test.ts index b937b20338d1..4460be6a02d1 100644 --- a/packages/core/test/lib/tracing/trace.test.ts +++ b/packages/core/test/lib/tracing/trace.test.ts @@ -354,6 +354,7 @@ describe('startSpan', () => { trace: { data: { 'sentry.source': 'custom', + 'sentry.sample_rate': 1, 'sentry.origin': 'manual', }, parent_span_id: innerParentSpanId, @@ -679,6 +680,7 @@ describe('startSpanManual', () => { trace: { data: { 'sentry.source': 'custom', + 'sentry.sample_rate': 1, 'sentry.origin': 'manual', }, parent_span_id: innerParentSpanId, @@ -932,6 +934,7 @@ describe('startInactiveSpan', () => { trace: { data: { 'sentry.source': 'custom', + 'sentry.sample_rate': 1, 'sentry.origin': 'manual', }, parent_span_id: innerParentSpanId,