fix: Create spanRecorder whenever transactions are sampled #3255
Conversation
There were cases in which the sample function would return early without correctly setting up a spanRecorder, causing child spans to be lost.
|
|
||
| // nothing to do if there's no client or if tracing is disabled | ||
| if (!client || !hasTracingEnabled(options)) { | ||
| function sample<T extends Transaction>(transaction: T, options: Options, samplingContext: SamplingContext): T { |
There was a problem hiding this comment.
This should probably be a method on Transaction, but didn't want to make that change now to keep this patch small.
I did however change the argument order to have transaction as the first argument, since this function operates (mutates!) the transaction, and removed the dependency on the hub as we only need to know about the client options.
There was a problem hiding this comment.
I think that the options should go last though, but it's just a nitpick :P
There was a problem hiding this comment.
yeah agree, I'll leave that for later so we don't have to go change more lines on the call sites.
| @@ -114,10 +117,6 @@ function sample<T extends Transaction>(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); | |||
There was a problem hiding this comment.
The culprit of #3254 was that in https://github.com/getsentry/sentry-javascript/pull/3255/files#diff-f5bf6e0cdf7709e5675fcdc3b4ff254dd68f3c9d1a399c8751e0fa1846fa85dbR54 we returned earlier and skipped initializing a span recorder.
Worked fine for forcing sampled: false, but clearly not the intended behavior for sampled: true.
| 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; |
There was a problem hiding this comment.
Not cool that we have to repeat ourselves in _startTransaction and startIdleTransaction, but letting it be for now as we may soon move away from idle transactions anyway.
size-limit report
|
|
|
||
| // nothing to do if there's no client or if tracing is disabled | ||
| if (!client || !hasTracingEnabled(options)) { | ||
| function sample<T extends Transaction>(transaction: T, options: Options, samplingContext: SamplingContext): T { |
There was a problem hiding this comment.
I think that the options should go last though, but it's just a nitpick :P
Co-authored-by: Kamil Ogórek <kamil.ogorek@gmail.com>
There were cases in which the sample function would return early without
correctly setting up a spanRecorder, causing child spans to be lost.
Fixes #3254.