Skip to content

Commit

Permalink
feat(core): Pass name & attributes to tracesSampler (#10426)
Browse files Browse the repository at this point in the history
Updates `tracesSampler` to receive `name` and `attributes` instead of
relying on `transactionContext` being passed.

---------

Co-authored-by: Luca Forstner <[email protected]>
  • Loading branch information
mydea and lforst authored Jan 31, 2024
1 parent eadf9c0 commit 369faf4
Show file tree
Hide file tree
Showing 7 changed files with 82 additions and 13 deletions.
7 changes: 7 additions & 0 deletions MIGRATION.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,13 @@ npx @sentry/migr8@latest
This will let you select which updates to run, and automatically update your code. Make sure to still review all code
changes!

## Deprecated `transactionContext` passed to `tracesSampler`

Instead of an `transactionContext` being passed to the `tracesSampler` callback, the callback will directly receive
`name` and `attributes` going forward. You can use these to make your sampling decisions, while `transactionContext`
will be removed in v8. Note that the `attributes` are only the attributes at span creation time, and some attributes may
only be set later during the span lifecycle (and thus not be available during sampling).

## Deprecate using `getClient()` to check if the SDK was initialized

In v8, `getClient()` will stop returning `undefined` if `Sentry.init()` was not called. For cases where this may be used
Expand Down
12 changes: 12 additions & 0 deletions packages/core/src/tracing/hubextensions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,14 @@ The transaction will not be sampled. Please use the ${configInstrumenter} instru
// eslint-disable-next-line deprecation/deprecation
let transaction = new Transaction(transactionContext, this);
transaction = sampleTransaction(transaction, options, {
name: transactionContext.name,
parentSampled: transactionContext.parentSampled,
transactionContext,
attributes: {
// eslint-disable-next-line deprecation/deprecation
...transactionContext.data,
...transactionContext.attributes,
},
...customSamplingContext,
});
if (transaction.isRecording()) {
Expand Down Expand Up @@ -106,8 +112,14 @@ export function startIdleTransaction(
delayAutoFinishUntilSignal,
);
transaction = sampleTransaction(transaction, options, {
name: transactionContext.name,
parentSampled: transactionContext.parentSampled,
transactionContext,
attributes: {
// eslint-disable-next-line deprecation/deprecation
...transactionContext.data,
...transactionContext.attributes,
},
...customSamplingContext,
});
if (transaction.isRecording()) {
Expand Down
30 changes: 30 additions & 0 deletions packages/core/test/lib/tracing/trace.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
addTracingExtensions,
getCurrentScope,
makeMain,
setCurrentClient,
spanToJSON,
withScope,
} from '../../../src';
Expand Down Expand Up @@ -357,6 +358,35 @@ describe('startSpan', () => {
expect(span).toBeDefined();
});
});

it('samples with a tracesSampler', () => {
const tracesSampler = jest.fn(() => {
return true;
});

const options = getDefaultTestClientOptions({ tracesSampler });
client = new TestClient(options);
setCurrentClient(client);

startSpan(
{ name: 'outer', attributes: { test1: 'aa', test2: 'aa' }, data: { test1: 'bb', test3: 'bb' } },
outerSpan => {
expect(outerSpan).toBeDefined();
},
);

expect(tracesSampler).toBeCalledTimes(1);
expect(tracesSampler).toHaveBeenLastCalledWith({
parentSampled: undefined,
name: 'outer',
attributes: {
test1: 'aa',
test2: 'aa',
test3: 'bb',
},
transactionContext: expect.objectContaining({ name: 'outer', parentSampled: undefined }),
});
});
});

describe('startSpanManual', () => {
Expand Down
1 change: 1 addition & 0 deletions packages/opentelemetry-node/src/spanprocessor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ export class SentrySpanProcessor implements OtelSpanProcessor {
const transaction = getCurrentHub().startTransaction({
name: otelSpan.name,
...traceCtx,
attributes: otelSpan.attributes,
instrumenter: 'otel',
startTimestamp: convertOtelTimeToSeconds(otelSpan.startTime),
spanId: otelSpanId,
Expand Down
8 changes: 5 additions & 3 deletions packages/opentelemetry/src/sampler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import type { Attributes, Context, SpanContext } from '@opentelemetry/api';
import { TraceFlags, isSpanContextValid, trace } from '@opentelemetry/api';
import type { Sampler, SamplingResult } from '@opentelemetry/sdk-trace-base';
import { SamplingDecision } from '@opentelemetry/sdk-trace-base';
import { hasTracingEnabled } from '@sentry/core';
import { SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, hasTracingEnabled } from '@sentry/core';
import type { Client, ClientOptions, SamplingContext } from '@sentry/types';
import { isNaN, logger } from '@sentry/utils';

Expand All @@ -27,7 +27,7 @@ export class SentrySampler implements Sampler {
traceId: string,
spanName: string,
_spanKind: unknown,
_attributes: unknown,
spanAttributes: unknown,
_links: unknown,
): SamplingResult {
const options = this._client.getOptions();
Expand All @@ -54,6 +54,8 @@ export class SentrySampler implements Sampler {
}

const sampleRate = getSampleRate(options, {
name: spanName,
attributes: spanAttributes,
transactionContext: {
name: spanName,
parentSampled,
Expand All @@ -62,7 +64,7 @@ export class SentrySampler implements Sampler {
});

const attributes: Attributes = {
[InternalSentrySemanticAttributes.SAMPLE_RATE]: Number(sampleRate),
[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: Number(sampleRate),
};

if (typeof parentSampled === 'boolean') {
Expand Down
6 changes: 3 additions & 3 deletions packages/opentelemetry/src/spanExporter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import type { ExportResult } from '@opentelemetry/core';
import { ExportResultCode } from '@opentelemetry/core';
import type { ReadableSpan, SpanExporter } from '@opentelemetry/sdk-trace-base';
import { SemanticAttributes } from '@opentelemetry/semantic-conventions';
import { flush, getCurrentScope } from '@sentry/core';
import { SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, flush, getCurrentScope } from '@sentry/core';
import type { Scope, Span as SentrySpan, SpanOrigin, TransactionSource } from '@sentry/types';
import { logger } from '@sentry/utils';

Expand Down Expand Up @@ -176,7 +176,7 @@ function createTransactionForOtelSpan(span: ReadableSpan): OpenTelemetryTransact
metadata: {
dynamicSamplingContext,
source,
sampleRate: span.attributes[InternalSentrySemanticAttributes.SAMPLE_RATE] as number | undefined,
sampleRate: span.attributes[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE] as number | undefined,
...metadata,
},
data: removeSentryAttributes(data),
Expand Down Expand Up @@ -267,7 +267,7 @@ function removeSentryAttributes(data: Record<string, unknown>): Record<string, u
delete cleanedData[InternalSentrySemanticAttributes.ORIGIN];
delete cleanedData[InternalSentrySemanticAttributes.OP];
delete cleanedData[InternalSentrySemanticAttributes.SOURCE];
delete cleanedData[InternalSentrySemanticAttributes.SAMPLE_RATE];
delete cleanedData[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE];
/* eslint-enable @typescript-eslint/no-dynamic-delete */

return cleanedData;
Expand Down
31 changes: 24 additions & 7 deletions packages/opentelemetry/test/trace.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { SpanKind } from '@opentelemetry/api';
import { TraceFlags, context, trace } from '@opentelemetry/api';
import type { ReadableSpan } from '@opentelemetry/sdk-trace-base';
import { Span as SpanClass } from '@opentelemetry/sdk-trace-base';
import { SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE } from '@sentry/core';
import type { PropagationContext } from '@sentry/types';

import { getClient } from '../src/custom/hub';
Expand Down Expand Up @@ -206,7 +207,7 @@ describe('trace', () => {
span => {
expect(span).toBeDefined();
expect(getSpanAttributes(span)).toEqual({
[InternalSentrySemanticAttributes.SAMPLE_RATE]: 1,
[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1,
});

expect(getSpanMetadata(span)).toEqual(undefined);
Expand All @@ -227,7 +228,7 @@ describe('trace', () => {
[InternalSentrySemanticAttributes.SOURCE]: 'task',
[InternalSentrySemanticAttributes.ORIGIN]: 'auto.test.origin',
[InternalSentrySemanticAttributes.OP]: 'my-op',
[InternalSentrySemanticAttributes.SAMPLE_RATE]: 1,
[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1,
});

expect(getSpanMetadata(span)).toEqual({ requestPath: 'test-path' });
Expand All @@ -253,7 +254,7 @@ describe('trace', () => {
expect(span).toBeDefined();
expect(getSpanName(span)).toEqual('outer');
expect(getSpanAttributes(span)).toEqual({
[InternalSentrySemanticAttributes.SAMPLE_RATE]: 1,
[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1,
test1: 'test 1',
test2: 2,
});
Expand Down Expand Up @@ -326,7 +327,7 @@ describe('trace', () => {

expect(span).toBeDefined();
expect(getSpanAttributes(span)).toEqual({
[InternalSentrySemanticAttributes.SAMPLE_RATE]: 1,
[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1,
});

expect(getSpanMetadata(span)).toEqual(undefined);
Expand All @@ -341,7 +342,7 @@ describe('trace', () => {

expect(span2).toBeDefined();
expect(getSpanAttributes(span2)).toEqual({
[InternalSentrySemanticAttributes.SAMPLE_RATE]: 1,
[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1,
[InternalSentrySemanticAttributes.SOURCE]: 'task',
[InternalSentrySemanticAttributes.ORIGIN]: 'auto.test.origin',
[InternalSentrySemanticAttributes.OP]: 'my-op',
Expand All @@ -366,7 +367,7 @@ describe('trace', () => {
expect(span).toBeDefined();
expect(getSpanName(span)).toEqual('outer');
expect(getSpanAttributes(span)).toEqual({
[InternalSentrySemanticAttributes.SAMPLE_RATE]: 1,
[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1,
test1: 'test 1',
test2: 2,
});
Expand Down Expand Up @@ -451,7 +452,7 @@ describe('trace', () => {
expect(span).toBeDefined();
expect(getSpanName(span)).toEqual('outer');
expect(getSpanAttributes(span)).toEqual({
[InternalSentrySemanticAttributes.SAMPLE_RATE]: 1,
[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1,
test1: 'test 1',
test2: 2,
});
Expand Down Expand Up @@ -688,6 +689,10 @@ describe('trace (sampling)', () => {
expect(tracesSampler).toBeCalledTimes(1);
expect(tracesSampler).toHaveBeenLastCalledWith({
parentSampled: undefined,
name: 'outer',
attributes: {
[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1,
},
transactionContext: { name: 'outer', parentSampled: undefined },
});

Expand All @@ -705,6 +710,8 @@ describe('trace (sampling)', () => {
expect(tracesSampler).toHaveBeenCalledTimes(3);
expect(tracesSampler).toHaveBeenLastCalledWith({
parentSampled: false,
name: 'inner2',
attributes: {},
transactionContext: { name: 'inner2', parentSampled: false },
});
});
Expand All @@ -727,6 +734,10 @@ describe('trace (sampling)', () => {
expect(tracesSampler).toBeCalledTimes(1);
expect(tracesSampler).toHaveBeenLastCalledWith({
parentSampled: undefined,
name: 'outer',
attributes: {
[SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: 1,
},
transactionContext: { name: 'outer', parentSampled: undefined },
});

Expand All @@ -744,6 +755,8 @@ describe('trace (sampling)', () => {
expect(tracesSampler).toHaveBeenCalledTimes(3);
expect(tracesSampler).toHaveBeenLastCalledWith({
parentSampled: false,
name: 'inner2',
attributes: {},
transactionContext: { name: 'inner2', parentSampled: false },
});

Expand All @@ -757,6 +770,8 @@ describe('trace (sampling)', () => {
expect(tracesSampler).toHaveBeenCalledTimes(4);
expect(tracesSampler).toHaveBeenLastCalledWith({
parentSampled: undefined,
name: 'outer3',
attributes: {},
transactionContext: { name: 'outer3', parentSampled: undefined },
});
});
Expand Down Expand Up @@ -799,6 +814,8 @@ describe('trace (sampling)', () => {
expect(tracesSampler).toBeCalledTimes(1);
expect(tracesSampler).toHaveBeenLastCalledWith({
parentSampled: true,
name: 'outer',
attributes: {},
transactionContext: {
name: 'outer',
parentSampled: true,
Expand Down

0 comments on commit 369faf4

Please sign in to comment.