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

Issue:
Decode values from `OTEL_RESOURCE_ATTRIBUTES`
While using the Go SDK I noticed an inconsistency in how `OTEL_RESOURCE_ATTRIBUTES` baggage values are encoded and decoded. This was due to a difference in the version of the W3C Baggage spec used for [baggage propagation](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/baggage/api.md#propagation) and the [`OTEL_RESOURCE_ATTRIBUTES` decoding](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/resource/sdk.md#specifying-resource-information-via-an-environment-variable).

https://github.com/open-telemetry/opentelemetry-specification/pull/2670 updates the W3C Baggage spec version used by the Resource spec to require percent-decoding values from  `OTEL_RESOURCE_ATTRIBUTES`.

Pull request description:
decode OTEL_RESOURCE_ATTRIBUTES values
per spec, resource attributes coming from environment should be decoded in the same way that they would be if coming from a header

Closes #786 

Patch:
diff --git a/src/SDK/Common/Configuration/Configuration.php b/src/SDK/Common/Configuration/Configuration.php
index 68d89b1d..cb45935b 100644
--- a/src/SDK/Common/Configuration/Configuration.php
+++ b/src/SDK/Common/Configuration/Configuration.php
@@ -4,6 +4,8 @@ declare(strict_types=1);
 
 namespace OpenTelemetry\SDK\Common\Configuration;
 
+use InvalidArgumentException;
+use OpenTelemetry\SDK\Behavior\LogsMessagesTrait;
 use OpenTelemetry\SDK\Common\Configuration\Parser\BooleanParser;
 use OpenTelemetry\SDK\Common\Configuration\Parser\ListParser;
 use OpenTelemetry\SDK\Common\Configuration\Parser\MapParser;
@@ -20,6 +22,8 @@ use UnexpectedValueException;
  */
 class Configuration
 {
+    use LogsMessagesTrait;
+
     public static function has(string $name): bool
     {
         return CompositeResolver::instance()->hasVariable($name);
@@ -48,14 +52,20 @@ class Configuration
 
     public static function getBoolean(string $key, bool $default = null): bool
     {
-        return BooleanParser::parse(
-            self::validateVariableValue(
-                CompositeResolver::instance()->resolve(
-                    self::validateVariableType($key, VariableTypes::BOOL),
-                    null === $default ? $default : ($default ? 'true' : 'false')
-                )
+        $resolved = self::validateVariableValue(
+            CompositeResolver::instance()->resolve(
+                self::validateVariableType($key, VariableTypes::BOOL),
+                null === $default ? $default : ($default ? 'true' : 'false')
             )
         );
+
+        try {
+            return BooleanParser::parse($resolved);
+        } catch (InvalidArgumentException $e) {
+            self::logWarning(sprintf('Invalid boolean value "%s" interpreted as "false" for %s', $resolved, $key));
+
+            return false;
+        }
     }
 
     public static function getMixed(string $key, $default = null)
diff --git a/src/SDK/Common/Configuration/KnownValues.php b/src/SDK/Common/Configuration/KnownValues.php
index 2d7548f4..f34a22dd 100644
--- a/src/SDK/Common/Configuration/KnownValues.php
+++ b/src/SDK/Common/Configuration/KnownValues.php
@@ -60,10 +60,6 @@ interface KnownValues
     public const VALUES_BOOLEAN = [
         self::VALUE_TRUE,
         self::VALUE_FALSE,
-        self::VALUE_ON,
-        self::VALUE_OFF,
-        self::VALUE_1,
-        self::VALUE_0,
     ];
 
     public const VALUES_COMPRESSION= [
diff --git a/src/SDK/Common/Configuration/Parser/BooleanParser.php b/src/SDK/Common/Configuration/Parser/BooleanParser.php
index 7ac2d61f..4141c61e 100644
--- a/src/SDK/Common/Configuration/Parser/BooleanParser.php
+++ b/src/SDK/Common/Configuration/Parser/BooleanParser.php
@@ -8,17 +8,8 @@ use InvalidArgumentException;
 
 class BooleanParser
 {
-    private const TRUTHY_VALUES = [
-        'true',
-        'on',
-        '1',
-    ];
-
-    private const FALSY_VALUES = [
-        'false',
-        'off',
-        '0',
-    ];
+    private const TRUE_VALUE = 'true';
+    private const FALSE_VALUE = 'false';
 
     /**
      * @param string|bool $value
@@ -28,11 +19,11 @@ class BooleanParser
         if (is_bool($value)) {
             return $value;
         }
-        if (in_array(strtolower($value), self::TRUTHY_VALUES)) {
+        if (strtolower($value) === self::TRUE_VALUE) {
             return true;
         }
 
-        if (in_array(strtolower($value), self::FALSY_VALUES)) {
+        if (strtolower($value) === self::FALSE_VALUE) {
             return false;
         }
 
diff --git a/src/SDK/Resource/Detectors/Environment.php b/src/SDK/Resource/Detectors/Environment.php
index 05a914c4..1c821bbf 100644
--- a/src/SDK/Resource/Detectors/Environment.php
+++ b/src/SDK/Resource/Detectors/Environment.php
@@ -19,7 +19,7 @@ final class Environment implements ResourceDetectorInterface
     public function getResource(): ResourceInfo
     {
         $attributes = Configuration::has(Variables::OTEL_RESOURCE_ATTRIBUTES)
-            ? Configuration::getMap(Variables::OTEL_RESOURCE_ATTRIBUTES, [])
+            ? self::decode(Configuration::getMap(Variables::OTEL_RESOURCE_ATTRIBUTES, []))
             : [];
 
         //@see https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/sdk-environment-variables.md#general-sdk-configuration
@@ -32,4 +32,9 @@ final class Environment implements ResourceDetectorInterface
 
         return ResourceInfo::create(Attributes::create($attributes), ResourceAttributes::SCHEMA_URL);
     }
+
+    private static function decode(array $attributes): array
+    {
+        return array_map('urldecode', $attributes);
+    }
 }


Test patch:
diff --git a/tests/Unit/SDK/Common/Configuration/ConfigurationTest.php b/tests/Unit/SDK/Common/Configuration/ConfigurationTest.php
index 71f43845..64b5ead9 100644
--- a/tests/Unit/SDK/Common/Configuration/ConfigurationTest.php
+++ b/tests/Unit/SDK/Common/Configuration/ConfigurationTest.php
@@ -188,8 +188,6 @@ class ConfigurationTest extends TestCase
         return [
             'false' => ['false', true, false],
             'true' => ['true', false, true],
-            'truthy' => ['1', false, true],
-            'falsey' => ['0', true, false],
             'TRUE' => ['TRUE', false, true],
             'FALSE' => ['FALSE', true, false],
         ];
diff --git a/tests/Unit/SDK/Common/Configuration/Parser/BooleanParserTest.php b/tests/Unit/SDK/Common/Configuration/Parser/BooleanParserTest.php
index 2d4a0b95..ed478211 100644
--- a/tests/Unit/SDK/Common/Configuration/Parser/BooleanParserTest.php
+++ b/tests/Unit/SDK/Common/Configuration/Parser/BooleanParserTest.php
@@ -16,17 +16,13 @@ class BooleanParserTest extends TestCase
     private const TRUTHY_VALUES = [
         'bool uppercase' => ['TRUE'],
         'bool lowercase' => ['true'],
-        'state uppercase' => ['ON'],
-        'state lowercase' => ['on'],
-        'int' => ['1'],
+        'bool mixed case' => ['True'],
     ];
 
     private const FALSY_VALUES = [
         'bool uppercase' => ['FALSE'],
         'bool lowercase' => ['false'],
-        'state uppercase' => ['OFF'],
-        'state lowercase' => ['off'],
-        'int' => ['0'],
+        'bool mixed case' => ['False'],
     ];
 
     private const NON_BOOLEAN_VALUES = [
@@ -35,6 +31,17 @@ class BooleanParserTest extends TestCase
         'float' => ['0.5'],
     ];
 
+    private const DISALLOWED_BOOLEAN_VALUES = [
+        ['ON'],
+        ['on'],
+        ['On'],
+        ['1'],
+        ['OFF'],
+        ['off'],
+        ['Off'],
+        ['0'],
+    ];
+
     /**
      * @dataProvider truthyValueProvider
      */
@@ -55,6 +62,20 @@ class BooleanParserTest extends TestCase
         );
     }
 
+    /**
+     * @dataProvider disallowedBooleanProvider
+     */
+    public function test_disallowed_boolean_type_values_throw_exception(string $value): void
+    {
+        $this->expectException(InvalidArgumentException::class);
+        BooleanParser::parse($value);
+    }
+
+    public function disallowedBooleanProvider(): array
+    {
+        return self::DISALLOWED_BOOLEAN_VALUES;
+    }
+
     /**
      * @dataProvider nonBooleanValueProvider
      */
diff --git a/tests/Unit/SDK/Resource/Detectors/EnvironmentTest.php b/tests/Unit/SDK/Resource/Detectors/EnvironmentTest.php
index 93788ca3..dae7e208 100644
--- a/tests/Unit/SDK/Resource/Detectors/EnvironmentTest.php
+++ b/tests/Unit/SDK/Resource/Detectors/EnvironmentTest.php
@@ -16,6 +16,13 @@ class EnvironmentTest extends TestCase
 {
     use EnvironmentVariables;
 
+    private Detectors\Environment $detector;
+
+    public function setUp(): void
+    {
+        $this->detector = new Detectors\Environment();
+    }
+
     public function tearDown(): void
     {
         $this->restoreEnvironmentVariables();
@@ -23,8 +30,7 @@ class EnvironmentTest extends TestCase
 
     public function test_environment_default_get_resource(): void
     {
-        $resouceDetector = new Detectors\Environment();
-        $resource = $resouceDetector->getResource();
+        $resource = $this->detector->getResource();
 
         $this->assertSame(ResourceAttributes::SCHEMA_URL, $resource->getSchemaUrl());
         $this->assertEmpty($resource->getAttributes());
@@ -35,19 +41,38 @@ class EnvironmentTest extends TestCase
     {
         $this->setEnvironmentVariable('OTEL_RESOURCE_ATTRIBUTES', 'key_foo=value_foo,key_bar=value_bar');
 
-        $resouceDetector = new Detectors\Environment();
-        $resource = $resouceDetector->getResource();
+        $resource = $this->detector->getResource();
 
         $this->assertSame('value_foo', $resource->getAttributes()->get('key_foo'));
         $this->assertSame('value_bar', $resource->getAttributes()->get('key_bar'));
     }
 
+    /**
+     * @dataProvider encodedResourceValueProvider
+     */
+    public function test_environment_get_resource_with_encoded_value(string $value, string $expected): void
+    {
+        $key = 'key';
+        $this->setEnvironmentVariable('OTEL_RESOURCE_ATTRIBUTES', sprintf('%s=%s', $key, $value));
+
+        $resource = $this->detector->getResource();
+
+        $this->assertSame($expected, $resource->getAttributes()->get($key));
+    }
+
+    public function encodedResourceValueProvider(): array
+    {
+        return [
+            ['%28%24foo%29', '($foo)'],
+            ['%21%40%23%24%25%5E', '!@#$%^'],
+        ];
+    }
+
     public function test_environment_get_resource_with_service_name(): void
     {
         $this->setEnvironmentVariable('OTEL_SERVICE_NAME', 'test-service');
 
-        $resouceDetector = new Detectors\Environment();
-        $resource = $resouceDetector->getResource();
+        $resource = $this->detector->getResource();
 
         $this->assertNotEmpty($resource->getAttributes());
         $this->assertSame('test-service', $resource->getAttributes()->get(ResourceAttributes::SERVICE_NAME));
@@ -57,8 +82,7 @@ class EnvironmentTest extends TestCase
     {
         $this->setEnvironmentVariable('OTEL_RESOURCE_ATTRIBUTES', 'service.name=test-service');
 
-        $resouceDetector = new Detectors\Environment();
-        $resource = $resouceDetector->getResource();
+        $resource = $this->detector->getResource();
 
         $this->assertNotEmpty($resource->getAttributes());
         $this->assertSame('test-service', $resource->getAttributes()->get(ResourceAttributes::SERVICE_NAME));
@@ -69,8 +93,7 @@ class EnvironmentTest extends TestCase
         $this->setEnvironmentVariable('OTEL_RESOURCE_ATTRIBUTES', 'service.name=env-test-service');
         $this->setEnvironmentVariable('OTEL_SERVICE_NAME', 'user-test-service');
 
-        $resouceDetector = new Detectors\Environment();
-        $resource = $resouceDetector->getResource();
+        $resource = $this->detector->getResource();
 
         $this->assertNotEmpty($resource->getAttributes());
         $this->assertSame('user-test-service', $resource->getAttributes()->get(ResourceAttributes::SERVICE_NAME));
diff --git a/tests/Unit/SDK/SdkTest.php b/tests/Unit/SDK/SdkTest.php
index 69047f31..ab52a78c 100644
--- a/tests/Unit/SDK/SdkTest.php
+++ b/tests/Unit/SDK/SdkTest.php
@@ -41,9 +41,7 @@ class SdkTest extends TestCase
     {
         return [
             ['true', true],
-            ['1', true],
             ['false', false],
-            ['0', false],
         ];
     }