tracestate + baggage todos (#894)

- remove baggage TODOs which have since been done.
- implement TODOs from tracestate to log warnings.
- tidy up tracestate code, simplify some const names.

src/API/Baggage/BaggageInterface.php

@@ -16,8 +16,6 @@ interface BaggageInterface extends ImplicitContextKeyedInterface
     /**
      * Returns the {@see API\BaggageInterface} from the provided *$context*,
      * falling back on {@see API\BaggageInterface::getEmpty()} if there is no baggage in the provided context.
-     *
-     * @todo Implement this in the API layer
      */
     public static function fromContext(ContextInterface $context): API\BaggageInterface;
 
@@ -29,8 +27,6 @@ interface BaggageInterface extends ImplicitContextKeyedInterface
     /**
      * Returns the current {@see Baggage} from the current {@see ContextInterface},
      * falling back on {@see API\BaggageInterface::getEmpty()} if there is no baggage in the current context.
-     *
-     * @todo Implement this in the API layer
      */
     public static function getCurrent(): API\BaggageInterface;
 

src/API/Trace/TraceState.php

@@ -5,12 +5,15 @@ declare(strict_types=1);
 namespace OpenTelemetry\API\Trace;
 
 use function array_reverse;
+use OpenTelemetry\API\Behavior\LogsMessagesTrait;
 use function strlen;
 
 class TraceState implements TraceStateInterface
 {
-    public const MAX_TRACESTATE_LIST_MEMBERS = 32;
+    use LogsMessagesTrait;
-    public const MAX_TRACESTATE_LENGTH = 512;
+
+    public const MAX_LIST_MEMBERS = 32; //@see https://www.w3.org/TR/trace-context/#tracestate-header-field-values
+    public const MAX_COMBINED_LENGTH = 512; //@see https://www.w3.org/TR/trace-context/#tracestate-limits
     public const LIST_MEMBERS_SEPARATOR = ',';
     public const LIST_MEMBER_KEY_VALUE_SPLITTER = '=';
     private const VALID_KEY_CHAR_RANGE = '[_0-9a-z-*\/]';
@@ -39,7 +42,6 @@ class TraceState implements TraceStateInterface
     {
         $clonedTracestate = clone $this;
 
-        //TODO: Log if we can't set the value
         if ($this->validateKey($key) && $this->validateValue($value)) {
 
             /*
@@ -54,6 +56,8 @@ class TraceState implements TraceStateInterface
 
             // Add new or updated entry to the back of the list.
             $clonedTracestate->traceState[$key] = $value;
+        } else {
+            self::logWarning('Invalid tracetrace key/value for: ' . $key);
         }
 
         return $clonedTracestate;
@@ -66,7 +70,6 @@ class TraceState implements TraceStateInterface
     {
         $clonedTracestate = clone $this;
 
-        //TODO: Log if we can't unset the value
         if ($key !== '') {
             unset($clonedTracestate->traceState[$key]);
         }
@@ -123,36 +126,30 @@ class TraceState implements TraceStateInterface
      */
     private function parse(string $rawTracestate): array
     {
-        $parsedTracestate = [];
+        if (strlen($rawTracestate) > self::MAX_COMBINED_LENGTH) {
+            self::logWarning('tracestate discarded, exceeds max combined length: ' . self::MAX_COMBINED_LENGTH);
 
-        if (strlen($rawTracestate) <= self::MAX_TRACESTATE_LENGTH) {
+            return [];
+        }
+        $parsedTracestate = [];
         $listMembers = explode(self::LIST_MEMBERS_SEPARATOR, $rawTracestate);
 
-            // Discard tracestate if entries exceed max length.
+        if (count($listMembers) > self::MAX_LIST_MEMBERS) {
-            if (count($listMembers) > self::MAX_TRACESTATE_LIST_MEMBERS) {
+            self::logWarning('tracestate discarded, too many 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));
 
             // There should only be one list-member per vendor separated by '='
-                // TODO: Log if we can't validate the key and value
+            if (count($vendor) !== 2 || !$this->validateKey($vendor[0]) || !$this->validateValue($vendor[1])) {
-                if (count($vendor) === 2 && ($this->validateKey($vendor[0]) && $this->validateValue($vendor[1]))) {
+                self::logWarning('tracestate discarded, invalid member: ' . $listMember);
-                    $parsedTracestate[$vendor[0]] = $vendor[1];
 
-                    continue;
-                }
-                $invalid = true;
-
-                break;
-            }
-            // Discard tracestate if any member is invalid.
-            if ($invalid) {
                 return [];
             }
+            $parsedTracestate[$vendor[0]] = $vendor[1];
         }
 
         /*

tests/Unit/API/Trace/TraceStateTest.php

@@ -75,7 +75,7 @@ class TraceStateTest extends TestCase
     public function test_max_tracestate_list_members(): void
     {
         // Build a tracestate with the max 32 values. Ex '0=0,1=1,...,31=31'
-        $rawTraceState = range(0, TraceState::MAX_TRACESTATE_LIST_MEMBERS - 1);
+        $rawTraceState = range(0, TraceState::MAX_LIST_MEMBERS - 1);
         array_walk($rawTraceState, static function (&$v, $k) {
             $v = 'k' . $k . TraceState::LIST_MEMBER_KEY_VALUE_SPLITTER . 'v' . $v;
         });
@@ -84,14 +84,14 @@ class TraceStateTest extends TestCase
          * @var array $rawTraceState
          * @see https://github.com/vimeo/psalm/issues/6394
          */
-        $this->assertCount(TraceState::MAX_TRACESTATE_LIST_MEMBERS, $rawTraceState);
+        $this->assertCount(TraceState::MAX_LIST_MEMBERS, $rawTraceState);
 
         $validTracestate = new TraceState(implode(TraceState::LIST_MEMBERS_SEPARATOR, $rawTraceState));
-        $this->assertSame(TraceState::MAX_TRACESTATE_LIST_MEMBERS, $validTracestate->getListMemberCount());
+        $this->assertSame(TraceState::MAX_LIST_MEMBERS, $validTracestate->getListMemberCount());
 
         // Add a list-member to the tracestate that exceeds the max of 32. This will cause the tracestate to be discarded.
         $rawTraceState[32] = 'k32' . TraceState::LIST_MEMBER_KEY_VALUE_SPLITTER . 'v32';
-        $this->assertCount(TraceState::MAX_TRACESTATE_LIST_MEMBERS + 1, $rawTraceState);
+        $this->assertCount(TraceState::MAX_LIST_MEMBERS + 1, $rawTraceState);
 
         $truncatedTracestate = new TraceState(implode(TraceState::LIST_MEMBERS_SEPARATOR, $rawTraceState));
         $this->assertSame(0, $truncatedTracestate->getListMemberCount());
@@ -100,21 +100,21 @@ class TraceStateTest extends TestCase
     public function test_max_tracestate_length(): void
     {
         // Build a vendor key with a length of 256 characters. The max characters allowed.
-        $vendorKey = str_repeat('k', TraceState::MAX_TRACESTATE_LENGTH / 2);
+        $vendorKey = str_repeat('k', TraceState::MAX_COMBINED_LENGTH / 2);
 
         // Build a vendor value with a length of 255 characters. One below the max allowed.
-        $vendorValue = str_repeat('v', TraceState::MAX_TRACESTATE_LENGTH / 2 - 1);
+        $vendorValue = str_repeat('v', TraceState::MAX_COMBINED_LENGTH / 2 - 1);
 
         // tracestate length = 513 characters (not accepted).
         $rawTraceState = $vendorKey . TraceState::LIST_MEMBER_KEY_VALUE_SPLITTER . $vendorValue . 'v';
-        $this->assertGreaterThan(TraceState::MAX_TRACESTATE_LENGTH, strlen($rawTraceState));
+        $this->assertGreaterThan(TraceState::MAX_COMBINED_LENGTH, strlen($rawTraceState));
 
         $validTracestate = new TraceState($rawTraceState);
         $this->assertNull($validTracestate->get($vendorKey));
 
         // tracestate length = 512 characters (accepted).
         $rawTraceState = $vendorKey . TraceState::LIST_MEMBER_KEY_VALUE_SPLITTER . $vendorValue;
-        $this->assertSame(TraceState::MAX_TRACESTATE_LENGTH, strlen($rawTraceState));
+        $this->assertSame(TraceState::MAX_COMBINED_LENGTH, strlen($rawTraceState));
 
         $validTracestate = new TraceState($rawTraceState);
         $this->assertSame($rawTraceState, (string) $validTracestate);