Skip to content

Commit 56e4005

Browse files
rtrefflerrtreffler
authored
fix(Aws): fixes wrong aws client name and region binding when appending middlewares (#326)
* 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. * style(Aws): addresses php-cs-fixer, phpstan, phan errors * fix(Aws): fixes instrumentation for Async calls; prevents repeated instrumentation for the same client Async calls would overwrite spans of different clients, so spl_object_has is used to keep span next to its client. Also since we can instrument many clients via consecutive calls to `instrumentClient` and `activate` check is added to prevent duplicate middleware being added. * chore(Aws): fixes psalm errors Removes unused methods in UsesServiceTrait.
1 parent c6dadab commit 56e4005

5 files changed

+378
-52
lines changed

‎src/Aws/phpunit.xml.dist

+3
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

+58-52
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
@@ -25,13 +23,12 @@ class AwsSdkInstrumentation implements InstrumentationInterface
2523
public const NAME = 'AWS SDK Instrumentation';
2624
public const VERSION = '0.0.1';
2725
public const SPAN_KIND = SpanKind::KIND_CLIENT;
28-
private TextMapPropagatorInterface $propagator;
29-
private TracerProviderInterface $tracerProvider;
30-
private $clients = [] ;
31-
private string $clientName;
32-
private string $region;
33-
private SpanInterface $span;
34-
private ScopeInterface $scope;
26+
27+
private array $clients = [];
28+
29+
private array $instrumentedClients = [];
30+
31+
private array $spanStorage = [];
3532

3633
public function getName(): string
3734
{
@@ -79,61 +76,70 @@ public function getTracer(): TracerInterface
7976
}
8077

8178
/** @psalm-api */
82-
public function instrumentClients($clientsArray) : void
79+
public function instrumentClients($clientsArray): void
8380
{
8481
$this->clients = $clientsArray;
8582
}
8683

87-
/** @psalm-suppress ArgumentTypeCoercion */
8884
public function activate(): bool
8985
{
9086
try {
91-
$middleware = Middleware::tap(function ($cmd, $_req) {
92-
$tracer = $this->getTracer();
93-
$propagator = $this->getPropagator();
94-
95-
$carrier = [];
96-
/** @phan-suppress-next-line PhanTypeMismatchArgument */
97-
$this->span = $tracer->spanBuilder($this->clientName)->setSpanKind(AwsSdkInstrumentation::SPAN_KIND)->startSpan();
98-
$this->scope = $this->span->activate();
99-
100-
$propagator->inject($carrier);
101-
102-
/** @psalm-suppress PossiblyInvalidArgument */
103-
$this->span->setAttributes([
104-
'rpc.method' => $cmd->getName(),
105-
'rpc.service' => $this->clientName,
106-
'rpc.system' => 'aws-api',
107-
'aws.region' => $this->region,
108-
]);
109-
});
110-
111-
/** @psalm-suppress PossiblyInvalidArgument */
112-
$end_middleware = Middleware::mapResult(function (ResultInterface $result) {
113-
/**
114-
* Some AWS SDK Funtions, such as S3Client->getObjectUrl() do not actually perform on the wire comms
115-
* with AWS Servers, and therefore do not return with a populated AWS\Result object with valid @metadata
116-
* Check for the presence of @metadata before extracting status code as these calls are still
117-
* instrumented.
118-
*/
119-
if (isset($result['@metadata'])) {
120-
$this->span->setAttributes([
121-
'http.status_code' => $result['@metadata']['statusCode'], //@phan-suppress-current-line PhanTypeMismatchDimFetch
122-
]);
87+
foreach ($this->clients as $client) {
88+
$hash = spl_object_hash($client);
89+
if (isset($this->instrumentedClients[$hash])) {
90+
continue;
12391
}
12492

125-
$this->span->end();
126-
$this->scope->detach();
93+
$clientName = $client->getApi()->getServiceName();
94+
$region = $client->getRegion();
12795

128-
return $result;
129-
});
96+
$client->getHandlerList()->prependInit(Middleware::tap(function ($cmd, $_req) use ($clientName, $region, $hash) {
97+
$tracer = $this->getTracer();
98+
$propagator = $this->getPropagator();
13099

131-
foreach ($this->clients as $client) {
132-
$this->clientName = $client->getApi()->getServiceName();
133-
$this->region = $client->getRegion();
100+
$carrier = [];
101+
/** @phan-suppress-next-line PhanTypeMismatchArgument */
102+
$span = $tracer->spanBuilder($clientName)->setSpanKind(AwsSdkInstrumentation::SPAN_KIND)->startSpan();
103+
$scope = $span->activate();
104+
$this->spanStorage[$hash] = [$span, $scope];
134105

135-
$client->getHandlerList()->prependInit($middleware, 'instrumentation');
136-
$client->getHandlerList()->appendSign($end_middleware, 'end_instrumentation');
106+
$propagator->inject($carrier);
107+
108+
/** @psalm-suppress PossiblyInvalidArgument */
109+
$span->setAttributes([
110+
'rpc.method' => $cmd->getName(),
111+
'rpc.service' => $clientName,
112+
'rpc.system' => 'aws-api',
113+
'aws.region' => $region,
114+
]);
115+
}), 'instrumentation');
116+
117+
$client->getHandlerList()->appendSign(Middleware::mapResult(function (ResultInterface $result) use ($hash) {
118+
if (empty($this->spanStorage[$hash])) {
119+
return $result;
120+
}
121+
[$span, $scope] = $this->spanStorage[$hash];
122+
unset($this->spanStorage[$hash]);
123+
124+
/*
125+
* Some AWS SDK Functions, such as S3Client->getObjectUrl() do not actually perform on the wire comms
126+
* with AWS Servers, and therefore do not return with a populated AWS\Result object with valid @metadata
127+
* Check for the presence of @metadata before extracting status code as these calls are still
128+
* instrumented.
129+
*/
130+
if (isset($result['@metadata'])) {
131+
$span->setAttributes([
132+
'http.status_code' => $result['@metadata']['statusCode'], // @phan-suppress-current-line PhanTypeMismatchDimFetch
133+
]);
134+
}
135+
136+
$span->end();
137+
$scope->detach();
138+
139+
return $result;
140+
}), 'end_instrumentation');
141+
142+
$this->instrumentedClients[$hash] = 1;
137143
}
138144
} catch (\Throwable $e) {
139145
return false;

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

+216
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,216 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace OpenTelemetry\Tests\Aws\Integration;
6+
7+
use Aws\AwsClientInterface;
8+
use Aws\EventBridge\EventBridgeClient;
9+
use Aws\S3\S3Client;
10+
use Aws\Sqs\SqsClient;
11+
use OpenTelemetry\Aws\AwsSdkInstrumentation;
12+
use OpenTelemetry\Aws\Xray\Propagator;
13+
use OpenTelemetry\SDK\Trace\ReadWriteSpanInterface;
14+
use OpenTelemetry\SDK\Trace\TracerProvider;
15+
use PHPUnit\Framework\TestCase;
16+
17+
class AwsSdkInstrumentationTest extends TestCase
18+
{
19+
use UsesServiceTrait;
20+
21+
private const HANDLERS_PER_ACTIVATION = 2; // one init and one sign middleware
22+
23+
private AwsSdkInstrumentation $awsSdkInstrumentation;
24+
25+
protected function setUp(): void
26+
{
27+
$this->awsSdkInstrumentation = new AwsSdkInstrumentation();
28+
}
29+
30+
public function testProperClientNameAndRegionIsPassedToSpanForSingleClientCall()
31+
{
32+
/** @var SqsClient $sqsClient */
33+
$sqsClient = $this->getTestClient('SQS', ['region' => 'eu-west-1']);
34+
/** @var S3Client $s3Client */
35+
$s3Client = $this->getTestClient('S3', ['region' => 'us-east-1']);
36+
$this->addMockResults($s3Client, [[]]);
37+
/** @var EventBridgeClient $eventBridgeClient */
38+
$eventBridgeClient = $this->getTestClient('EventBridge', ['region' => 'ap-southeast-2']);
39+
40+
$spanProcessor = new CollectingSpanProcessor();
41+
$this->awsSdkInstrumentation->instrumentClients([$sqsClient, $s3Client, $eventBridgeClient]);
42+
$this->awsSdkInstrumentation->setPropagator(new Propagator());
43+
$this->awsSdkInstrumentation->setTracerProvider(new TracerProvider([$spanProcessor]));
44+
$this->awsSdkInstrumentation->init();
45+
$this->awsSdkInstrumentation->activate();
46+
47+
$s3Client->listBuckets();
48+
49+
$collectedSpans = $spanProcessor->getCollectedSpans();
50+
$this->assertCount(1, $collectedSpans);
51+
52+
/** @var ReadWriteSpanInterface $span */
53+
$span = reset($collectedSpans);
54+
$this->assertTrue($span->hasEnded());
55+
56+
$attributes = $span->toSpanData()->getAttributes()->toArray();
57+
$this->assertArrayHasKey('rpc.service', $attributes);
58+
$this->assertSame('s3', $attributes['rpc.service']);
59+
$this->assertArrayHasKey('aws.region', $attributes);
60+
$this->assertSame('us-east-1', $attributes['aws.region']);
61+
}
62+
63+
public function testProperClientNameAndRegionIsPassedToSpanForDoubleCallToSameClient()
64+
{
65+
/** @var SqsClient $sqsClient */
66+
$sqsClient = $this->getTestClient('SQS', ['region' => 'eu-west-1']);
67+
/** @var S3Client $s3Client */
68+
$s3Client = $this->getTestClient('S3', ['region' => 'us-east-1']);
69+
$this->addMockResults($s3Client, [[], []]);
70+
/** @var EventBridgeClient $eventBridgeClient */
71+
$eventBridgeClient = $this->getTestClient('EventBridge', ['region' => 'ap-southeast-2']);
72+
73+
$spanProcessor = new CollectingSpanProcessor();
74+
$this->awsSdkInstrumentation->instrumentClients([$sqsClient, $s3Client, $eventBridgeClient]);
75+
$this->awsSdkInstrumentation->setPropagator(new Propagator());
76+
$this->awsSdkInstrumentation->setTracerProvider(new TracerProvider([$spanProcessor]));
77+
$this->awsSdkInstrumentation->init();
78+
$this->awsSdkInstrumentation->activate();
79+
80+
$s3Client->listBuckets();
81+
$s3Client->listObjects(['Bucket' => 'foo']);
82+
83+
$collectedSpans = $spanProcessor->getCollectedSpans();
84+
$this->assertCount(2, $collectedSpans);
85+
86+
/** @var ReadWriteSpanInterface $span */
87+
foreach ($collectedSpans as $span) {
88+
$this->assertTrue($span->hasEnded());
89+
$attributes = $span->toSpanData()->getAttributes()->toArray();
90+
$this->assertArrayHasKey('rpc.service', $attributes);
91+
$this->assertSame('s3', $attributes['rpc.service']);
92+
$this->assertArrayHasKey('aws.region', $attributes);
93+
$this->assertSame('us-east-1', $attributes['aws.region']);
94+
}
95+
}
96+
97+
public function testProperClientNameAndRegionIsPassedToSpanForDoubleCallToDifferentClients()
98+
{
99+
/** @var SqsClient $sqsClient */
100+
$sqsClient = $this->getTestClient('SQS', ['region' => 'eu-west-1']);
101+
/** @var S3Client $s3Client */
102+
$s3Client = $this->getTestClient('S3', ['region' => 'us-east-1']);
103+
$this->addMockResults($s3Client, [[]]);
104+
/** @var EventBridgeClient $eventBridgeClient */
105+
$eventBridgeClient = $this->getTestClient('EventBridge', ['region' => 'ap-southeast-2']);
106+
$this->addMockResults($eventBridgeClient, [[]]);
107+
108+
$spanProcessor = new CollectingSpanProcessor();
109+
$this->awsSdkInstrumentation->instrumentClients([$sqsClient, $s3Client, $eventBridgeClient]);
110+
$this->awsSdkInstrumentation->setPropagator(new Propagator());
111+
$this->awsSdkInstrumentation->setTracerProvider(new TracerProvider([$spanProcessor]));
112+
$this->awsSdkInstrumentation->init();
113+
$this->awsSdkInstrumentation->activate();
114+
115+
$eventBridgeClient->putEvents([
116+
'Entries' => [
117+
[
118+
'Version' => 1,
119+
'EventBusName' => 'foo',
120+
'Source' => 'bar',
121+
'DetailType' => 'type',
122+
'Detail' => '{}',
123+
],
124+
],
125+
]);
126+
$s3Client->listBuckets();
127+
128+
$collectedSpans = $spanProcessor->getCollectedSpans();
129+
$this->assertCount(2, $collectedSpans);
130+
131+
/** @var ReadWriteSpanInterface $span */
132+
$span = array_pop($collectedSpans);
133+
$this->assertTrue($span->hasEnded());
134+
$attributes = $span->toSpanData()->getAttributes()->toArray();
135+
$this->assertArrayHasKey('rpc.service', $attributes);
136+
$this->assertSame('s3', $attributes['rpc.service']);
137+
$this->assertArrayHasKey('aws.region', $attributes);
138+
$this->assertSame('us-east-1', $attributes['aws.region']);
139+
140+
/** @var ReadWriteSpanInterface $span */
141+
$span = array_pop($collectedSpans);
142+
$this->assertTrue($span->hasEnded());
143+
$attributes = $span->toSpanData()->getAttributes()->toArray();
144+
$this->assertArrayHasKey('rpc.service', $attributes);
145+
$this->assertSame('eventbridge', $attributes['rpc.service']);
146+
$this->assertArrayHasKey('aws.region', $attributes);
147+
$this->assertSame('ap-southeast-2', $attributes['aws.region']);
148+
}
149+
150+
public function testSpansFromDifferentClientsAreNotOverwritingOneAnother()
151+
{
152+
try {
153+
/** @var SqsClient $sqsClient */
154+
$sqsClient = $this->getTestClient('SQS', ['region' => 'eu-west-1']);
155+
$this->addMockResults($sqsClient, [[]]);
156+
/** @var S3Client $s3Client */
157+
$s3Client = $this->getTestClient('S3', ['region' => 'us-east-1']);
158+
$this->addMockResults($s3Client, [[]]);
159+
160+
$spanProcessor = new CollectingSpanProcessor();
161+
$this->awsSdkInstrumentation->instrumentClients([$sqsClient, $s3Client]);
162+
$this->awsSdkInstrumentation->setPropagator(new Propagator());
163+
$this->awsSdkInstrumentation->setTracerProvider(new TracerProvider([$spanProcessor]));
164+
$this->awsSdkInstrumentation->init();
165+
$this->awsSdkInstrumentation->activate();
166+
167+
$sqsClient->listQueuesAsync();
168+
$s3Client->listBucketsAsync();
169+
170+
$collectedSpans = $spanProcessor->getCollectedSpans();
171+
$this->assertCount(2, $collectedSpans);
172+
173+
/** @var ReadWriteSpanInterface $span */
174+
$span = array_shift($collectedSpans);
175+
$attributes = $span->toSpanData()->getAttributes()->toArray();
176+
$this->assertArrayHasKey('rpc.service', $attributes);
177+
$this->assertSame('sqs', $attributes['rpc.service']);
178+
179+
/** @var ReadWriteSpanInterface $span */
180+
$span = array_shift($collectedSpans);
181+
$attributes = $span->toSpanData()->getAttributes()->toArray();
182+
$this->assertArrayHasKey('rpc.service', $attributes);
183+
$this->assertSame('s3', $attributes['rpc.service']);
184+
} catch (\Throwable $throwable) {
185+
/** @phpstan-ignore-next-line */
186+
$this->assertFalse(true, sprintf('Exception %s occurred: %s', get_class($throwable), $throwable->getMessage()));
187+
}
188+
}
189+
190+
public function testPreventsRepeatedInstrumentationOfSameClient()
191+
{
192+
$clients = [
193+
'SQS' => $sqsClient = $this->getTestClient('SQS', ['region' => 'eu-west-1']),
194+
'S3' => $s3Client = $this->getTestClient('S3', ['region' => 'us-east-1']),
195+
'EventBridge' => $eventBridgeClient = $this->getTestClient('EventBridge', ['region' => 'ap-southeast-2']),
196+
];
197+
198+
$preInstrumentationHandlersCount = array_map(static fn (AwsClientInterface $client) => $client->getHandlerList()->count(), $clients);
199+
200+
$this->awsSdkInstrumentation->instrumentClients([$sqsClient, $eventBridgeClient]);
201+
$this->awsSdkInstrumentation->init();
202+
$this->awsSdkInstrumentation->activate();
203+
204+
$this->awsSdkInstrumentation->instrumentClients([$s3Client, $eventBridgeClient]);
205+
$this->awsSdkInstrumentation->init();
206+
$this->awsSdkInstrumentation->activate();
207+
208+
foreach ($clients as $name => $client) {
209+
$this->assertSame(
210+
$preInstrumentationHandlersCount[$name] + self::HANDLERS_PER_ACTIVATION,
211+
$client->getHandlerList()->count(),
212+
sprintf('Failed asserting that %s client was instrumented once', $name)
213+
);
214+
}
215+
}
216+
}

0 commit comments

Comments
 (0)