From 507596e8aba26ffce28a2ab7d138e93cf1df9e74 Mon Sep 17 00:00:00 2001 From: JonasBa Date: Thu, 9 Mar 2023 10:10:32 -0500 Subject: [PATCH 1/4] feat(profiling): add profilesSampler --- package.json | 2 +- src/hubextensions.test.ts | 121 +++++++++++++++++++++++++++++++++++++- src/hubextensions.ts | 63 +++++++++++++++++--- src/utils.test.ts | 25 ++++++++ src/utils.ts | 30 ++++++++++ 5 files changed, 231 insertions(+), 10 deletions(-) diff --git a/package.json b/package.json index e9730829..01109b0b 100644 --- a/package.json +++ b/package.json @@ -44,7 +44,7 @@ "benchmark:server": "node benchmarks/cpu/benchmark.server.js", "benchmark:format": "node benchmarks/format/benchmark.format.js", "benchmark:integration": "node benchmarks/cpu/benchmark.integration.base.js && node benchmarks/cpu/benchmark.integration.disabled.js && node benchmarks/cpu/benchmark.integration.js", - "test:watch": "jest --watch --silent", + "test:watch": "jest --watch", "test": "jest --config jest.config.ts", "prettier": "prettier --config ./.prettierrc --write" }, diff --git a/src/hubextensions.test.ts b/src/hubextensions.test.ts index 9d56fc2b..48a6b463 100644 --- a/src/hubextensions.test.ts +++ b/src/hubextensions.test.ts @@ -3,6 +3,7 @@ import type { ClientOptions, Hub, Context, + Client, Transaction, TransactionMetadata } from '@sentry/types'; @@ -36,7 +37,13 @@ function makeTransactionMock(options = {}): Transaction { } as unknown as Transaction; } -function makeHubMock({ profilesSampleRate }: { profilesSampleRate: number | undefined }): Hub { +function makeHubMock({ + profilesSampleRate, + client +}: { + profilesSampleRate: number | undefined; + client?: Partial>>; +}): Hub { return { getClient: jest.fn().mockImplementation(() => { return { @@ -44,7 +51,8 @@ function makeHubMock({ profilesSampleRate }: { profilesSampleRate: number | unde return { profilesSampleRate } as unknown as ClientOptions; - }) + }), + ...(client ?? {}) }; }) } as unknown as Hub; @@ -131,4 +139,113 @@ describe('hubextensions', () => { expect(startProfilingSpy).not.toHaveBeenCalledTimes(1); expect(stopProfilingSpy).not.toHaveBeenCalledTimes(1); }); + + it('disabled if neither profilesSampler and profilesSampleRate are not set', () => { + const hub = makeHubMock({ profilesSampleRate: undefined }); + const startTransaction = jest.fn().mockImplementation(() => makeTransactionMock()); + + const maybeStartTransactionWithProfiling = __PRIVATE__wrapStartTransactionWithProfiling(startTransaction); + const samplingContext = { beep: 'boop' }; + const transaction = maybeStartTransactionWithProfiling.call(hub, { name: '' }, samplingContext); + transaction.finish(); + + const startProfilingSpy = jest.spyOn(profiler, 'startProfiling'); + expect(startProfilingSpy).not.toHaveBeenCalled(); + }); + + it('does not call startProfiling if profilesSampler returns invalid rate', () => { + const startProfilingSpy = jest.spyOn(profiler, 'startProfiling'); + const options = { profilesSampler: jest.fn().mockReturnValue(NaN) }; + const hub = makeHubMock({ + profilesSampleRate: undefined, + client: { + // @ts-expect-error mock this + getOptions: () => options + } + }); + const startTransaction = jest.fn().mockImplementation(() => makeTransactionMock()); + + const maybeStartTransactionWithProfiling = __PRIVATE__wrapStartTransactionWithProfiling(startTransaction); + const samplingContext = { beep: 'boop' }; + const transaction = maybeStartTransactionWithProfiling.call(hub, { name: '' }, samplingContext); + transaction.finish(); + + expect(options.profilesSampler).toHaveBeenCalledWith(samplingContext); + expect(startProfilingSpy).not.toHaveBeenCalled(); + }); + + it('does not call startProfiling if profilesSampleRate is invalid', () => { + const startProfilingSpy = jest.spyOn(profiler, 'startProfiling'); + const options = { profilesSampler: jest.fn().mockReturnValue(NaN) }; + const hub = makeHubMock({ + profilesSampleRate: NaN, + client: { + // @ts-expect-error mock this + getOptions: () => options + } + }); + const startTransaction = jest.fn().mockImplementation(() => makeTransactionMock()); + + const maybeStartTransactionWithProfiling = __PRIVATE__wrapStartTransactionWithProfiling(startTransaction); + const samplingContext = { beep: 'boop' }; + const transaction = maybeStartTransactionWithProfiling.call(hub, { name: '' }, samplingContext); + transaction.finish(); + + expect(options.profilesSampler).toHaveBeenCalledWith(samplingContext); + expect(startProfilingSpy).not.toHaveBeenCalled(); + }); + + it('calls profilesSampler with sampling context', () => { + const options = { profilesSampler: jest.fn() }; + const hub = makeHubMock({ + profilesSampleRate: undefined, + client: { + // @ts-expect-error mock this + getOptions: () => options + } + }); + const startTransaction = jest.fn().mockImplementation(() => makeTransactionMock()); + + const maybeStartTransactionWithProfiling = __PRIVATE__wrapStartTransactionWithProfiling(startTransaction); + const samplingContext = { beep: 'boop' }; + const transaction = maybeStartTransactionWithProfiling.call(hub, { name: '' }, samplingContext); + transaction.finish(); + + expect(options.profilesSampler).toHaveBeenCalledWith(samplingContext); + }); + + it('prioritizes profilesSampler outcome over profilesSampleRate', () => { + const startProfilingSpy = jest.spyOn(profiler, 'startProfiling'); + const options = { profilesSampler: jest.fn().mockReturnValue(1) }; + const hub = makeHubMock({ + profilesSampleRate: 0, + client: { + // @ts-expect-error mock this + getOptions: () => options + } + }); + const startTransaction = jest.fn().mockImplementation(() => makeTransactionMock()); + + const maybeStartTransactionWithProfiling = __PRIVATE__wrapStartTransactionWithProfiling(startTransaction); + const samplingContext = { beep: 'boop' }; + const transaction = maybeStartTransactionWithProfiling.call(hub, { name: '' }, samplingContext); + transaction.finish(); + + expect(startProfilingSpy).toHaveBeenCalled(); + }); + + it('infers sampling based off customSamplingContext.parentSampled', () => { + const startProfilingSpy = jest.spyOn(profiler, 'startProfiling'); + const hub = makeHubMock({ + profilesSampleRate: 1 + }); + const startTransaction = jest.fn().mockImplementation(() => makeTransactionMock()); + + const maybeStartTransactionWithProfiling = __PRIVATE__wrapStartTransactionWithProfiling(startTransaction); + const samplingContext = { parentSampled: 0 }; + const transaction = maybeStartTransactionWithProfiling.call(hub, { name: '' }, samplingContext); + transaction.finish(); + + expect(startProfilingSpy).not.toHaveBeenCalled(); + }); }); diff --git a/src/hubextensions.ts b/src/hubextensions.ts index 53acab53..18187dd4 100644 --- a/src/hubextensions.ts +++ b/src/hubextensions.ts @@ -4,6 +4,7 @@ import { logger, uuid4 } from '@sentry/utils'; import { isDebugBuild } from './env'; import { CpuProfilerBindings } from './cpu_profiler'; +import { isValidSampleRate } from './utils'; const MAX_PROFILE_DURATION_MS = 30 * 1000; @@ -30,26 +31,74 @@ export function __PRIVATE__wrapStartTransactionWithProfiling(startTransaction: S const profile_id = uuid4(); const uniqueTransactionName = `${transactionContext.name} ${profile_id}`; - // profilesSampleRate is multiplied with tracesSampleRate to get the final sampling rate. + // profilesSampleRate is multiplied with tracesSampleRate to get the final sampling rate. We dont perform + // the actual multiplication to get the final rate, but we discard the profile if the transaction was sampled, + // so anything after this block from here is based on the transaction sampling. if (!transaction.sampled) { return transaction; } - // @ts-expect-error profilesSampleRate is not part of the options type yet - const profilesSampleRate = this.getClient()?.getOptions?.()?.profilesSampleRate; - if (profilesSampleRate === undefined) { + const client = this.getClient(); + if (!client) { + if (isDebugBuild()) { + logger.log('[Profiling] Profiling disabled, no client found.'); + } + return transaction; + } + const options = client.getOptions(); + if (!options) { + if (isDebugBuild()) { + logger.log('[Profiling] Profiling disabled, no options found.'); + } + return transaction; + } + + // @ts-expect-error sampler is not yer part of the sdk options + const profilesSampler = options.profilesSampler; + // @ts-expect-error sampler is not yer part of the sdk options + let profilesSampleRate: number | undefined = options.profilesSampleRate; + + // Prefer sampler to sample rate if both are provided. + if (typeof profilesSampler === 'function') { + profilesSampleRate = profilesSampler(customSamplingContext); + } else if (customSamplingContext && customSamplingContext['parentSampled'] !== undefined) { + profilesSampleRate = customSamplingContext['parentSampled']; + } + + // 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(profilesSampleRate)) { + if (isDebugBuild()) { + logger.warn('[Profiling] Discarding profile because of invalid sample rate.'); + } + return transaction; + } + + // if the function returned 0 (or false), or if `profileSampleRate` is 0, it's a sign the profile should be dropped + if (!profilesSampleRate) { if (isDebugBuild()) { logger.log( - '[Profiling] Profiling disabled, enable it by setting `profilesSampleRate` option to SDK init call.' + `[Profiling] Discarding profile because ${ + typeof profilesSampler === 'function' + ? 'profileSampler returned 0 or false' + : 'a negative sampling decision was inherited or profileSampleRate is set to 0' + }` ); } 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. + const sampled = Math.random() < (profilesSampleRate as number | boolean); // Check if we should sample this profile - if (Math.random() > profilesSampleRate) { + if (!sampled) { if (isDebugBuild()) { - logger.log('[Profiling] Skip profiling transaction due to sampling.'); + logger.log( + `[Profiling] Discarding profile because it's not included in the random sample (sampling rate = ${Number( + profilesSampleRate + )})` + ); } return transaction; } diff --git a/src/utils.test.ts b/src/utils.test.ts index 89149dab..90382344 100644 --- a/src/utils.test.ts +++ b/src/utils.test.ts @@ -1,5 +1,6 @@ import type { SdkMetadata, DsnComponents } from '@sentry/types'; import type { ProfiledEvent } from './utils'; +import { isValidSampleRate } from './utils'; import { maybeRemoveProfileFromSdkMetadata, @@ -255,3 +256,27 @@ describe('createProfilingEventEnvelope', () => { expect(profile.transaction.trace_id).toBe('trace_id'); }); }); + +describe('isValidSampleRate', () => { + it.each([ + [0, true], + [0.1, true], + [1, true], + [true, true], + [false, true], + // invalid values + [1.1, false], + [-0.1, false], + [NaN, false], + [Infinity, false], + [null, false], + [undefined, false], + ['', false], + [' ', false], + [{}, false], + [[], false], + [() => null, false] + ])('value %s is %s', (input, expected) => { + expect(isValidSampleRate(input)).toBe(expected); + }); +}); diff --git a/src/utils.ts b/src/utils.ts index fa59bf68..3e071fb4 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -274,3 +274,33 @@ export function getProjectRootDirectory(): string | null { const components = path.resolve(root_directory).split('/node_modules'); return components?.[0] ?? null; } + +/** + * Checks the given sample rate to make sure it is valid type and value (a boolean, or a number between 0 and 1). + */ +export function isValidSampleRate(rate: unknown): boolean { + // we need to check NaN explicitly because it's of type 'number' and therefore wouldn't get caught by this typecheck + if ((typeof rate !== 'number' && typeof rate !== 'boolean') || (typeof rate === 'number' && isNaN(rate))) { + if (isDebugBuild()) { + logger.warn( + `[Profiling] Invalid sample rate. Sample rate must be a boolean or a number between 0 and 1. Got ${JSON.stringify( + rate + )} of type ${JSON.stringify(typeof rate)}.` + ); + } + return false; + } + + if (typeof rate === 'boolean') { + return true; + } + + // in case sampleRate is a boolean, it will get automatically cast to 1 if it's true and 0 if it's false + if (rate < 0 || rate > 1) { + if (isDebugBuild()) { + logger.warn(`[Profiling] Invalid sample rate. Sample rate must be between 0 and 1. Got ${rate}.`); + } + return false; + } + return true; +} From c95f6fcc3f54e8605c4af50f009fdddd6c4be45e Mon Sep 17 00:00:00 2001 From: JonasBa Date: Fri, 10 Mar 2023 08:36:39 -0500 Subject: [PATCH 2/4] ref(utils): remove unnecessar bool check --- src/utils.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/utils.ts b/src/utils.ts index 3e071fb4..c85d55a0 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -291,10 +291,6 @@ export function isValidSampleRate(rate: unknown): boolean { return false; } - if (typeof rate === 'boolean') { - return true; - } - // in case sampleRate is a boolean, it will get automatically cast to 1 if it's true and 0 if it's false if (rate < 0 || rate > 1) { if (isDebugBuild()) { From d0f46091b959df20c14939c3f7821ae7d33728aa Mon Sep 17 00:00:00 2001 From: JonasBa Date: Fri, 10 Mar 2023 09:46:47 -0500 Subject: [PATCH 3/4] ref(profiling): disable support for distributed tracing --- src/hubextensions.test.ts | 4 +++- src/hubextensions.ts | 7 +++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/hubextensions.test.ts b/src/hubextensions.test.ts index 48a6b463..f67e8c66 100644 --- a/src/hubextensions.test.ts +++ b/src/hubextensions.test.ts @@ -234,7 +234,9 @@ describe('hubextensions', () => { expect(startProfilingSpy).toHaveBeenCalled(); }); - it('infers sampling based off customSamplingContext.parentSampled', () => { + // This is for distributed tracing see + // https://github.com/getsentry/profiling-node/pull/109#discussion_r1132434757 + it.skip('infers sampling based off customSamplingContext.parentSampled', () => { const startProfilingSpy = jest.spyOn(profiler, 'startProfiling'); const hub = makeHubMock({ profilesSampleRate: 1 diff --git a/src/hubextensions.ts b/src/hubextensions.ts index 18187dd4..158021a5 100644 --- a/src/hubextensions.ts +++ b/src/hubextensions.ts @@ -61,9 +61,12 @@ export function __PRIVATE__wrapStartTransactionWithProfiling(startTransaction: S // Prefer sampler to sample rate if both are provided. if (typeof profilesSampler === 'function') { profilesSampleRate = profilesSampler(customSamplingContext); - } else if (customSamplingContext && customSamplingContext['parentSampled'] !== undefined) { - profilesSampleRate = customSamplingContext['parentSampled']; } + // @TODO: enable this block if we want distributed tracing support + // see https://github.com/getsentry/profiling-node/pull/109#discussion_r1132434757 + // else if (customSamplingContext && customSamplingContext['parentSampled'] !== undefined) { + // profilesSampleRate = customSamplingContext['parentSampled']; + // } // 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.) From d6d755e66d20541e23cb13ca3a9c51259fc10849 Mon Sep 17 00:00:00 2001 From: JonasBa Date: Fri, 10 Mar 2023 10:07:30 -0500 Subject: [PATCH 4/4] ref(profiling): remove parentSampled --- src/hubextensions.test.ts | 17 ----------------- src/hubextensions.ts | 5 ----- 2 files changed, 22 deletions(-) diff --git a/src/hubextensions.test.ts b/src/hubextensions.test.ts index f67e8c66..beece357 100644 --- a/src/hubextensions.test.ts +++ b/src/hubextensions.test.ts @@ -233,21 +233,4 @@ describe('hubextensions', () => { expect(startProfilingSpy).toHaveBeenCalled(); }); - - // This is for distributed tracing see - // https://github.com/getsentry/profiling-node/pull/109#discussion_r1132434757 - it.skip('infers sampling based off customSamplingContext.parentSampled', () => { - const startProfilingSpy = jest.spyOn(profiler, 'startProfiling'); - const hub = makeHubMock({ - profilesSampleRate: 1 - }); - const startTransaction = jest.fn().mockImplementation(() => makeTransactionMock()); - - const maybeStartTransactionWithProfiling = __PRIVATE__wrapStartTransactionWithProfiling(startTransaction); - const samplingContext = { parentSampled: 0 }; - const transaction = maybeStartTransactionWithProfiling.call(hub, { name: '' }, samplingContext); - transaction.finish(); - - expect(startProfilingSpy).not.toHaveBeenCalled(); - }); }); diff --git a/src/hubextensions.ts b/src/hubextensions.ts index 158021a5..6a64d0d0 100644 --- a/src/hubextensions.ts +++ b/src/hubextensions.ts @@ -62,11 +62,6 @@ export function __PRIVATE__wrapStartTransactionWithProfiling(startTransaction: S if (typeof profilesSampler === 'function') { profilesSampleRate = profilesSampler(customSamplingContext); } - // @TODO: enable this block if we want distributed tracing support - // see https://github.com/getsentry/profiling-node/pull/109#discussion_r1132434757 - // else if (customSamplingContext && customSamplingContext['parentSampled'] !== undefined) { - // profilesSampleRate = customSamplingContext['parentSampled']; - // } // 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.)