Skip to content

feat(scopes): use TransactionSampler #2120

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.

Sign up for GitHub

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

Open
Litarnus wants to merge 1 commit into from
+23 −36
Open

feat(scopes): use TransactionSampler #2120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions src/State/Hub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,9 +236,7 @@ public function getIntegration(string $className): ?IntegrationInterface
*/
public function startTransaction(TransactionContext $context, array $customSamplingContext = []): Transaction
{
$transaction = new Transaction($context, $this);

return (new TransactionSampler($this->getClient()->getOptions()))->startTransaction($transaction, $context, $customSamplingContext);
return TransactionSampler::startTransaction($this->getClient()->getOptions(), $context, $customSamplingContext);
}

/**
Expand Down
5 changes: 3 additions & 2 deletions src/Tracing/Transaction.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

use Sentry\Event;
use Sentry\EventId;
use Sentry\Options;
use Sentry\Profiling\Profiler;
use Sentry\SentrySdk;
use Sentry\State\HubInterface;
Expand Down Expand Up @@ -119,10 +120,10 @@ public function initSpanRecorder(int $maxSpans = 1000): self
return $this;
}

public function initProfiler(): Profiler
public function initProfiler(?Options $options = null): Profiler
{
if ($this->profiler === null) {
$this->profiler = new Profiler($this->hub->getClient()->getOptions());
$this->profiler = new Profiler($options ?? $this->hub->getClient()->getOptions());
}

return $this->profiler;
Expand Down
39 changes: 17 additions & 22 deletions src/Tracing/TransactionSampler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,24 +13,19 @@
*/
final class TransactionSampler
{
/**
* @var Options
*/
private $options;

public function __construct(Options $options)
private function __construct()
{
$this->options = $options;
}

/**
* @param array<string, mixed> $customSamplingContext Additional context that will be passed to the {@see SamplingContext}
*/
public function startTransaction(Transaction $transaction, TransactionContext $context, array $customSamplingContext = []): Transaction
public static function startTransaction(Options $options, TransactionContext $context, array $customSamplingContext = []): Transaction
{
$logger = $this->options->getLoggerOrNullLogger();
$transaction = new Transaction($context);
Comment thread
cursor[bot] marked this conversation as resolved.

@sentry sentry Bot Jun 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The Transaction constructor now defaults to the global hub instead of using the specific hub instance on which startTransaction() was called, affecting multi-hub scenarios.
Severity: MEDIUM

Suggested Fix

The TransactionSampler::startTransaction method should be updated to accept the HubInterface instance as a parameter. This hub instance should then be passed explicitly to the Transaction constructor, restoring the previous behavior and ensuring the transaction is associated with the correct hub on which it was started.

Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: src/Tracing/TransactionSampler.php#L25

Potential issue: The refactoring of `TransactionSampler::startTransaction` to a static
method removed the passing of the `HubInterface` instance to the `Transaction`
constructor. Consequently, the constructor now defaults to using the global hub via
`SentrySdk::getCurrentHub()`. In scenarios where multiple hubs are instantiated and
used, calling `startTransaction()` on a specific, non-global hub will cause the
resulting transaction to be incorrectly associated with the global hub. This leads to
the transaction event being captured by the wrong hub and its Dynamic Sampling Context
(DSC) being populated from the wrong client's options, breaking the API contract for
multi-hub usage.

Also affects:

  • src/State/Hub.php:239~239

Did we get this right? 👍 / 👎 to inform future reviews.

Comment thread
cursor[bot] marked this conversation as resolved.
Comment thread
cursor[bot] marked this conversation as resolved.
Comment thread
cursor[bot] marked this conversation as resolved.

@sentry sentry Bot Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: In multi-hub scenarios, transactions are incorrectly associated with the global hub instead of the hub that created them, causing events to be sent to the wrong project.
Severity: HIGH

Suggested Fix

The static TransactionSampler::startTransaction method should be modified to accept the calling HubInterface instance as an argument. This hub instance must then be passed to the Transaction constructor, like new Transaction($context, $hub), to ensure the transaction is correctly associated with the hub that initiated its creation.

Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: src/Tracing/TransactionSampler.php#L25

Potential issue: The refactoring of `Hub::startTransaction` to use the static
`TransactionSampler::startTransaction` method has introduced a regression. The call to
`new Transaction($context)` inside the sampler no longer receives the calling hub
instance. As a result, the `Transaction` constructor defaults to using the global hub
via `SentrySdk::getCurrentHub()`. In a multi-hub scenario, if a transaction is started
on a non-current hub, subsequent operations like `finish()` will be executed against the
wrong hub, causing events to be captured by the wrong client and sent to the incorrect
DSN.

$logger = $options->getLoggerOrNullLogger();

if (!$this->options->isTracingEnabled()) {
if (!$options->isTracingEnabled()) {
$transaction->setSampled(false);

$logger->warning(\sprintf('Transaction [%s] was started but tracing is not enabled.', (string) $transaction->getTraceId()), ['context' => $context]);
Expand All @@ -45,7 +40,7 @@ public function startTransaction(Transaction $transaction, TransactionContext $c
$sampleRand = $context->getMetadata()->getSampleRand() ?? 0.0;

if ($transaction->getSampled() === null) {
$tracesSampler = $this->options->getTracesSampler();
$tracesSampler = $options->getTracesSampler();

if ($tracesSampler !== null) {
$sampleRate = $tracesSampler($samplingContext);
Expand All @@ -56,15 +51,15 @@ public function startTransaction(Transaction $transaction, TransactionContext $c
$sampleRate = $parentSampleRate;
$sampleSource = 'parent:sample_rate';
} else {
$sampleRate = $this->getSampleRate(
$sampleRate = self::getSampleRate(
$samplingContext->getParentSampled(),
$this->options->getTracesSampleRate() ?? 0
$options->getTracesSampleRate() ?? 0
);
$sampleSource = $samplingContext->getParentSampled() !== null ? 'parent:sampling_decision' : 'config:traces_sample_rate';
}
}

if (!$this->isValidSampleRate($sampleRate)) {
if (!self::isValidSampleRate($sampleRate)) {
$transaction->setSampled(false);

$logger->warning(\sprintf('Transaction [%s] was started but not sampled because sample rate (decided by %s) is invalid.', (string) $transaction->getTraceId(), $sampleSource), ['context' => $context]);
Expand Down Expand Up @@ -102,31 +97,31 @@ public function startTransaction(Transaction $transaction, TransactionContext $c
$transaction->initSpanRecorder();

$profilesSampleSource = 'config:profiles_sample_rate';
$profilesSampler = $this->options->getProfilesSampler();
$profilesSampler = $options->getProfilesSampler();

if ($profilesSampler !== null) {
$profilesSampleRate = $profilesSampler($samplingContext);
$profilesSampleSource = 'config:profiles_sampler';
} else {
$profilesSampleRate = $this->options->getProfilesSampleRate();
$profilesSampleRate = $options->getProfilesSampleRate();
}

if ($profilesSampleRate === null) {
$logger->info(\sprintf('Transaction [%s] is not profiling because neither `profiles_sample_rate` nor `profiles_sampler` option is set.', (string) $transaction->getTraceId()));
} elseif (!$this->isValidSampleRate($profilesSampleRate)) {
} elseif (!self::isValidSampleRate($profilesSampleRate)) {
$logger->warning(\sprintf('Transaction [%s] is not profiling because profile sample rate (decided by %s) is invalid.', (string) $transaction->getTraceId(), $profilesSampleSource));
} elseif ($this->sampleRate($profilesSampleRate)) {
} elseif (self::sampleRate($profilesSampleRate)) {
$logger->info(\sprintf('Transaction [%s] started profiling because it was sampled.', (string) $transaction->getTraceId()));

$transaction->initProfiler()->start();
$transaction->initProfiler($options)->start();
} else {
$logger->info(\sprintf('Transaction [%s] is not profiling because it was not sampled.', (string) $transaction->getTraceId()));
}

return $transaction;
}

private function getSampleRate(?bool $hasParentBeenSampled, float $fallbackSampleRate): float
private static function getSampleRate(?bool $hasParentBeenSampled, float $fallbackSampleRate): float
{
if ($hasParentBeenSampled === true) {
return 1.0;
Expand All @@ -142,7 +137,7 @@ private function getSampleRate(?bool $hasParentBeenSampled, float $fallbackSampl
/**
* @param mixed $sampleRate
*/
private function sampleRate($sampleRate): bool
private static function sampleRate($sampleRate): bool
{
if (!\is_float($sampleRate) && !\is_int($sampleRate)) {
return false;
Expand All @@ -162,7 +157,7 @@ private function sampleRate($sampleRate): bool
/**
* @param mixed $sampleRate
*/
private function isValidSampleRate($sampleRate): bool
private static function isValidSampleRate($sampleRate): bool
{
if (!\is_float($sampleRate) && !\is_int($sampleRate)) {
return false;
Expand Down
2 changes: 1 addition & 1 deletion tests/State/HubTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -838,7 +838,7 @@ public function testStartTransactionWithCustomSamplingContext(): void
public function testStartTransactionStartsProfilerWithProfilesSampler(): void
{
$client = $this->createMock(ClientInterface::class);
$client->expects($this->exactly(2))
$client->expects($this->once())
->method('getOptions')
->willReturn(new Options([
'traces_sample_rate' => 1.0,
Expand Down
9 changes: 1 addition & 8 deletions tests/Tracing/TransactionSamplerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,7 @@
namespace Sentry\Tests\Tracing;

use PHPUnit\Framework\TestCase;
use Sentry\ClientInterface;
use Sentry\Options;
use Sentry\State\Hub;
use Sentry\Tracing\DynamicSamplingContext;
use Sentry\Tracing\SamplingContext;
use Sentry\Tracing\Transaction;
Expand Down Expand Up @@ -320,11 +318,6 @@ public function testUpdatesTheDscSampleRate(): void
*/
private function sampleTransaction(Options $options, TransactionContext $transactionContext, array $customSamplingContext = []): Transaction
{
$client = $this->createMock(ClientInterface::class);
$client->method('getOptions')->willReturn($options);

$transaction = new Transaction($transactionContext, new Hub($client));

return (new TransactionSampler($options))->startTransaction($transaction, $transactionContext, $customSamplingContext);
return TransactionSampler::startTransaction($options, $transactionContext, $customSamplingContext);
}
}