Skip to content

feat(core): Add span.isRecording() instead of span.sampled #10034

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
Jan 5, 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
1 change: 1 addition & 0 deletions MIGRATION.md
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ In v8, the Span class is heavily reworked. The following properties & methods ar
* `span.setName(newName)`: Use `span.updateName(newName)` instead.
* `span.toTraceparent()`: use `spanToTraceHeader(span)` util instead.
* `span.getTraceContext()`: Use `spanToTraceContext(span)` utility function instead.
* `span.sampled`: Use `span.isRecording()` instead.

## Deprecate `pushScope` & `popScope` in favor of `withScope`

Expand Down
6 changes: 3 additions & 3 deletions packages/astro/test/server/meta.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { vi } from 'vitest';
import { getTracingMetaTags, isValidBaggageString } from '../../src/server/meta';

const mockedSpan = {
sampled: true,
isRecording: () => true,
traceId: '12345678901234567890123456789012',
spanId: '1234567890123456',
transaction: {
Expand Down Expand Up @@ -70,7 +70,7 @@ describe('getTracingMetaTags', () => {
const tags = getTracingMetaTags(
// @ts-expect-error - only passing a partial span object
{
sampled: true,
isRecording: () => true,
traceId: '12345678901234567890123456789012',
spanId: '1234567890123456',
transaction: undefined,
Expand All @@ -93,7 +93,7 @@ describe('getTracingMetaTags', () => {
const tags = getTracingMetaTags(
// @ts-expect-error - only passing a partial span object
{
sampled: true,
isRecording: () => true,
traceId: '12345678901234567890123456789012',
spanId: '1234567890123456',
transaction: undefined,
Expand Down
2 changes: 1 addition & 1 deletion packages/browser/src/profiling/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -515,7 +515,7 @@ export function shouldProfileTransaction(transaction: Transaction): boolean {
return false;
}

if (!transaction.sampled) {
if (!transaction.isRecording()) {
if (DEBUG_BUILD) {
logger.log('[Profiling] Discarding profile because transaction was not sampled.');
}
Expand Down
6 changes: 3 additions & 3 deletions packages/browser/test/unit/profiling/hubextensions.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ describe('BrowserProfilingIntegration', () => {
// @ts-expect-error force api to be undefined
global.window.Profiler = undefined;
// set sampled to true so that profiling does not early return
const mockTransaction = { sampled: true } as Transaction;
const mockTransaction = { isRecording: () => true } as Transaction;
expect(() => onProfilingStartRouteTransaction(mockTransaction)).not.toThrow();
});
it('does not throw if constructor throws', () => {
Expand All @@ -80,8 +80,8 @@ describe('BrowserProfilingIntegration', () => {
}
}

// set sampled to true so that profiling does not early return
const mockTransaction = { sampled: true } as Transaction;
// set isRecording to true so that profiling does not early return
const mockTransaction = { isRecording: () => true } as Transaction;

// @ts-expect-error override with our own constructor
global.window.Profiler = Profiler;
Expand Down
2 changes: 1 addition & 1 deletion packages/bun/test/integrations/bunserver.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ describe('Bun Serve Integration', () => {
client.on('finishTransaction', transaction => {
expect(transaction.traceId).toBe(TRACE_ID);
expect(transaction.parentSpanId).toBe(PARENT_SPAN_ID);
expect(transaction.sampled).toBe(true);
expect(transaction.isRecording()).toBe(true);

expect(transaction.metadata?.dynamicSamplingContext).toStrictEqual({ version: '1.0', environment: 'production' });
});
Expand Down
5 changes: 3 additions & 2 deletions packages/core/src/tracing/hubextensions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ function _startTransaction(
The transaction will not be sampled. Please use the ${configInstrumenter} instrumentation to start transactions.`,
);

// eslint-disable-next-line deprecation/deprecation
transactionContext.sampled = false;
}

Expand All @@ -64,7 +65,7 @@ The transaction will not be sampled. Please use the ${configInstrumenter} instru
transactionContext,
...customSamplingContext,
});
if (transaction.sampled) {
if (transaction.isRecording()) {
transaction.initSpanRecorder(options._experiments && (options._experiments.maxSpans as number));
}
if (client && client.emit) {
Expand Down Expand Up @@ -94,7 +95,7 @@ export function startIdleTransaction(
transactionContext,
...customSamplingContext,
});
if (transaction.sampled) {
if (transaction.isRecording()) {
transaction.initSpanRecorder(options._experiments && (options._experiments.maxSpans as number));
}
if (client && client.emit) {
Expand Down
7 changes: 7 additions & 0 deletions packages/core/src/tracing/sampling.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,16 @@ export function sampleTransaction<T extends Transaction>(
): T {
// nothing to do if tracing is not enabled
if (!hasTracingEnabled(options)) {
// eslint-disable-next-line deprecation/deprecation
transaction.sampled = false;
return transaction;
}

// 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) {
transaction.setMetadata({
// eslint-disable-next-line deprecation/deprecation
sampleRate: Number(transaction.sampled),
});
return transaction;
Expand Down Expand Up @@ -60,6 +63,7 @@ export function sampleTransaction<T extends Transaction>(
// 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;
}
Expand All @@ -74,15 +78,18 @@ export function sampleTransaction<T extends Transaction>(
: 'a negative sampling decision was inherited or tracesSampleRate is set to 0'
}`,
);
// eslint-disable-next-line deprecation/deprecation
transaction.sampled = false;
return transaction;
}

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

// if we're not going to keep it, we're done
// eslint-disable-next-line deprecation/deprecation
if (!transaction.sampled) {
DEBUG_BUILD &&
logger.log(
Expand Down
8 changes: 8 additions & 0 deletions packages/core/src/tracing/span.ts
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,7 @@ export class Span implements SpanInterface {
}
// We want to include booleans as well here
if ('sampled' in spanContext) {
// eslint-disable-next-line deprecation/deprecation
this.sampled = spanContext.sampled;
}
if (spanContext.op) {
Expand Down Expand Up @@ -193,6 +194,7 @@ export class Span implements SpanInterface {
const childSpan = new Span({
...spanContext,
parentSpanId: this.spanId,
// eslint-disable-next-line deprecation/deprecation
sampled: this.sampled,
traceId: this.traceId,
});
Expand Down Expand Up @@ -351,6 +353,7 @@ export class Span implements SpanInterface {
this.endTimestamp = spanContext.endTimestamp;
this.op = spanContext.op;
this.parentSpanId = spanContext.parentSpanId;
// eslint-disable-next-line deprecation/deprecation
this.sampled = spanContext.sampled;
this.spanId = spanContext.spanId || this.spanId;
this.startTimestamp = spanContext.startTimestamp || this.startTimestamp;
Expand Down Expand Up @@ -400,6 +403,11 @@ export class Span implements SpanInterface {
});
}

/** @inheritdoc */
public isRecording(): boolean {
return !this.endTimestamp && !!this.sampled;
}

/**
* Get the merged data for this span.
* For now, this combines `data` and `attributes` together,
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/utils/spanUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ export function spanToTraceContext(span: Span): TraceContext {
* Convert a Span to a Sentry trace header.
*/
export function spanToTraceHeader(span: Span): string {
return generateSentryTraceHeader(span.traceId, span.spanId, span.sampled);
return generateSentryTraceHeader(span.traceId, span.spanId, span.isRecording());
}

/**
Expand Down
17 changes: 17 additions & 0 deletions packages/core/test/lib/tracing/span.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,23 @@ describe('span', () => {
});
});

describe('isRecording', () => {
it('returns true for sampled span', () => {
const span = new Span({ sampled: true });
expect(span.isRecording()).toEqual(true);
});

it('returns false for sampled, finished span', () => {
const span = new Span({ sampled: true, endTimestamp: Date.now() });
expect(span.isRecording()).toEqual(false);
});

it('returns false for unsampled span', () => {
const span = new Span({ sampled: false });
expect(span.isRecording()).toEqual(false);
});
});

// Ensure that attributes & data are merged together
describe('_getData', () => {
it('works without data & attributes', () => {
Expand Down
1 change: 1 addition & 0 deletions packages/tracing-internal/src/browser/browsertracing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,7 @@ export class BrowserTracing implements Integration {
this._latestRouteName = finalContext.name;
this._latestRouteSource = finalContext.metadata && finalContext.metadata.source;

// eslint-disable-next-line deprecation/deprecation
if (finalContext.sampled === false) {
DEBUG_BUILD && logger.log(`[Tracing] Will not send ${finalContext.op} transaction because of beforeNavigate.`);
}
Expand Down
8 changes: 8 additions & 0 deletions packages/types/src/span.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ export interface SpanContext {

/**
* Was this span chosen to be sent as part of the sample?
*
* @deprecated Use `isRecording()` instead.
*/
sampled?: boolean;

Expand Down Expand Up @@ -268,4 +270,10 @@ export interface Span extends SpanContext {
trace_id: string;
origin?: SpanOrigin;
};

/**
* If this is span is actually recording data.
* This will return false if tracing is disabled, this span was not sampled or if the span is already finished.
*/
isRecording(): boolean;
}