Repository: open-telemetry/opentelemetry-php (php)

Issue:
RuntimeException(code: 400)
Following example https://github.com/open-telemetry/opentelemetry-php/blob/main/examples/traces/exporters/otlp_http_json.php
fails with following issue 
```
[2023-04-12T12:18:06.647406+00:00] otlp-example.ERROR: Export failure {"exception":"[object] (RuntimeException(code: 400): Bad Request at /Users/pdelewski/Projects/agent-apm-php/vendor/open-telemetry/sdk/Common/Export/Http/PsrTransport.php:106
```
It seems to be related to incorrect format of `trace_id` and `span_id` 

Pull request description:
Fix otlp json trace id and span id
Fixes #969, alternative to #971; uses the approach mentioned in https://github.com/open-telemetry/opentelemetry-php/pull/971#issuecomment-1509849033.

Patch:
diff --git a/src/Contrib/Otlp/LogsConverter.php b/src/Contrib/Otlp/LogsConverter.php
index df980cb3..07888769 100644
--- a/src/Contrib/Otlp/LogsConverter.php
+++ b/src/Contrib/Otlp/LogsConverter.php
@@ -18,6 +18,13 @@ use OpenTelemetry\SDK\Resource\ResourceInfo;
 
 class LogsConverter
 {
+    private ProtobufSerializer $serializer;
+
+    public function __construct(?ProtobufSerializer $serializer = null)
+    {
+        $this->serializer = $serializer ?? ProtobufSerializer::getDefault();
+    }
+
     /**
      * @param iterable<ReadableLogRecord> $logs
      * @psalm-suppress InvalidArgument
@@ -76,8 +83,8 @@ class LogsConverter
         $pLogRecord->setObservedTimeUnixNano($record->getObservedTimestamp() ?? 0);
         $spanContext = $record->getSpanContext();
         if ($spanContext !== null && $spanContext->isValid()) {
-            $pLogRecord->setTraceId($spanContext->getTraceIdBinary());
-            $pLogRecord->setSpanId($spanContext->getSpanIdBinary());
+            $pLogRecord->setTraceId($this->serializer->serializeTraceId($spanContext->getTraceIdBinary()));
+            $pLogRecord->setSpanId($this->serializer->serializeSpanId($spanContext->getSpanIdBinary()));
             $pLogRecord->setFlags($spanContext->getTraceFlags());
         }
         $severityNumber = $record->getSeverityNumber();
diff --git a/src/Contrib/Otlp/LogsExporter.php b/src/Contrib/Otlp/LogsExporter.php
index 36855a4a..fb100391 100644
--- a/src/Contrib/Otlp/LogsExporter.php
+++ b/src/Contrib/Otlp/LogsExporter.php
@@ -42,7 +42,7 @@ class LogsExporter implements LogRecordExporterInterface
     public function export(iterable $batch, ?CancellationInterface $cancellation = null): FutureInterface
     {
         return $this->transport
-            ->send($this->serializer->serialize((new LogsConverter())->convert($batch)), $cancellation)
+            ->send($this->serializer->serialize((new LogsConverter($this->serializer))->convert($batch)), $cancellation)
             ->map(function (?string $payload): bool {
                 if ($payload === null) {
                     return true;
diff --git a/src/Contrib/Otlp/MetricConverter.php b/src/Contrib/Otlp/MetricConverter.php
index 868276f5..584c4136 100644
--- a/src/Contrib/Otlp/MetricConverter.php
+++ b/src/Contrib/Otlp/MetricConverter.php
@@ -27,6 +27,13 @@ use function serialize;
 
 final class MetricConverter
 {
+    private ProtobufSerializer $serializer;
+
+    public function __construct(?ProtobufSerializer $serializer = null)
+    {
+        $this->serializer = $serializer ?? ProtobufSerializer::getDefault();
+    }
+
     /**
      * @param iterable<SDK\Metrics\Data\Metric> $batch
      */
@@ -218,8 +225,8 @@ final class MetricConverter
         $pExemplar = new Exemplar();
         $this->setFilteredAttributes($pExemplar, $exemplar->attributes);
         $pExemplar->setTimeUnixNano($exemplar->timestamp);
-        $pExemplar->setSpanId(hex2bin((string) $exemplar->spanId));
-        $pExemplar->setTraceId(hex2bin((string) $exemplar->traceId));
+        $pExemplar->setSpanId($this->serializer->serializeSpanId(hex2bin((string) $exemplar->spanId)));
+        $pExemplar->setTraceId($this->serializer->serializeTraceId(hex2bin((string) $exemplar->traceId)));
         if (is_int($exemplar->value)) {
             $pExemplar->setAsInt($exemplar->value);
         }
diff --git a/src/Contrib/Otlp/MetricExporter.php b/src/Contrib/Otlp/MetricExporter.php
index 2f5a2a39..d9f26fd5 100644
--- a/src/Contrib/Otlp/MetricExporter.php
+++ b/src/Contrib/Otlp/MetricExporter.php
@@ -52,7 +52,7 @@ final class MetricExporter implements MetricExporterInterface
     public function export(iterable $batch): bool
     {
         return $this->transport
-            ->send($this->serializer->serialize((new MetricConverter())->convert($batch)))
+            ->send($this->serializer->serialize((new MetricConverter($this->serializer))->convert($batch)))
             ->map(function (?string $payload): bool {
                 if ($payload === null) {
                     return true;
diff --git a/src/Contrib/Otlp/ProtobufSerializer.php b/src/Contrib/Otlp/ProtobufSerializer.php
index 076f643a..c244d006 100644
--- a/src/Contrib/Otlp/ProtobufSerializer.php
+++ b/src/Contrib/Otlp/ProtobufSerializer.php
@@ -5,6 +5,8 @@ declare(strict_types=1);
 namespace OpenTelemetry\Contrib\Otlp;
 
 use AssertionError;
+use function base64_decode;
+use function bin2hex;
 use Exception;
 use Google\Protobuf\Internal\Message;
 use InvalidArgumentException;
@@ -29,6 +31,11 @@ final class ProtobufSerializer
         $this->contentType = $contentType;
     }
 
+    public static function getDefault(): ProtobufSerializer
+    {
+        return new self(self::PROTOBUF);
+    }
+
     /**
      * @psalm-param TransportInterface<SUPPORTED_CONTENT_TYPES> $transport
      */
@@ -44,6 +51,32 @@ final class ProtobufSerializer
         }
     }
 
+    public function serializeTraceId(string $traceId): string
+    {
+        switch ($this->contentType) {
+            case self::PROTOBUF:
+                return $traceId;
+            case self::JSON:
+            case self::NDJSON:
+                return base64_decode(bin2hex($traceId));
+            default:
+                throw new AssertionError();
+        }
+    }
+
+    public function serializeSpanId(string $spanId): string
+    {
+        switch ($this->contentType) {
+            case self::PROTOBUF:
+                return $spanId;
+            case self::JSON:
+            case self::NDJSON:
+                return base64_decode(bin2hex($spanId));
+            default:
+                throw new AssertionError();
+        }
+    }
+
     public function serialize(Message $message): string
     {
         switch ($this->contentType) {
@@ -71,7 +104,8 @@ final class ProtobufSerializer
                 break;
             case self::JSON:
             case self::NDJSON:
-                $message->mergeFromJsonString($payload);
+                // @phan-suppress-next-line PhanParamTooManyInternal
+                $message->mergeFromJsonString($payload, true);
 
                 break;
             default:
diff --git a/src/Contrib/Otlp/SpanConverter.php b/src/Contrib/Otlp/SpanConverter.php
index 99fee406..1a8b4369 100644
--- a/src/Contrib/Otlp/SpanConverter.php
+++ b/src/Contrib/Otlp/SpanConverter.php
@@ -26,6 +26,13 @@ use function spl_object_id;
 
 final class SpanConverter
 {
+    private ProtobufSerializer $serializer;
+
+    public function __construct(?ProtobufSerializer $serializer = null)
+    {
+        $this->serializer = $serializer ?? ProtobufSerializer::getDefault();
+    }
+
     public function convert(iterable $spans): ExportTraceServiceRequest
     {
         $pExportTraceServiceRequest = new ExportTraceServiceRequest();
@@ -139,11 +146,11 @@ final class SpanConverter
     private function convertSpan(SpanDataInterface $span): Span
     {
         $pSpan = new Span();
-        $pSpan->setTraceId($span->getContext()->getTraceIdBinary());
-        $pSpan->setSpanId($span->getContext()->getSpanIdBinary());
+        $pSpan->setTraceId($this->serializer->serializeTraceId($span->getContext()->getTraceIdBinary()));
+        $pSpan->setSpanId($this->serializer->serializeSpanId($span->getContext()->getSpanIdBinary()));
         $pSpan->setTraceState((string) $span->getContext()->getTraceState());
         if ($span->getParentContext()->isValid()) {
-            $pSpan->setParentSpanId($span->getParentContext()->getSpanIdBinary());
+            $pSpan->setParentSpanId($this->serializer->serializeSpanId($span->getParentContext()->getSpanIdBinary()));
         }
         $pSpan->setName($span->getName());
         $pSpan->setKind($this->convertSpanKind($span->getKind()));
@@ -163,8 +170,8 @@ final class SpanConverter
         foreach ($span->getLinks() as $link) {
             /** @psalm-suppress InvalidArgument */
             $pSpan->getLinks()[] = $pLink = new Link();
-            $pLink->setTraceId($link->getSpanContext()->getTraceIdBinary());
-            $pLink->setSpanId($link->getSpanContext()->getSpanIdBinary());
+            $pLink->setTraceId($this->serializer->serializeTraceId($link->getSpanContext()->getTraceIdBinary()));
+            $pLink->setSpanId($this->serializer->serializeSpanId($link->getSpanContext()->getSpanIdBinary()));
             $pLink->setTraceState((string) $link->getSpanContext()->getTraceState());
             $this->setAttributes($pLink, $link->getAttributes());
         }
diff --git a/src/Contrib/Otlp/SpanExporter.php b/src/Contrib/Otlp/SpanExporter.php
index 5ef2e4c3..a496206f 100644
--- a/src/Contrib/Otlp/SpanExporter.php
+++ b/src/Contrib/Otlp/SpanExporter.php
@@ -38,7 +38,7 @@ final class SpanExporter implements SpanExporterInterface
     public function export(iterable $batch, ?CancellationInterface $cancellation = null): FutureInterface
     {
         return $this->transport
-            ->send($this->serializer->serialize((new SpanConverter())->convert($batch)), $cancellation)
+            ->send($this->serializer->serialize((new SpanConverter($this->serializer))->convert($batch)), $cancellation)
             ->map(function (?string $payload): bool {
                 if ($payload === null) {
                     return true;


Test patch:
diff --git a/tests/Unit/Contrib/Otlp/SpanExporterTest.php b/tests/Unit/Contrib/Otlp/SpanExporterTest.php
index f14db6d7..035d3fda 100644
--- a/tests/Unit/Contrib/Otlp/SpanExporterTest.php
+++ b/tests/Unit/Contrib/Otlp/SpanExporterTest.php
@@ -4,12 +4,17 @@ declare(strict_types=1);
 
 namespace OpenTelemetry\Tests\Unit\Contrib\Otlp;
 
+use function fseek;
+use OpenTelemetry\API\Trace\SpanContext;
 use OpenTelemetry\Contrib\Otlp\SpanExporter;
+use OpenTelemetry\SDK\Common\Export\Stream\StreamTransport;
 use OpenTelemetry\SDK\Common\Export\TransportInterface;
 use OpenTelemetry\SDK\Common\Future\CompletedFuture;
 use OpenTelemetry\SDK\Common\Future\ErrorFuture;
+use OpenTelemetry\Tests\Unit\SDK\Util\SpanData;
 use PHPUnit\Framework\MockObject\MockObject;
 use PHPUnit\Framework\TestCase;
+use function stream_get_contents;
 
 /**
  * @covers \OpenTelemetry\Contrib\Otlp\SpanExporter
@@ -61,4 +66,42 @@ class SpanExporterTest extends TestCase
         $this->transport->expects($this->once())->method('forceFlush');
         $this->exporter->forceFlush();
     }
+
+    public function test_json_span_and_trace_id_hex_format(): void
+    {
+        $stream = fopen('php://memory', 'a+b');
+        $transport = new StreamTransport($stream, 'application/json');
+        $exporter = new SpanExporter($transport);
+
+        $exporter->export([
+            (new SpanData())->setContext(SpanContext::create('0af7651916cd43dd8448eb211c80319c', 'b7ad6b7169203331')),
+        ]);
+
+        fseek($stream, 0);
+        $this->assertJsonStringEqualsJsonString(<<<TRACE
+            {
+                "resourceSpans": [
+                    {
+                        "resource": {},
+                        "scopeSpans": [
+                            {
+                                "scope": {},
+                                "spans": [
+                                    {
+                                        "traceId": "0af7651916cd43dd8448eb211c80319c",
+                                        "spanId": "b7ad6b7169203331",
+                                        "name": "test-span-data",
+                                        "kind": "SPAN_KIND_INTERNAL",
+                                        "startTimeUnixNano": "1505855794194009601",
+                                        "endTimeUnixNano": "1505855799465726528",
+                                        "status": {}
+                                    }
+                                ]
+                            }
+                        ]
+                    }
+                ]
+            }
+            TRACE, stream_get_contents($stream));
+    }
 }