rework ServiceInfoFactory priorities (#1644)

* rework ServiceInfoFactory priorities
- default/all: load detectors from registry last
- env-provided detectors: load in same order as default/all, then from registry in requested order

* split service.instance.id into own detector
to avoid always generating service.instance.id (which is useless for fpm/apache), split it
into its own detector, ServiceInstance, and don't use it by default. If it is enabled, ensure
it runs before registry-provided detectors.

* Update src/SDK/Resource/ResourceInfoFactory.php

Co-authored-by: Chris Lightfoot-Wild <[email protected]>

* override

---------

Co-authored-by: Chris Lightfoot-Wild <[email protected]>

Author: brettmc

src/SDK/Common/Configuration/KnownValues.php

@@ -197,6 +197,7 @@ interface KnownValues
     public const VALUE_DETECTORS_SDK = 'sdk';
     public const VALUE_DETECTORS_SDK_PROVIDED = 'sdk_provided';
     public const VALUE_DETECTORS_SERVICE = 'service';
+    public const VALUE_DETECTORS_SERVICE_INSTANCE = 'service_instance';
     public const VALUE_DETECTORS_COMPOSER = 'composer';
     public const OTEL_PHP_DETECTORS = [
         self::VALUE_ALL,

src/SDK/Resource/Detectors/Service.php

@@ -10,7 +10,6 @@
 use OpenTelemetry\SDK\Resource\ResourceDetectorInterface;
 use OpenTelemetry\SDK\Resource\ResourceInfo;
 use OpenTelemetry\SemConv\ResourceAttributes;
-use Ramsey\Uuid\Uuid;
 
 /**
  * @see https://github.com/open-telemetry/semantic-conventions/tree/main/docs/resource#service-experimental
@@ -20,18 +19,13 @@ final class Service implements ResourceDetectorInterface
     #[\Override]
     public function getResource(): ResourceInfo
     {
-        static $serviceInstanceId;
-        $serviceInstanceId ??= Uuid::uuid4()->toString();
         $serviceName = Configuration::has(Variables::OTEL_SERVICE_NAME)
             ? Configuration::getString(Variables::OTEL_SERVICE_NAME)
             : null;
 
         $attributes = [
-            ResourceAttributes::SERVICE_INSTANCE_ID => $serviceInstanceId,
+            ResourceAttributes::SERVICE_NAME => $serviceName,
         ];
-        if ($serviceName !== null) {
-            $attributes[ResourceAttributes::SERVICE_NAME] = $serviceName;
-        }
 
         return ResourceInfo::create(Attributes::create($attributes), ResourceAttributes::SCHEMA_URL);
     }

src/SDK/Resource/Detectors/ServiceInstance.php

@@ -0,0 +1,33 @@
+<?php
+
+declare(strict_types=1);
+
+namespace OpenTelemetry\SDK\Resource\Detectors;
+
+use OpenTelemetry\SDK\Common\Attribute\Attributes;
+use OpenTelemetry\SDK\Resource\ResourceDetectorInterface;
+use OpenTelemetry\SDK\Resource\ResourceInfo;
+use OpenTelemetry\SemConv\ResourceAttributes;
+use Ramsey\Uuid\Uuid;
+
+/**
+ * @see https://github.com/open-telemetry/semantic-conventions/tree/main/docs/resource#service-experimental
+ *
+ * Spec deviation: Service Instance ID is not generated by the Service detector, as UUID is not useful in shared-nothing
+ * PHP setups (FPM, Apache), and cannot be replaced by a more useful value due to Service being the last detector.
+ */
+final class ServiceInstance implements ResourceDetectorInterface
+{
+    #[\Override]
+    public function getResource(): ResourceInfo
+    {
+        static $serviceInstanceId;
+        $serviceInstanceId ??= Uuid::uuid4()->toString();
+
+        $attributes = [
+            ResourceAttributes::SERVICE_INSTANCE_ID => $serviceInstanceId,
+        ];
+
+        return ResourceInfo::create(Attributes::create($attributes), ResourceAttributes::SCHEMA_URL);
+    }
+}

src/SDK/Resource/ResourceInfoFactory.php

@@ -29,50 +29,51 @@ public static function defaultResource(): ResourceInfo
             return (new Detectors\Composite([
                 new Detectors\Host(),
                 new Detectors\Process(),
-                ...Registry::resourceDetectors(),
                 new Detectors\Environment(),
                 new Detectors\Sdk(),
                 new Detectors\Service(),
+                ...Registry::resourceDetectors(),
             ]))->getResource();
         }
 
+        /**
+         * Process env-provided detectors:
+         * - host, process, environment, composer first if requested
+         * - sdk, service always
+         * - any other detectors registered in the registry if requested
+         */
         $resourceDetectors = [];
-
-        foreach ($detectors as $detector) {
-            switch ($detector) {
-                case Values::VALUE_DETECTORS_ENVIRONMENT:
-                    $resourceDetectors[] = new Detectors\Environment();
-
-                    break;
-                case Values::VALUE_DETECTORS_HOST:
-                    $resourceDetectors[] = new Detectors\Host();
-
-                    break;
-                case Values::VALUE_DETECTORS_PROCESS:
-                    $resourceDetectors[] = new Detectors\Process();
-
-                    break;
-
-                case Values::VALUE_DETECTORS_COMPOSER:
-                    $resourceDetectors[] = new Detectors\Composer();
-
-                    break;
-                case Values::VALUE_DETECTORS_SDK_PROVIDED: //deprecated
-                case Values::VALUE_DETECTORS_OS: //deprecated
-                case Values::VALUE_DETECTORS_PROCESS_RUNTIME: //deprecated
-                case Values::VALUE_NONE:
-
-                    break;
-                default:
-                    try {
-                        $resourceDetectors[] = Registry::resourceDetector($detector);
-                    } catch (RuntimeException $e) {
-                        self::logWarning($e->getMessage());
-                    }
+        foreach ([
+            Values::VALUE_DETECTORS_HOST => Detectors\Host::class,
+            Values::VALUE_DETECTORS_PROCESS => Detectors\Process::class,
+            Values::VALUE_DETECTORS_ENVIRONMENT => Detectors\Environment::class,
+            Values::VALUE_DETECTORS_COMPOSER => Detectors\Composer::class,
+            Values::VALUE_DETECTORS_SERVICE_INSTANCE => Detectors\ServiceInstance::class,
+        ] as $detector => $class) {
+            if (in_array($detector, $detectors)) {
+                $resourceDetectors[] = new $class();
+                $detectors = array_diff($detectors, [$detector]);
             }
         }
         $resourceDetectors [] = new Detectors\Sdk();
         $resourceDetectors [] = new Detectors\Service();
+        // Don't try to load mandatory + deprecated detectors
+        $detectors = array_diff($detectors, [
+            Values::VALUE_DETECTORS_SDK,
+            Values::VALUE_DETECTORS_SERVICE,
+            Values::VALUE_DETECTORS_SDK_PROVIDED, //deprecated
+            Values::VALUE_DETECTORS_OS, //deprecated
+            Values::VALUE_DETECTORS_PROCESS_RUNTIME, //deprecated
+            Values::VALUE_NONE,
+        ]);
+
+        foreach ($detectors as $detector) {
+            try {
+                $resourceDetectors[] = Registry::resourceDetector($detector);
+            } catch (RuntimeException $e) {
+                self::logWarning($e->getMessage());
+            }
+        }
 
         return (new Detectors\Composite($resourceDetectors))->getResource();
     }

tests/Integration/Config/ConfigurationTest.php

@@ -127,7 +127,6 @@ public function test_resource_include_exclude(): void
             'process.executable.path',
             'process.owner',
             'process.runtime.name',
-            'service.instance.id',
             'service.name',
             'telemetry.distro.name',
             'telemetry.distro.version',
@@ -148,7 +147,6 @@ public function test_resource_defaults(): void
     {
         $expectedKeys = [
             'service.name',
-            'service.instance.id',
             'telemetry.distro.name',
             'telemetry.distro.version',
             'telemetry.sdk.language',

tests/Integration/SDK/Resource/ResourceInfoFactoryTest.php

@@ -171,4 +171,16 @@ public function test_composer_default_resources(): void
         $this->assertEquals('open-telemetry/opentelemetry', $resource->getAttributes()->get(ResourceAttributes::SERVICE_NAME));
         $this->assertEquals(InstalledVersions::getRootPackage()['pretty_version'], $resource->getAttributes()->get(ResourceAttributes::SERVICE_VERSION));
     }
+
+    public function test_env_doesnt_clobber_service(): void
+    {
+        //service.name should be provided by mandatory Service detector, and should not be clobbered by environment detector
+        $this->setEnvironmentVariable('OTEL_SERVICE_NAME', 'test-service');
+        $this->setEnvironmentVariable('OTEL_RESOURCE_ATTRIBUTES', 'service.name=ignore-me');
+        $this->setEnvironmentVariable('OTEL_PHP_DETECTORS', 'env');
+
+        $resource = ResourceInfoFactory::defaultResource();
+
+        $this->assertEquals('test-service', $resource->getAttributes()->get(ResourceAttributes::SERVICE_NAME));
+    }
 }

tests/Unit/SDK/Resource/Detectors/ServiceInstanceIdTest.php

@@ -0,0 +1,45 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Unit\SDK\Resource\Detectors;
+
+use OpenTelemetry\SDK\Resource\Detectors;
+use OpenTelemetry\SemConv\ResourceAttributes;
+use OpenTelemetry\Tests\TestState;
+use PHPUnit\Framework\Attributes\CoversClass;
+use PHPUnit\Framework\TestCase;
+
+#[CoversClass(Detectors\ServiceInstance::class)]
+class ServiceInstanceIdTest extends TestCase
+{
+    use TestState;
+
+    const UUID_REGEX = '/^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i';
+
+    private Detectors\ServiceInstance $detector;
+
+    #[\Override]
+    public function setUp(): void
+    {
+        $this->detector = new Detectors\ServiceInstance();
+    }
+
+    public function test_service_get_resource_with_default_service_instance_id(): void
+    {
+        $resource = $this->detector->getResource();
+
+        $this->assertNotEmpty($resource->getAttributes());
+
+        $id = $resource->getAttributes()->get(ResourceAttributes::SERVICE_INSTANCE_ID);
+        $this->assertMatchesRegularExpression(self::UUID_REGEX, $id);
+    }
+
+    public function test_service_get_resource_multiple_calls_same_service_instance_id(): void
+    {
+        $resource1 = $this->detector->getResource();
+        $resource2 = $this->detector->getResource();
+
+        $this->assertSame($resource1->getAttributes()->get(ResourceAttributes::SERVICE_INSTANCE_ID), $resource2->getAttributes()->get(ResourceAttributes::SERVICE_INSTANCE_ID));
+    }
+}

tests/Unit/SDK/Resource/Detectors/ServiceTest.php

@@ -15,8 +15,6 @@ class ServiceTest extends TestCase
 {
     use TestState;
 
-    const UUID_REGEX = '/^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i';
-
     private Detectors\Service $detector;
 
     #[\Override]
@@ -25,24 +23,6 @@ public function setUp(): void
         $this->detector = new Detectors\Service();
     }
 
-    public function test_service_get_resource_with_default_service_instance_id(): void
-    {
-        $resource = $this->detector->getResource();
-
-        $this->assertNotEmpty($resource->getAttributes());
-
-        $id = $resource->getAttributes()->get(ResourceAttributes::SERVICE_INSTANCE_ID);
-        $this->assertMatchesRegularExpression(self::UUID_REGEX, $id);
-    }
-
-    public function test_service_get_resource_multiple_calls_same_service_instance_id(): void
-    {
-        $resource1 = $this->detector->getResource();
-        $resource2 = $this->detector->getResource();
-
-        $this->assertSame($resource1->getAttributes()->get(ResourceAttributes::SERVICE_INSTANCE_ID), $resource2->getAttributes()->get(ResourceAttributes::SERVICE_INSTANCE_ID));
-    }
-
     public function test_sdk_get_resource_with_service_name(): void
     {
         $this->setEnvironmentVariable('OTEL_SERVICE_NAME', 'test-service');

tests/Unit/SDK/Resource/ResourceInfoFactoryTest.php

@@ -108,10 +108,10 @@ public function test_resource_with_invalid_environment_variable(): void
     #[Group('compliance')]
     public function test_resource_from_environment_service_name_takes_precedence_over_resource_attribute(): void
     {
-        $this->setEnvironmentVariable('OTEL_RESOURCE_ATTRIBUTES', 'service.name=bar');
-        $this->setEnvironmentVariable('OTEL_SERVICE_NAME', 'foo');
+        $this->setEnvironmentVariable('OTEL_RESOURCE_ATTRIBUTES', 'service.name=from-resource-attributes');
+        $this->setEnvironmentVariable('OTEL_SERVICE_NAME', 'from-service-name');
         $resource = ResourceInfoFactory::defaultResource();
-        $this->assertEquals('foo', $resource->getAttributes()->get('service.name'));
+        $this->assertEquals('from-service-name', $resource->getAttributes()->get('service.name'));
     }
 
     #[Group('compliance')]
@@ -189,11 +189,20 @@ public function test_environment_get_resource_service_name_precedence_over_resou
     {
         $this->setEnvironmentVariable('OTEL_RESOURCE_ATTRIBUTES', 'service.name=from-env');
         $this->setEnvironmentVariable('OTEL_SERVICE_NAME', 'from-service-name');
-        $this->setEnvironmentVariable('OTEL_PHP_DETECTORS', 'environment');
+        $this->setEnvironmentVariable('OTEL_PHP_DETECTORS', 'env');
 
         $resource = ResourceInfoFactory::defaultResource();
 
         $this->assertNotEmpty($resource->getAttributes());
         $this->assertSame('from-service-name', $resource->getAttributes()->get(ResourceAttributes::SERVICE_NAME));
     }
+
+    public function test_service_instance_detector(): void
+    {
+        $this->setEnvironmentVariable('OTEL_PHP_DETECTORS', 'service_instance');
+
+        $resource = ResourceInfoFactory::defaultResource();
+
+        $this->assertNotEmpty($resource->getAttributes()->get(ResourceAttributes::SERVICE_INSTANCE_ID));
+    }
 }