From e4039d917ff275fe6df31fec315b77e6139287d5 Mon Sep 17 00:00:00 2001 From: Tobias Bachert Date: Sun, 31 Mar 2024 17:39:55 +0200 Subject: [PATCH 1/7] Port tracestate tests --- tests/Unit/API/Trace/TraceStateOTelTest.php | 222 ++++++++++++++++++++ 1 file changed, 222 insertions(+) create mode 100644 tests/Unit/API/Trace/TraceStateOTelTest.php diff --git a/tests/Unit/API/Trace/TraceStateOTelTest.php b/tests/Unit/API/Trace/TraceStateOTelTest.php new file mode 100644 index 000000000..239e5a714 --- /dev/null +++ b/tests/Unit/API/Trace/TraceStateOTelTest.php @@ -0,0 +1,222 @@ +with('key', 'value'); + + $this->assertNull($traceState->get('key')); + } + + /** + * @dataProvider emptyTraceState + */ + public function testSetReturnsTraceStateWithSetValue(TraceStateInterface $traceState): void { + $traceState = $traceState->with('key', 'value'); + + $this->assertSame('value', $traceState->get('key')); + } + + /** + * @dataProvider emptyTraceState + */ + public function testSetReturnsTraceStateWithOverriddenValue(TraceStateInterface $traceState): void { + $traceState = $traceState->with('key', 'value'); + $traceState = $traceState->with('key', 'other'); + + $this->assertSame('other', $traceState->get('key')); + } + + /** + * @dataProvider emptyTraceState + */ + public function testSetIsAddedToBeginningOfTraceState(TraceStateInterface $traceState): void { + $traceState = $traceState->with('key1', 'value1'); + $traceState = $traceState->with('key2', 'value2'); + + $this->assertSame('key2=value2,key1=value1', (string) $traceState); + } + + /** + * @dataProvider emptyTraceState + */ + public function testSetIsAddedToBeginningOfTraceStateOnOverriddenValue(TraceStateInterface $traceState): void { + $traceState = $traceState->with('key1', 'value1'); + $traceState = $traceState->with('key2', 'value2'); + $traceState = $traceState->with('key1', 'value3'); + + $this->assertSame('key1=value3,key2=value2', (string) $traceState); + } + + /** + * @dataProvider emptyTraceState + */ + public function testSetIsAddedToBeginningOfTraceStateOnOverriddenSameValue(TraceStateInterface $traceState): void { + $traceState = $traceState->with('key1', 'value1'); + $traceState = $traceState->with('key2', 'value2'); + $traceState = $traceState->with('key1', 'value1'); + + $this->assertSame('key1=value1,key2=value2', (string) $traceState); + } + + /** + * @dataProvider emptyTraceState + */ + public function testDeleteDoesNotModifyOriginalTraceState(TraceStateInterface $traceState): void { + $traceState = $traceState->with('key', 'value'); + $traceState->without('key'); + + $this->assertSame('value', $traceState->get('key')); + } + + /** + * @dataProvider emptyTraceState + */ + public function testDeleteReturnsTraceStateWithUnsetValue(TraceStateInterface $traceState): void { + $traceState = $traceState->with('key', 'value'); + $traceState = $traceState->without('key'); + + $this->assertNull($traceState->get('key')); + } + + /** + * @dataProvider emptyTraceState + */ + public function testAllowsOnly32Members(TraceStateInterface $traceState): void { + for ($i = 0; $i < 33; $i++) { + $traceState = $traceState->with('key' . $i, 'test'); + } + + $this->assertSame(32, $traceState->getListMemberCount()); + } + + /** + * @dataProvider validKeyValue + */ + public function testValidKeyValue(TraceStateInterface $traceState, string $key, string $value): void { + $traceState = $traceState->with($key, $value); + + $this->assertSame($value, $traceState->get($key)); + } + + /** + * @dataProvider invalidKeyValue + */ + public function testInvalidKeyValue(TraceStateInterface $traceState, string $key, string $value): void { + $traceState = $traceState->with($key, $value); + + $this->assertNull($traceState->get($key)); + } + + public static function emptyTraceState(): array { + return [ + [new TraceState()], + ]; + } + + public static function validKeyValue(): array { + return [ + [new TraceState(), 'valid@key', 'value'], + [new TraceState(), 'valid@key', '0'], + [new TraceState(), 'abcdefghijklmnopqrstuvwxyz0123456789_-*/', 'value'], + [new TraceState(), 'abcdefghijklmnopqrstuvwxyz0123456789_-*/@a1234_-*/', 'value'], + [new TraceState(), 'key', strtr(implode(range(chr(0x20), chr(0x7E))), [',' => '', '=' => ''])], + [new TraceState(), str_repeat('a', 256), 'value'], + [new TraceState(), 'key', str_repeat('a', 256)], + ]; + } + + public static function invalidKeyValue(): array { + return [ + [new TraceState(), '', 'value'], + [new TraceState(), 'invalid.key', 'value'], + [new TraceState(), 'invalid-key@', 'value'], + [new TraceState(), '0invalid-key', 'value'], + [new TraceState(), str_repeat('a', 257), 'value'], + [new TraceState(), 'invalid@key_____________', 'value'], + [new TraceState(), '-invalid-key@id', 'value'], + [new TraceState(), 'invalid-key@0id', 'value'], + [new TraceState(), 'invalid-key@@id', 'value'], + + [new TraceState(), 'key', ''], + [new TraceState(), 'key', 'invalid-value '], + [new TraceState(), 'key', 'invalid,value'], + [new TraceState(), 'key', 'invalid=value'], + [new TraceState(), 'key', str_repeat('a', 257)], + ]; + } + + /** + * @dataProvider toStringConfigurations + */ + public function testToString(TraceStateInterface $traceState, array $entries, ?int $limit, string $expected): void { + $this->markTestSkipped('Not yet implemented'); + + foreach (array_reverse($entries) as $key => $value) { + $traceState = $traceState->with($key, $value); + } + + $this->assertSame($expected, $traceState->toString(limit: $limit)); + } + + public static function toStringConfigurations(): array { + return [ + [new TraceState(), [], null, ''], + [new TraceState(), ['key' => 'value'], null, 'key=value'], + [new TraceState(), ['key' => 'value', 'key2' => 'value2'], null, 'key=value,key2=value2'], + [new TraceState(), ['key' => 'value', 'key2' => 'value2'], 21, 'key=value,key2=value2'], + [new TraceState(), ['key' => 'value', 'key2' => 'value2'], 14, 'key=value'], + [new TraceState(), ['key' => 'value', 'key2' => 'value2'], 9, 'key=value'], + [new TraceState(), ['key' => 'value', 'key2' => 'value2'], 5, ''], + [new TraceState(), ['key' => 'value', 'a' => 'b'], 5, ''], + [new TraceState(), [str_repeat('a', 10) => str_repeat('v', 50), 'key' => 'value', 'key2' => 'value2'], 50, ''], + [new TraceState(), [str_repeat('a', 10) => str_repeat('v', 150), 'key' => 'value', 'key2' => 'value2'], 50, 'key=value,key2=value2'], + [new TraceState(), [str_repeat('a', 10) => str_repeat('v', 117), 'key' => 'value', 'key2' => 'value2'], 50, ''], + [new TraceState(), [str_repeat('a', 10) => str_repeat('v', 118), 'key' => 'value', 'key2' => 'value2'], 50, 'key=value,key2=value2'], + [new TraceState(), [str_repeat('a', 10) => str_repeat('v', 118), 'key' => 'value', 'key2' => 'value2'], 14, 'key=value'], + ]; + } + + /** + * @dataProvider parseConfigurations + */ + public function testParse(string|array $tracestate, ?string $expected): void { + $traceState = new TraceState($tracestate); + $this->assertSame($expected ?? '', (string) $traceState); + } + + public static function parseConfigurations(): array { + return [ + ['key=value', 'key=value'], + ['key=value,key2=value2', 'key=value,key2=value2'], + ['key=value,key2=value2,key=value3', 'key=value,key2=value2'], + + // [['key=value', 'key2=value2'], 'key=value,key2=value2'], + // [['key=value', 'key=value2'], 'key=value'], + + ['', ''], + [' ', ''], + ['key=value,,key2=value2', 'key=value,key2=value2'], + ['key=value, ,key2=value2', 'key=value,key2=value2'], + + ['0', null], + ['key =value', null], + ]; + } +} From f25945e88f88e8eb6c97cdccfe35003ba792fc14 Mon Sep 17 00:00:00 2001 From: Tobias Bachert Date: Sun, 31 Mar 2024 18:05:22 +0200 Subject: [PATCH 2/7] Fix tracestate implementation --- src/API/Trace/TraceState.php | 190 ++++++++++++------------ src/API/Trace/TraceStateInterface.php | 10 ++ tests/Unit/API/Trace/TraceStateTest.php | 17 +-- 3 files changed, 108 insertions(+), 109 deletions(-) diff --git a/src/API/Trace/TraceState.php b/src/API/Trace/TraceState.php index b398f2dba..1916307df 100644 --- a/src/API/Trace/TraceState.php +++ b/src/API/Trace/TraceState.php @@ -4,15 +4,22 @@ namespace OpenTelemetry\API\Trace; -use function array_reverse; use OpenTelemetry\API\Behavior\LogsMessagesTrait; +use function count; +use function end; +use function explode; +use function key; +use function prev; +use function sprintf; use function strlen; +use function trim; class TraceState implements TraceStateInterface { use LogsMessagesTrait; public const MAX_LIST_MEMBERS = 32; //@see https://www.w3.org/TR/trace-context/#tracestate-header-field-values + /** @deprecated will be removed */ 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 = '='; @@ -23,139 +30,130 @@ class TraceState implements TraceStateInterface private const VALID_VALUE_BASE_REGEX = '/^[ -~]{0,255}[!-~]$/'; private const INVALID_VALUE_COMMA_EQUAL_REGEX = '/,|=/'; - /** @var string[] */ - private array $traceState = []; + /** @var array */ + private array $traceState; - public function __construct(string $rawTracestate = null) + public function __construct(?string $rawTracestate = null) { - if ($rawTracestate === null || trim($rawTracestate) === '') { - return; - } - $this->traceState = $this->parse($rawTracestate); + $this->traceState = self::parse($rawTracestate ?? ''); } - /** - * {@inheritdoc} - */ public function with(string $key, string $value): TraceStateInterface { - $clonedTracestate = clone $this; - - if ($this->validateKey($key) && $this->validateValue($value)) { - - /* - * Only one entry per key is allowed. In this case we need to overwrite the vendor entry - * upon reentry to the tracing system and ensure the updated entry is at the beginning of - * the list. This means we place it the back for now and it will be at the beginning once - * we reverse the order back during __toString(). - */ - if (array_key_exists($key, $clonedTracestate->traceState)) { - unset($clonedTracestate->traceState[$key]); - } - - // Add new or updated entry to the back of the list. - $clonedTracestate->traceState[$key] = $value; - } else { - self::logWarning('Invalid tracetrace key/value for: ' . $key); + if (!self::validateMember($this->traceState, $key, $value)) { + self::logWarning('Invalid tracestate key/value for: ' . $key); + return $this; } - return $clonedTracestate; + $clone = clone $this; + $clone->traceState = [$key => $value] + $this->traceState; + + return $clone; } - /** - * {@inheritdoc} - */ public function without(string $key): TraceStateInterface { - $clonedTracestate = clone $this; - - if ($key !== '') { - unset($clonedTracestate->traceState[$key]); + if (!isset($this->traceState[$key])) { + return $this; } - return $clonedTracestate; + $clone = clone $this; + unset($clone->traceState[$key]); + + return $clone; } - /** - * {@inheritdoc} - */ public function get(string $key): ?string { return $this->traceState[$key] ?? null; } - /** - * {@inheritdoc} - */ public function getListMemberCount(): int { return count($this->traceState); } - /** - * {@inheritdoc} - */ - public function __toString(): string - { - if ($this->traceState === []) { - return ''; + public function toString(?int $limit = null): string { + $traceState = $this->traceState; + + if ($limit !== null) { + $length = 0; + foreach ($traceState as $key => $value) { + $length and $length += 1; + $length += strlen($key) + 1 + strlen($value); + } + if ($length > $limit) { + // Entries larger than 128 characters long SHOULD be removed first. + foreach ([128, 0] as $threshold) { + // Then entries SHOULD be removed starting from the end of tracestate. + for ($value = end($traceState); $key = key($traceState);) { + $entry = strlen($key) + 1 + strlen($value); + $value = prev($traceState); + if ($entry <= $threshold) { + continue; + } + + unset($traceState[$key]); + if (($length -= $entry + 1) <= $limit) { + break 2; + } + } + } + } } - $traceStateString=''; - foreach (array_reverse($this->traceState) as $k => $v) { - $traceStateString .=$k . self::LIST_MEMBER_KEY_VALUE_SPLITTER . $v . self::LIST_MEMBERS_SEPARATOR; + + $s = ''; + foreach ($traceState as $key => $value) { + $s and $s .= ','; + $s .= $key; + $s .= '='; + $s .= $value; } - return rtrim($traceStateString, ','); + return $s; } - /** - * Parse the raw tracestate header into the TraceState object. Since new or updated entries must - * be added to the beginning of the list, the key-value pairs in the TraceState object will be - * stored in reverse order. This ensures new entries added to the TraceState object are at the - * beginning when we reverse the order back again while building the final tracestate header. - * - * Ex: - * tracestate = 'vendor1=value1,vendor2=value2' - * - * || - * \/ - * - * $this->tracestate = ['vendor2' => 'value2' ,'vendor1' => 'value1'] - * - */ - private function parse(string $rawTracestate): array + public function __toString(): string { - if (strlen($rawTracestate) > self::MAX_COMBINED_LENGTH) { - self::logWarning('tracestate discarded, exceeds max combined length: ' . self::MAX_COMBINED_LENGTH); + return $this->toString(); + } - return []; - } - $parsedTracestate = []; - $listMembers = explode(self::LIST_MEMBERS_SEPARATOR, $rawTracestate); + private static function parse(string $rawTracestate): array + { + $traceState = []; + $members = explode(',', $rawTracestate); + foreach ($members as $member) { + if (($member = trim($member, " \t")) === '') { + continue; + } - if (count($listMembers) > self::MAX_LIST_MEMBERS) { - self::logWarning('tracestate discarded, too many members'); + $member = explode('=', $member, 2); + if (count($member) !== 2) { + self::logWarning(sprintf('Incomplete list member in tracestate "%s"', $rawTracestate)); + return []; + } - return []; - } + [$key, $value] = $member; + if (!self::validateMember($traceState, $key, $value)) { + self::logWarning(sprintf('Invalid list member "%s=%s" in tracestate "%s"', $key, $value, $rawTracestate)); + return []; + } - foreach ($listMembers as $listMember) { - $vendor = explode(self::LIST_MEMBER_KEY_VALUE_SPLITTER, trim($listMember)); + $traceState[$key] ??= $value; + } - // There should only be one list-member per vendor separated by '=' - if (count($vendor) !== 2 || !$this->validateKey($vendor[0]) || !$this->validateValue($vendor[1])) { - self::logWarning('tracestate discarded, invalid member: ' . $listMember); + return $traceState; + } - return []; - } - $parsedTracestate[$vendor[0]] = $vendor[1]; + private static function validateMember(array $traceState, string $key, string $value): bool { + if (!isset($traceState[$key]) && !self::validateKey($key) || !self::validateValue($value)) { + return false; + } + if (count($traceState) === self::MAX_LIST_MEMBERS && !isset($traceState[$key])) { + return false; } - /* - * Reversing the tracestate ensures the new entries added to the TraceState object are at - * the beginning when we reverse it back during __toString(). - */ - return array_reverse($parsedTracestate); + return true; } /** @@ -168,7 +166,7 @@ private function parse(string $rawTracestate): array * * @see https://www.w3.org/TR/trace-context/#key */ - private function validateKey(string $key): bool + private static function validateKey(string $key): bool { return preg_match(self::VALID_KEY_REGEX, $key) !== 0; } @@ -180,7 +178,7 @@ private function validateKey(string $key): bool * * @see https://www.w3.org/TR/trace-context/#value */ - private function validateValue(string $key): bool + private static function validateValue(string $key): bool { return (preg_match(self::VALID_VALUE_BASE_REGEX, $key) !== 0) && (preg_match(self::INVALID_VALUE_COMMA_EQUAL_REGEX, $key) === 0); diff --git a/src/API/Trace/TraceStateInterface.php b/src/API/Trace/TraceStateInterface.php index 394ea1b95..d76bd437e 100644 --- a/src/API/Trace/TraceStateInterface.php +++ b/src/API/Trace/TraceStateInterface.php @@ -50,6 +50,16 @@ public function get(string $key): ?string; */ public function getListMemberCount(): int; + /** + * Returns the concatenated string representation. + * + * @param int|null $limit maximum length of the returned representation + * @return string the string representation + * + * @see https://www.w3.org/TR/trace-context/#tracestate-limits + */ + public function toString(?int $limit = null): string; + /** * Returns a string representation of this TraceSate */ diff --git a/tests/Unit/API/Trace/TraceStateTest.php b/tests/Unit/API/Trace/TraceStateTest.php index 890bd31aa..36501d668 100644 --- a/tests/Unit/API/Trace/TraceStateTest.php +++ b/tests/Unit/API/Trace/TraceStateTest.php @@ -124,22 +124,13 @@ public function test_max_tracestate_list_members(): void 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_COMBINED_LENGTH / 2); + $vendorKey = str_repeat('k', 256); - // Build a vendor value with a length of 255 characters. One below the max allowed. - $vendorValue = str_repeat('v', TraceState::MAX_COMBINED_LENGTH / 2 - 1); + // Build a vendor value with a length of 256 characters. The max characters allowed. + $vendorValue = str_repeat('v', 256); - // tracestate length = 513 characters (not accepted). - $rawTraceState = $vendorKey . TraceState::LIST_MEMBER_KEY_VALUE_SPLITTER . $vendorValue . 'v'; - $this->assertGreaterThan(TraceState::MAX_COMBINED_LENGTH, strlen($rawTraceState)); - - $validTracestate = new TraceState($rawTraceState); - $this->assertNull($validTracestate->get($vendorKey)); - - // tracestate length = 512 characters (accepted). + // tracestate length = 513 characters (accepted). $rawTraceState = $vendorKey . TraceState::LIST_MEMBER_KEY_VALUE_SPLITTER . $vendorValue; - $this->assertSame(TraceState::MAX_COMBINED_LENGTH, strlen($rawTraceState)); - $validTracestate = new TraceState($rawTraceState); $this->assertSame($rawTraceState, (string) $validTracestate); } From 591336dcacb1e476510b562197806fe8c3c093cf Mon Sep 17 00:00:00 2001 From: Tobias Bachert Date: Sun, 31 Mar 2024 18:08:54 +0200 Subject: [PATCH 3/7] Fix W3C test service --- tests/TraceContext/W3CTestService/index.php | 2 +- tests/TraceContext/W3CTestService/trace-context-test.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/TraceContext/W3CTestService/index.php b/tests/TraceContext/W3CTestService/index.php index 891d45e9d..b2787964e 100644 --- a/tests/TraceContext/W3CTestService/index.php +++ b/tests/TraceContext/W3CTestService/index.php @@ -26,7 +26,7 @@ function main(): void [ new BatchSpanProcessor( new ZipkinExporter( - PsrTransportFactory::discover()->create('http://zipkin:9412/api/v2/spans') + PsrTransportFactory::discover()->create('http://zipkin:9412/api/v2/spans', 'application/json') ), ClockFactory::getDefault() ), diff --git a/tests/TraceContext/W3CTestService/trace-context-test.sh b/tests/TraceContext/W3CTestService/trace-context-test.sh index 7a6c8077b..c012c0cd7 100755 --- a/tests/TraceContext/W3CTestService/trace-context-test.sh +++ b/tests/TraceContext/W3CTestService/trace-context-test.sh @@ -22,4 +22,4 @@ rm -rf trace-context git clone https://github.com/w3c/trace-context.git # Run the test -python3 "trace-context/test/test.py" http://127.0.0.1:8001/test +SPEC_LEVEL=1 python3 "trace-context/test/test.py" http://127.0.0.1:8001/test From c74e63d7ade94bb88f022507b47275c1a6ff0295 Mon Sep 17 00:00:00 2001 From: Tobias Bachert Date: Sun, 31 Mar 2024 18:18:33 +0200 Subject: [PATCH 4/7] Fix style --- src/API/Trace/TraceState.php | 26 ++++---- tests/Unit/API/Trace/TraceStateOTelTest.php | 73 +++++++++++++-------- 2 files changed, 58 insertions(+), 41 deletions(-) diff --git a/src/API/Trace/TraceState.php b/src/API/Trace/TraceState.php index 1916307df..91eac6339 100644 --- a/src/API/Trace/TraceState.php +++ b/src/API/Trace/TraceState.php @@ -4,11 +4,11 @@ namespace OpenTelemetry\API\Trace; -use OpenTelemetry\API\Behavior\LogsMessagesTrait; use function count; use function end; use function explode; use function key; +use OpenTelemetry\API\Behavior\LogsMessagesTrait; use function prev; use function sprintf; use function strlen; @@ -42,6 +42,7 @@ public function with(string $key, string $value): TraceStateInterface { if (!self::validateMember($this->traceState, $key, $value)) { self::logWarning('Invalid tracestate key/value for: ' . $key); + return $this; } @@ -73,13 +74,14 @@ public function getListMemberCount(): int return count($this->traceState); } - public function toString(?int $limit = null): string { + public function toString(?int $limit = null): string + { $traceState = $this->traceState; if ($limit !== null) { $length = 0; foreach ($traceState as $key => $value) { - $length and $length += 1; + $length && ($length += 1); $length += strlen($key) + 1 + strlen($value); } if ($length > $limit) { @@ -104,7 +106,7 @@ public function toString(?int $limit = null): string { $s = ''; foreach ($traceState as $key => $value) { - $s and $s .= ','; + $s && ($s .= ','); $s .= $key; $s .= '='; $s .= $value; @@ -130,12 +132,14 @@ private static function parse(string $rawTracestate): array $member = explode('=', $member, 2); if (count($member) !== 2) { self::logWarning(sprintf('Incomplete list member in tracestate "%s"', $rawTracestate)); + return []; } [$key, $value] = $member; if (!self::validateMember($traceState, $key, $value)) { self::logWarning(sprintf('Invalid list member "%s=%s" in tracestate "%s"', $key, $value, $rawTracestate)); + return []; } @@ -145,15 +149,11 @@ private static function parse(string $rawTracestate): array return $traceState; } - private static function validateMember(array $traceState, string $key, string $value): bool { - if (!isset($traceState[$key]) && !self::validateKey($key) || !self::validateValue($value)) { - return false; - } - if (count($traceState) === self::MAX_LIST_MEMBERS && !isset($traceState[$key])) { - return false; - } - - return true; + private static function validateMember(array $traceState, string $key, string $value): bool + { + return (isset($traceState[$key]) || self::validateKey($key)) + && self::validateValue($value) + && (count($traceState) < self::MAX_LIST_MEMBERS || isset($traceState[$key])); } /** diff --git a/tests/Unit/API/Trace/TraceStateOTelTest.php b/tests/Unit/API/Trace/TraceStateOTelTest.php index 239e5a714..b9bef6dcb 100644 --- a/tests/Unit/API/Trace/TraceStateOTelTest.php +++ b/tests/Unit/API/Trace/TraceStateOTelTest.php @@ -1,24 +1,29 @@ -with('key', 'value'); $this->assertNull($traceState->get('key')); @@ -27,7 +32,8 @@ public function testSetDoesNotModifyOriginalTraceState(TraceStateInterface $trac /** * @dataProvider emptyTraceState */ - public function testSetReturnsTraceStateWithSetValue(TraceStateInterface $traceState): void { + public function test_set_returns_trace_state_with_set_value(TraceStateInterface $traceState): void + { $traceState = $traceState->with('key', 'value'); $this->assertSame('value', $traceState->get('key')); @@ -36,7 +42,8 @@ public function testSetReturnsTraceStateWithSetValue(TraceStateInterface $traceS /** * @dataProvider emptyTraceState */ - public function testSetReturnsTraceStateWithOverriddenValue(TraceStateInterface $traceState): void { + public function test_set_returns_trace_state_with_overridden_value(TraceStateInterface $traceState): void + { $traceState = $traceState->with('key', 'value'); $traceState = $traceState->with('key', 'other'); @@ -46,7 +53,8 @@ public function testSetReturnsTraceStateWithOverriddenValue(TraceStateInterface /** * @dataProvider emptyTraceState */ - public function testSetIsAddedToBeginningOfTraceState(TraceStateInterface $traceState): void { + public function test_set_is_added_to_beginning_of_trace_state(TraceStateInterface $traceState): void + { $traceState = $traceState->with('key1', 'value1'); $traceState = $traceState->with('key2', 'value2'); @@ -56,7 +64,8 @@ public function testSetIsAddedToBeginningOfTraceState(TraceStateInterface $trace /** * @dataProvider emptyTraceState */ - public function testSetIsAddedToBeginningOfTraceStateOnOverriddenValue(TraceStateInterface $traceState): void { + public function test_set_is_added_to_beginning_of_trace_state_on_overridden_value(TraceStateInterface $traceState): void + { $traceState = $traceState->with('key1', 'value1'); $traceState = $traceState->with('key2', 'value2'); $traceState = $traceState->with('key1', 'value3'); @@ -67,7 +76,8 @@ public function testSetIsAddedToBeginningOfTraceStateOnOverriddenValue(TraceStat /** * @dataProvider emptyTraceState */ - public function testSetIsAddedToBeginningOfTraceStateOnOverriddenSameValue(TraceStateInterface $traceState): void { + public function test_set_is_added_to_beginning_of_trace_state_on_overridden_same_value(TraceStateInterface $traceState): void + { $traceState = $traceState->with('key1', 'value1'); $traceState = $traceState->with('key2', 'value2'); $traceState = $traceState->with('key1', 'value1'); @@ -78,7 +88,8 @@ public function testSetIsAddedToBeginningOfTraceStateOnOverriddenSameValue(Trace /** * @dataProvider emptyTraceState */ - public function testDeleteDoesNotModifyOriginalTraceState(TraceStateInterface $traceState): void { + public function test_delete_does_not_modify_original_trace_state(TraceStateInterface $traceState): void + { $traceState = $traceState->with('key', 'value'); $traceState->without('key'); @@ -88,7 +99,8 @@ public function testDeleteDoesNotModifyOriginalTraceState(TraceStateInterface $t /** * @dataProvider emptyTraceState */ - public function testDeleteReturnsTraceStateWithUnsetValue(TraceStateInterface $traceState): void { + public function test_delete_returns_trace_state_with_unset_value(TraceStateInterface $traceState): void + { $traceState = $traceState->with('key', 'value'); $traceState = $traceState->without('key'); @@ -98,7 +110,8 @@ public function testDeleteReturnsTraceStateWithUnsetValue(TraceStateInterface $t /** * @dataProvider emptyTraceState */ - public function testAllowsOnly32Members(TraceStateInterface $traceState): void { + public function test_allows_only32_members(TraceStateInterface $traceState): void + { for ($i = 0; $i < 33; $i++) { $traceState = $traceState->with('key' . $i, 'test'); } @@ -109,7 +122,8 @@ public function testAllowsOnly32Members(TraceStateInterface $traceState): void { /** * @dataProvider validKeyValue */ - public function testValidKeyValue(TraceStateInterface $traceState, string $key, string $value): void { + public function test_valid_key_value(TraceStateInterface $traceState, string $key, string $value): void + { $traceState = $traceState->with($key, $value); $this->assertSame($value, $traceState->get($key)); @@ -118,19 +132,22 @@ public function testValidKeyValue(TraceStateInterface $traceState, string $key, /** * @dataProvider invalidKeyValue */ - public function testInvalidKeyValue(TraceStateInterface $traceState, string $key, string $value): void { + public function test_invalid_key_value(TraceStateInterface $traceState, string $key, string $value): void + { $traceState = $traceState->with($key, $value); $this->assertNull($traceState->get($key)); } - public static function emptyTraceState(): array { + public static function emptyTraceState(): array + { return [ [new TraceState()], ]; } - public static function validKeyValue(): array { + public static function validKeyValue(): array + { return [ [new TraceState(), 'valid@key', 'value'], [new TraceState(), 'valid@key', '0'], @@ -142,7 +159,8 @@ public static function validKeyValue(): array { ]; } - public static function invalidKeyValue(): array { + public static function invalidKeyValue(): array + { return [ [new TraceState(), '', 'value'], [new TraceState(), 'invalid.key', 'value'], @@ -165,9 +183,8 @@ public static function invalidKeyValue(): array { /** * @dataProvider toStringConfigurations */ - public function testToString(TraceStateInterface $traceState, array $entries, ?int $limit, string $expected): void { - $this->markTestSkipped('Not yet implemented'); - + public function test_to_string(TraceStateInterface $traceState, array $entries, ?int $limit, string $expected): void + { foreach (array_reverse($entries) as $key => $value) { $traceState = $traceState->with($key, $value); } @@ -175,7 +192,8 @@ public function testToString(TraceStateInterface $traceState, array $entries, ?i $this->assertSame($expected, $traceState->toString(limit: $limit)); } - public static function toStringConfigurations(): array { + public static function toStringConfigurations(): array + { return [ [new TraceState(), [], null, ''], [new TraceState(), ['key' => 'value'], null, 'key=value'], @@ -196,20 +214,19 @@ public static function toStringConfigurations(): array { /** * @dataProvider parseConfigurations */ - public function testParse(string|array $tracestate, ?string $expected): void { + public function test_parse(string $tracestate, ?string $expected): void + { $traceState = new TraceState($tracestate); $this->assertSame($expected ?? '', (string) $traceState); } - public static function parseConfigurations(): array { + public static function parseConfigurations(): array + { return [ ['key=value', 'key=value'], ['key=value,key2=value2', 'key=value,key2=value2'], ['key=value,key2=value2,key=value3', 'key=value,key2=value2'], - // [['key=value', 'key2=value2'], 'key=value,key2=value2'], - // [['key=value', 'key=value2'], 'key=value'], - ['', ''], [' ', ''], ['key=value,,key2=value2', 'key=value,key2=value2'], From bace440ea13a17c0bd2f08da99f0e728a474f7d7 Mon Sep 17 00:00:00 2001 From: Tobias Bachert Date: Sun, 31 Mar 2024 18:27:00 +0200 Subject: [PATCH 5/7] Move tests into `TraceStateTest` --- tests/Unit/API/Trace/TraceStateOTelTest.php | 239 -------------------- tests/Unit/API/Trace/TraceStateTest.php | 66 ++++++ 2 files changed, 66 insertions(+), 239 deletions(-) delete mode 100644 tests/Unit/API/Trace/TraceStateOTelTest.php diff --git a/tests/Unit/API/Trace/TraceStateOTelTest.php b/tests/Unit/API/Trace/TraceStateOTelTest.php deleted file mode 100644 index b9bef6dcb..000000000 --- a/tests/Unit/API/Trace/TraceStateOTelTest.php +++ /dev/null @@ -1,239 +0,0 @@ -with('key', 'value'); - - $this->assertNull($traceState->get('key')); - } - - /** - * @dataProvider emptyTraceState - */ - public function test_set_returns_trace_state_with_set_value(TraceStateInterface $traceState): void - { - $traceState = $traceState->with('key', 'value'); - - $this->assertSame('value', $traceState->get('key')); - } - - /** - * @dataProvider emptyTraceState - */ - public function test_set_returns_trace_state_with_overridden_value(TraceStateInterface $traceState): void - { - $traceState = $traceState->with('key', 'value'); - $traceState = $traceState->with('key', 'other'); - - $this->assertSame('other', $traceState->get('key')); - } - - /** - * @dataProvider emptyTraceState - */ - public function test_set_is_added_to_beginning_of_trace_state(TraceStateInterface $traceState): void - { - $traceState = $traceState->with('key1', 'value1'); - $traceState = $traceState->with('key2', 'value2'); - - $this->assertSame('key2=value2,key1=value1', (string) $traceState); - } - - /** - * @dataProvider emptyTraceState - */ - public function test_set_is_added_to_beginning_of_trace_state_on_overridden_value(TraceStateInterface $traceState): void - { - $traceState = $traceState->with('key1', 'value1'); - $traceState = $traceState->with('key2', 'value2'); - $traceState = $traceState->with('key1', 'value3'); - - $this->assertSame('key1=value3,key2=value2', (string) $traceState); - } - - /** - * @dataProvider emptyTraceState - */ - public function test_set_is_added_to_beginning_of_trace_state_on_overridden_same_value(TraceStateInterface $traceState): void - { - $traceState = $traceState->with('key1', 'value1'); - $traceState = $traceState->with('key2', 'value2'); - $traceState = $traceState->with('key1', 'value1'); - - $this->assertSame('key1=value1,key2=value2', (string) $traceState); - } - - /** - * @dataProvider emptyTraceState - */ - public function test_delete_does_not_modify_original_trace_state(TraceStateInterface $traceState): void - { - $traceState = $traceState->with('key', 'value'); - $traceState->without('key'); - - $this->assertSame('value', $traceState->get('key')); - } - - /** - * @dataProvider emptyTraceState - */ - public function test_delete_returns_trace_state_with_unset_value(TraceStateInterface $traceState): void - { - $traceState = $traceState->with('key', 'value'); - $traceState = $traceState->without('key'); - - $this->assertNull($traceState->get('key')); - } - - /** - * @dataProvider emptyTraceState - */ - public function test_allows_only32_members(TraceStateInterface $traceState): void - { - for ($i = 0; $i < 33; $i++) { - $traceState = $traceState->with('key' . $i, 'test'); - } - - $this->assertSame(32, $traceState->getListMemberCount()); - } - - /** - * @dataProvider validKeyValue - */ - public function test_valid_key_value(TraceStateInterface $traceState, string $key, string $value): void - { - $traceState = $traceState->with($key, $value); - - $this->assertSame($value, $traceState->get($key)); - } - - /** - * @dataProvider invalidKeyValue - */ - public function test_invalid_key_value(TraceStateInterface $traceState, string $key, string $value): void - { - $traceState = $traceState->with($key, $value); - - $this->assertNull($traceState->get($key)); - } - - public static function emptyTraceState(): array - { - return [ - [new TraceState()], - ]; - } - - public static function validKeyValue(): array - { - return [ - [new TraceState(), 'valid@key', 'value'], - [new TraceState(), 'valid@key', '0'], - [new TraceState(), 'abcdefghijklmnopqrstuvwxyz0123456789_-*/', 'value'], - [new TraceState(), 'abcdefghijklmnopqrstuvwxyz0123456789_-*/@a1234_-*/', 'value'], - [new TraceState(), 'key', strtr(implode(range(chr(0x20), chr(0x7E))), [',' => '', '=' => ''])], - [new TraceState(), str_repeat('a', 256), 'value'], - [new TraceState(), 'key', str_repeat('a', 256)], - ]; - } - - public static function invalidKeyValue(): array - { - return [ - [new TraceState(), '', 'value'], - [new TraceState(), 'invalid.key', 'value'], - [new TraceState(), 'invalid-key@', 'value'], - [new TraceState(), '0invalid-key', 'value'], - [new TraceState(), str_repeat('a', 257), 'value'], - [new TraceState(), 'invalid@key_____________', 'value'], - [new TraceState(), '-invalid-key@id', 'value'], - [new TraceState(), 'invalid-key@0id', 'value'], - [new TraceState(), 'invalid-key@@id', 'value'], - - [new TraceState(), 'key', ''], - [new TraceState(), 'key', 'invalid-value '], - [new TraceState(), 'key', 'invalid,value'], - [new TraceState(), 'key', 'invalid=value'], - [new TraceState(), 'key', str_repeat('a', 257)], - ]; - } - - /** - * @dataProvider toStringConfigurations - */ - public function test_to_string(TraceStateInterface $traceState, array $entries, ?int $limit, string $expected): void - { - foreach (array_reverse($entries) as $key => $value) { - $traceState = $traceState->with($key, $value); - } - - $this->assertSame($expected, $traceState->toString(limit: $limit)); - } - - public static function toStringConfigurations(): array - { - return [ - [new TraceState(), [], null, ''], - [new TraceState(), ['key' => 'value'], null, 'key=value'], - [new TraceState(), ['key' => 'value', 'key2' => 'value2'], null, 'key=value,key2=value2'], - [new TraceState(), ['key' => 'value', 'key2' => 'value2'], 21, 'key=value,key2=value2'], - [new TraceState(), ['key' => 'value', 'key2' => 'value2'], 14, 'key=value'], - [new TraceState(), ['key' => 'value', 'key2' => 'value2'], 9, 'key=value'], - [new TraceState(), ['key' => 'value', 'key2' => 'value2'], 5, ''], - [new TraceState(), ['key' => 'value', 'a' => 'b'], 5, ''], - [new TraceState(), [str_repeat('a', 10) => str_repeat('v', 50), 'key' => 'value', 'key2' => 'value2'], 50, ''], - [new TraceState(), [str_repeat('a', 10) => str_repeat('v', 150), 'key' => 'value', 'key2' => 'value2'], 50, 'key=value,key2=value2'], - [new TraceState(), [str_repeat('a', 10) => str_repeat('v', 117), 'key' => 'value', 'key2' => 'value2'], 50, ''], - [new TraceState(), [str_repeat('a', 10) => str_repeat('v', 118), 'key' => 'value', 'key2' => 'value2'], 50, 'key=value,key2=value2'], - [new TraceState(), [str_repeat('a', 10) => str_repeat('v', 118), 'key' => 'value', 'key2' => 'value2'], 14, 'key=value'], - ]; - } - - /** - * @dataProvider parseConfigurations - */ - public function test_parse(string $tracestate, ?string $expected): void - { - $traceState = new TraceState($tracestate); - $this->assertSame($expected ?? '', (string) $traceState); - } - - public static function parseConfigurations(): array - { - return [ - ['key=value', 'key=value'], - ['key=value,key2=value2', 'key=value,key2=value2'], - ['key=value,key2=value2,key=value3', 'key=value,key2=value2'], - - ['', ''], - [' ', ''], - ['key=value,,key2=value2', 'key=value,key2=value2'], - ['key=value, ,key2=value2', 'key=value,key2=value2'], - - ['0', null], - ['key =value', null], - ]; - } -} diff --git a/tests/Unit/API/Trace/TraceStateTest.php b/tests/Unit/API/Trace/TraceStateTest.php index 36501d668..4589e421c 100644 --- a/tests/Unit/API/Trace/TraceStateTest.php +++ b/tests/Unit/API/Trace/TraceStateTest.php @@ -4,8 +4,10 @@ namespace OpenTelemetry\Tests\API\Unit\Trace; +use function array_reverse; use OpenTelemetry\API\LoggerHolder; use OpenTelemetry\API\Trace\TraceState; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; use Psr\Log\LoggerInterface; use function str_repeat; @@ -86,6 +88,16 @@ public function test_without_tracestate_value(): void $this->assertSame('value2', $tracestate->get('vendor2')); } + public function test_with_allows_only32_members(): void + { + $traceState = new TraceState(); + for ($i = 0; $i < 33; $i++) { + $traceState = $traceState->with('key' . $i, 'test'); + } + + $this->assertSame(32, $traceState->getListMemberCount()); + } + public function test_to_string_tracestate(): void { $tracestate = new TraceState('vendor1=value1'); @@ -201,4 +213,58 @@ public function test_validate_value(): void $this->assertSame(257, strlen($invalidValue)); $this->assertNull($tracestate->get($validKey)); } + + #[DataProvider('toStringProvider')] + public function test_to_string(array $entries, ?int $limit, string $expected): void + { + $traceState = new TraceState(); + foreach (array_reverse($entries) as $key => $value) { + $traceState = $traceState->with($key, $value); + } + + $this->assertSame($expected, $traceState->toString(limit: $limit)); + } + + public static function toStringProvider(): array + { + return [ + [[], null, ''], + [['key' => 'value'], null, 'key=value'], + [['key' => 'value', 'key2' => 'value2'], null, 'key=value,key2=value2'], + [['key' => 'value', 'key2' => 'value2'], 21, 'key=value,key2=value2'], + [['key' => 'value', 'key2' => 'value2'], 14, 'key=value'], + [['key' => 'value', 'key2' => 'value2'], 9, 'key=value'], + [['key' => 'value', 'key2' => 'value2'], 5, ''], + [['key' => 'value', 'a' => 'b'], 5, ''], + [[str_repeat('a', 10) => str_repeat('v', 50), 'key' => 'value', 'key2' => 'value2'], 50, ''], + [[str_repeat('a', 10) => str_repeat('v', 150), 'key' => 'value', 'key2' => 'value2'], 50, 'key=value,key2=value2'], + [[str_repeat('a', 10) => str_repeat('v', 117), 'key' => 'value', 'key2' => 'value2'], 50, ''], + [[str_repeat('a', 10) => str_repeat('v', 118), 'key' => 'value', 'key2' => 'value2'], 50, 'key=value,key2=value2'], + [[str_repeat('a', 10) => str_repeat('v', 118), 'key' => 'value', 'key2' => 'value2'], 14, 'key=value'], + ]; + } + + #[DataProvider('parseProvider')] + public function test_parse(string $tracestate, ?string $expected): void + { + $traceState = new TraceState($tracestate); + $this->assertSame($expected ?? '', (string) $traceState); + } + + public static function parseProvider(): array + { + return [ + ['key=value', 'key=value'], + ['key=value,key2=value2', 'key=value,key2=value2'], + ['key=value,key2=value2,key=value3', 'key=value,key2=value2'], + + ['', ''], + [' ', ''], + ['key=value,,key2=value2', 'key=value,key2=value2'], + ['key=value, ,key2=value2', 'key=value,key2=value2'], + + ['0', null], + ['key =value', null], + ]; + } } From b7e254b643da9aca0b140c04700af30500a0982c Mon Sep 17 00:00:00 2001 From: Tobias Bachert Date: Mon, 1 Apr 2024 00:15:42 +0200 Subject: [PATCH 6/7] Assert that warning is triggered when parsing invalid tracestate --- tests/Unit/API/Trace/TraceStateTest.php | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/Unit/API/Trace/TraceStateTest.php b/tests/Unit/API/Trace/TraceStateTest.php index 4589e421c..6ed3f1a47 100644 --- a/tests/Unit/API/Trace/TraceStateTest.php +++ b/tests/Unit/API/Trace/TraceStateTest.php @@ -5,6 +5,7 @@ namespace OpenTelemetry\Tests\API\Unit\Trace; use function array_reverse; +use OpenTelemetry\API\Behavior\Internal\Logging; use OpenTelemetry\API\LoggerHolder; use OpenTelemetry\API\Trace\TraceState; use PHPUnit\Framework\Attributes\DataProvider; @@ -25,6 +26,7 @@ public function setUp(): void { $this->logger = $this->createMock(LoggerInterface::class); LoggerHolder::set($this->logger); + Logging::reset(); } public function test_get_tracestate_value(): void @@ -247,6 +249,11 @@ public static function toStringProvider(): array #[DataProvider('parseProvider')] public function test_parse(string $tracestate, ?string $expected): void { + $this->logger + ->expects($expected === null ? $this->once() : $this->never()) + ->method('log') + ->with('warning', $this->anything(), $this->anything()); + $traceState = new TraceState($tracestate); $this->assertSame($expected ?? '', (string) $traceState); } From cb4e37fd83ec5df06b7886471f6ae25a1c580d9a Mon Sep 17 00:00:00 2001 From: Tobias Bachert Date: Mon, 1 Apr 2024 00:34:23 +0200 Subject: [PATCH 7/7] Add additional tests for valid/invalid keys/values --- tests/Unit/API/Trace/TraceStateTest.php | 82 +++++++++++++++---------- 1 file changed, 51 insertions(+), 31 deletions(-) diff --git a/tests/Unit/API/Trace/TraceStateTest.php b/tests/Unit/API/Trace/TraceStateTest.php index 6ed3f1a47..3d04febde 100644 --- a/tests/Unit/API/Trace/TraceStateTest.php +++ b/tests/Unit/API/Trace/TraceStateTest.php @@ -12,7 +12,6 @@ use PHPUnit\Framework\TestCase; use Psr\Log\LoggerInterface; use function str_repeat; -use function strlen; /** * @covers \OpenTelemetry\API\Trace\TraceState @@ -149,7 +148,7 @@ public function test_max_tracestate_length(): void $this->assertSame($rawTraceState, (string) $validTracestate); } - public function test_validate_key(): void + public function test_parse_validate_key(): void { // Valid keys $validKeys = 'a-b=1,c*d=2,e/f=3,g_h=4,01@i-j=5'; @@ -168,25 +167,9 @@ public function test_validate_key(): void // Drop all keys on an invalid key $this->assertSame(0, $tracestate->getListMemberCount()); - - // Tests a valid key with a length of 256 characters. The max characters allowed. - $validKey = str_repeat('k', 256); - $validValue = 'v'; - $tracestate = new TraceState($validKey . TraceState::LIST_MEMBER_KEY_VALUE_SPLITTER . $validValue); - - $this->assertSame(256, strlen($validKey)); - $this->assertSame($validValue, $tracestate->get($validKey)); - $this->assertSame($validKey . TraceState::LIST_MEMBER_KEY_VALUE_SPLITTER . $validValue, (string) $tracestate); - - // Tests an invalid key with a length of 257 characters. One more than the max characters allowed. - $invalidKey = str_repeat('k', 257); - $tracestate = new TraceState($invalidKey . TraceState::LIST_MEMBER_KEY_VALUE_SPLITTER . $validValue); - - $this->assertSame(257, strlen($invalidKey)); - $this->assertNull($tracestate->get($invalidKey)); } - public function test_validate_value(): void + public function test_parse_validate_value(): void { // Tests values are within the range of 0x20 to 0x7E characters $tracestate = 'char1=value' . chr(0x19) . '1' @@ -198,22 +181,59 @@ public function test_validate_value(): void // Entire tracestate is dropped since the value for char1 is invalid. $this->assertSame(0, $parsedTracestate->getListMemberCount()); + } + + #[DataProvider('validKeyValueProvider')] + public function test_valid_key_value(string $key, string $value): void + { + $traceState = new TraceState(); + $traceState = $traceState->with($key, $value); + + $this->assertSame($value, $traceState->get($key)); + $this->assertSame($key . '=' . $value, (string) $traceState); + } - // Tests a valid value with a length of 256 characters. The max allowed. - $validValue = str_repeat('v', 256); - $validKey = 'k'; - $tracestate = new TraceState($validKey . TraceState::LIST_MEMBER_KEY_VALUE_SPLITTER . $validValue); + public static function validKeyValueProvider(): array + { + return [ + ['valid@key', 'value'], + ['valid@key', '0'], + ['abcdefghijklmnopqrstuvwxyz0123456789_-*/', 'value'], + ['abcdefghijklmnopqrstuvwxyz0123456789_-*/@a1234_-*/', 'value'], + ['key', strtr(implode(range(chr(0x20), chr(0x7E))), [',' => '', '=' => ''])], + [str_repeat('a', 256), 'value'], + ['key', str_repeat('a', 256)], + ]; + } - $this->assertSame(256, strlen($validValue)); - $this->assertSame($validValue, $tracestate->get($validKey)); - $this->assertSame($validKey . TraceState::LIST_MEMBER_KEY_VALUE_SPLITTER . $validValue, (string) $tracestate); + #[DataProvider('invalidKeyValueProvider')] + public function test_invalid_key_value(string $key, string $value): void + { + $traceState = new TraceState(); + $traceState = $traceState->with($key, $value); - // Tests an invalid value with a length of 257 characters. One more than the max allowed. - $invalidValue = str_repeat('v', 257); - $tracestate = new TraceState($validKey . TraceState::LIST_MEMBER_KEY_VALUE_SPLITTER . $invalidValue); + $this->assertNull($traceState->get($key)); + } - $this->assertSame(257, strlen($invalidValue)); - $this->assertNull($tracestate->get($validKey)); + public static function invalidKeyValueProvider(): array + { + return [ + ['', 'value'], + ['invalid.key', 'value'], + ['invalid-key@', 'value'], + ['0invalid-key', 'value'], + [str_repeat('a', 257), 'value'], + ['invalid@key_____________', 'value'], + ['-invalid-key@id', 'value'], + ['invalid-key@0id', 'value'], + ['invalid-key@@id', 'value'], + + ['key', ''], + ['key', 'invalid-value '], + ['key', 'invalid,value'], + ['key', 'invalid=value'], + ['key', str_repeat('a', 257)], + ]; } #[DataProvider('toStringProvider')]