fix(Aws): fixes wrong aws client name and region binding when appending middlewares

src/Aws/phpunit.xml.dist

@@ -39,6 +39,9 @@
         <testsuite name="unit">
             <directory>tests/Unit</directory>
         </testsuite>
+        <testsuite name="integration">
+            <directory>tests/Integration</directory>
+        </testsuite>
     </testsuites>
 
 </phpunit>

src/Aws/src/AwsSdkInstrumentation.php

@@ -8,12 +8,10 @@
 use Aws\ResultInterface;
 use OpenTelemetry\API\Instrumentation\InstrumentationInterface;
 use OpenTelemetry\API\Instrumentation\InstrumentationTrait;
-use OpenTelemetry\API\Trace\SpanInterface;
 use OpenTelemetry\API\Trace\SpanKind;
 use OpenTelemetry\API\Trace\TracerInterface;
 use OpenTelemetry\API\Trace\TracerProviderInterface;
 use OpenTelemetry\Context\Propagation\TextMapPropagatorInterface;
-use OpenTelemetry\Context\ScopeInterface;
 
 /**
  * @experimental
@@ -28,10 +26,6 @@ class AwsSdkInstrumentation implements InstrumentationInterface
     private TextMapPropagatorInterface $propagator;
     private TracerProviderInterface $tracerProvider;
     private $clients = [] ;
-    private string $clientName;
-    private string $region;
-    private SpanInterface $span;
-    private ScopeInterface $scope;
 
     public function getName(): string
     {
@@ -87,52 +81,55 @@ public function instrumentClients($clientsArray) : void
     public function activate(): bool
     {
         try {
-            $middleware = Middleware::tap(function ($cmd, $req) {
-                $tracer = $this->getTracer();
-                $propagator = $this->getPropagator();
-
-                $carrier = [];
-                /** @phan-suppress-next-line PhanTypeMismatchArgument */
-                $this->span = $tracer->spanBuilder($this->clientName)->setSpanKind(AwsSdkInstrumentation::SPAN_KIND)->startSpan();
-                $this->scope = $this->span->activate();
-
-                $propagator->inject($carrier);
-
-                /** @psalm-suppress PossiblyInvalidArgument */
-                $this->span->setAttributes([
-                    'rpc.method' => $cmd->getName(),
-                    'rpc.service' => $this->clientName,
-                    'rpc.system' => 'aws-api',
-                    'aws.region' => $this->region,
-                    ]);
-            });
-
-            /** @psalm-suppress PossiblyInvalidArgument */
-            $end_middleware = Middleware::mapResult(function (ResultInterface $result) {
-                /**
-                 * Some AWS SDK Funtions, such as S3Client->getObjectUrl() do not actually perform on the wire comms
-                 * with AWS Servers, and therefore do not return with a populated AWS\Result object with valid @metadata
-                 * Check for the presence of @metadata before extracting status code as these calls are still
-                 * instrumented.
-                 */
-                if (isset($result['@metadata'])) {
-                    $this->span->setAttributes([
-                        'http.status_code' => $result['@metadata']['statusCode'], //@phan-suppress-current-line PhanTypeMismatchDimFetch
-                    ]);
-                }
-
-                $this->span->end();
-                $this->scope->detach();
-
-                return $result;
-            });
-
             foreach ($this->clients as $client) {
-                $this->clientName = $client->getApi()->getServiceName();
-                $this->region = $client->getRegion();
+                $clientName = $client->getApi()->getServiceName();
+                $region = $client->getRegion();
+                $span = null;
+                $scope = null;
+
+                $client->getHandlerList()->prependInit(Middleware::tap(function ($cmd, $req) use ($clientName, $region, &$span, &$scope) {
+                    $tracer = $this->getTracer();
+                    $propagator = $this->getPropagator();
+
+                    $carrier = [];
+                    /** @phan-suppress-next-line PhanTypeMismatchArgument */
+                    $span = $tracer->spanBuilder($clientName)->setSpanKind(AwsSdkInstrumentation::SPAN_KIND)->startSpan();
+                    $scope = $span->activate();
+
+                    $propagator->inject($carrier);
+
+                    /** @psalm-suppress PossiblyInvalidArgument */
+                    $span->setAttributes([
+                        'rpc.method' => $cmd->getName(),
+                        'rpc.service' => $clientName,
+                        'rpc.system' => 'aws-api',
+                        'aws.region' => $region,
+                    ]);
+                }), 'instrumentation');
 
-                $client->getHandlerList()->prependInit($middleware, 'instrumentation');
-                $client->getHandlerList()->appendSign($end_middleware, 'end_instrumentation');
+                /** @psalm-suppress PossiblyInvalidArgument */
+                $client->getHandlerList()->appendSign(Middleware::mapResult(function (ResultInterface $result) use (&$span, &$scope) {
+                    if (null === $span || null === $scope) {
+                        return $result;
+                    }
+
+                    /**
+                     * Some AWS SDK Functions, such as S3Client->getObjectUrl() do not actually perform on the wire comms
+                     * with AWS Servers, and therefore do not return with a populated AWS\Result object with valid @metadata
+                     * Check for the presence of @metadata before extracting status code as these calls are still
+                     * instrumented.
+                     */
+                    if (isset($result['@metadata'])) {
+                        $span->setAttributes([
+                            'http.status_code' => $result['@metadata']['statusCode'], //@phan-suppress-current-line PhanTypeMismatchDimFetch
+                        ]);
+                    }
+
+                    $span->end();
+                    $scope->detach();
+
+                    return $result;
+                }), 'end_instrumentation');
             }
         } catch (\Throwable $e) {
             return false;

src/Aws/tests/Integration/AwsSdkInstrumentationTest.php

@@ -0,0 +1,134 @@
+<?php
+
+declare(strict_types=1);
+
+namespace OpenTelemetry\Tests\Aws\Integration;
+
+use OpenTelemetry\Aws\AwsSdkInstrumentation;
+use OpenTelemetry\Aws\Xray\Propagator;
+use OpenTelemetry\SDK\Trace\ReadWriteSpanInterface;
+use OpenTelemetry\SDK\Trace\TracerProvider;
+use PHPUnit\Framework\TestCase;
+
+class AwsSdkInstrumentationTest extends TestCase
+{
+    use UsesServiceTrait;
+
+    private AwsSdkInstrumentation $awsSdkInstrumentation;
+
+    protected function setUp(): void
+    {
+        $this->awsSdkInstrumentation = new AwsSdkInstrumentation();
+    }
+
+    public function testProperClientNameAndRegionIsPassedToSpanForSingleClientCall()
+    {
+        $sqsClient = $this->getTestClient('SQS', ['region' => 'eu-west-1']);
+        $s3Client = $this->getTestClient('S3', ['region' => 'us-east-1']);
+        $this->addMockResults($s3Client, [[]]);
+        $eventBridgeClient = $this->getTestClient('EventBridge', ['region' => 'ap-southeast-2']);
+
+        $spanProcessor = new CollectingSpanProcessor();
+        $this->awsSdkInstrumentation->instrumentClients([$sqsClient, $s3Client, $eventBridgeClient]);
+        $this->awsSdkInstrumentation->setPropagator(new Propagator());
+        $this->awsSdkInstrumentation->setTracerProvider(new TracerProvider([$spanProcessor]));
+        $this->awsSdkInstrumentation->init();
+        $this->awsSdkInstrumentation->activate();
+
+        $s3Client->listBuckets();
+
+        $collectedSpans = $spanProcessor->getCollectedSpans();
+        $this->assertCount(1, $collectedSpans);
+
+        /** @var ReadWriteSpanInterface $span */
+        $span = reset($collectedSpans);
+        $this->assertTrue($span->hasEnded());
+
+        $attributes = $span->toSpanData()->getAttributes()->toArray();
+        $this->assertArrayHasKey('rpc.service', $attributes);
+        $this->assertSame('s3', $attributes['rpc.service']);
+        $this->assertArrayHasKey('aws.region', $attributes);
+        $this->assertSame('us-east-1', $attributes['aws.region']);
+    }
+
+    public function testProperClientNameAndRegionIsPassedToSpanForDoubleCallToSameClient()
+    {
+        $sqsClient = $this->getTestClient('SQS', ['region' => 'eu-west-1']);
+        $s3Client = $this->getTestClient('S3', ['region' => 'us-east-1']);
+        $this->addMockResults($s3Client, [[], []]);
+        $eventBridgeClient = $this->getTestClient('EventBridge', ['region' => 'ap-southeast-2']);
+
+        $spanProcessor = new CollectingSpanProcessor();
+        $this->awsSdkInstrumentation->instrumentClients([$sqsClient, $s3Client, $eventBridgeClient]);
+        $this->awsSdkInstrumentation->setPropagator(new Propagator());
+        $this->awsSdkInstrumentation->setTracerProvider(new TracerProvider([$spanProcessor]));
+        $this->awsSdkInstrumentation->init();
+        $this->awsSdkInstrumentation->activate();
+
+        $s3Client->listBuckets();
+        $s3Client->listObjects(['Bucket' => 'foo']);
+
+        $collectedSpans = $spanProcessor->getCollectedSpans();
+        $this->assertCount(2, $collectedSpans);
+
+        /** @var ReadWriteSpanInterface $span */
+        foreach ($collectedSpans as $span) {
+            $this->assertTrue($span->hasEnded());
+            $attributes = $span->toSpanData()->getAttributes()->toArray();
+            $this->assertArrayHasKey('rpc.service', $attributes);
+            $this->assertSame('s3', $attributes['rpc.service']);
+            $this->assertArrayHasKey('aws.region', $attributes);
+            $this->assertSame('us-east-1', $attributes['aws.region']);
+        }
+    }
+
+    public function testProperClientNameAndRegionIsPassedToSpanForDoubleCallToDifferentClients()
+    {
+        $sqsClient = $this->getTestClient('SQS', ['region' => 'eu-west-1']);
+        $s3Client = $this->getTestClient('S3', ['region' => 'us-east-1']);
+        $this->addMockResults($s3Client, [[]]);
+        $eventBridgeClient = $this->getTestClient('EventBridge', ['region' => 'ap-southeast-2']);
+        $this->addMockResults($eventBridgeClient, [[]]);
+
+        $spanProcessor = new CollectingSpanProcessor();
+        $this->awsSdkInstrumentation->instrumentClients([$sqsClient, $s3Client, $eventBridgeClient]);
+        $this->awsSdkInstrumentation->setPropagator(new Propagator());
+        $this->awsSdkInstrumentation->setTracerProvider(new TracerProvider([$spanProcessor]));
+        $this->awsSdkInstrumentation->init();
+        $this->awsSdkInstrumentation->activate();
+
+        $eventBridgeClient->putEvents([
+            'Entries' => [
+                [
+                    'Version' => 1,
+                    'EventBusName' => 'foo',
+                    'Source' => 'bar',
+                    'DetailType' => 'type',
+                    'Detail' => '{}'
+                ]
+            ]
+        ]);
+        $s3Client->listBuckets();
+
+        $collectedSpans = $spanProcessor->getCollectedSpans();
+        $this->assertCount(2, $collectedSpans);
+
+        /** @var ReadWriteSpanInterface $span */
+        $span = array_pop($collectedSpans);
+        $this->assertTrue($span->hasEnded());
+        $attributes = $span->toSpanData()->getAttributes()->toArray();
+        $this->assertArrayHasKey('rpc.service', $attributes);
+        $this->assertSame('s3', $attributes['rpc.service']);
+        $this->assertArrayHasKey('aws.region', $attributes);
+        $this->assertSame('us-east-1', $attributes['aws.region']);
+
+        /** @var ReadWriteSpanInterface $span */
+        $span = array_pop($collectedSpans);
+        $this->assertTrue($span->hasEnded());
+        $attributes = $span->toSpanData()->getAttributes()->toArray();
+        $this->assertArrayHasKey('rpc.service', $attributes);
+        $this->assertSame('eventbridge', $attributes['rpc.service']);
+        $this->assertArrayHasKey('aws.region', $attributes);
+        $this->assertSame('ap-southeast-2', $attributes['aws.region']);
+    }
+}

src/Aws/tests/Integration/CollectingSpanProcessor.php

@@ -0,0 +1,39 @@
+<?php
+
+namespace OpenTelemetry\Tests\Aws\Integration;
+
+use OpenTelemetry\Context\ContextInterface;
+use OpenTelemetry\SDK\Common\Future\CancellationInterface;
+use OpenTelemetry\SDK\Trace\ReadableSpanInterface;
+use OpenTelemetry\SDK\Trace\ReadWriteSpanInterface;
+use OpenTelemetry\SDK\Trace\SpanProcessorInterface;
+
+class CollectingSpanProcessor implements SpanProcessorInterface
+{
+    private array $collectedSpans = [];
+
+    public function onStart(ReadWriteSpanInterface $span, ContextInterface $parentContext): void
+    {
+        $this->collectedSpans[$span->getContext()->getSpanId()] = $span;
+    }
+
+    public function onEnd(ReadableSpanInterface $span): void
+    {
+        $this->collectedSpans[$span->getContext()->getSpanId()] = $span;
+    }
+
+    public function forceFlush(?CancellationInterface $cancellation = null): bool
+    {
+        return true;
+    }
+
+    public function shutdown(?CancellationInterface $cancellation = null): bool
+    {
+        return true;
+    }
+
+    public function getCollectedSpans(): array
+    {
+        return $this->collectedSpans;
+    }
+}