Fix W3C test service

src/API/Trace/Propagation/TraceContextPropagator.php

@@ -93,20 +93,32 @@ private static function extractImpl($carrier, PropagationGetterInterface $getter
             return SpanContext::getInvalid();
         }
 
-        // Traceparent = {version}-{trace-id}-{parent-id}-{trace-flags}
+        // traceParent = {version}-{trace-id}-{parent-id}-{trace-flags}
         $pieces = explode('-', $traceparent);
 
-        // Unable to extract traceparent. Expected 4 values
-        if (count($pieces) !== 4) {
+        // If the header does not have at least 4 pieces, it is invalid -- restart the trace.
+        if (count($pieces) < 4) {
             return SpanContext::getInvalid();
         }
 
         [$version, $traceId, $spanId, $traceFlags] = $pieces;
 
-        // Validates the version, traceId, spanId and traceFlags
-        // Returns an invalid spanContext if any of the checks fail
-        if ($version !== self::VERSION || !SpanContext::isValidTraceId($traceId) ||
-            !SpanContext::isValidSpanId($spanId) || !SpanContext::isValidTraceFlag($traceFlags)) {
+        /**
+         * Return invalid if:
+         * - Version is invalid (not 2 char hex or 'ff')
+         * - Trace version, trace ID, span ID or trace flag are invalid
+         */
+        if (!SpanContext::isValidTraceVersion($version)
+            || !SpanContext::isValidTraceId($traceId)
+            || !SpanContext::isValidSpanId($spanId)
+            || !SpanContext::isValidTraceFlag($traceFlags)
+        ) {
+            return SpanContext::getInvalid();
+        }
+
+        // Return invalid if the trace version is not a future version but still has > 4 pieces.
+        $versionIsFuture = hexdec($version) > hexdec(self::VERSION);
+        if (count($pieces) > 4 && !$versionIsFuture) {
             return SpanContext::getInvalid();
         }
 

src/API/Trace/SpanContext.php

@@ -11,6 +11,7 @@
 final class SpanContext implements API\SpanContextInterface
 {
     public const INVALID_TRACE = '00000000000000000000000000000000';
+    public const TRACE_VERSION_REGEX = '/^(?!ff)[\da-f]{2}$/';
     public const VALID_TRACE = '/^[0-9a-f]{32}$/';
     public const TRACE_LENGTH = 32;
     public const INVALID_SPAN = '0000000000000000';
@@ -56,6 +57,15 @@ public static function getInvalid(): API\SpanContextInterface
         return self::$invalidContext;
     }
 
+    /**
+     * @param string $traceVersion
+     * @return bool Returns a value that indicates whether a trace version is valid.
+     */
+    public static function isValidTraceVersion(string $traceVersion): bool
+    {
+        return 1 === preg_match(self::TRACE_VERSION_REGEX, $traceVersion);
+    }
+
     /**
      * @return bool Returns a value that indicates whether a trace id is valid
      */

src/API/Trace/TraceState.php

@@ -7,6 +7,7 @@
 use function array_reverse;
 use function array_walk;
 use function implode;
+use function strlen;
 
 class TraceState implements TraceStateInterface
 {
@@ -20,6 +21,8 @@ class TraceState implements TraceStateInterface
     private const VALID_KEY_REGEX = '/^(?:' . self::VALID_KEY . '|' . self::VALID_VENDOR_KEY . ')$/';
     private const VALID_VALUE_BASE_REGEX = '/^[ -~]{0,255}[!-~]$/';
     private const INVALID_VALUE_COMMA_EQUAL_REGEX = '/,|=/';
+    private const SERVER_CONCAT_HEADERS_REGEX = '/;(?=[^,=;]*=|$)/';
+    private const TRAILING_LEADING_SEPARATOR_REGEX = '/^' . self::LIST_MEMBERS_SEPARATOR . '+|' . self::LIST_MEMBERS_SEPARATOR . '+$/';
 
     /**
      * @var string[]
@@ -134,16 +137,29 @@ private function parse(string $rawTracestate): array
     {
         $parsedTracestate = [];
 
-        if (\strlen($rawTracestate) <= self::MAX_TRACESTATE_LENGTH) {
-            $listMembers = explode(self::LIST_MEMBERS_SEPARATOR, $rawTracestate);
+        /**
+         * Some servers concatenate multiple headers with ';' -- we need to replace these with ','
+         * This is still a workaround and doesn't get around the problem fully, specifically it doesn't
+         * handle edge cases where the header has a trailing ';' or an empty trace state.
+         * We also need to trim trailing separators from the header, found when a header is empty.
+         * @var string $sanitizedTraceState
+         */
+        $sanitizedTraceState = (function () use ($rawTracestate) {
+            $fixedSeparators = preg_replace(self::SERVER_CONCAT_HEADERS_REGEX, self::LIST_MEMBERS_SEPARATOR, $rawTracestate);
 
-            if (count($listMembers) > self::MAX_TRACESTATE_LIST_MEMBERS) {
+            return preg_replace(self::TRAILING_LEADING_SEPARATOR_REGEX, '', $fixedSeparators);
+        })();
+
+        if (strlen($sanitizedTraceState) <= self::MAX_TRACESTATE_LENGTH) {
+            $listMembers = explode(self::LIST_MEMBERS_SEPARATOR, $sanitizedTraceState);
 
-                // Truncate the tracestate if it exceeds the maximum list-members allowed
-                // TODO: Log a message when truncation occurs
-                $listMembers = array_slice($listMembers, 0, self::MAX_TRACESTATE_LIST_MEMBERS);
+            // Discard tracestate if entries exceed max length.
+            if (count($listMembers) > self::MAX_TRACESTATE_LIST_MEMBERS) {
+                // TODO: Log a message when we discard.
+                return [];
             }
 
+            $invalid = false;
             foreach ($listMembers as $listMember) {
                 $vendor = explode(self::LIST_MEMBER_KEY_VALUE_SPLITTER, trim($listMember));
 
@@ -153,8 +169,17 @@ private function parse(string $rawTracestate): array
                     // TODO: Log if we can't validate the key and value
                     if ($this->validateKey($vendor[0]) && $this->validateValue($vendor[1])) {
                         $parsedTracestate[$vendor[0]] = $vendor[1];
+
+                        continue;
                     }
                 }
+                $invalid = true;
+
+                break;
+            }
+            // Discard tracestate if any member is invalid.
+            if ($invalid) {
+                return [];
             }
         }
 

tests/TraceContext/W3CTestService/TestController.php

@@ -4,9 +4,10 @@
 
 namespace App\Controller;
 
-use GuzzleHttp\Client;
 use OpenTelemetry\API\Trace\Propagation\TraceContextPropagator;
-use OpenTelemetry\API\Trace\SpanContext;
+use OpenTelemetry\Context\Context;
+use Symfony\Component\HttpClient\HttpClient;
+use Symfony\Component\HttpClient\Psr18Client;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\Response;
 use Symfony\Component\Routing\Annotation\Route;
@@ -15,39 +16,52 @@ class TestController
 {
     /**
      * @Route("/test")
+     * @throws \Psr\Http\Client\ClientExceptionInterface
      */
     public function index(Request $request): Response
     {
         global $tracer;
+        if (!$tracer) {
+            return new Response('internal error', Response::HTTP_INTERNAL_SERVER_ERROR);
+        }
+
+        $traceCtxPropagator = TraceContextPropagator::getInstance();
+
+        try {
+            $context = $traceCtxPropagator->extract($request->headers->all());
+        } catch (\InvalidArgumentException $th) {
+            $context = new Context();
+        }
 
-        $array = $request->request->all();
         $body = json_decode($request->getContent(), true);
+        if (!$body) {
+            return new Response('Invalid JSON', Response::HTTP_BAD_REQUEST);
+        }
 
         foreach ($body as $case) {
-            if ($tracer) {
-                $headers = ['content-type' => 'application/json'];
-                $url = $case['url'];
-                $arguments = $case['arguments'];
+            $headers = ['content-type' => 'application/json'];
+            $url = $case['url'];
+            $arguments = $case['arguments'];
 
-                try {
-                    $context = TraceContextPropagator::extract($request->headers->all());
-                } catch (\InvalidArgumentException $th) {
-                    $context = SpanContext::generate();
-                }
+            $span = $tracer->spanBuilder($url)->setParent($context)->startSpan();
+            $context = $span->storeInContext($context);
+            $scope = $context->activate();
 
-                $span = $tracer->startAndActivateSpanFromContext($url, $context, true);
-                TraceContextPropagator::inject($carrier, null, $context);
+            $traceCtxPropagator->inject($headers, null, $context);
 
-                $client = new Client([
+            $client = new Psr18Client(HttpClient::create(
+                [
                     'base_uri' => $url,
                     'timeout'  => 2.0,
-                ]);
+                ]
+            ));
+
+            $testServiceRequest = new \Nyholm\Psr7\Request('POST', $url, $headers, json_encode($arguments));
 
-                $testServiceRequest = new \GuzzleHttp\Psr7\Request('POST', $url, $headers, json_encode($arguments));
-                $response = $client->sendRequest($testServiceRequest);
+            $client->sendRequest($testServiceRequest);
 
-                $span->end();
-            }
+            $span->end();
+            $scope->detach();
         }
 
         return new Response(

tests/TraceContext/W3CTestService/index.php

@@ -4,46 +4,41 @@
 require_once __DIR__ . '/../vendor/autoload.php';
 
 use App\Kernel;
-use OpenTelemetry\API\Trace as API;
-use OpenTelemetry\Context\Context;
 use OpenTelemetry\Contrib\Jaeger\Exporter as JaegerExporter;
-use OpenTelemetry\SDK\Common\Time\ClockFactory;
-use OpenTelemetry\SDK\Trace\Sampler\AlwaysOnSampler;
-use OpenTelemetry\SDK\Trace\SamplingResult;
 use OpenTelemetry\SDK\Trace\SpanProcessor\BatchSpanProcessor;
 use OpenTelemetry\SDK\Trace\TracerProvider;
 use Symfony\Component\Dotenv\Dotenv;
+use Symfony\Component\ErrorHandler\Debug;
+use Symfony\Component\HttpClient\Psr18Client;
 use Symfony\Component\HttpFoundation\Request;
 
-(new Dotenv())->bootEnv(dirname(__DIR__) . '/.env');
+// @todo remove
+Debug::enable();
 
-$sampler = new AlwaysOnSampler();
-$samplingResult = $sampler->shouldSample(
-    Context::getCurrent(),
-    md5((string) microtime(true)),
-    'io.opentelemetry.example',
-    API\SpanKind::KIND_INTERNAL
-);
+(new Dotenv())->bootEnv(dirname(__DIR__) . '/.env');
 
-$exporter = new JaegerExporter(
-    'W3C Trace-Context Test Service',
-    'http://localhost:9412/api/v2/spans'
-);
+$httpClient = new Psr18Client();
 
-if (SamplingResult::RECORD_AND_SAMPLE === $samplingResult->getDecision()) {
-    $tracer = (new TracerProvider())
-        ->addSpanProcessor(new BatchSpanProcessor($exporter, ClockFactory::getDefault()))
-        ->getTracer('io.opentelemetry.contrib.php');
+$tracer = (new TracerProvider([
+    new BatchSpanProcessor(
+        new JaegerExporter(
+            'W3C Trace-Context Test Service',
+            'http://localhost:9412/api/v2/spans',
+            $httpClient,
+            $httpClient,
+            $httpClient,
+        )
+    ),
+]))->getTracer('W3C Trace-Context Test Service');
 
-    $request = Request::createFromGlobals();
-    $span = $tracer->startAndActivateSpan($request->getUri());
-}
+$request = Request::createFromGlobals();
+$span = $tracer->spanBuilder($request->getUri())->startSpan();
+$spanScope = $span->activate();
 
 $kernel = new Kernel($_SERVER['APP_ENV'], (bool) $_SERVER['APP_DEBUG']);
 $response = $kernel->handle($request);
 $response->send();
 $kernel->terminate($request, $response);
 
-if (SamplingResult::RECORD_AND_SAMPLED === $samplingResult->getDecision()) {
-    $span->end();
-}
+$span->end();
+$spanScope->detach();

tests/TraceContext/W3CTestService/symfony-setup

@@ -8,25 +8,28 @@ curl -sS https://get.symfony.com/cli/installer | bash
 mv /root/.symfony/bin/symfony /usr/local/bin/symfony
 
 # Set up Symfony test application
-cd tests/TraceContext/W3CTestService
+cd tests/TraceContext/W3CTestService || { printf "Something went wrong: could not cd into W3CTestService directory."; exit 1; }
 rm -rf test_app
 symfony new test_app --version=5.2
 
 # Move our files into the test application
 cp index.php test_app/public/index.php
 cp TestController.php test_app/src/Controller/TestController.php
-cd test_app
 
-composer require annotations
+cd test_app || { printf "Something went wrong: could not cd into test_app."; exit 1; }
 
 # Link the local opentelemetry changes to vendor/open-telemetry
-composer require open-telemetry/opentelemetry "*"
 composer config repositories.local \
     '{"type": "path", "url": "/usr/src/open-telemetry/", "options": {"symLink": true }}' \
     --file composer.json
 composer config minimum-stability dev --file composer.json
-composer require php-http/mock-client "*"
+
+composer require symfony/http-client
+composer require annotations
 composer require nyholm/psr7 "*"
+composer require open-telemetry/opentelemetry "*"
+composer require php-http/mock-client "*"
+
 rm composer.lock
 composer clearcache
 composer install
@@ -41,7 +44,7 @@ cd ..
 rm -rf trace-context
 git clone https://github.com/w3c/trace-context.git
 
-cd trace-context/test
+cd trace-context/test || { printf "Failed to cd to trace-context/test" ; exit; }
 
 # Run the test
 python3 test.py http://127.0.0.1:8001/test

tests/Unit/API/Trace/Propagation/TraceContextPropagatorTest.php

@@ -258,6 +258,57 @@ public function test_extract_empty_header(): void
         ]);
     }
 
+    public function test_extract_future_version(): void
+    {
+        $carrierFuture = [
+            TraceContextPropagator::TRACEPARENT => 'aa-' . self::TRACE_ID_BASE16 . '-' . self::SPAN_ID_BASE16 . '-' . '00',
+            TraceContextPropagator::TRACESTATE => self::TRACESTATE_NOT_DEFAULT_ENCODING,
+        ];
+
+        // Tolerant of future versions with same parts.
+        $this->assertEquals(
+            SpanContext::createFromRemoteParent(self::TRACE_ID_BASE16, self::SPAN_ID_BASE16, SpanContextInterface::TRACE_FLAG_DEFAULT, $this->traceState),
+            $this->getSpanContext($this->traceContextPropagator->extract($carrierFuture)),
+        );
+
+        $carrierFutureMoreParts = [
+            TraceContextPropagator::TRACEPARENT => 'af-' . self::TRACE_ID_BASE16 . '-' . self::SPAN_ID_BASE16 . '-' . '00' . '-000-this-is-the-future',
+            TraceContextPropagator::TRACESTATE => self::TRACESTATE_NOT_DEFAULT_ENCODING,
+        ];
+
+        // Tolerant of future versions with more parts.
+        $this->assertEquals(
+            SpanContext::createFromRemoteParent(self::TRACE_ID_BASE16, self::SPAN_ID_BASE16, SpanContextInterface::TRACE_FLAG_DEFAULT, $this->traceState),
+            $this->getSpanContext($this->traceContextPropagator->extract($carrierFutureMoreParts)),
+        );
+    }
+
+    public function test_invalid_traceparent_version_0xff(): void
+    {
+        $this->assertInvalid([
+            TraceContextPropagator::TRACEPARENT => 'ff-' . self::TRACE_ID_BASE16 . '-' . self::SPAN_ID_BASE16 . '-' . '00',
+        ]);
+    }
+
+    public function test_invalid_traceparent_version(): void
+    {
+        $this->assertInvalid([
+            TraceContextPropagator::TRACEPARENT => 'aaa-' . self::TRACE_ID_BASE16 . '-' . self::SPAN_ID_BASE16 . '-' . '00',
+        ]);
+
+        $this->assertInvalid([
+            TraceContextPropagator::TRACEPARENT => 'gx-' . self::TRACE_ID_BASE16 . '-' . self::SPAN_ID_BASE16 . '-' . '00',
+        ]);
+    }
+
+    public function test_invalid_trace_format(): void
+    {
+        // More than 4 parts to the trace but not a future version.
+        $this->assertInvalid([
+            TraceContextPropagator::TRACEPARENT => '00-' . self::TRACE_ID_BASE16 . '-' . self::SPAN_ID_BASE16 . '-' . '00' . '-000-this-is-not-the-future',
+        ]);
+    }
+
     public function test_invalid_trace_id(): void
     {
         $this->assertInvalid([