From cfb13bc57f24f77643b71c826ecbd134bab5c652 Mon Sep 17 00:00:00 2001 From: Rodolfo Carvalho Date: Fri, 12 Feb 2021 12:23:21 +0100 Subject: [PATCH 1/3] fix: Create spanRecorder whenever transactions are sampled There were cases in which the sample function would return early without correctly setting up a spanRecorder, causing child spans to be lost. --- packages/tracing/src/hubextensions.ts | 43 +++++++++++++++++---------- packages/tracing/test/span.test.ts | 12 ++++++++ 2 files changed, 40 insertions(+), 15 deletions(-) diff --git a/packages/tracing/src/hubextensions.ts b/packages/tracing/src/hubextensions.ts index 5baf4aae9e04..fbd2ebec17f3 100644 --- a/packages/tracing/src/hubextensions.ts +++ b/packages/tracing/src/hubextensions.ts @@ -1,5 +1,11 @@ import { getMainCarrier, Hub } from '@sentry/hub'; -import { CustomSamplingContext, SamplingContext, TransactionContext, TransactionSamplingMethod } from '@sentry/types'; +import { + CustomSamplingContext, + Options, + SamplingContext, + TransactionContext, + TransactionSamplingMethod, +} from '@sentry/types'; import { logger } from '@sentry/utils'; import { registerErrorInstrumentation } from './errors'; @@ -33,12 +39,9 @@ function traceHeaders(this: Hub): { [key: string]: string } { * * @returns The given transaction with its `sampled` value set */ -function sample(hub: Hub, transaction: T, samplingContext: SamplingContext): T { - const client = hub.getClient(); - const options = (client && client.getOptions()) || {}; - - // nothing to do if there's no client or if tracing is disabled - if (!client || !hasTracingEnabled(options)) { +function sample(transaction: T, options: Options, samplingContext: SamplingContext): T { + // nothing to do if tracing is not enabled + if (!hasTracingEnabled(options)) { transaction.sampled = false; return transaction; } @@ -114,10 +117,6 @@ function sample(hub: Hub, transaction: T, samplingContext return transaction; } - // at this point we know we're keeping the transaction, whether because of an inherited decision or because it got - // lucky with the dice roll - transaction.initSpanRecorder(options._experiments?.maxSpans as number); - logger.log(`[Tracing] starting ${transaction.op} transaction - ${transaction.name}`); return transaction; } @@ -165,12 +164,19 @@ function _startTransaction( transactionContext: TransactionContext, customSamplingContext?: CustomSamplingContext, ): Transaction { - const transaction = new Transaction(transactionContext, this); - return sample(this, transaction, { + const client = this.getClient(); + const options = (client && client.getOptions()) || {}; + + let transaction = new Transaction(transactionContext, this); + transaction = sample(transaction, options, { parentSampled: transactionContext.parentSampled, transactionContext, ...customSamplingContext, }); + if (transaction.sampled) { + transaction.initSpanRecorder(options._experiments?.maxSpans as number); + } + return transaction; } /** @@ -183,12 +189,19 @@ export function startIdleTransaction( onScope?: boolean, customSamplingContext?: CustomSamplingContext, ): IdleTransaction { - const transaction = new IdleTransaction(transactionContext, hub, idleTimeout, onScope); - return sample(hub, transaction, { + const client = hub.getClient(); + const options = (client && client.getOptions()) || {}; + + let transaction = new IdleTransaction(transactionContext, hub, idleTimeout, onScope); + transaction = sample(transaction, options, { parentSampled: transactionContext.parentSampled, transactionContext, ...customSamplingContext, }); + if (transaction.sampled) { + transaction.initSpanRecorder(options._experiments?.maxSpans as number); + } + return transaction; } /** diff --git a/packages/tracing/test/span.test.ts b/packages/tracing/test/span.test.ts index caf16dd26c29..ec8cea1cd299 100644 --- a/packages/tracing/test/span.test.ts +++ b/packages/tracing/test/span.test.ts @@ -167,6 +167,18 @@ describe('Span', () => { expect(spy.mock.calls[0][0].contexts.trace).toEqual(transaction.getTraceContext()); }); + // See https://github.com/getsentry/sentry-javascript/issues/3254 + test('finish a transaction + child span + sampled:true', () => { + const spy = jest.spyOn(hub as any, 'captureEvent') as any; + const transaction = hub.startTransaction({ name: 'test', op: 'parent', sampled: true }); + const childSpan = transaction.startChild({ op: 'child' }); + childSpan.finish(); + transaction.finish(); + expect(spy).toHaveBeenCalled(); + expect(spy.mock.calls[0][0].spans).toHaveLength(1); + expect(spy.mock.calls[0][0].contexts.trace).toEqual(transaction.getTraceContext()); + }); + test("finish a child span shouldn't trigger captureEvent", () => { const spy = jest.spyOn(hub as any, 'captureEvent') as any; const transaction = hub.startTransaction({ name: 'test' }); From d4a4d82a10202249212e7708ed252121250c8034 Mon Sep 17 00:00:00 2001 From: Rodolfo Carvalho Date: Fri, 12 Feb 2021 13:51:23 +0100 Subject: [PATCH 2/3] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Kamil Ogórek --- packages/tracing/src/hubextensions.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/tracing/src/hubextensions.ts b/packages/tracing/src/hubextensions.ts index fbd2ebec17f3..0109f301bfa4 100644 --- a/packages/tracing/src/hubextensions.ts +++ b/packages/tracing/src/hubextensions.ts @@ -164,8 +164,7 @@ function _startTransaction( transactionContext: TransactionContext, customSamplingContext?: CustomSamplingContext, ): Transaction { - const client = this.getClient(); - const options = (client && client.getOptions()) || {}; + const options = hub.getClient()?.getOptions() || {}; let transaction = new Transaction(transactionContext, this); transaction = sample(transaction, options, { @@ -189,8 +188,7 @@ export function startIdleTransaction( onScope?: boolean, customSamplingContext?: CustomSamplingContext, ): IdleTransaction { - const client = hub.getClient(); - const options = (client && client.getOptions()) || {}; + const options = hub.getClient()?.getOptions() || {}; let transaction = new IdleTransaction(transactionContext, hub, idleTimeout, onScope); transaction = sample(transaction, options, { From 19e03fa9defba7dbc24b51b409493392385011a2 Mon Sep 17 00:00:00 2001 From: Rodolfo Carvalho Date: Fri, 12 Feb 2021 14:00:08 +0100 Subject: [PATCH 3/3] Fix arg name --- packages/tracing/src/hubextensions.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/tracing/src/hubextensions.ts b/packages/tracing/src/hubextensions.ts index 0109f301bfa4..e041a9e33b3b 100644 --- a/packages/tracing/src/hubextensions.ts +++ b/packages/tracing/src/hubextensions.ts @@ -164,7 +164,7 @@ function _startTransaction( transactionContext: TransactionContext, customSamplingContext?: CustomSamplingContext, ): Transaction { - const options = hub.getClient()?.getOptions() || {}; + const options = this.getClient()?.getOptions() || {}; let transaction = new Transaction(transactionContext, this); transaction = sample(transaction, options, {