Make span exporters spec compliant (#450)

* Update `SpanExporterInterface` Status constants to better leverage psalm Implement `forceFlush` for all exporters * Make CI happy * Spec said Export not Shutdown
open-telemetry · Oct 13, 2021 · f5f57f7 · f5f57f7
1 parent ef2ccc4
commit f5f57f7
Show file tree

Hide file tree

Showing 13 changed files with 198 additions and 102 deletions.
diff --git a/src/Contrib/Jaeger/Exporter.php b/src/Contrib/Jaeger/Exporter.php
@@ -10,6 +10,7 @@
 
 class Exporter extends Zipkin\Exporter
 {
+    /** @inheritDoc */
     public static function fromConnectionString(string $endpointUrl, string $name, $args = null)
     {
         $factory = new HttpFactory();

diff --git a/src/Contrib/Newrelic/Exporter.php b/src/Contrib/Newrelic/Exporter.php
@@ -108,11 +108,11 @@ public function __construct(
     public function export(iterable $spans): int
     {
         if (!$this->running) {
-            return Trace\SpanExporterInterface::FAILED_NOT_RETRYABLE;
+            return self::STATUS_FAILED_NOT_RETRYABLE;
         }
 
         if (empty($spans)) {
-            return Trace\SpanExporterInterface::SUCCESS;
+            return self::STATUS_SUCCESS;
         }
 
         $convertedSpans = [];
@@ -136,29 +136,37 @@ public function export(iterable $spans): int
 
             $response = $this->client->sendRequest($request);
         } catch (RequestExceptionInterface $e) {
-            return Trace\SpanExporterInterface::FAILED_NOT_RETRYABLE;
+            return self::STATUS_FAILED_NOT_RETRYABLE;
         } catch (NetworkExceptionInterface | ClientExceptionInterface $e) {
-            return Trace\SpanExporterInterface::FAILED_RETRYABLE;
+            return self::STATUS_FAILED_RETRYABLE;
         }
 
         if ($response->getStatusCode() >= 400 && $response->getStatusCode() < 500) {
-            return Trace\SpanExporterInterface::FAILED_NOT_RETRYABLE;
+            return self::STATUS_FAILED_NOT_RETRYABLE;
         }
 
         if ($response->getStatusCode() >= 500 && $response->getStatusCode() < 600) {
-            return Trace\SpanExporterInterface::FAILED_RETRYABLE;
+            return self::STATUS_FAILED_RETRYABLE;
         }
 
-        return Trace\SpanExporterInterface::SUCCESS;
+        return self::STATUS_SUCCESS;
     }
 
+    /** @inheritDoc */
     public function shutdown(): bool
     {
         $this->running = false;
 
         return true;
     }
 
+    /** @inheritDoc */
+    public function forceFlush(): bool
+    {
+        return true;
+    }
+
+    /** @inheritDoc */
     public static function fromConnectionString(string $endpointUrl, string $name, $args)
     {
         if ($args == false) {

diff --git a/src/Contrib/OtlpGrpc/Exporter.php b/src/Contrib/OtlpGrpc/Exporter.php
@@ -126,11 +126,11 @@ public function getClientOptions(): array
     public function export(iterable $spans): int
     {
         if (!$this->running) {
-            return Trace\SpanExporterInterface::FAILED_NOT_RETRYABLE;
+            return self::STATUS_FAILED_NOT_RETRYABLE;
         }
 
         if (empty($spans)) {
-            return Trace\SpanExporterInterface::SUCCESS;
+            return self::STATUS_SUCCESS;
         }
 
         $resourcespans = [$this->spanConverter->as_otlp_resource_span($spans)];
@@ -142,7 +142,7 @@ public function export(iterable $spans): int
         [$response, $status] = $this->client->Export($request)->wait();
 
         if ($status->code === \Grpc\STATUS_OK) {
-            return Trace\SpanExporterInterface::SUCCESS;
+            return self::STATUS_SUCCESS;
         }
 
         if (in_array($status->code, [
@@ -155,11 +155,11 @@ public function export(iterable $spans): int
             \Grpc\STATUS_UNAVAILABLE,
             \Grpc\STATUS_DATA_LOSS,
             \Grpc\STATUS_UNAUTHENTICATED,
-        ])) {
-            return Trace\SpanExporterInterface::FAILED_RETRYABLE;
+        ], true)) {
+            return self::STATUS_FAILED_RETRYABLE;
         }
 
-        return Trace\SpanExporterInterface::FAILED_NOT_RETRYABLE;
+        return self::STATUS_FAILED_NOT_RETRYABLE;
     }
 
     public function setHeader($key, $value)
@@ -196,19 +196,23 @@ public function metadataFromHeaders($headers): array
         return $metadata;
     }
 
+    /** @inheritDoc */
     public function shutdown(): bool
     {
         $this->running = false;
 
         return true;
     }
 
+    /** @inheritDoc */
+    public function forceFlush(): bool
+    {
+        return true;
+    }
+
+    /** @inheritDoc */
     public static function fromConnectionString(string $endpointUrl = null, string $name = null, $args = null)
     {
         return new Exporter();
     }
-    // public function getHeaders()
-    // {
-    //     return $this->metadataFromHeaders($this->headers);
-    // }
 }
diff --git a/src/Contrib/OtlpHttp/Exporter.php b/src/Contrib/OtlpHttp/Exporter.php
@@ -124,11 +124,11 @@ public function __construct(
     public function export(iterable $spans): int
     {
         if (!$this->running) {
-            return Trace\SpanExporterInterface::FAILED_NOT_RETRYABLE;
+            return self::STATUS_FAILED_NOT_RETRYABLE;
         }
 
         if (empty($spans)) {
-            return Trace\SpanExporterInterface::SUCCESS;
+            return self::STATUS_SUCCESS;
         }
 
         $resourcespans = [$this->spanConverter->as_otlp_resource_span($spans)];
@@ -160,20 +160,20 @@ public function export(iterable $spans): int
 
             $response = $this->client->sendRequest($request);
         } catch (RequestExceptionInterface $e) {
-            return Trace\SpanExporterInterface::FAILED_NOT_RETRYABLE;
+            return self::STATUS_FAILED_NOT_RETRYABLE;
         } catch (NetworkExceptionInterface | ClientExceptionInterface $e) {
-            return Trace\SpanExporterInterface::FAILED_RETRYABLE;
+            return self::STATUS_FAILED_RETRYABLE;
         }
 
         if ($response->getStatusCode() >= 400 && $response->getStatusCode() < 500) {
-            return Trace\SpanExporterInterface::FAILED_NOT_RETRYABLE;
+            return self::STATUS_FAILED_NOT_RETRYABLE;
         }
 
         if ($response->getStatusCode() >= 500 && $response->getStatusCode() < 600) {
-            return Trace\SpanExporterInterface::FAILED_RETRYABLE;
+            return self::STATUS_FAILED_RETRYABLE;
         }
 
-        return Trace\SpanExporterInterface::SUCCESS;
+        return self::STATUS_SUCCESS;
     }
 
     /**
@@ -203,13 +203,21 @@ public function processHeaders($headers): array
         return $metadata;
     }
 
+    /** @inheritDoc */
     public function shutdown(): bool
     {
         $this->running = false;
 
         return true;
     }
 
+    /** @inheritDoc */
+    public function forceFlush(): bool
+    {
+        return true;
+    }
+
+    /** @inheritDoc */
     public static function fromConnectionString(string $endpointUrl = null, string $name = null, $args = null)
     {
         $factory = new HttpFactory();

diff --git a/src/Contrib/Zipkin/Exporter.php b/src/Contrib/Zipkin/Exporter.php
@@ -79,11 +79,11 @@ public function __construct(
     public function export(iterable $spans): int
     {
         if (!$this->running) {
-            return Trace\SpanExporterInterface::FAILED_NOT_RETRYABLE;
+            return self::STATUS_FAILED_NOT_RETRYABLE;
         }
 
         if (empty($spans)) {
-            return Trace\SpanExporterInterface::SUCCESS;
+            return self::STATUS_SUCCESS;
         }
 
         $convertedSpans = [];
@@ -100,29 +100,37 @@ public function export(iterable $spans): int
 
             $response = $this->client->sendRequest($request);
         } catch (RequestExceptionInterface $e) {
-            return Trace\SpanExporterInterface::FAILED_NOT_RETRYABLE;
+            return self::STATUS_FAILED_NOT_RETRYABLE;
         } catch (NetworkExceptionInterface | ClientExceptionInterface $e) {
-            return Trace\SpanExporterInterface::FAILED_RETRYABLE;
+            return self::STATUS_FAILED_RETRYABLE;
         }
 
         if ($response->getStatusCode() >= 400 && $response->getStatusCode() < 500) {
-            return Trace\SpanExporterInterface::FAILED_NOT_RETRYABLE;
+            return self::STATUS_FAILED_NOT_RETRYABLE;
         }
 
         if ($response->getStatusCode() >= 500 && $response->getStatusCode() < 600) {
-            return Trace\SpanExporterInterface::FAILED_RETRYABLE;
+            return self::STATUS_FAILED_RETRYABLE;
         }
 
-        return Trace\SpanExporterInterface::SUCCESS;
+        return self::STATUS_SUCCESS;
     }
 
+    /** @inheritDoc */
     public function shutdown(): bool
     {
         $this->running = false;
 
         return true;
     }
 
+    /** @inheritDoc */
+    public function forceFlush(): bool
+    {
+        return true;
+    }
+
+    /** @inheritDoc */
     public static function fromConnectionString(string $endpointUrl, string $name, $args = null)
     {
         $factory = new HttpFactory();

diff --git a/src/Contrib/ZipkinToNewrelic/Exporter.php b/src/Contrib/ZipkinToNewrelic/Exporter.php
@@ -101,11 +101,11 @@ public function __construct(
     public function export(iterable $spans): int
     {
         if (!$this->running) {
-            return Exporter::FAILED_NOT_RETRYABLE;
+            return self::STATUS_FAILED_NOT_RETRYABLE;
         }
 
         if (empty($spans)) {
-            return Trace\SpanExporterInterface::SUCCESS;
+            return self::STATUS_SUCCESS;
         }
 
         $convertedSpans = [];
@@ -125,29 +125,36 @@ public function export(iterable $spans): int
 
             $response = $this->client->sendRequest($request);
         } catch (RequestExceptionInterface $e) {
-            return Trace\SpanExporterInterface::FAILED_NOT_RETRYABLE;
+            return self::STATUS_FAILED_NOT_RETRYABLE;
         } catch (NetworkExceptionInterface | ClientExceptionInterface $e) {
-            return Trace\SpanExporterInterface::FAILED_RETRYABLE;
+            return self::STATUS_FAILED_RETRYABLE;
         }
 
         if ($response->getStatusCode() >= 400 && $response->getStatusCode() < 500) {
-            return Trace\SpanExporterInterface::FAILED_NOT_RETRYABLE;
+            return self::STATUS_FAILED_NOT_RETRYABLE;
         }
 
         if ($response->getStatusCode() >= 500 && $response->getStatusCode() < 600) {
-            return Trace\SpanExporterInterface::FAILED_RETRYABLE;
+            return self::STATUS_FAILED_RETRYABLE;
         }
 
-        return Trace\SpanExporterInterface::SUCCESS;
+        return self::STATUS_SUCCESS;
     }
 
+    /** @inheritDoc */
     public function shutdown(): bool
     {
         $this->running = false;
 
         return true;
     }
 
+    public function forceFlush(): bool
+    {
+        return true;
+    }
+
+    /** @inheritDoc */
     public static function fromConnectionString(string $endpointUrl, string $name, $args)
     {
         if ($args == false) {

diff --git a/src/SDK/Trace/SpanExporterInterface.php b/src/SDK/Trace/SpanExporterInterface.php
@@ -6,17 +6,15 @@
 
 /**
  * @see https://github.com/open-telemetry/opentelemetry-specification/blob/v1.7.0/specification/trace/sdk.md#span-exporter
- *
- * @todo: Define `forceFlush`: https://github.com/open-telemetry/opentelemetry-specification/blob/v1.7.0/specification/trace/sdk.md#forceflush-2
  */
 interface SpanExporterInterface
 {
     /**
      * Possible return values as outlined in the OpenTelemetry spec
      */
-    const SUCCESS = 0;
-    const FAILED_NOT_RETRYABLE = 1;
-    const FAILED_RETRYABLE = 2;
+    public const STATUS_SUCCESS = 0;
+    public const STATUS_FAILED_NOT_RETRYABLE = 1;
+    public const STATUS_FAILED_RETRYABLE = 2;
 
     public static function fromConnectionString(string $endpointUrl, string $name, string $args);
 
@@ -25,11 +23,13 @@ public static function fromConnectionString(string $endpointUrl, string $name, s
      *
      * @see https://github.com/open-telemetry/opentelemetry-specification/blob/v1.7.0/specification/trace/sdk.md#exportbatch
      *
-     * @todo: Update return type to be STATUS_* versus a plain `int`.
-     * @return int
+     * @psalm-return SpanExporterInterface::STATUS_*
      */
     public function export(iterable $spans): int;
 
     /** @see https://github.com/open-telemetry/opentelemetry-specification/blob/v1.7.0/specification/trace/sdk.md#shutdown-2 */
     public function shutdown(): bool;
+
+    /** @see https://github.com/open-telemetry/opentelemetry-specification/blob/v1.7.0/specification/trace/sdk.md#forceflush-2 */
+    public function forceFlush(): bool;
 }
diff --git a/tests/Contrib/Unit/JaegerExporterTest.php b/tests/Contrib/Unit/JaegerExporterTest.php
@@ -13,6 +13,16 @@
 
 class JaegerExporterTest extends TestCase
 {
+    public function test_shutdown(): void
+    {
+        $this->assertTrue((new Exporter('test.jaeger', 'https://localhost:1234/foo', new Client(), new HttpFactory(), new HttpFactory()))->shutdown());
+    }
+
+    public function test_forceFlush(): void
+    {
+        $this->assertTrue((new Exporter('test.jaeger', 'https://localhost:1234/foo', new Client(), new HttpFactory(), new HttpFactory()))->forceFlush());
+    }
+
     /**
      * @test
      * @dataProvider invalidDsnDataProvider
@@ -54,6 +64,6 @@ public function failsIfNotRunning()
         $span = $this->createMock(SpanData::class);
         $exporter->shutdown();
 
-        $this->assertSame(Exporter::FAILED_NOT_RETRYABLE, $exporter->export([$span]));
+        $this->assertSame(Exporter::STATUS_FAILED_NOT_RETRYABLE, $exporter->export([$span]));
     }
 }