Skip to content

Commit a00a991

Browse files
rtrefflerrtreffler
committed
fix(Aws): fixes wrong aws client name and region binding when appending middlewares
Binding happened outside the foreach loop resulting in binding the same values for all instrumented clients. Similar problem was affecting span and scope object variables, where multiple clients could overwrite each other spans.
1 parent ee99079 commit a00a991

File tree

5 files changed

+372
-50
lines changed

5 files changed

+372
-50
lines changed

‎src/Aws/phpunit.xml.dist‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,9 @@
3939
<testsuite name="unit">
4040
<directory>tests/Unit</directory>
4141
</testsuite>
42+
<testsuite name="integration">
43+
<directory>tests/Integration</directory>
44+
</testsuite>
4245
</testsuites>
4346

4447
</phpunit>

‎src/Aws/src/AwsSdkInstrumentation.php‎

Lines changed: 47 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -8,12 +8,10 @@
88
use Aws\ResultInterface;
99
use OpenTelemetry\API\Instrumentation\InstrumentationInterface;
1010
use OpenTelemetry\API\Instrumentation\InstrumentationTrait;
11-
use OpenTelemetry\API\Trace\SpanInterface;
1211
use OpenTelemetry\API\Trace\SpanKind;
1312
use OpenTelemetry\API\Trace\TracerInterface;
1413
use OpenTelemetry\API\Trace\TracerProviderInterface;
1514
use OpenTelemetry\Context\Propagation\TextMapPropagatorInterface;
16-
use OpenTelemetry\Context\ScopeInterface;
1715

1816
/**
1917
* @experimental
@@ -28,10 +26,6 @@ class AwsSdkInstrumentation implements InstrumentationInterface
2826
private TextMapPropagatorInterface $propagator;
2927
private TracerProviderInterface $tracerProvider;
3028
private $clients = [] ;
31-
private string $clientName;
32-
private string $region;
33-
private SpanInterface $span;
34-
private ScopeInterface $scope;
3529

3630
public function getName(): string
3731
{
@@ -87,52 +81,55 @@ public function instrumentClients($clientsArray) : void
8781
public function activate(): bool
8882
{
8983
try {
90-
$middleware = Middleware::tap(function ($cmd, $req) {
91-
$tracer = $this->getTracer();
92-
$propagator = $this->getPropagator();
93-
94-
$carrier = [];
95-
/** @phan-suppress-next-line PhanTypeMismatchArgument */
96-
$this->span = $tracer->spanBuilder($this->clientName)->setSpanKind(AwsSdkInstrumentation::SPAN_KIND)->startSpan();
97-
$this->scope = $this->span->activate();
98-
99-
$propagator->inject($carrier);
100-
101-
/** @psalm-suppress PossiblyInvalidArgument */
102-
$this->span->setAttributes([
103-
'rpc.method' => $cmd->getName(),
104-
'rpc.service' => $this->clientName,
105-
'rpc.system' => 'aws-api',
106-
'aws.region' => $this->region,
107-
]);
108-
});
109-
110-
/** @psalm-suppress PossiblyInvalidArgument */
111-
$end_middleware = Middleware::mapResult(function (ResultInterface $result) {
112-
/**
113-
* Some AWS SDK Funtions, such as S3Client->getObjectUrl() do not actually perform on the wire comms
114-
* with AWS Servers, and therefore do not return with a populated AWS\Result object with valid @metadata
115-
* Check for the presence of @metadata before extracting status code as these calls are still
116-
* instrumented.
117-
*/
118-
if (isset($result['@metadata'])) {
119-
$this->span->setAttributes([
120-
'http.status_code' => $result['@metadata']['statusCode'], //@phan-suppress-current-line PhanTypeMismatchDimFetch
121-
]);
122-
}
123-
124-
$this->span->end();
125-
$this->scope->detach();
126-
127-
return $result;
128-
});
129-
13084
foreach ($this->clients as $client) {
131-
$this->clientName = $client->getApi()->getServiceName();
132-
$this->region = $client->getRegion();
85+
$clientName = $client->getApi()->getServiceName();
86+
$region = $client->getRegion();
87+
$span = null;
88+
$scope = null;
89+
90+
$client->getHandlerList()->prependInit(Middleware::tap(function ($cmd, $req) use ($clientName, $region, &$span, &$scope) {
91+
$tracer = $this->getTracer();
92+
$propagator = $this->getPropagator();
93+
94+
$carrier = [];
95+
/** @phan-suppress-next-line PhanTypeMismatchArgument */
96+
$span = $tracer->spanBuilder($clientName)->setSpanKind(AwsSdkInstrumentation::SPAN_KIND)->startSpan();
97+
$scope = $span->activate();
98+
99+
$propagator->inject($carrier);
100+
101+
/** @psalm-suppress PossiblyInvalidArgument */
102+
$span->setAttributes([
103+
'rpc.method' => $cmd->getName(),
104+
'rpc.service' => $clientName,
105+
'rpc.system' => 'aws-api',
106+
'aws.region' => $region,
107+
]);
108+
}), 'instrumentation');
133109

134-
$client->getHandlerList()->prependInit($middleware, 'instrumentation');
135-
$client->getHandlerList()->appendSign($end_middleware, 'end_instrumentation');
110+
/** @psalm-suppress PossiblyInvalidArgument */
111+
$client->getHandlerList()->appendSign(Middleware::mapResult(function (ResultInterface $result) use (&$span, &$scope) {
112+
if (null === $span || null === $scope) {
113+
return $result;
114+
}
115+
116+
/**
117+
* Some AWS SDK Functions, such as S3Client->getObjectUrl() do not actually perform on the wire comms
118+
* with AWS Servers, and therefore do not return with a populated AWS\Result object with valid @metadata
119+
* Check for the presence of @metadata before extracting status code as these calls are still
120+
* instrumented.
121+
*/
122+
if (isset($result['@metadata'])) {
123+
$span->setAttributes([
124+
'http.status_code' => $result['@metadata']['statusCode'], //@phan-suppress-current-line PhanTypeMismatchDimFetch
125+
]);
126+
}
127+
128+
$span->end();
129+
$scope->detach();
130+
131+
return $result;
132+
}), 'end_instrumentation');
136133
}
137134
} catch (\Throwable $e) {
138135
return false;

‎src/Aws/tests/Integration/AwsSdkInstrumentationTest.php‎

Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,134 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace OpenTelemetry\Tests\Aws\Integration;
6+
7+
use OpenTelemetry\Aws\AwsSdkInstrumentation;
8+
use OpenTelemetry\Aws\Xray\Propagator;
9+
use OpenTelemetry\SDK\Trace\ReadWriteSpanInterface;
10+
use OpenTelemetry\SDK\Trace\TracerProvider;
11+
use PHPUnit\Framework\TestCase;
12+
13+
class AwsSdkInstrumentationTest extends TestCase
14+
{
15+
use UsesServiceTrait;
16+
17+
private AwsSdkInstrumentation $awsSdkInstrumentation;
18+
19+
protected function setUp(): void
20+
{
21+
$this->awsSdkInstrumentation = new AwsSdkInstrumentation();
22+
}
23+
24+
public function testProperClientNameAndRegionIsPassedToSpanForSingleClientCall()
25+
{
26+
$sqsClient = $this->getTestClient('SQS', ['region' => 'eu-west-1']);
27+
$s3Client = $this->getTestClient('S3', ['region' => 'us-east-1']);
28+
$this->addMockResults($s3Client, [[]]);
29+
$eventBridgeClient = $this->getTestClient('EventBridge', ['region' => 'ap-southeast-2']);
30+
31+
$spanProcessor = new CollectingSpanProcessor();
32+
$this->awsSdkInstrumentation->instrumentClients([$sqsClient, $s3Client, $eventBridgeClient]);
33+
$this->awsSdkInstrumentation->setPropagator(new Propagator());
34+
$this->awsSdkInstrumentation->setTracerProvider(new TracerProvider([$spanProcessor]));
35+
$this->awsSdkInstrumentation->init();
36+
$this->awsSdkInstrumentation->activate();
37+
38+
$s3Client->listBuckets();
39+
40+
$collectedSpans = $spanProcessor->getCollectedSpans();
41+
$this->assertCount(1, $collectedSpans);
42+
43+
/** @var ReadWriteSpanInterface $span */
44+
$span = reset($collectedSpans);
45+
$this->assertTrue($span->hasEnded());
46+
47+
$attributes = $span->toSpanData()->getAttributes()->toArray();
48+
$this->assertArrayHasKey('rpc.service', $attributes);
49+
$this->assertSame('s3', $attributes['rpc.service']);
50+
$this->assertArrayHasKey('aws.region', $attributes);
51+
$this->assertSame('us-east-1', $attributes['aws.region']);
52+
}
53+
54+
public function testProperClientNameAndRegionIsPassedToSpanForDoubleCallToSameClient()
55+
{
56+
$sqsClient = $this->getTestClient('SQS', ['region' => 'eu-west-1']);
57+
$s3Client = $this->getTestClient('S3', ['region' => 'us-east-1']);
58+
$this->addMockResults($s3Client, [[], []]);
59+
$eventBridgeClient = $this->getTestClient('EventBridge', ['region' => 'ap-southeast-2']);
60+
61+
$spanProcessor = new CollectingSpanProcessor();
62+
$this->awsSdkInstrumentation->instrumentClients([$sqsClient, $s3Client, $eventBridgeClient]);
63+
$this->awsSdkInstrumentation->setPropagator(new Propagator());
64+
$this->awsSdkInstrumentation->setTracerProvider(new TracerProvider([$spanProcessor]));
65+
$this->awsSdkInstrumentation->init();
66+
$this->awsSdkInstrumentation->activate();
67+
68+
$s3Client->listBuckets();
69+
$s3Client->listObjects(['Bucket' => 'foo']);
70+
71+
$collectedSpans = $spanProcessor->getCollectedSpans();
72+
$this->assertCount(2, $collectedSpans);
73+
74+
/** @var ReadWriteSpanInterface $span */
75+
foreach ($collectedSpans as $span) {
76+
$this->assertTrue($span->hasEnded());
77+
$attributes = $span->toSpanData()->getAttributes()->toArray();
78+
$this->assertArrayHasKey('rpc.service', $attributes);
79+
$this->assertSame('s3', $attributes['rpc.service']);
80+
$this->assertArrayHasKey('aws.region', $attributes);
81+
$this->assertSame('us-east-1', $attributes['aws.region']);
82+
}
83+
}
84+
85+
public function testProperClientNameAndRegionIsPassedToSpanForDoubleCallToDifferentClients()
86+
{
87+
$sqsClient = $this->getTestClient('SQS', ['region' => 'eu-west-1']);
88+
$s3Client = $this->getTestClient('S3', ['region' => 'us-east-1']);
89+
$this->addMockResults($s3Client, [[]]);
90+
$eventBridgeClient = $this->getTestClient('EventBridge', ['region' => 'ap-southeast-2']);
91+
$this->addMockResults($eventBridgeClient, [[]]);
92+
93+
$spanProcessor = new CollectingSpanProcessor();
94+
$this->awsSdkInstrumentation->instrumentClients([$sqsClient, $s3Client, $eventBridgeClient]);
95+
$this->awsSdkInstrumentation->setPropagator(new Propagator());
96+
$this->awsSdkInstrumentation->setTracerProvider(new TracerProvider([$spanProcessor]));
97+
$this->awsSdkInstrumentation->init();
98+
$this->awsSdkInstrumentation->activate();
99+
100+
$eventBridgeClient->putEvents([
101+
'Entries' => [
102+
[
103+
'Version' => 1,
104+
'EventBusName' => 'foo',
105+
'Source' => 'bar',
106+
'DetailType' => 'type',
107+
'Detail' => '{}'
108+
]
109+
]
110+
]);
111+
$s3Client->listBuckets();
112+
113+
$collectedSpans = $spanProcessor->getCollectedSpans();
114+
$this->assertCount(2, $collectedSpans);
115+
116+
/** @var ReadWriteSpanInterface $span */
117+
$span = array_pop($collectedSpans);
118+
$this->assertTrue($span->hasEnded());
119+
$attributes = $span->toSpanData()->getAttributes()->toArray();
120+
$this->assertArrayHasKey('rpc.service', $attributes);
121+
$this->assertSame('s3', $attributes['rpc.service']);
122+
$this->assertArrayHasKey('aws.region', $attributes);
123+
$this->assertSame('us-east-1', $attributes['aws.region']);
124+
125+
/** @var ReadWriteSpanInterface $span */
126+
$span = array_pop($collectedSpans);
127+
$this->assertTrue($span->hasEnded());
128+
$attributes = $span->toSpanData()->getAttributes()->toArray();
129+
$this->assertArrayHasKey('rpc.service', $attributes);
130+
$this->assertSame('eventbridge', $attributes['rpc.service']);
131+
$this->assertArrayHasKey('aws.region', $attributes);
132+
$this->assertSame('ap-southeast-2', $attributes['aws.region']);
133+
}
134+
}

‎src/Aws/tests/Integration/CollectingSpanProcessor.php‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
<?php
2+
3+
namespace OpenTelemetry\Tests\Aws\Integration;
4+
5+
use OpenTelemetry\Context\ContextInterface;
6+
use OpenTelemetry\SDK\Common\Future\CancellationInterface;
7+
use OpenTelemetry\SDK\Trace\ReadableSpanInterface;
8+
use OpenTelemetry\SDK\Trace\ReadWriteSpanInterface;
9+
use OpenTelemetry\SDK\Trace\SpanProcessorInterface;
10+
11+
class CollectingSpanProcessor implements SpanProcessorInterface
12+
{
13+
private array $collectedSpans = [];
14+
15+
public function onStart(ReadWriteSpanInterface $span, ContextInterface $parentContext): void
16+
{
17+
$this->collectedSpans[$span->getContext()->getSpanId()] = $span;
18+
}
19+
20+
public function onEnd(ReadableSpanInterface $span): void
21+
{
22+
$this->collectedSpans[$span->getContext()->getSpanId()] = $span;
23+
}
24+
25+
public function forceFlush(?CancellationInterface $cancellation = null): bool
26+
{
27+
return true;
28+
}
29+
30+
public function shutdown(?CancellationInterface $cancellation = null): bool
31+
{
32+
return true;
33+
}
34+
35+
public function getCollectedSpans(): array
36+
{
37+
return $this->collectedSpans;
38+
}
39+
}

0 commit comments

Comments
 (0)