From 02d94216e1c6c84957822108792f8e0fe25d4d7f Mon Sep 17 00:00:00 2001
From: Brett McBride <brett.a.mcbride@gmail.com>
Date: Wed, 7 Dec 2022 14:07:33 +1100
Subject: [PATCH] span status compliance (#884)

implement spec-compliance for span status:
- Ok is final
- Unset should be ignored
- Error can be updated to Ok

ref: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#set-status
---
 src/SDK/Trace/Span.php            |  9 ++++++++
 tests/Unit/SDK/Trace/SpanTest.php | 37 +++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/src/SDK/Trace/Span.php b/src/SDK/Trace/Span.php
index e4175739c..f72ec1bd7 100644
--- a/src/SDK/Trace/Span.php
+++ b/src/SDK/Trace/Span.php
@@ -256,6 +256,15 @@ public function setStatus(string $code, string $description = null): self
             return $this;
         }
 
+        // An attempt to set value Unset SHOULD be ignored.
+        if ($code === API\StatusCode::STATUS_UNSET) {
+            return $this;
+        }
+
+        // When span status is set to Ok it SHOULD be considered final and any further attempts to change it SHOULD be ignored.
+        if ($this->status->getCode() === API\StatusCode::STATUS_OK) {
+            return $this;
+        }
         $this->status = StatusData::create($code, $description);
 
         return $this;
diff --git a/tests/Unit/SDK/Trace/SpanTest.php b/tests/Unit/SDK/Trace/SpanTest.php
index c38ef0a82..daf032508 100644
--- a/tests/Unit/SDK/Trace/SpanTest.php
+++ b/tests/Unit/SDK/Trace/SpanTest.php
@@ -13,6 +13,7 @@
 use OpenTelemetry\API\Trace\NonRecordingSpan;
 use OpenTelemetry\API\Trace\SpanContext;
 use OpenTelemetry\API\Trace\SpanContextValidator;
+use OpenTelemetry\API\Trace\StatusCode;
 use OpenTelemetry\Context\Context;
 use OpenTelemetry\SDK\Common\Attribute\Attributes;
 use OpenTelemetry\SDK\Common\Attribute\AttributesInterface;
@@ -747,6 +748,42 @@ public function test_add_event_order_preserved(): void
         $this->assertEvent($events[2], 'c', Attributes::create(['key' => 2]), self::START_EPOCH);
     }
 
+    /**
+     * @dataProvider statusCodeProvider
+     * @group trace-compliance
+     * @psalm-param StatusCode::STATUS_* $code
+     *
+     * When span status is set to Ok it SHOULD be considered final and any further attempts to change it SHOULD be ignored.
+     */
+    public function test_set_status_after_ok_is_ignored(string $code): void
+    {
+        $span = $this->createTestRootSpan();
+        $span->setStatus(API\StatusCode::STATUS_OK);
+        $this->assertSame(API\StatusCode::STATUS_OK, $span->toSpanData()->getStatus()->getCode());
+        $span->setStatus($code);
+        $this->assertNotSame($code, $span->toSpanData()->getStatus()->getCode(), 'update after Ok was ignored');
+        $span->setStatus(API\StatusCode::STATUS_OK);
+        $this->assertSame(API\StatusCode::STATUS_OK, $span->toSpanData()->getStatus()->getCode());
+    }
+
+    public function statusCodeProvider(): array
+    {
+        return [
+            [API\StatusCode::STATUS_UNSET],
+            [API\StatusCode::STATUS_ERROR],
+        ];
+    }
+
+    /**
+     * @group trace-compliance
+     */
+    public function test_can_set_status_to_ok_after_error(): void
+    {
+        $span = $this->createTestRootSpan();
+        $span->setStatus(API\StatusCode::STATUS_ERROR);
+        $this->assertSame(API\StatusCode::STATUS_ERROR, $span->toSpanData()->getStatus()->getCode());
+    }
+
     private function createTestRootSpan(): Span
     {
         return $this