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

Issue:
Jaeger thrift exporter exports spanIds as "7fffffff" for some IDs
The Jaeger `SpanConverter` converts the hexadecimal `spanId` to an integer [here](https://github.com/open-telemetry/opentelemetry-php/blob/2b72c92fcb30ed241c64c2bea613e7d3bb6ac7cb/src/Contrib/Jaeger/SpanConverter.php#L103), which is then, according to the [thrift span specification](https://github.com/open-telemetry/opentelemetry-php/blob/2b72c92fcb30ed241c64c2bea613e7d3bb6ac7cb/thrift/jaeger/SpanRef.php#L42), converted to a 64-bit **signed** integer.

The problem is that intval will interpret the hexadecimal `spanId` as an **unsigned** integer. Thus any ID that starts with `8` or greater will get capped at the maximum 64-bit signed integer value of `7FFFFFFF`.

I guess this also affects `traceId`s.

Pull request:
Fix span and trace ids to be the correct unsigned ints they need to be when converted for Thrift
Resolves #631 or at least attempts to...

FYI I still haven't functionally tested any of the Jaeger changes yet, so this is mostly going off of the issue discussion + a re-reading of the spec section (mentioned in a code comment) and re-visiting my understanding of how Go and Java handles this

Patch:
diff --git a/.phan/config.php b/.phan/config.php
index 5cfe547f..cddfb637 100644
--- a/.phan/config.php
+++ b/.phan/config.php
@@ -370,7 +370,8 @@ return [
         'vendor/phpunit/phpunit/src',
         'vendor/promphp/prometheus_client_php/src',
         'vendor/google/protobuf/src',
-        'vendor/nyholm/dsn/src'
+        'vendor/nyholm/dsn/src',
+        'vendor/brick/math/src',
     ],
 
     // A list of individual files to include in analysis
diff --git a/composer.json b/composer.json
index 6cc18eb5..5c4326f6 100644
--- a/composer.json
+++ b/composer.json
@@ -8,10 +8,11 @@
     "require": {
         "php": "^7.4 || ^8.0",
         "ext-json": "*",
-        "packaged/thrift": "^0.15.0",
+        "brick/math": "^0.9.3",
         "google/protobuf": "^3.3.0",
         "grpc/grpc": "^1.30",
         "nyholm/dsn": "^2.0.0",
+        "packaged/thrift": "^0.15.0",
         "php-http/async-client-implementation": "^1.0",
         "php-http/discovery": "^1.14",
         "promphp/prometheus_client_php": "^2.2.1",
diff --git a/src/Contrib/Jaeger/IdConverter.php b/src/Contrib/Jaeger/IdConverter.php
new file mode 100644
index 00000000..0f2fdd3c
--- /dev/null
+++ b/src/Contrib/Jaeger/IdConverter.php
@@ -0,0 +1,45 @@
+<?php
+
+declare(strict_types=1);
+
+namespace OpenTelemetry\Contrib\Jaeger;
+
+use Brick\Math\BigInteger;
+
+/**
+ * An implementation of the section of the spec around converting OTEL's span/trace id representations to Thrift's format
+ * https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk_exporters/jaeger.md#ids
+ */
+class IdConverter
+{
+    const HEX_STR_FOR_2_TO_THE_POWER_63 = '8000000000000000';
+
+    public static function convertOtelToJaegerTraceIds(string $traceId): array
+    {
+        $traceIdLow = self::convertHexStringToSignedInt(substr($traceId, 0, 16));
+        $traceIdHigh = self::convertHexStringToSignedInt(substr($traceId, 16, 32));
+
+        return [
+            'traceIdLow' => $traceIdLow,
+            'traceIdHigh' => $traceIdHigh,
+        ];
+    }
+
+    public static function convertOtelToJaegerSpanId(string $spanId): int
+    {
+        return self::convertHexStringToSignedInt($spanId);
+    }
+
+    private static function convertHexStringToSignedInt(string $hexString): int
+    {
+        //This is using high precision arithmetic because PHP doesn't support integers greater than 2^63 - 1 (PHP_INT_MAX on a 64-bit system)
+        //But the input 16-digit hexadecimal number can represent any number between 0 and 2^64 - 1, which built-ins like intval can't handle without capping outputs at 2^63 - 1
+        $hexStringAsInteger = BigInteger::fromBase($hexString, 16);
+
+        $shiftedInteger = $hexStringAsInteger->minus(BigInteger::fromBase(self::HEX_STR_FOR_2_TO_THE_POWER_63, 16));
+
+        $signedInt = $shiftedInteger->toInt();
+
+        return $signedInt;
+    }
+}
diff --git a/src/Contrib/Jaeger/SpanConverter.php b/src/Contrib/Jaeger/SpanConverter.php
index b981a51b..d1bfadde 100644
--- a/src/Contrib/Jaeger/SpanConverter.php
+++ b/src/Contrib/Jaeger/SpanConverter.php
@@ -97,10 +97,10 @@ class SpanConverter implements SpanConverterInterface
         [
             'traceIdLow' => $traceIdLow,
             'traceIdHigh' => $traceIdHigh
-        ] = self::convertOtelToJaegerTraceIds($span->getContext()->getTraceID());
+        ] = IdConverter::convertOtelToJaegerTraceIds($span->getContext()->getTraceID());
 
-        $spanId = intval($span->getContext()->getSpanID(), 16);
-        $parentSpanId = intval($span->getParentSpanId(), 16);
+        $spanId = IdConverter::convertOtelToJaegerSpanId($span->getContext()->getSpanID());
+        $parentSpanId = IdConverter::convertOtelToJaegerSpanId($span->getParentSpanId());
 
         return [
             'traceIdLow' => $traceIdLow,
@@ -110,22 +110,6 @@ class SpanConverter implements SpanConverterInterface
         ];
     }
 
-    private static function convertOtelToJaegerTraceIds(string $traceId): array
-    {
-        $traceIdLow = intval(substr($traceId, 0, 16), 16);
-        $traceIdHigh = intval(substr($traceId, 16, 32), 16);
-
-        return [
-            'traceIdLow' => $traceIdLow,
-            'traceIdHigh' => $traceIdHigh,
-        ];
-    }
-
-    private static function convertOtelToJaegerSpanId(string $spanId): int
-    {
-        return intval($spanId, 16);
-    }
-
     private static function convertOtelSpanKindToJaeger(SpanDataInterface $span): ?string
     {
         switch ($span->getKind()) {
@@ -242,9 +226,9 @@ class SpanConverter implements SpanConverterInterface
         [
             'traceIdLow' => $traceIdLow,
             'traceIdHigh' => $traceIdHigh,
-        ] = self::convertOtelToJaegerTraceIds($link->getSpanContext()->getTraceId());
+        ] = IdConverter::convertOtelToJaegerTraceIds($link->getSpanContext()->getTraceId());
 
-        $integerSpanId = self::convertOtelToJaegerSpanId($link->getSpanContext()->getSpanId());
+        $integerSpanId = IdConverter::convertOtelToJaegerSpanId($link->getSpanContext()->getSpanId());
 
         return new SpanRef([
             'refType' => SpanRefType::FOLLOWS_FROM,


Test patch:
diff --git a/tests/Unit/Contrib/IdConverterTest.php b/tests/Unit/Contrib/IdConverterTest.php
new file mode 100644
index 00000000..076ddb08
--- /dev/null
+++ b/tests/Unit/Contrib/IdConverterTest.php
@@ -0,0 +1,32 @@
+<?php
+
+declare(strict_types=1);
+
+namespace OpenTelemetry\Tests\Contrib\Unit;
+
+use OpenTelemetry\Contrib\Jaeger\IdConverter;
+use PHPUnit\Framework\TestCase;
+
+/**
+ * @covers OpenTelemetry\Contrib\Jaeger\IdConverter
+ */
+class IdConverterTest extends TestCase
+{
+    /**
+     * @dataProvider edgeCaseData
+     */
+    public function test_correctly_converts_edge_cases($spanId, $convertedSpanId)
+    {
+        $this->assertSame($convertedSpanId, IdConverter::convertOtelToJaegerSpanId($spanId));
+    }
+
+    public function edgeCaseData(): iterable
+    {
+        yield '0 -> -2^63' => ['0000000000000000', PHP_INT_MIN];
+        yield '1 -> -2^63 + 1' => ['0000000000000001', -9223372036854775807];
+        yield '2^63 - 1 -> -1' => ['7FFFFFFFFFFFFFFF', -1];
+        yield '2^63 -> 0' => ['8000000000000000', 0];
+        yield '2^64 - 2 -> 2^63 - 2' => ['FFFFFFFFFFFFFFFE', 9223372036854775806];
+        yield '2^64 - 1 -> 2^63 - 1' => ['FFFFFFFFFFFFFFFF', 9223372036854775807];
+    }
+}
diff --git a/tests/Unit/Contrib/JaegerSpanConverterTest.php b/tests/Unit/Contrib/JaegerSpanConverterTest.php
index cace395e..6123fdce 100644
--- a/tests/Unit/Contrib/JaegerSpanConverterTest.php
+++ b/tests/Unit/Contrib/JaegerSpanConverterTest.php
@@ -19,6 +19,7 @@ use PHPUnit\Framework\TestCase;
 
 /**
  * @covers OpenTelemetry\Contrib\Jaeger\SpanConverter
+ * @covers OpenTelemetry\Contrib\Jaeger\IdConverter
  * @covers OpenTelemetry\Contrib\Jaeger\TagFactory\TagFactory
  */
 class JaegerSpanConverterTest extends TestCase
@@ -30,10 +31,10 @@ class JaegerSpanConverterTest extends TestCase
 
         [$convertedSpan] = (new SpanConverter())->convert([$span]);
 
-        $this->assertSame(0, $convertedSpan->traceIdLow);
-        $this->assertSame(0, $convertedSpan->traceIdHigh);
-        $this->assertSame(0, $convertedSpan->spanId);
-        $this->assertSame(0, $convertedSpan->parentSpanId);
+        $this->assertSame(PHP_INT_MIN, $convertedSpan->traceIdLow);
+        $this->assertSame(PHP_INT_MIN, $convertedSpan->traceIdHigh);
+        $this->assertSame(PHP_INT_MIN, $convertedSpan->spanId);
+        $this->assertSame(PHP_INT_MIN, $convertedSpan->parentSpanId);
         $this->assertSame('otelSpanName', $convertedSpan->operationName);
         $this->assertSame([], $convertedSpan->references);
         $this->assertSame(0, $convertedSpan->flags);
@@ -226,8 +227,8 @@ class JaegerSpanConverterTest extends TestCase
         [$convertedSpan] = (new SpanConverter())->convert([$span]);
 
         $this->assertSame(1, $convertedSpan->references[0]->refType);
-        $this->assertSame(0, $convertedSpan->references[0]->traceIdLow);
-        $this->assertSame(0, $convertedSpan->references[0]->traceIdHigh);
-        $this->assertSame(0, $convertedSpan->references[0]->spanId);
+        $this->assertSame(PHP_INT_MIN, $convertedSpan->references[0]->traceIdLow);
+        $this->assertSame(PHP_INT_MIN, $convertedSpan->references[0]->traceIdHigh);
+        $this->assertSame(PHP_INT_MIN, $convertedSpan->references[0]->spanId);
     }
 }