Repository: getsentry/sentry-php (php)

Issue:
Scope::getTransaction returns null when Transaction isn't sampled
### Environment

How do you use Sentry?
Sentry SaaS (sentry.io)

Which SDK and version?
sentry-php@3.4.0

### Steps to Reproduce

1. initialize sentry with a very low sample rate for easier repro
2. call `$transaction = startTransaction`
3. call `SentrySdk::getCurrentHub()->setSpan($transaction)`
4. call `SentrySdk::getCurrentHub()->getTransaction()`

### Expected Result

I expected to get the instance of the Transaction I just started.

### Actual Result

`getTransaction` returned null because when you call `startTransaction` and this transaction isn't getting sampled it does not initialize the SpanRecorder (Hub.php:256). However, `getTransaction` gets the transaction from `Scope` which checks for an active SpanRecorder and returns null otherwise.

This behavior is not documented anywhere, I think it would suffice to add a note to the `Hub::getTransaction` method that if it's not sampled it will return null.


Pull request description:
Fix `Scope::getTransaction()` so that it returns also unsampled transactions
Fixes #1333: `Scope::getTransaction()` was using the `SpanRecorder` to find the active transaction to return, but if the transaction is unsampled the recorder doesn't keep track of it and so `null` is returned. By adding a reference to the parent transaction inside each span, we can bypass the recorder and directly retrieve it, regardless of its sampling status. The original fix was implemented in getsentry/sentry-javascript#2952, and this is just a porting of it.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index d6e57b1..e065bc6 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,8 @@
 
 ## Unreleased
 
+- Fix `Scope::getTransaction()` so that it returns also unsampled transactions (#1334)
+
 ## 3.6.1 (2022-06-27)
 
 - Set the `sentry-trace` header when using the tracing middleware (#1331)
diff --git a/src/State/Scope.php b/src/State/Scope.php
index 78cd452..a708f8f 100644
--- a/src/State/Scope.php
+++ b/src/State/Scope.php
@@ -408,12 +408,8 @@ public function setSpan(?Span $span): self
      */
     public function getTransaction(): ?Transaction
     {
-        $span = $this->span;
-
-        if (null !== $span && null !== $span->getSpanRecorder() && !empty($span->getSpanRecorder()->getSpans())) {
-            // The first span in the recorder is considered to be a Transaction
-            /** @var Transaction */
-            return $span->getSpanRecorder()->getSpans()[0];
+        if (null !== $this->span) {
+            return $this->span->getTransaction();
         }
 
         return null;
diff --git a/src/Tracing/Span.php b/src/Tracing/Span.php
index a198361..2095596 100644
--- a/src/Tracing/Span.php
+++ b/src/Tracing/Span.php
@@ -7,7 +7,7 @@
 use Sentry\EventId;
 
 /**
- * This class stores all the information about a Span.
+ * This class stores all the information about a span.
  */
 class Span
 {
@@ -22,22 +22,22 @@ class Span
     protected $traceId;
 
     /**
-     * @var string|null Description of the Span
+     * @var string|null Description of the span
      */
     protected $description;
 
     /**
-     * @var string|null Operation of the Span
+     * @var string|null Operation of the span
      */
     protected $op;
 
     /**
-     * @var SpanStatus|null Completion status of the Span
+     * @var SpanStatus|null Completion status of the span
      */
     protected $status;
 
     /**
-     * @var SpanId|null ID of the parent Span
+     * @var SpanId|null ID of the parent span
      */
     protected $parentSpanId;
 
@@ -47,7 +47,7 @@ class Span
     protected $sampled;
 
     /**
-     * @var array<string, string> A List of tags associated to this Span
+     * @var array<string, string> A List of tags associated to this span
      */
     protected $tags = [];
 
@@ -67,10 +67,15 @@ class Span
     protected $endTimestamp;
 
     /**
-     * @var SpanRecorder|null Reference instance to the SpanRecorder
+     * @var SpanRecorder|null Reference instance to the {@see SpanRecorder}
      */
     protected $spanRecorder;
 
+    /**
+     * @var Transaction|null The transaction containing this span
+     */
+    protected $transaction;
+
     /**
      * Constructor.
      *
@@ -390,6 +395,7 @@ public function startChild(SpanContext $context): self
         $context->setTraceId($this->traceId);
 
         $span = new self($context);
+        $span->transaction = $this->transaction;
         $span->spanRecorder = $this->spanRecorder;
 
         if (null != $span->spanRecorder) {
@@ -417,6 +423,14 @@ public function detachSpanRecorder(): void
         $this->spanRecorder = null;
     }
 
+    /**
+     * Returns the transaction containing this span.
+     */
+    public function getTransaction(): ?Transaction
+    {
+        return $this->transaction;
+    }
+
     /**
      * Returns a string that can be used for the `sentry-trace` header.
      */
diff --git a/src/Tracing/Transaction.php b/src/Tracing/Transaction.php
index 222361f..d3cfb00 100644
--- a/src/Tracing/Transaction.php
+++ b/src/Tracing/Transaction.php
@@ -38,6 +38,7 @@ public function __construct(TransactionContext $context, ?HubInterface $hub = nu
 
         $this->hub = $hub ?? SentrySdk::getCurrentHub();
         $this->name = $context->getName();
+        $this->transaction = $this;
     }
 
     /**


Test patch:
diff --git a/tests/State/HubTest.php b/tests/State/HubTest.php
index 85b4d58..5a47f57 100644
--- a/tests/State/HubTest.php
+++ b/tests/State/HubTest.php
@@ -731,4 +731,51 @@ public function testStartTransactionWithCustomSamplingContext(): void
         $hub = new Hub($client);
         $hub->startTransaction(new TransactionContext(), $customSamplingContext);
     }
+
+    public function testGetTransactionReturnsInstanceSetOnTheScopeIfTransactionIsNotSampled(): void
+    {
+        $client = $this->createMock(ClientInterface::class);
+        $client->expects($this->once())
+            ->method('getOptions')
+            ->willReturn(new Options(['traces_sample_rate' => 1]));
+
+        $hub = new Hub($client);
+        $transaction = $hub->startTransaction(new TransactionContext(TransactionContext::DEFAULT_NAME, false));
+
+        $hub->configureScope(static function (Scope $scope) use ($transaction): void {
+            $scope->setSpan($transaction);
+        });
+
+        $this->assertSame($transaction, $hub->getTransaction());
+    }
+
+    public function testGetTransactionReturnsInstanceSetOnTheScopeIfTransactionIsSampled(): void
+    {
+        $client = $this->createMock(ClientInterface::class);
+        $client->expects($this->once())
+            ->method('getOptions')
+            ->willReturn(new Options(['traces_sample_rate' => 1]));
+
+        $hub = new Hub($client);
+        $transaction = $hub->startTransaction(new TransactionContext(TransactionContext::DEFAULT_NAME, true));
+
+        $hub->configureScope(static function (Scope $scope) use ($transaction): void {
+            $scope->setSpan($transaction);
+        });
+
+        $this->assertSame($transaction, $hub->getTransaction());
+    }
+
+    public function testGetTransactionReturnsNullIfNoTransactionIsSetOnTheScope(): void
+    {
+        $client = $this->createMock(ClientInterface::class);
+        $client->expects($this->once())
+            ->method('getOptions')
+            ->willReturn(new Options(['traces_sample_rate' => 1]));
+
+        $hub = new Hub($client);
+        $hub->startTransaction(new TransactionContext(TransactionContext::DEFAULT_NAME, true));
+
+        $this->assertNull($hub->getTransaction());
+    }
 }