diff --git a/packages/core/src/request.ts b/packages/core/src/request.ts index 15d9acecf2ed..14084d942c33 100644 --- a/packages/core/src/request.ts +++ b/packages/core/src/request.ts @@ -20,6 +20,10 @@ export function sessionToSentryRequest(session: Session, api: API): SentryReques /** Creates a SentryRequest from an event. */ export function eventToSentryRequest(event: Event, api: API): SentryRequest { + // since JS has no Object.prototype.pop() + const { __sentry_samplingMethod: samplingMethod, __sentry_sampleRate: sampleRate, ...otherTags } = event.tags || {}; + event.tags = otherTags; + const useEnvelope = event.type === 'transaction'; const req: SentryRequest = { @@ -41,6 +45,11 @@ export function eventToSentryRequest(event: Event, api: API): SentryRequest { }); const itemHeaders = JSON.stringify({ type: event.type, + + // TODO: Right now, sampleRate may or may not be defined (it won't be in the cases of inheritance and + // explicitly-set sampling decisions). Are we good with that? + sample_rates: [{ id: samplingMethod, rate: sampleRate }], + // The content-type is assumed to be 'application/json' and not part of // the current spec for transaction items, so we don't bloat the request // body with it. diff --git a/packages/core/test/lib/request.test.ts b/packages/core/test/lib/request.test.ts new file mode 100644 index 000000000000..be71d72cc8b8 --- /dev/null +++ b/packages/core/test/lib/request.test.ts @@ -0,0 +1,56 @@ +import { Event, TransactionSamplingMethod } from '@sentry/types'; + +import { API } from '../../src/api'; +import { eventToSentryRequest } from '../../src/request'; + +describe('eventToSentryRequest', () => { + const api = new API('https://dogsarebadatkeepingsecrets@squirrelchasers.ingest.sentry.io/12312012'); + const event: Event = { + contexts: { trace: { trace_id: '1231201211212012', span_id: '12261980', op: 'pageload' } }, + environment: 'dogpark', + event_id: '0908201304152013', + release: 'off.leash.park', + spans: [], + transaction: '/dogs/are/great/', + type: 'transaction', + user: { id: '1121', username: 'CharlieDog', ip_address: '11.21.20.12' }, + }; + + [ + { method: TransactionSamplingMethod.Rate, rate: '0.1121', dog: 'Charlie' }, + { method: TransactionSamplingMethod.Sampler, rate: '0.1231', dog: 'Maisey' }, + { method: TransactionSamplingMethod.Inheritance, dog: 'Cory' }, + { method: TransactionSamplingMethod.Explicit, dog: 'Bodhi' }, + + // this shouldn't ever happen (at least the method should always be included in tags), but good to know that things + // won't blow up if it does + { dog: 'Lucy' }, + ].forEach(({ method, rate, dog }) => { + it(`adds transaction sampling information to item header - ${method}, ${rate}, ${dog}`, () => { + // TODO kmclb - once tag types are loosened, don't need to cast to string here + event.tags = { __sentry_samplingMethod: String(method), __sentry_sampleRate: String(rate), dog }; + + const result = eventToSentryRequest(event as Event, api); + + const [envelopeHeaderString, itemHeaderString, eventString] = result.body.split('\n'); + + const envelope = { + envelopeHeader: JSON.parse(envelopeHeaderString), + itemHeader: JSON.parse(itemHeaderString), + event: JSON.parse(eventString), + }; + + // the right stuff is added to the item header + expect(envelope.itemHeader).toEqual({ + type: 'transaction', + // TODO kmclb - once tag types are loosened, don't need to cast to string here + sample_rates: [{ id: String(method), rate: String(rate) }], + }); + + // show that it pops the right tags and leaves the rest alone + expect('__sentry_samplingMethod' in envelope.event.tags).toBe(false); + expect('__sentry_sampleRate' in envelope.event.tags).toBe(false); + expect('dog' in envelope.event.tags).toBe(true); + }); + }); +}); diff --git a/packages/tracing/src/hubextensions.ts b/packages/tracing/src/hubextensions.ts index b083258a4bdc..7b2f2aec308d 100644 --- a/packages/tracing/src/hubextensions.ts +++ b/packages/tracing/src/hubextensions.ts @@ -1,5 +1,5 @@ import { getActiveDomain, getMainCarrier, Hub } from '@sentry/hub'; -import { CustomSamplingContext, SamplingContext, TransactionContext } from '@sentry/types'; +import { CustomSamplingContext, SamplingContext, TransactionContext, TransactionSamplingMethod } from '@sentry/types'; import { dynamicRequire, extractNodeRequestData, @@ -28,18 +28,6 @@ function traceHeaders(this: Hub): { [key: string]: string } { return {}; } -/** - * Implements sampling inheritance and falls back to user-provided static rate if no parent decision is available. - * - * @param parentSampled: The parent transaction's sampling decision, if any. - * @param givenRate: The rate to use if no parental decision is available. - * - * @returns The parent's sampling decision (if one exists), or the provided static rate - */ -function _inheritOrUseGivenRate(parentSampled: boolean | undefined, givenRate: unknown): boolean | unknown { - return parentSampled !== undefined ? parentSampled : givenRate; -} - /** * Makes a sampling decision for the given transaction and stores it on the transaction. * @@ -64,15 +52,35 @@ function sample(hub: Hub, transaction: T, samplingContext // if the user has forced a sampling decision by passing a `sampled` value in their transaction context, go with that if (transaction.sampled !== undefined) { + transaction.tags = { ...transaction.tags, __sentry_samplingMethod: TransactionSamplingMethod.Explicit }; return transaction; } // we would have bailed already if neither `tracesSampler` nor `tracesSampleRate` were defined, so one of these should // work; prefer the hook if so - const sampleRate = - typeof options.tracesSampler === 'function' - ? options.tracesSampler(samplingContext) - : _inheritOrUseGivenRate(samplingContext.parentSampled, options.tracesSampleRate); + let sampleRate; + if (typeof options.tracesSampler === 'function') { + sampleRate = options.tracesSampler(samplingContext); + // cast the rate to a number first in case it's a boolean + transaction.tags = { + ...transaction.tags, + __sentry_samplingMethod: TransactionSamplingMethod.Sampler, + // TODO kmclb - once tag types are loosened, don't need to cast to string here + __sentry_sampleRate: String(Number(sampleRate)), + }; + } else if (samplingContext.parentSampled !== undefined) { + sampleRate = samplingContext.parentSampled; + transaction.tags = { ...transaction.tags, __sentry_samplingMethod: TransactionSamplingMethod.Inheritance }; + } else { + sampleRate = options.tracesSampleRate; + // cast the rate to a number first in case it's a boolean + transaction.tags = { + ...transaction.tags, + __sentry_samplingMethod: TransactionSamplingMethod.Rate, + // TODO kmclb - once tag types are loosened, don't need to cast to string here + __sentry_sampleRate: String(Number(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.) @@ -88,7 +96,7 @@ function sample(hub: Hub, transaction: T, samplingContext `[Tracing] Discarding transaction because ${ typeof options.tracesSampler === 'function' ? 'tracesSampler returned 0 or false' - : 'tracesSampleRate is set to 0' + : 'a negative sampling decision was inherited or tracesSampleRate is set to 0' }`, ); transaction.sampled = false; diff --git a/packages/tracing/test/hub.test.ts b/packages/tracing/test/hub.test.ts index f9c8f9e2fba4..4d8733422bb4 100644 --- a/packages/tracing/test/hub.test.ts +++ b/packages/tracing/test/hub.test.ts @@ -171,7 +171,7 @@ describe('Hub', () => { }); describe('sample()', () => { - it('should not sample transactions when tracing is disabled', () => { + it('should set sampled = false when tracing is disabled', () => { // neither tracesSampleRate nor tracesSampler is defined -> tracing disabled const hub = new Hub(new BrowserClient({})); const transaction = hub.startTransaction({ name: 'dogpark' }); @@ -219,7 +219,7 @@ describe('Hub', () => { expect(transaction.sampled).toBe(true); }); - it('should not try to override positive sampling decision provided in transaction context', () => { + it('should not try to override explicitly set positive sampling decision', () => { // so that the decision otherwise would be false const tracesSampler = jest.fn().mockReturnValue(0); const hub = new Hub(new BrowserClient({ tracesSampler })); @@ -228,7 +228,7 @@ describe('Hub', () => { expect(transaction.sampled).toBe(true); }); - it('should not try to override negative sampling decision provided in transaction context', () => { + it('should not try to override explicitly set negative sampling decision', () => { // so that the decision otherwise would be true const tracesSampler = jest.fn().mockReturnValue(1); const hub = new Hub(new BrowserClient({ tracesSampler })); @@ -255,6 +255,40 @@ describe('Hub', () => { expect(tracesSampler).toHaveBeenCalled(); expect(transaction.sampled).toBe(true); }); + + it('should record sampling method when sampling decision is explicitly set', () => { + const tracesSampler = jest.fn().mockReturnValue(0.1121); + const hub = new Hub(new BrowserClient({ tracesSampler })); + const transaction = hub.startTransaction({ name: 'dogpark', sampled: true }); + + expect(transaction.tags).toEqual(expect.objectContaining({ __sentry_samplingMethod: 'explicitly_set' })); + }); + + it('should record sampling method and rate when sampling decision comes from tracesSampler', () => { + const tracesSampler = jest.fn().mockReturnValue(0.1121); + const hub = new Hub(new BrowserClient({ tracesSampler })); + const transaction = hub.startTransaction({ name: 'dogpark' }); + + expect(transaction.tags).toEqual( + expect.objectContaining({ __sentry_samplingMethod: 'client_sampler', __sentry_sampleRate: '0.1121' }), + ); + }); + + it('should record sampling method when sampling decision is inherited', () => { + const hub = new Hub(new BrowserClient({ tracesSampleRate: 0.1121 })); + const transaction = hub.startTransaction({ name: 'dogpark', parentSampled: true }); + + expect(transaction.tags).toEqual(expect.objectContaining({ __sentry_samplingMethod: 'inheritance' })); + }); + + it('should record sampling method and rate when sampling decision comes from traceSampleRate', () => { + const hub = new Hub(new BrowserClient({ tracesSampleRate: 0.1121 })); + const transaction = hub.startTransaction({ name: 'dogpark' }); + + expect(transaction.tags).toEqual( + expect.objectContaining({ __sentry_samplingMethod: 'client_rate', __sentry_sampleRate: '0.1121' }), + ); + }); }); describe('isValidSampleRate()', () => { diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 9268df55f8d9..7f1733dddb28 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -32,6 +32,7 @@ export { TraceparentData, Transaction, TransactionContext, + TransactionSamplingMethod, } from './transaction'; export { Thread } from './thread'; export { Transport, TransportOptions, TransportClass } from './transport'; diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts index 2a6bde18e5a4..f212fad6701e 100644 --- a/packages/types/src/transaction.ts +++ b/packages/types/src/transaction.ts @@ -100,3 +100,10 @@ export interface SamplingContext extends CustomSamplingContext { } export type Measurements = Record; + +export enum TransactionSamplingMethod { + Explicit = 'explicitly_set', + Sampler = 'client_sampler', + Rate = 'client_rate', + Inheritance = 'inheritance', +}