-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Create spanRecorder whenever transactions are sampled #3255
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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<T extends Transaction>(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<T extends Transaction>(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<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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
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()) || {}; | ||
rhcarvalho marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
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()) || {}; | ||
rhcarvalho marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not cool that we have to repeat ourselves in |
||
} | ||
|
||
/** | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the
options
should go last though, but it's just a nitpick :PThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah agree, I'll leave that for later so we don't have to go change more lines on the call sites.