implement SpanProcessor::onEnding

.github/workflows/php.yml

@@ -79,26 +79,26 @@ jobs:
     - name: Validate composer.json
       run: composer validate
 
-    - name: Cache Composer packages
-      id: composer-cache
-      uses: actions/cache@v4
-      with:
-        path: vendor
-        key: ${{ runner.os }}-${{ matrix.php-version }}-php-${{ hashFiles('**/composer.json') }}
-        restore-keys: |
-          ${{ runner.os }}-${{ matrix.php-version }}-php-
-    - name: Cache test tools
-      id: test-tools-cache
-      uses: actions/cache@v4
-      with:
-        path: vendor-bin
-        key: ${{ runner.os }}-${{ matrix.php-version }}-php-${{ hashFiles('**/composer.json') }}
-        restore-keys: |
-          ${{ runner.os }}-${{ matrix.php-version }}-php-
+#    - name: Cache Composer packages
+#      id: composer-cache
+#      uses: actions/cache@v4
+#      with:
+#        path: vendor
+#        key: ${{ runner.os }}-${{ matrix.php-version }}-php-${{ hashFiles('**/composer.json') }}
+#        restore-keys: |
+#          ${{ runner.os }}-${{ matrix.php-version }}-php-
+#    - name: Cache test tools
+#      id: test-tools-cache
+#      uses: actions/cache@v4
+#      with:
+#        path: vendor-bin
+#        key: ${{ runner.os }}-${{ matrix.php-version }}-php-${{ hashFiles('**/composer.json') }}
+#        restore-keys: |
+#          ${{ runner.os }}-${{ matrix.php-version }}-php-
 
     - name: Install dependencies
       id: composer
-      if: steps.composer-cache.outputs.cache-hit != 'true'
+#      if: steps.composer-cache.outputs.cache-hit != 'true' || steps.test-tools-cache.outputs.cache-hit != 'true'
       run: composer install --prefer-dist --no-progress ${{ matrix.composer_args }}
 
     - name: Check Style

composer.json

@@ -85,6 +85,7 @@
     "require-dev": {
         "ext-grpc": "*",
         "grpc/grpc": "^1.30",
+        "amphp/amp": "^3",
         "bamarni/composer-bin-plugin": "^1.8",
         "dg/bypass-finals": "^1.4",
         "guzzlehttp/guzzle": "^7.4",

src/SDK/Trace/Span.php

@@ -138,6 +138,10 @@ public function setAttribute(string $key, $value): self
     /** @inheritDoc */
     public function setAttributes(iterable $attributes): self
     {
+        if ($this->hasEnded) {
+            return $this;
+        }
+
         foreach ($attributes as $key => $value) {
             $this->attributesBuilder[$key] = $value;
         }
@@ -244,19 +248,25 @@ public function setStatus(string $code, ?string $description = null): self
         return $this;
     }
 
-    /** @inheritDoc */
+    /**
+     * @inheritDoc
+     * @psalm-suppress NoInterfaceProperties
+     **/
     public function end(?int $endEpochNanos = null): void
     {
         if ($this->hasEnded) {
             return;
         }
 
         $this->endEpochNanos = $endEpochNanos ?? Clock::getDefault()->now();
-        $this->hasEnded = true;
-
-        $this->checkForDroppedElements();
+        $span = clone $this;
+        $this->hasEnded = true; // prevent further modifications to the span by async code
+        $this->spanProcessor->onEnding($span);
+        $span->hasEnded = true;
+        $span->end();
 
         $this->spanProcessor->onEnd($this);
+        $this->checkForDroppedElements();
     }
 
     /** @inheritDoc */

src/SDK/Trace/SpanProcessor/BatchSpanProcessor.php

@@ -143,6 +143,10 @@ public function onStart(ReadWriteSpanInterface $span, ContextInterface $parentCo
     {
     }
 
+    public function onEnding(ReadWriteSpanInterface &$span): void
+    {
+    }
+
     public function onEnd(ReadableSpanInterface $span): void
     {
         if ($this->closed) {

src/SDK/Trace/SpanProcessor/MultiSpanProcessor.php

@@ -76,4 +76,11 @@ public function forceFlush(?CancellationInterface $cancellation = null): bool
 
         return $result;
     }
+
+    public function onEnding(ReadWriteSpanInterface &$span): void
+    {
+        foreach ($this->processors as $processor) {
+            $processor->onEnding($span);
+        }
+    }
 }

src/SDK/Trace/SpanProcessor/NoopSpanProcessor.php

@@ -44,4 +44,8 @@ public function shutdown(?CancellationInterface $cancellation = null): bool
     {
         return $this->forceFlush();
     }
+
+    public function onEnding(ReadWriteSpanInterface &$span): void
+    {
+    } //@codeCoverageIgnore
 }

src/SDK/Trace/SpanProcessor/SimpleSpanProcessor.php

@@ -113,4 +113,8 @@ private function flush(Closure $task, string $taskName, bool $propagateResult, C
 
         return $success;
     }
+
+    public function onEnding(ReadWriteSpanInterface &$span): void
+    {
+    }
 }

src/SDK/Trace/SpanProcessorInterface.php

@@ -15,6 +15,12 @@ interface SpanProcessorInterface
      */
     public function onStart(ReadWriteSpanInterface $span, ContextInterface $parentContext): void;
 
+    /**
+     * @experimental
+     * @see https://github.com/open-telemetry/opentelemetry-specification/blob/v1.41.0/specification/trace/sdk.md#onending
+     */
+    public function onEnding(ReadWriteSpanInterface &$span): void;
+
     /**
      * @see https://github.com/open-telemetry/opentelemetry-specification/blob/v1.7.0/specification/trace/sdk.md#onendspan
      */

tests/Integration/SDK/Trace/test_async_cannot_modify_span_during_onending.phpt

@@ -0,0 +1,37 @@
+--TEST--
+Test that async code cannot update span during OnEnding
+--DESCRIPTION--
+The SDK MUST guarantee that the span can no longer be modified by any other thread before invoking OnEnding of the first SpanProcessor.
+--FILE--
+<?php
+require_once 'vendor/autoload.php';
+
+use OpenTelemetry\SDK\Trace\TracerProviderBuilder;
+use OpenTelemetry\SDK\Trace\ReadWriteSpanInterface;
+use OpenTelemetry\SDK\Trace\ReadableSpanInterface;
+use OpenTelemetry\SDK\Common\Future\CancellationInterface;
+use OpenTelemetry\SDK\Trace\SpanProcessorInterface;
+use OpenTelemetry\Context\ContextInterface;
+
+$tracerProvider = (new TracerProviderBuilder())
+    ->addSpanProcessor(new class implements SpanProcessorInterface {
+
+        public function onEnding(ReadWriteSpanInterface &$span): void {
+            $spanName = $span->getName();
+
+            Amp\delay(0);
+            assert($span->getName() === $spanName);
+        }
+
+        public function onStart(ReadWriteSpanInterface $span, ContextInterface $parentContext): void {}
+        public function onEnd(ReadableSpanInterface $span): void {}
+        public function forceFlush(?CancellationInterface $cancellation = null): bool { return true; }
+        public function shutdown(?CancellationInterface $cancellation = null): bool { return true; }
+    })
+    ->build();
+
+$span = $tracerProvider->getTracer('test')->spanBuilder('span')->startSpan();
+Amp\async($span->updateName(...), 'should-not-update');
+$span->end();
+?>
+--EXPECT--

tests/Integration/SDK/TracerTest.php

@@ -12,13 +12,15 @@
 use OpenTelemetry\Context\Context;
 use OpenTelemetry\SDK\Common\Attribute\AttributesInterface;
 use OpenTelemetry\SDK\Common\Configuration\Variables;
+use OpenTelemetry\SDK\Trace\ReadWriteSpanInterface;
 use OpenTelemetry\SDK\Trace\Sampler\AlwaysOffSampler;
 use OpenTelemetry\SDK\Trace\SamplerInterface;
 use OpenTelemetry\SDK\Trace\SamplingResult;
 use OpenTelemetry\SDK\Trace\Span;
 use OpenTelemetry\SDK\Trace\SpanBuilder;
 use OpenTelemetry\SDK\Trace\SpanExporter\InMemoryExporter;
 use OpenTelemetry\SDK\Trace\SpanLimitsBuilder;
+use OpenTelemetry\SDK\Trace\SpanProcessor\MultiSpanProcessor;
 use OpenTelemetry\SDK\Trace\SpanProcessor\SimpleSpanProcessor;
 use OpenTelemetry\SDK\Trace\SpanProcessorInterface;
 use OpenTelemetry\SDK\Trace\TracerProvider;
@@ -154,4 +156,29 @@ public function test_general_identity_attributes_are_retained_if_enabled(): void
         $this->assertCount(3, $attributes);
         $this->assertSame(0, $attributes->getDroppedAttributesCount());
     }
+
+    #[Group('trace-compliance')]
+    /**
+     * The Span object MUST still be mutable (i.e., SetAttribute, AddLink, AddEvent can be called) while OnEnding is called.
+     */
+    public function test_span_processor_onending_can_mutate_span(): void
+    {
+        $one = $this->createMock(SpanProcessorInterface::class);
+        $one->expects($this->once())->method('onEnding')->willReturnCallback(function (ReadWriteSpanInterface $span) {
+            $span->setAttribute('foo', 'bar');
+            $this->assertCount(0, $span->toSpanData()->getEvents());
+            $span->addEvent('new-event', ['baz' => 'bat']);
+            $span->updateName('updated');
+        });
+        $two = $this->createMock(SpanProcessorInterface::class);
+        $two->expects($this->once())->method('onEnding')->willReturnCallback(function (ReadWriteSpanInterface $span) {
+            $this->assertSame('updated', $span->getName());
+            $this->assertCount(1, $span->toSpanData()->getEvents());
+            $this->assertSame('bar', $span->getAttribute('foo'));
+        });
+        $multi = new MultiSpanProcessor($one, $two);
+        $tracerProvider = new TracerProvider($multi);
+        $span = $tracerProvider->getTracer('test')->spanBuilder('test')->startSpan();
+        $span->end();
+    }
 }

tests/Unit/SDK/Trace/SpanTest.php

@@ -142,6 +142,16 @@ public function test_end_span(): void
             ->once();
     }
 
+    #[Group('trace-compliance')]
+    public function test_end_span_calls_processor_onending(): void
+    {
+        $span = $this->createTestSpan(API\SpanKind::KIND_CONSUMER);
+        $span->end();
+        $this->spanProcessor
+            ->shouldHaveReceived('onEnding')
+            ->once();
+    }
+
     public function test_get_start_epoch_nanos(): void
     {
         $span = $this->createTestSpan(API\SpanKind::KIND_INTERNAL);

vendor-bin/psalm/composer.json

@@ -3,5 +3,8 @@
         "vimeo/psalm": "^5.24",
         "psalm/plugin-mockery": "^1",
         "psalm/plugin-phpunit": "^0.18.4"
+    },
+    "replace": {
+        "amphp/amp": "*"
     }
 }