fix(otel): Missing addLink method and Fiber handling (#2849)

* fix(otel): Missing method and Fiber handling * tests: `test_opentelemetry_{1|beta}` * tests: Properly set composer to 1.0.* * fix: remove `cleanup_opentelemetry` * fix: php 7.4 unpacking issue * tests: skip fiber test on coverage * style: remove useless require * remove comment * perf: condition check * perf: Class lookup optimization Co-authored-by: Bob Weinand <bob.weinand@datadoghq.com> --------- Co-authored-by: Bob Weinand <bob.weinand@datadoghq.com>
DataDog · Sep 17, 2024 · 17b3c7a · 17b3c7a
1 parent 501f386
commit 17b3c7a
Show file tree

Hide file tree

Showing 11 changed files with 223 additions and 112 deletions.
diff --git a/Makefile b/Makefile
@@ -846,6 +846,7 @@ TEST_INTEGRATIONS_81 := \
 	test_integrations_mysqli \
 	test_integrations_openai \
 	test_opentelemetry_1 \
+	test_opentelemetry_beta \
 	test_integrations_guzzle7 \
 	test_integrations_pcntl \
 	test_integrations_pdo \
@@ -898,6 +899,7 @@ TEST_INTEGRATIONS_82 := \
 	test_integrations_mysqli \
 	test_integrations_openai \
 	test_opentelemetry_1 \
+	test_opentelemetry_beta \
 	test_integrations_guzzle7 \
 	test_integrations_pcntl \
 	test_integrations_pdo \
@@ -958,6 +960,7 @@ TEST_INTEGRATIONS_83 := \
 	test_integrations_mysqli \
 	test_integrations_openai \
 	test_opentelemetry_1 \
+	test_opentelemetry_beta \
 	test_integrations_guzzle7 \
 	test_integrations_pcntl \
 	test_integrations_pdo \
@@ -1133,10 +1136,27 @@ benchmarks: benchmarks_run_dependencies call_benchmarks
 
 benchmarks_opcache: benchmarks_run_dependencies call_benchmarks_opcache
 
-test_opentelemetry_1: global_test_run_dependencies tests/Frameworks/Custom/OpenTelemetry/composer.lock-php$(PHP_MAJOR_MINOR) tests/OpenTelemetry/composer.lock-php$(PHP_MAJOR_MINOR)
+define setup_opentelemetry
+	cp $(1) $(dir $(1))/composer.json
+endef
+
+define run_opentelemetry_tests
 	$(eval TEST_EXTRA_ENV=$(shell [ $(PHP_MAJOR_MINOR) -ge 81 ] && echo "OTEL_PHP_FIBERS_ENABLED=1" || echo '') DD_TRACE_OTEL_ENABLED=1 DD_TRACE_GENERATE_ROOT_SPAN=0)
 	$(call run_tests,--testsuite=opentelemetry1 $(TESTS))
 	$(eval TEST_EXTRA_ENV=)
+endef
+
+_test_opentelemetry_beta_setup: global_test_run_dependencies
+	$(call setup_opentelemetry,tests/OpenTelemetry/composer-beta.json)
+
+test_opentelemetry_beta: _test_opentelemetry_beta_setup tests/Frameworks/Custom/OpenTelemetry/composer.lock-php$(PHP_MAJOR_MINOR) tests/OpenTelemetry/composer.lock-php$(PHP_MAJOR_MINOR)
+	$(call run_opentelemetry_tests)
+
+_test_opentelemetry_1_setup: global_test_run_dependencies
+	$(call setup_opentelemetry,tests/OpenTelemetry/composer-1.json)
+
+test_opentelemetry_1: _test_opentelemetry_1_setup tests/Frameworks/Custom/OpenTelemetry/composer.lock-php$(PHP_MAJOR_MINOR) tests/OpenTelemetry/composer.lock-php$(PHP_MAJOR_MINOR)
+	$(call run_opentelemetry_tests)
 
 test_opentracing_10: global_test_run_dependencies tests/OpenTracer1Unit/composer.lock-php$(PHP_MAJOR_MINOR) tests/Frameworks/Custom/OpenTracing/composer.lock-php$(PHP_MAJOR_MINOR)
 	$(call run_tests,tests/OpenTracer1Unit)

diff --git a/src/DDTrace/OpenTelemetry/Context.php b/src/DDTrace/OpenTelemetry/Context.php
@@ -27,6 +27,9 @@ final class Context implements ContextInterface
     /** @var ContextStorageInterface&ExecutionContextAwareInterface */
     private static ContextStorageInterface $storage;
 
+    /** @var string $storageClass */
+    private static string $storageClass = '';
+
     // Optimization for spans to avoid copying the context array.
     private static ContextKeyInterface $spanContextKey;
     private ?object $span = null;
@@ -58,8 +61,13 @@ public static function setStorage(ContextStorageInterface $storage): void
      */
     public static function storage(): ContextStorageInterface
     {
-        /** @psalm-suppress RedundantPropertyInitializationCheck */
-        return self::$storage ??= new ContextStorage();
+        if (self::$storageClass === '') {
+            self::$storageClass = class_exists('OpenTelemetry\Context\FiberBoundContextStorageExecutionAwareBC')
+                ? 'OpenTelemetry\Context\FiberBoundContextStorageExecutionAwareBC' // v1.1+
+                : 'OpenTelemetry\Context\ContextStorage';
+        }
+
+        return self::$storage ??= new self::$storageClass();
     }
 
     /**

diff --git a/src/DDTrace/OpenTelemetry/Span.php b/src/DDTrace/OpenTelemetry/Span.php
@@ -112,28 +112,18 @@ private function __construct(
             foreach ($links as $link) {
                 /** @var LinkInterface $link */
                 $linkContext = $link->getSpanContext();
-
-                $spanLink = new SpanLink();
-                $spanLink->traceId = $linkContext->getTraceId();
-                $spanLink->spanId = $linkContext->getSpanId();
-                $spanLink->traceState = (string)$linkContext->getTraceState(); // __toString()
-                $spanLink->attributes = $link->getAttributes()->toArray();
-                $spanLink->droppedAttributesCount = 0; // Attributes limit aren't supported/meaningful in DD
-
-                // Save the link
-                ObjectKVStore::put($spanLink, "link", $link);
-                $span->links[] = $spanLink;
+                $span->links[] = $this->createAndSaveSpanLink($linkContext, $link->getAttributes()->toArray(), $link);
             }
 
             foreach ($events as $event) {
                 /** @var EventInterface $event */
 
                 $spanEvent = new SpanEvent(
-                    $event->getName(), 
+                    $event->getName(),
                     $event->getAttributes()->toArray(),
                     $event->getEpochNanos()
                 );
-     
+
                 // Save the event
                 ObjectKVStore::put($spanEvent, "event", $event);
                 $span->events[] = $spanEvent;
@@ -235,17 +225,33 @@ public function toSpanData(): SpanDataInterface
         $this->updateSpanLinks();
         $this->updateSpanEvents();
 
-        return new ImmutableSpan(
-            $this,
-            $this->getName(),
-            $this->links,
-            $this->events,
-            Attributes::create(array_merge($this->span->meta, $this->span->metrics)),
-            0,
-            StatusData::create($this->status->getCode(), $this->status->getDescription()),
-            $hasEnded ? $this->span->getStartTime() + $this->span->getDuration() : 0,
-            $this->hasEnded()
-        );
+        if (method_exists(SpanInterface::class, 'addLink')) {
+            // v1.1 backward compatibility: totalRecordedLinks parameter added
+            return new ImmutableSpan(
+                $this,
+                $this->getName(),
+                $this->links,
+                $this->events,
+                Attributes::create(array_merge($this->span->meta, $this->span->metrics)),
+                $this->totalRecordedEvents,
+                $this->totalRecordedLinks,
+                StatusData::create($this->status->getCode(), $this->status->getDescription()),
+                $hasEnded ? $this->span->getStartTime() + $this->span->getDuration() : 0,
+                $this->hasEnded(),
+            );
+        } else {
+            return new ImmutableSpan(
+                $this,
+                $this->getName(),
+                $this->links,
+                $this->events,
+                Attributes::create(array_merge($this->span->meta, $this->span->metrics)),
+                $this->totalRecordedEvents,
+                StatusData::create($this->status->getCode(), $this->status->getDescription()),
+                $hasEnded ? $this->span->getStartTime() + $this->span->getDuration() : 0,
+                $this->hasEnded(),
+            );
+        }
     }
 
     /**
@@ -358,14 +364,27 @@ public function setAttributes(iterable $attributes): SpanInterface
         return $this;
     }
 
+    /**
+     * @inheritDoc
+     */
+    public function addLink(SpanContextInterface $context, iterable $attributes = []): SpanInterface
+    {
+        if ($this->hasEnded() || !$context->isValid()) {
+            return $this;
+        }
+
+        $this->span->links[] = $this->createAndSaveSpanLink($context, $attributes);
+        return $this;
+    }
+
     /**
      * @inheritDoc
      */
     public function addEvent(string $name, iterable $attributes = [], int $timestamp = null): SpanInterface
     {
         if (!$this->hasEnded()) {
             $this->span->events[] = new SpanEvent(
-                $name, 
+                $name,
                 $attributes,
                 $timestamp ?? (int)(microtime(true) * 1e9)
             );
@@ -522,7 +541,7 @@ private function updateSpanEvents()
     {
         $datadogSpanEvents = $this->span->events;
         $this->span->meta["events"] = count($this->events);
-    
+
         $otel = [];
         foreach ($datadogSpanEvents as $datadogSpanEvent) {
             $exceptionAttributes = [];
@@ -539,17 +558,35 @@ private function updateSpanEvents()
                 $event = new Event(
                     $datadogSpanEvent->name,
                     (int)$datadogSpanEvent->timestamp,
-                    Attributes::create(array_merge($exceptionAttributes, \is_array($datadogSpanEvent->attributes) ? $datadogSpanEvent->attributes : iterator_to_array($datadogSpanEvent->attributes)))                
+                    Attributes::create(array_merge($exceptionAttributes, \is_array($datadogSpanEvent->attributes) ? $datadogSpanEvent->attributes : iterator_to_array($datadogSpanEvent->attributes)))
                 );
 
                 // Save the event
                 ObjectKVStore::put($datadogSpanEvent, "event", $event);
             }
             $otel[] = $event;
         }
-    
+
         // Update the events
         $this->events = $otel;
         $this->totalRecordedEvents = count($otel);
     }
+
+    private function createAndSaveSpanLink(SpanContextInterface $context, iterable $attributes = [], LinkInterface $link = null)
+    {
+        $spanLink = new SpanLink();
+        $spanLink->traceId = $context->getTraceId();
+        $spanLink->spanId = $context->getSpanId();
+        $spanLink->traceState = (string)$context->getTraceState(); // __toString()
+        $spanLink->attributes = $attributes;
+        $spanLink->droppedAttributesCount = 0; // Attributes limit aren't supported/meaningful in DD
+
+        // Save the link
+        if (is_null($link)) {
+            $link = new Link($context, Attributes::create($attributes));
+        }
+        ObjectKVStore::put($spanLink, "link", $link);
+
+        return $spanLink;
+    }
 }
diff --git a/...lemetry/Integration/Context/FiberTest.php → ...y/Integration/Context/Fiber/FiberTest.php b/...lemetry/Integration/Context/FiberTest.php → ...y/Integration/Context/Fiber/FiberTest.php
@@ -13,7 +13,7 @@
 use OpenTelemetry\API\Trace\Span;
 use OpenTelemetry\API\Trace\SpanKind;
 use OpenTelemetry\Context\Context;
-use OpenTelemetry\Context\ContextStorage;
+use OpenTelemetry\Context\ExecutionContextAwareInterface;
 use OpenTelemetry\SDK\Trace\TracerProvider;
 use function DDTrace\close_span;
 use function DDTrace\start_span;
@@ -31,78 +31,6 @@ public function ddSetUp(): void
     public function ddTearDown()
     {
         parent::ddTearDown();
-        Context::setStorage(new ContextStorage()); // Reset OpenTelemetry context
-    }
-
-    /**
-     * @throws \Throwable
-     */
-    public function test_context_switching_ffi_observer()
-    {
-        $key = Context::createKey('-');
-        $scope = Context::getCurrent()
-            ->with($key, 'main')
-            ->activate();
-
-        $fiber = new Fiber(function () use ($key) {
-            $scope = Context::getCurrent()
-                ->with($key, 'fiber')
-                ->activate();
-
-            $this->assertSame('fiber:fiber', 'fiber:' . Context::getCurrent()->get($key));
-
-            Fiber::suspend();
-
-            $this->assertSame('fiber:fiber', 'fiber:' . Context::getCurrent()->get($key));
-
-            $scope->detach();
-        });
-
-        $this->assertSame('main:main', 'main:' . Context::getCurrent()->get($key));
-
-        $fiber->start();
-
-        $this->assertSame('main:main', 'main:' . Context::getCurrent()->get($key));
-
-        $fiber->resume();
-
-        $this->assertSame('main:main', 'main:' . Context::getCurrent()->get($key));
-
-        $scope->detach();
-    }
-
-    public function test_context_switching_ffi_observer_registered_on_startup()
-    {
-        $key = Context::createKey('-');
-
-        $fiber = new Fiber(function () use ($key) {
-            $scope = Context::getCurrent()
-                ->with($key, 'fiber')
-                ->activate();
-
-            $this->assertSame('fiber:fiber', 'fiber:' . Context::getCurrent()->get($key));
-
-            Fiber::suspend();
-
-            $this->assertSame('fiber:fiber', 'fiber:' . Context::getCurrent()->get($key));
-
-            $scope->detach();
-        });
-
-
-        $fiber->start();
-
-        $this->assertSame('main:', 'main:' . Context::getCurrent()->get($key));
-
-        $scope = Context::getCurrent()
-            ->with($key, 'main')
-            ->activate();
-
-        $fiber->resume();
-
-        $this->assertSame('main:main', 'main:' . Context::getCurrent()->get($key));
-
-        $scope->detach();
     }
 
     public function testFiberInteroperabilityStackSwitch()

diff --git a/tests/OpenTelemetry/Integration/Context/Fiber/test_context_switching_ffi_observer.phpt b/tests/OpenTelemetry/Integration/Context/Fiber/test_context_switching_ffi_observer.phpt
@@ -0,0 +1,46 @@
+--TEST--
+Context switches on execution context switch.
+--SKIPIF--
+<?php if (PHP_VERSION_ID < 80100 || !extension_loaded('ffi') || getenv('PHPUNIT_COVERAGE')) die('skip requires PHP8.1 and FFI'); ?>
+--ENV--
+OTEL_PHP_FIBERS_ENABLED=1
+--FILE--
+<?php
+use OpenTelemetry\Context\Context;
+
+require_once './tests/OpenTelemetry/vendor/autoload.php';
+
+$key = Context::createKey('-');
+$scope = Context::getCurrent()
+    ->with($key, 'main')
+    ->activate();
+
+$fiber = new Fiber(function() use ($key) {
+    $scope = Context::getCurrent()
+        ->with($key, 'fiber')
+        ->activate();
+
+    echo 'fiber:' . Context::getCurrent()->get($key), PHP_EOL;
+
+    Fiber::suspend();
+    echo 'fiber:' . Context::getCurrent()->get($key), PHP_EOL;
+
+    $scope->detach();
+});
+
+echo 'main:' . Context::getCurrent()->get($key), PHP_EOL;
+
+$fiber->start();
+echo 'main:' . Context::getCurrent()->get($key), PHP_EOL;
+
+$fiber->resume();
+echo 'main:' . Context::getCurrent()->get($key), PHP_EOL;
+
+$scope->detach();
+?>
+--EXPECT--
+main:main
+fiber:fiber
+main:main
+fiber:fiber
+main:main