From 8615bac0c9f480546a0bc1f8dd70d9066fb63eef Mon Sep 17 00:00:00 2001 From: Brent Shaffer Date: Tue, 6 Oct 2026 19:10:32 +0000 Subject: [PATCH 1/7] chore(Gax): address PR #9573 review feedback --- Gax/src/GapicClientTrait.php | 37 ++-- Gax/src/Middleware/TracingMiddleware.php | 30 +-- Gax/src/Options/ClientOptions.php | 2 + Gax/src/Telemetry/SpanAttributes.php | 3 +- Gax/src/Telemetry/TelemetryTrait.php | 59 ++---- Gax/src/Transport/GrpcTransport.php | 65 +++--- Gax/tests/Unit/GapicClientTraitTest.php | 52 ++--- .../Unit/Middleware/TracingMiddlewareTest.php | 196 +++++++++++++----- Gax/tests/Unit/Options/ClientOptionsTest.php | 37 +++- .../Unit/Transport/GrpcTransportTest.php | 59 +++++- 10 files changed, 331 insertions(+), 209 deletions(-) diff --git a/Gax/src/GapicClientTrait.php b/Gax/src/GapicClientTrait.php index 8f5abb63c6c4..8ca6fc4450a8 100644 --- a/Gax/src/GapicClientTrait.php +++ b/Gax/src/GapicClientTrait.php @@ -72,9 +72,10 @@ trait GapicClientTrait private ?TransportInterface $transport = null; private ?HeaderCredentialsInterface $credentialsWrapper = null; - private ?TracerProviderInterface $openTelemetryTracerProvider = null; private array $telemetryOptions = []; private string $apiEndpoint = ''; + private ?string $serverAddress = null; + private ?int $serverPort = null; /** @var RetrySettings[] $retrySettings */ private array $retrySettings = []; private string $serviceName = ''; @@ -269,6 +270,8 @@ protected function getCredentialsWrapper() * The code generator version of the GAPIC library. * @type callable $clientCertSource * A callable which returns the client cert as a string. + * @type TracerProviderInterface|null $openTelemetryTracerProvider + * An OpenTelemetry TracerProvider to use for tracing API calls. * } * @throws ValidationException */ @@ -379,29 +382,27 @@ private function setClientOptions(array $options) } $this->apiEndpoint = $options['apiEndpoint']; - $this->openTelemetryTracerProvider = $options['openTelemetryTracerProvider'] ?? null; - $telemetryOptions = [ + if ($this->apiEndpoint !== '') { + [$serverAddress, $serverPort] = self::normalizeServiceAddress($this->apiEndpoint); + $this->serverAddress = $serverAddress; + $this->serverPort = (int) $serverPort; + } + $this->telemetryOptions = [ 'openTelemetryTracerProvider' => $options['openTelemetryTracerProvider'] ?? null, - 'clientVersion' => $options['libVersion'] ?? null, + 'clientVersion' => $options['gapicVersion'] ?? null, ]; - $this->telemetryOptions = $telemetryOptions; $transport = $options['transport'] ?: self::defaultTransport(); - if ($transport instanceof TransportInterface) { - if (method_exists($transport, 'setTelemetryOptions')) { - $transport->setTelemetryOptions($telemetryOptions); - } - $this->transport = $transport; - } else { - $this->transport = $this->createTransport( + $this->transport = $transport instanceof TransportInterface + ? $transport + : $this->createTransport( $options['apiEndpoint'], $transport, $options['transportConfig'], $options['clientCertSource'], $hasEmulator, - $telemetryOptions + $this->telemetryOptions ); - } } /** @@ -775,14 +776,12 @@ private function createCallStack(array $callConstructionOptions) $callStack = $fn($callStack); } - if ($this->openTelemetryTracerProvider) { - [$serverAddress, $serverPort] = self::normalizeServiceAddress($this->apiEndpoint); + if (!empty($this->telemetryOptions['openTelemetryTracerProvider'])) { $systemName = $this->transport instanceof GrpcTransport ? 'grpc' : 'http'; $callStack = new TracingMiddleware( $callStack, - $this->openTelemetryTracerProvider, - $serverAddress, - (int) $serverPort, + $this->serverAddress, + $this->serverPort, $systemName, $this->telemetryOptions ); diff --git a/Gax/src/Middleware/TracingMiddleware.php b/Gax/src/Middleware/TracingMiddleware.php index 4ddf5f0c2095..1a71f61ed2fc 100644 --- a/Gax/src/Middleware/TracingMiddleware.php +++ b/Gax/src/Middleware/TracingMiddleware.php @@ -39,7 +39,6 @@ use GuzzleHttp\Promise\PromiseInterface; use OpenTelemetry\API\Trace\SpanKind; use OpenTelemetry\API\Trace\StatusCode; -use OpenTelemetry\API\Trace\TracerProviderInterface; use Throwable; /** @@ -53,31 +52,29 @@ class TracingMiddleware implements MiddlewareInterface /** @var MiddlewareInterface|callable */ private $nextHandler; - private string $serverAddress; - private int $serverPort; + private ?string $serverAddress; + private ?int $serverPort; private string $systemName; /** * @param MiddlewareInterface|callable $nextHandler - * @param TracerProviderInterface|null $openTelemetryTracerProvider - * @param string $serverAddress - * @param int $serverPort + * @param string|null $serverAddress + * @param int|null $serverPort * @param string $systemName * @param array $telemetryOptions */ public function __construct( $nextHandler, - ?TracerProviderInterface $openTelemetryTracerProvider = null, - string $serverAddress = '', - int $serverPort = 443, + ?string $serverAddress = null, + ?int $serverPort = null, string $systemName = 'grpc', array $telemetryOptions = [] ) { $this->nextHandler = $nextHandler; - $this->serverAddress = $serverAddress; + $this->serverAddress = $serverAddress ?: null; $this->serverPort = $serverPort; $this->systemName = $systemName; - $this->initTelemetry($telemetryOptions, $openTelemetryTracerProvider); + $this->initTelemetry($telemetryOptions); } /** @@ -85,7 +82,7 @@ public function __construct( */ public function __invoke(Call $call, array $options) { - if (!$this->openTelemetryTracerProvider) { + if (!$this->openTelemetryTracerProvider || $call->getCallType() !== Call::UNARY_CALL) { return ($this->nextHandler)($call, $options); } @@ -129,12 +126,17 @@ public function __invoke(Call $call, array $options) function () use ($result, $span) { $waitScope = $span->activate(); try { - $result->wait(); + $result->wait(false); } finally { $waitScope->detach(); } }, - [$result, 'cancel'] + function () use ($result, $span) { + $span->setStatus(StatusCode::STATUS_ERROR, 'Call cancelled'); + $span->setAttribute(SpanAttributes::ERROR_TYPE, 'CANCELLED'); + $span->end(); + $result->cancel(); + } ); $result->then( diff --git a/Gax/src/Options/ClientOptions.php b/Gax/src/Options/ClientOptions.php index 72302618cec2..37d27ce78366 100644 --- a/Gax/src/Options/ClientOptions.php +++ b/Gax/src/Options/ClientOptions.php @@ -173,6 +173,8 @@ class ClientOptions implements ArrayAccess, OptionsInterface * The API key to be used for the client. * @type null|false|LoggerInterface * A PSR-3 compliant logger. + * @type TracerProviderInterface|null $openTelemetryTracerProvider + * An OpenTelemetry TracerProvider to use for tracing API calls. * } */ public function __construct(array $options) diff --git a/Gax/src/Telemetry/SpanAttributes.php b/Gax/src/Telemetry/SpanAttributes.php index c663383061e5..73e8f5e45465 100644 --- a/Gax/src/Telemetry/SpanAttributes.php +++ b/Gax/src/Telemetry/SpanAttributes.php @@ -43,8 +43,7 @@ final class SpanAttributes public const RPC_SYSTEM_NAME = 'rpc.system.name'; public const RPC_RESPONSE_STATUS_CODE = 'rpc.response.status_code'; - // HTTP & Network attributes - public const HTTP_RESPONSE_STATUS_CODE = 'http.response.status_code'; + // Network attributes public const SERVER_ADDRESS = 'server.address'; public const SERVER_PORT = 'server.port'; diff --git a/Gax/src/Telemetry/TelemetryTrait.php b/Gax/src/Telemetry/TelemetryTrait.php index d716f0ce6a6b..995fffb64342 100644 --- a/Gax/src/Telemetry/TelemetryTrait.php +++ b/Gax/src/Telemetry/TelemetryTrait.php @@ -38,7 +38,6 @@ use OpenTelemetry\API\Trace\SpanKind; use OpenTelemetry\API\Trace\StatusCode; use OpenTelemetry\API\Trace\TracerProviderInterface; -use Psr\Http\Message\ResponseInterface; use Throwable; /** @@ -55,33 +54,23 @@ trait TelemetryTrait * Sets telemetry options and initializes tracing properties. * * @param array $telemetryOptions - * @param TracerProviderInterface|null $openTelemetryTracerProvider * @return $this */ - public function setTelemetryOptions( - array $telemetryOptions, - ?TracerProviderInterface $openTelemetryTracerProvider = null - ): self { - $this->initTelemetry($telemetryOptions, $openTelemetryTracerProvider); + public function setTelemetryOptions(array $telemetryOptions): self + { + $this->initTelemetry($telemetryOptions); return $this; } /** - * Initializes telemetry properties from an options array and optional tracer provider. + * Initializes telemetry properties from an options array. * * @param array $telemetryOptions - * @param TracerProviderInterface|null $openTelemetryTracerProvider */ - private function initTelemetry( - array $telemetryOptions, - ?TracerProviderInterface $openTelemetryTracerProvider = null - ): void { - $this->openTelemetryTracerProvider = $openTelemetryTracerProvider - ?? $telemetryOptions['openTelemetryTracerProvider'] - ?? null; - $this->clientVersion = $telemetryOptions['clientVersion'] - ?? $telemetryOptions['libVersion'] - ?? null; + private function initTelemetry(array $telemetryOptions): void + { + $this->openTelemetryTracerProvider = $telemetryOptions['openTelemetryTracerProvider'] ?? null; + $this->clientVersion = $telemetryOptions['clientVersion'] ?? null; } /** @@ -97,19 +86,6 @@ private static function getTelemetryDefaultConfig(): array ]; } - /** - * Returns the telemetry options populated from this instance. - * - * @return array - */ - private function getTelemetryOptions(): array - { - return [ - 'openTelemetryTracerProvider' => $this->openTelemetryTracerProvider, - 'clientVersion' => $this->clientVersion, - ]; - } - /** * Builds and starts a span with standard client metadata attributes. * @@ -153,26 +129,19 @@ private function recordException(?SpanInterface $span, Throwable $e, bool $end = return; } - $statusCode = null; - if (method_exists($e, 'getResponse') && $e->getResponse() instanceof ResponseInterface) { - $statusCode = $e->getResponse()->getStatusCode(); - $span->setAttribute(SpanAttributes::HTTP_RESPONSE_STATUS_CODE, $statusCode); - } - - $errorType = null; - if ($statusCode !== null) { - $errorType = (string) $statusCode; - } elseif ($e instanceof ApiException && $e->getStatus()) { - $errorType = $e->getStatus(); + if ($e instanceof ApiException) { + $errorType = $e->getReason() ?: $e->getStatus() ?: get_class($e); + $message = $e->getBasicMessage() ?? $e->getMessage(); } else { $errorType = get_class($e); + $message = $e->getMessage(); } $span->recordException($e); - $span->setStatus(StatusCode::STATUS_ERROR, $e->getMessage()); + $span->setStatus(StatusCode::STATUS_ERROR, $message); $span->setAttribute(SpanAttributes::ERROR_TYPE, $errorType); $span->setAttribute(SpanAttributes::EXCEPTION_TYPE, get_class($e)); - $span->setAttribute(SpanAttributes::STATUS_MESSAGE, $e->getMessage()); + $span->setAttribute(SpanAttributes::STATUS_MESSAGE, $message); if ($end) { $span->end(); diff --git a/Gax/src/Transport/GrpcTransport.php b/Gax/src/Transport/GrpcTransport.php index 85b5e84432cd..3169b24480ca 100644 --- a/Gax/src/Transport/GrpcTransport.php +++ b/Gax/src/Transport/GrpcTransport.php @@ -71,8 +71,8 @@ class GrpcTransport extends BaseStub implements TransportInterface use TelemetryTrait; private null|LoggerInterface $logger; - private string $serverAddress = ''; - private int $serverPort = 443; + private ?string $serverAddress = null; + private ?int $serverPort = null; /** * @param string $hostname @@ -109,7 +109,7 @@ public function __construct( parent::__construct($hostname, $opts, $channel); $this->logger = $logger; if ($hostname !== '') { - list($addr, $port) = self::normalizeServiceAddress($hostname); + [$addr, $port] = self::normalizeServiceAddress($hostname); $this->serverAddress = $addr; $this->serverPort = (int) $port; } @@ -310,27 +310,34 @@ public function startUnaryCall(Call $call, array $options) ); } - $unaryCall = $this->_simpleRequest( - '/' . $call->getMethod(), - $call->getMessage(), - [$call->getDecodeType(), 'decode'], - isset($options['headers']) ? $options['headers'] : [], - $this->getCallOptions($options) - ); + try { + $unaryCall = $this->_simpleRequest( + '/' . $call->getMethod(), + $call->getMessage(), + [$call->getDecodeType(), 'decode'], + isset($options['headers']) ? $options['headers'] : [], + $this->getCallOptions($options) + ); - if ($this->logger) { - $requestEvent = new RpcLogEvent(); + if ($this->logger) { + $requestEvent = new RpcLogEvent(); - $requestEvent->headers = $headers; - $requestEvent->payload = $call->getMessage()->serializeToJsonString(); - $requestEvent->retryAttempt = $options['retryAttempt'] ?? null; - $requestEvent->serviceName = $options['serviceName'] ?? null; - $requestEvent->rpcName = $call->getMethod(); - $requestEvent->processId = (int) getmypid(); - $requestEvent->requestId = crc32((string) spl_object_id($call) . getmypid()); - $requestEvent->url = $this->getGrpcUrl(); + $requestEvent->headers = $headers; + $requestEvent->payload = $call->getMessage()->serializeToJsonString(); + $requestEvent->retryAttempt = $options['retryAttempt'] ?? null; + $requestEvent->serviceName = $options['serviceName'] ?? null; + $requestEvent->rpcName = $call->getMethod(); + $requestEvent->processId = (int) getmypid(); + $requestEvent->requestId = crc32((string) spl_object_id($call) . getmypid()); + $requestEvent->url = $this->getGrpcUrl(); - $this->logRequest($requestEvent); + $this->logRequest($requestEvent); + } + } catch (Throwable $e) { + if ($span) { + $this->recordException($span, $e, true); + } + throw $e; } /** @var Promise $promise */ @@ -352,20 +359,24 @@ function () use ($unaryCall, $options, &$promise, $requestEvent, $span) { } if ($status->code == Code::OK) { - if ($span) { - $span->setAttribute(SpanAttributes::RPC_RESPONSE_STATUS_CODE, 'OK'); - $span->setStatus(StatusCode::STATUS_OK); - } if (isset($options['metadataCallback'])) { $metadataCallback = $options['metadataCallback']; $metadataCallback($unaryCall->getMetadata()); } + if ($span) { + $span->setAttribute(SpanAttributes::RPC_RESPONSE_STATUS_CODE, 'OK'); + $span->setStatus(StatusCode::STATUS_OK); + } $promise->resolve($response); } else { + $apiException = ApiException::createFromStdClass($status); if ($span) { - $span->setAttribute(SpanAttributes::RPC_RESPONSE_STATUS_CODE, Code::name($status->code)); + $span->setAttribute( + SpanAttributes::RPC_RESPONSE_STATUS_CODE, + $apiException->getStatus() + ); } - throw ApiException::createFromStdClass($status); + throw $apiException; } } catch (Throwable $e) { if ($span) { diff --git a/Gax/tests/Unit/GapicClientTraitTest.php b/Gax/tests/Unit/GapicClientTraitTest.php index 1ccf3f133a15..244434cc38bc 100644 --- a/Gax/tests/Unit/GapicClientTraitTest.php +++ b/Gax/tests/Unit/GapicClientTraitTest.php @@ -1977,48 +1977,27 @@ public function testGetServiceScopes() ); } - public function testPreInstantiatedTransportReceivesTelemetryOptions() + public function testSetClientOptionsPopulatesTelemetryOptionsFromGapicVersion(): void { - $transport = new class() implements TransportInterface { - public ?array $telemetryOptions = null; - - public function setTelemetryOptions(array $telemetryOptions): void - { - $this->telemetryOptions = $telemetryOptions; - } - - public function startUnaryCall(Call $call, array $options) - { - } - - public function startServerStreamingCall(Call $call, array $options) - { - } - - public function startClientStreamingCall(Call $call, array $options) - { - } - - public function startBidiStreamingCall(Call $call, array $options) - { - } - - public function close() - { - } - }; - + $transport = $this->prophesize(TransportInterface::class)->reveal(); $tracerProvider = $this->createMock(TracerProviderInterface::class); + $client = new StubGapicClient(); $options = $client->buildClientOptions([ 'transport' => $transport, 'openTelemetryTracerProvider' => $tracerProvider, + 'gapicVersion' => '2.3.4', + 'apiEndpoint' => 'secretmanager.googleapis.com:8443', ]); $client->setClientOptions($options); $this->assertSame($transport, $client->getTransport()); - $this->assertNotNull($transport->telemetryOptions); - $this->assertSame($tracerProvider, $transport->telemetryOptions['openTelemetryTracerProvider']); + $this->assertSame([ + 'openTelemetryTracerProvider' => $tracerProvider, + 'clientVersion' => '2.3.4', + ], $client->get('telemetryOptions')); + $this->assertSame('secretmanager.googleapis.com', $client->get('serverAddress')); + $this->assertSame(8443, $client->get('serverPort')); } public function testCreateCallStackIncludesTracingMiddlewareWhenTracingEnabled(): void @@ -2031,6 +2010,7 @@ public function testCreateCallStackIncludesTracingMiddlewareWhenTracingEnabled() $tracerProvider->expects($this->once()) ->method('getTracer') + ->with('google-cloud-php', '1.2.3') ->willReturn($tracer); $tracer->expects($this->once()) @@ -2074,8 +2054,12 @@ public function testCreateCallStackIncludesTracingMiddlewareWhenTracingEnabled() $client = new StubGapicClient(); $client->set('transport', $transport->reveal()); $client->set('credentialsWrapper', $credentialsWrapper->reveal()); - $client->set('apiEndpoint', 'secretmanager.googleapis.com:443'); - $client->set('openTelemetryTracerProvider', $tracerProvider); + $client->set('serverAddress', 'secretmanager.googleapis.com'); + $client->set('serverPort', 443); + $client->set('telemetryOptions', [ + 'openTelemetryTracerProvider' => $tracerProvider, + 'clientVersion' => '1.2.3', + ]); $callStack = $client->createCallStack([ 'retrySettings' => RetrySettings::constructDefault(), diff --git a/Gax/tests/Unit/Middleware/TracingMiddlewareTest.php b/Gax/tests/Unit/Middleware/TracingMiddlewareTest.php index cc8dc8d7b16a..12d5a5572506 100644 --- a/Gax/tests/Unit/Middleware/TracingMiddlewareTest.php +++ b/Gax/tests/Unit/Middleware/TracingMiddlewareTest.php @@ -54,7 +54,7 @@ class TracingMiddlewareTest extends TestCase { public function testTracingDisabledReturnsHandlerResult(): void { - $call = $this->createMock(Call::class); + $call = new Call('test/method'); $nextHandlerCalled = false; $nextHandler = function ($call, $options) use (&$nextHandlerCalled) { $nextHandlerCalled = true; @@ -119,8 +119,7 @@ public function testUnaryCallSuccessEmitsT3Span(): void $scope->expects($this->once()) ->method('detach'); - $call = $this->createMock(Call::class); - $call->method('getMethod')->willReturn($method); + $call = new Call($method); $nextHandler = function ($c, $opts) { return new FulfilledPromise('response-payload'); @@ -128,11 +127,13 @@ public function testUnaryCallSuccessEmitsT3Span(): void $middleware = new TracingMiddleware( $nextHandler, - $tracerProvider, 'secretmanager.googleapis.com', 443, 'grpc', - ['clientVersion' => '1.0.0'] + [ + 'openTelemetryTracerProvider' => $tracerProvider, + 'clientVersion' => '1.0.0', + ] ); $promise = $middleware($call, []); @@ -179,20 +180,19 @@ public function testUnaryCallFailureRecordsExceptionAndErrorStatus(): void $scope->expects($this->once()) ->method('detach'); - $call = $this->createMock(Call::class); - $call->method('getMethod')->willReturn($method); + $call = new Call($method); - $apiException = new ApiException('Secret not found', 5, 'NOT_FOUND'); + $apiException = ApiException::createFromRestApiResponse('Secret not found', 5); $nextHandler = function ($c, $opts) use ($apiException) { return new RejectedPromise($apiException); }; $middleware = new TracingMiddleware( $nextHandler, - $tracerProvider, 'secretmanager.googleapis.com', 443, - 'grpc' + 'grpc', + ['openTelemetryTracerProvider' => $tracerProvider] ); $promise = $middleware($call, []); @@ -209,7 +209,7 @@ public function testUnaryCallFailureRecordsExceptionAndErrorStatus(): void } } - public function testSynchronousExceptionInHandlerRecordsErrorAndRethrows(): void + public function testUnaryCallFailureUsesErrorInfoReasonWhenPresent(): void { $tracerProvider = $this->createMock(TracerProviderInterface::class); $tracer = $this->createMock(TracerInterface::class); @@ -217,8 +217,6 @@ public function testSynchronousExceptionInHandlerRecordsErrorAndRethrows(): void $span = $this->createMock(SpanInterface::class); $scope = $this->createMock(ScopeInterface::class); - $method = 'google.cloud.secretmanager.v1.SecretManagerService/AccessSecretVersion'; - $tracerProvider->method('getTracer')->willReturn($tracer); $tracer->method('spanBuilder')->willReturn($spanBuilder); $spanBuilder->method('setSpanKind')->willReturnSelf(); @@ -235,42 +233,51 @@ public function testSynchronousExceptionInHandlerRecordsErrorAndRethrows(): void $span->expects($this->once()) ->method('setStatus') - ->with(StatusCode::STATUS_ERROR, 'Validation failed'); + ->with(StatusCode::STATUS_ERROR, 'API key not valid'); $span->expects($this->once()) ->method('end'); - $scope->expects($this->once()) - ->method('detach'); - - $call = $this->createMock(Call::class); - $call->method('getMethod')->willReturn($method); - - $nextHandler = function ($c, $opts) { - throw new ValidationException('Validation failed'); + $call = new Call('google.cloud.secretmanager.v1.SecretManagerService/AccessSecretVersion'); + + $apiException = ApiException::createFromRestApiResponse( + 'API key not valid', + 3, + [ + [ + '@type' => 'type.googleapis.com/google.rpc.ErrorInfo', + 'reason' => 'API_KEY_INVALID', + 'domain' => 'googleapis.com', + 'metadata' => ['service' => 'secretmanager.googleapis.com'], + ], + ] + ); + $nextHandler = function () use ($apiException) { + return new RejectedPromise($apiException); }; $middleware = new TracingMiddleware( $nextHandler, - $tracerProvider, 'secretmanager.googleapis.com', 443, - 'grpc' + 'http', + ['openTelemetryTracerProvider' => $tracerProvider] ); - $this->expectException(ValidationException::class); - $this->expectExceptionMessage('Validation failed'); + $promise = $middleware($call, []); + + $this->expectException(ApiException::class); try { - $middleware($call, []); + $promise->wait(); } finally { - $this->assertSame(ValidationException::class, $recordedAttributes[SpanAttributes::ERROR_TYPE]); - $this->assertSame(ValidationException::class, $recordedAttributes[SpanAttributes::EXCEPTION_TYPE]); - $this->assertSame('Validation failed', $recordedAttributes[SpanAttributes::STATUS_MESSAGE]); + $this->assertSame('API_KEY_INVALID', $recordedAttributes[SpanAttributes::ERROR_TYPE]); + $this->assertSame(ApiException::class, $recordedAttributes[SpanAttributes::EXCEPTION_TYPE]); + $this->assertSame('API key not valid', $recordedAttributes[SpanAttributes::STATUS_MESSAGE]); } } - public function testStreamingCallSuccess(): void + public function testSynchronousExceptionInHandlerRecordsErrorAndRethrows(): void { $tracerProvider = $this->createMock(TracerProviderInterface::class); $tracer = $this->createMock(TracerInterface::class); @@ -278,7 +285,7 @@ public function testStreamingCallSuccess(): void $span = $this->createMock(SpanInterface::class); $scope = $this->createMock(ScopeInterface::class); - $method = 'google.cloud.pubsub.v1.Subscriber/StreamingPull'; + $method = 'google.cloud.secretmanager.v1.SecretManagerService/AccessSecretVersion'; $tracerProvider->method('getTracer')->willReturn($tracer); $tracer->method('spanBuilder')->willReturn($spanBuilder); @@ -287,9 +294,16 @@ public function testStreamingCallSuccess(): void $spanBuilder->method('startSpan')->willReturn($span); $span->method('activate')->willReturn($scope); + $recordedAttributes = []; + $span->method('setAttribute') + ->willReturnCallback(function ($key, $val) use (&$recordedAttributes, $span) { + $recordedAttributes[$key] = $val; + return $span; + }); + $span->expects($this->once()) ->method('setStatus') - ->with(StatusCode::STATUS_OK); + ->with(StatusCode::STATUS_ERROR, 'Validation failed'); $span->expects($this->once()) ->method('end'); @@ -297,8 +311,39 @@ public function testStreamingCallSuccess(): void $scope->expects($this->once()) ->method('detach'); - $call = $this->createMock(Call::class); - $call->method('getMethod')->willReturn($method); + $call = new Call($method); + + $nextHandler = function ($c, $opts) { + throw new ValidationException('Validation failed'); + }; + + $middleware = new TracingMiddleware( + $nextHandler, + 'secretmanager.googleapis.com', + 443, + 'grpc', + ['openTelemetryTracerProvider' => $tracerProvider] + ); + + $this->expectException(ValidationException::class); + $this->expectExceptionMessage('Validation failed'); + + try { + $middleware($call, []); + } finally { + $this->assertSame(ValidationException::class, $recordedAttributes[SpanAttributes::ERROR_TYPE]); + $this->assertSame(ValidationException::class, $recordedAttributes[SpanAttributes::EXCEPTION_TYPE]); + $this->assertSame('Validation failed', $recordedAttributes[SpanAttributes::STATUS_MESSAGE]); + } + } + + public function testStreamingCallSkipsTracing(): void + { + $tracerProvider = $this->createMock(TracerProviderInterface::class); + $tracerProvider->expects($this->never())->method('getTracer'); + + $method = 'google.cloud.pubsub.v1.Subscriber/StreamingPull'; + $call = new Call($method, null, null, [], Call::BIDI_STREAMING_CALL); $mockStream = new stdClass(); $nextHandler = function ($c, $opts) use ($mockStream) { @@ -307,10 +352,10 @@ public function testStreamingCallSuccess(): void $middleware = new TracingMiddleware( $nextHandler, - $tracerProvider, 'pubsub.googleapis.com', 443, - 'grpc' + 'grpc', + ['openTelemetryTracerProvider' => $tracerProvider] ); $result = $middleware($call, []); @@ -363,8 +408,7 @@ public function testPendingPromiseWaitActivatesAndDetachesScopeDuringWait(): voi $span->expects($this->once()) ->method('end'); - $call = $this->createMock(Call::class); - $call->method('getMethod')->willReturn($method); + $call = new Call($method); // Create a pending promise whose waitfn verifies scopes $innerWaitExecuted = false; @@ -388,10 +432,10 @@ function () use ( $middleware = new TracingMiddleware( $nextHandler, - $tracerProvider, 'secretmanager.googleapis.com', 443, - 'grpc' + 'grpc', + ['openTelemetryTracerProvider' => $tracerProvider] ); $wrappedPromise = $middleware($call, []); @@ -451,8 +495,7 @@ public function testPendingPromiseWaitRejectionDetachesScopeAndRecordsException( $span->expects($this->once()) ->method('end'); - $call = $this->createMock(Call::class); - $call->method('getMethod')->willReturn($method); + $call = new Call($method); $apiException = new ApiException('Call failed in wait', 14, 'UNAVAILABLE'); $innerPromise = new Promise(function () use (&$innerPromise, $apiException) { @@ -465,10 +508,10 @@ public function testPendingPromiseWaitRejectionDetachesScopeAndRecordsException( $middleware = new TracingMiddleware( $nextHandler, - $tracerProvider, 'secretmanager.googleapis.com', 443, - 'grpc' + 'grpc', + ['openTelemetryTracerProvider' => $tracerProvider] ); $wrappedPromise = $middleware($call, []); @@ -484,7 +527,7 @@ public function testPendingPromiseWaitRejectionDetachesScopeAndRecordsException( } } - public function testPendingPromiseCancellationCancelsInnerPromise(): void + public function testPendingPromiseWaitFalseDoesNotThrowOnRejection(): void { $tracerProvider = $this->createMock(TracerProviderInterface::class); $tracer = $this->createMock(TracerInterface::class); @@ -499,8 +542,60 @@ public function testPendingPromiseCancellationCancelsInnerPromise(): void $spanBuilder->method('startSpan')->willReturn($span); $span->method('activate')->willReturn($scope); - $call = $this->createMock(Call::class); - $call->method('getMethod')->willReturn('some/method'); + $span->expects($this->once()) + ->method('setStatus') + ->with(StatusCode::STATUS_ERROR, 'Call failed in wait'); + $span->expects($this->once()) + ->method('end'); + + $call = new Call('some/method'); + $apiException = new ApiException('Call failed in wait', 14, 'UNAVAILABLE'); + $innerPromise = new Promise(function () use (&$innerPromise, $apiException) { + $innerPromise->reject($apiException); + }); + + $middleware = new TracingMiddleware( + fn () => $innerPromise, + 'secretmanager.googleapis.com', + 443, + 'grpc', + ['openTelemetryTracerProvider' => $tracerProvider] + ); + + $wrappedPromise = $middleware($call, []); + $wrappedPromise->wait(false); + $this->assertSame('rejected', $wrappedPromise->getState()); + } + + public function testPendingPromiseCancellationEndsSpanAndCancelsInnerPromise(): void + { + $tracerProvider = $this->createMock(TracerProviderInterface::class); + $tracer = $this->createMock(TracerInterface::class); + $spanBuilder = $this->createMock(SpanBuilderInterface::class); + $span = $this->createMock(SpanInterface::class); + $scope = $this->createMock(ScopeInterface::class); + + $tracerProvider->method('getTracer')->willReturn($tracer); + $tracer->method('spanBuilder')->willReturn($spanBuilder); + $spanBuilder->method('setSpanKind')->willReturnSelf(); + $spanBuilder->method('setAttribute')->willReturnSelf(); + $spanBuilder->method('startSpan')->willReturn($span); + $span->method('activate')->willReturn($scope); + + $recordedAttributes = []; + $span->method('setAttribute') + ->willReturnCallback(function ($key, $val) use (&$recordedAttributes, $span) { + $recordedAttributes[$key] = $val; + return $span; + }); + + $span->expects($this->once()) + ->method('setStatus') + ->with(StatusCode::STATUS_ERROR, 'Call cancelled'); + $span->expects($this->once()) + ->method('end'); + + $call = new Call('some/method'); $cancelled = false; $innerPromise = new Promise( @@ -515,15 +610,16 @@ function () use (&$cancelled) { function () use ($innerPromise) { return $innerPromise; }, - $tracerProvider, 'test.googleapis.com', 443, - 'grpc' + 'grpc', + ['openTelemetryTracerProvider' => $tracerProvider] ); $wrappedPromise = $middleware($call, []); $wrappedPromise->cancel(); $this->assertTrue($cancelled); + $this->assertSame('CANCELLED', $recordedAttributes[SpanAttributes::ERROR_TYPE]); } } diff --git a/Gax/tests/Unit/Options/ClientOptionsTest.php b/Gax/tests/Unit/Options/ClientOptionsTest.php index 17904a9f1c1e..6fd39630b405 100644 --- a/Gax/tests/Unit/Options/ClientOptionsTest.php +++ b/Gax/tests/Unit/Options/ClientOptionsTest.php @@ -1,18 +1,33 @@ reveal()); $transport->setTelemetryOptions([ + 'openTelemetryTracerProvider' => $tracerProvider, 'clientVersion' => '1.0.0', - ], $tracerProvider); + ]); $call = new Call($method, Status::class, new MockRequest()); $promise = $transport->startUnaryCall($call, []); @@ -795,6 +796,8 @@ public function testStartUnaryCallEmitsT4ClientSpanOnSuccess(): void $this->assertSame($response, $result); $this->assertSame('grpc', $attributes[SpanAttributes::RPC_SYSTEM_NAME]); $this->assertSame($method, $attributes[SpanAttributes::RPC_METHOD]); + $this->assertArrayNotHasKey(SpanAttributes::SERVER_ADDRESS, $attributes); + $this->assertArrayNotHasKey(SpanAttributes::SERVER_PORT, $attributes); $this->assertSame('OK', $recordedSpanAttributes[SpanAttributes::RPC_RESPONSE_STATUS_CODE]); } @@ -822,7 +825,7 @@ public function testStartUnaryCallEmitsT4ClientSpanOnFailure(): void $span->expects($this->once()) ->method('setStatus') - ->with($this->equalTo(StatusCode::STATUS_ERROR), $this->stringContains('Resource not found')); + ->with(StatusCode::STATUS_ERROR, 'Resource not found'); $span->expects($this->once()) ->method('end'); @@ -837,7 +840,9 @@ public function testStartUnaryCallEmitsT4ClientSpanOnFailure(): void ->willReturn([null, $status]); $transport = new MockGrpcTransport($unaryCall->reveal()); - $transport->setTelemetryOptions([], $tracerProvider); + $transport->setTelemetryOptions([ + 'openTelemetryTracerProvider' => $tracerProvider, + ]); $call = new Call($method, Status::class, new MockRequest()); $promise = $transport->startUnaryCall($call, []); @@ -849,13 +854,53 @@ public function testStartUnaryCallEmitsT4ClientSpanOnFailure(): void } finally { $this->assertSame('NOT_FOUND', $recordedSpanAttributes[SpanAttributes::RPC_RESPONSE_STATUS_CODE]); $this->assertSame('NOT_FOUND', $recordedSpanAttributes[SpanAttributes::ERROR_TYPE]); - $this->assertStringContainsString( - 'Resource not found', - $recordedSpanAttributes[SpanAttributes::STATUS_MESSAGE] - ); + $this->assertSame('Resource not found', $recordedSpanAttributes[SpanAttributes::STATUS_MESSAGE]); } } + public function testStartUnaryCallEndsSpanOnSynchronousException(): void + { + $tracerProvider = $this->createMock(TracerProviderInterface::class); + $tracer = $this->createMock(TracerInterface::class); + $spanBuilder = $this->createMock(SpanBuilderInterface::class); + $span = $this->createMock(SpanInterface::class); + + $tracerProvider->method('getTracer')->willReturn($tracer); + $tracer->method('spanBuilder')->willReturn($spanBuilder); + $spanBuilder->method('setSpanKind')->willReturnSelf(); + $spanBuilder->method('setAttribute')->willReturnSelf(); + $spanBuilder->method('startSpan')->willReturn($span); + + $span->expects($this->once()) + ->method('setStatus') + ->with(StatusCode::STATUS_ERROR, 'Auth callback failed'); + $span->expects($this->once()) + ->method('end'); + + $credentialsWrapper = $this->prophesize(CredentialsWrapper::class); + $credentialsWrapper->checkUniverseDomain()->shouldBeCalledOnce(); + $credentialsWrapper->getAuthorizationHeaderCallback(null) + ->willThrow(new \RuntimeException('Auth callback failed')); + + $transport = new MockGrpcTransport(null); + $transport->setTelemetryOptions([ + 'openTelemetryTracerProvider' => $tracerProvider, + ]); + + $call = new Call( + 'google.cloud.secretmanager.v1.SecretManagerService/AccessSecretVersion', + Status::class, + new MockRequest() + ); + + $this->expectException(\RuntimeException::class); + $this->expectExceptionMessage('Auth callback failed'); + + $transport->startUnaryCall($call, [ + 'credentialsWrapper' => $credentialsWrapper->reveal(), + ]); + } + public function testBuildSetsTelemetryOptions(): void { $tracerProvider = $this->createMock(TracerProviderInterface::class); From 48f5749c08a2eac8ade3313bf75c6d00810c3601 Mon Sep 17 00:00:00 2001 From: Brent Shaffer Date: Wed, 7 Oct 2026 19:31:38 +0000 Subject: [PATCH 2/7] fix(Gax): ignore invalid default apiEndpoint when TransportInterface is provided --- Gax/src/GapicClientTrait.php | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/Gax/src/GapicClientTrait.php b/Gax/src/GapicClientTrait.php index 8ca6fc4450a8..3671e108756b 100644 --- a/Gax/src/GapicClientTrait.php +++ b/Gax/src/GapicClientTrait.php @@ -383,9 +383,22 @@ private function setClientOptions(array $options) $this->apiEndpoint = $options['apiEndpoint']; if ($this->apiEndpoint !== '') { - [$serverAddress, $serverPort] = self::normalizeServiceAddress($this->apiEndpoint); - $this->serverAddress = $serverAddress; - $this->serverPort = (int) $serverPort; + try { + [$serverAddress, $serverPort] = self::normalizeServiceAddress($this->apiEndpoint); + $this->serverAddress = $serverAddress; + $this->serverPort = (int) $serverPort; + } catch (ValidationException $e) { + if (!$options['transport'] instanceof TransportInterface) { + throw $e; + } + if (defined('self::SERVICE_ADDRESS')) { + [$serverAddress, $serverPort] = self::normalizeServiceAddress( + self::SERVICE_ADDRESS // @phpstan-ignore-line + ); + $this->serverAddress = $serverAddress; + $this->serverPort = (int) $serverPort; + } + } } $this->telemetryOptions = [ 'openTelemetryTracerProvider' => $options['openTelemetryTracerProvider'] ?? null, From 27d4514806759d01f10bf0d8e4c247f96d6dbc95 Mon Sep 17 00:00:00 2001 From: Brent Shaffer Date: Wed, 7 Oct 2026 22:15:08 +0000 Subject: [PATCH 3/7] test(Gax): pass apiEndpoint in ShowcaseTest until gapic-generator-php#889 is merged --- Gax/src/GapicClientTrait.php | 19 +++---------------- Gax/tests/Conformance/ShowcaseTest.php | 6 ++++++ 2 files changed, 9 insertions(+), 16 deletions(-) diff --git a/Gax/src/GapicClientTrait.php b/Gax/src/GapicClientTrait.php index 3671e108756b..8ca6fc4450a8 100644 --- a/Gax/src/GapicClientTrait.php +++ b/Gax/src/GapicClientTrait.php @@ -383,22 +383,9 @@ private function setClientOptions(array $options) $this->apiEndpoint = $options['apiEndpoint']; if ($this->apiEndpoint !== '') { - try { - [$serverAddress, $serverPort] = self::normalizeServiceAddress($this->apiEndpoint); - $this->serverAddress = $serverAddress; - $this->serverPort = (int) $serverPort; - } catch (ValidationException $e) { - if (!$options['transport'] instanceof TransportInterface) { - throw $e; - } - if (defined('self::SERVICE_ADDRESS')) { - [$serverAddress, $serverPort] = self::normalizeServiceAddress( - self::SERVICE_ADDRESS // @phpstan-ignore-line - ); - $this->serverAddress = $serverAddress; - $this->serverPort = (int) $serverPort; - } - } + [$serverAddress, $serverPort] = self::normalizeServiceAddress($this->apiEndpoint); + $this->serverAddress = $serverAddress; + $this->serverPort = (int) $serverPort; } $this->telemetryOptions = [ 'openTelemetryTracerProvider' => $options['openTelemetryTracerProvider'] ?? null, diff --git a/Gax/tests/Conformance/ShowcaseTest.php b/Gax/tests/Conformance/ShowcaseTest.php index d489ee0c8ff5..0f89ae92ad45 100644 --- a/Gax/tests/Conformance/ShowcaseTest.php +++ b/Gax/tests/Conformance/ShowcaseTest.php @@ -77,6 +77,9 @@ public function provideTransport() public function testFailWithDetails(TransportInterface $transport): void { $echoClient = new EchoClient([ + // @TODO: Remove apiEndpoint once https://github.com/googleapis/gapic-generator-php/pull/889 is merged + // and the Showcase client is regenerated. + 'apiEndpoint' => 'localhost:7469', 'credentials' => new InsecureCredentialsWrapper(), 'transport' => $transport, ]); @@ -112,6 +115,9 @@ public function testPqc(TransportInterface $transport): void }; $echoClient = new EchoClient([ + // @TODO: Remove apiEndpoint once https://github.com/googleapis/gapic-generator-php/pull/889 is merged + // and the Showcase client is regenerated. + 'apiEndpoint' => 'localhost:7469', 'credentials' => new InsecureCredentialsWrapper(), 'transport' => $transport ]); From 19c6a49b55972ef2e6a8407b983f9caf88cc19f3 Mon Sep 17 00:00:00 2001 From: Brent Shaffer Date: Thu, 8 Oct 2026 00:49:53 +0000 Subject: [PATCH 4/7] fix(Gax): only normalize apiEndpoint when tracing is enabled and revert ShowcaseTest --- Gax/src/GapicClientTrait.php | 12 +++++------- Gax/tests/Conformance/ShowcaseTest.php | 6 ------ 2 files changed, 5 insertions(+), 13 deletions(-) diff --git a/Gax/src/GapicClientTrait.php b/Gax/src/GapicClientTrait.php index 8ca6fc4450a8..eab772c4d5f6 100644 --- a/Gax/src/GapicClientTrait.php +++ b/Gax/src/GapicClientTrait.php @@ -73,7 +73,6 @@ trait GapicClientTrait private ?TransportInterface $transport = null; private ?HeaderCredentialsInterface $credentialsWrapper = null; private array $telemetryOptions = []; - private string $apiEndpoint = ''; private ?string $serverAddress = null; private ?int $serverPort = null; /** @var RetrySettings[] $retrySettings */ @@ -381,16 +380,15 @@ private function setClientOptions(array $options) ); } - $this->apiEndpoint = $options['apiEndpoint']; - if ($this->apiEndpoint !== '') { - [$serverAddress, $serverPort] = self::normalizeServiceAddress($this->apiEndpoint); - $this->serverAddress = $serverAddress; - $this->serverPort = (int) $serverPort; - } $this->telemetryOptions = [ 'openTelemetryTracerProvider' => $options['openTelemetryTracerProvider'] ?? null, 'clientVersion' => $options['gapicVersion'] ?? null, ]; + if (!empty($this->telemetryOptions['openTelemetryTracerProvider']) && $options['apiEndpoint'] !== '') { + [$serverAddress, $serverPort] = self::normalizeServiceAddress($options['apiEndpoint']); + $this->serverAddress = $serverAddress; + $this->serverPort = (int) $serverPort; + } $transport = $options['transport'] ?: self::defaultTransport(); $this->transport = $transport instanceof TransportInterface diff --git a/Gax/tests/Conformance/ShowcaseTest.php b/Gax/tests/Conformance/ShowcaseTest.php index 0f89ae92ad45..d489ee0c8ff5 100644 --- a/Gax/tests/Conformance/ShowcaseTest.php +++ b/Gax/tests/Conformance/ShowcaseTest.php @@ -77,9 +77,6 @@ public function provideTransport() public function testFailWithDetails(TransportInterface $transport): void { $echoClient = new EchoClient([ - // @TODO: Remove apiEndpoint once https://github.com/googleapis/gapic-generator-php/pull/889 is merged - // and the Showcase client is regenerated. - 'apiEndpoint' => 'localhost:7469', 'credentials' => new InsecureCredentialsWrapper(), 'transport' => $transport, ]); @@ -115,9 +112,6 @@ public function testPqc(TransportInterface $transport): void }; $echoClient = new EchoClient([ - // @TODO: Remove apiEndpoint once https://github.com/googleapis/gapic-generator-php/pull/889 is merged - // and the Showcase client is regenerated. - 'apiEndpoint' => 'localhost:7469', 'credentials' => new InsecureCredentialsWrapper(), 'transport' => $transport ]); From 5f7fa18eef45fca947d8d9afb424b2df91506786 Mon Sep 17 00:00:00 2001 From: Brent Shaffer Date: Thu, 8 Oct 2026 17:22:17 +0000 Subject: [PATCH 5/7] fix(Gax): only normalize GrpcTransport hostname when telemetry is active and ignore invalid formats --- Gax/src/GapicClientTrait.php | 10 ++++--- Gax/src/Transport/GrpcTransport.php | 26 ++++++++++++++++--- .../Unit/Transport/GrpcTransportTest.php | 18 +++++++++++++ 3 files changed, 47 insertions(+), 7 deletions(-) diff --git a/Gax/src/GapicClientTrait.php b/Gax/src/GapicClientTrait.php index eab772c4d5f6..f8316443022b 100644 --- a/Gax/src/GapicClientTrait.php +++ b/Gax/src/GapicClientTrait.php @@ -385,9 +385,13 @@ private function setClientOptions(array $options) 'clientVersion' => $options['gapicVersion'] ?? null, ]; if (!empty($this->telemetryOptions['openTelemetryTracerProvider']) && $options['apiEndpoint'] !== '') { - [$serverAddress, $serverPort] = self::normalizeServiceAddress($options['apiEndpoint']); - $this->serverAddress = $serverAddress; - $this->serverPort = (int) $serverPort; + try { + [$serverAddress, $serverPort] = self::normalizeServiceAddress($options['apiEndpoint']); + $this->serverAddress = $serverAddress; + $this->serverPort = (int) $serverPort; + } catch (ValidationException $e) { + // Ignore invalid apiEndpoint formats when setting span attributes + } } $transport = $options['transport'] ?: self::defaultTransport(); diff --git a/Gax/src/Transport/GrpcTransport.php b/Gax/src/Transport/GrpcTransport.php index 3169b24480ca..47dc01b9dc07 100644 --- a/Gax/src/Transport/GrpcTransport.php +++ b/Gax/src/Transport/GrpcTransport.php @@ -71,6 +71,7 @@ class GrpcTransport extends BaseStub implements TransportInterface use TelemetryTrait; private null|LoggerInterface $logger; + private string $hostname; private ?string $serverAddress = null; private ?int $serverPort = null; @@ -108,11 +109,28 @@ public function __construct( parent::__construct($hostname, $opts, $channel); $this->logger = $logger; - if ($hostname !== '') { - [$addr, $port] = self::normalizeServiceAddress($hostname); - $this->serverAddress = $addr; - $this->serverPort = (int) $port; + $this->hostname = $hostname; + } + + /** + * Sets telemetry options and initializes tracing properties. + * + * @param array $telemetryOptions + * @return $this + */ + public function setTelemetryOptions(array $telemetryOptions): self + { + $this->initTelemetry($telemetryOptions); + if ($this->openTelemetryTracerProvider && $this->hostname !== '') { + try { + [$addr, $port] = self::normalizeServiceAddress($this->hostname); + $this->serverAddress = $addr; + $this->serverPort = (int) $port; + } catch (ValidationException $e) { + // Ignore invalid hostname formats when setting span attributes + } } + return $this; } /** diff --git a/Gax/tests/Unit/Transport/GrpcTransportTest.php b/Gax/tests/Unit/Transport/GrpcTransportTest.php index 72dfa4b0c164..c1aef355722c 100644 --- a/Gax/tests/Unit/Transport/GrpcTransportTest.php +++ b/Gax/tests/Unit/Transport/GrpcTransportTest.php @@ -920,4 +920,22 @@ public function testBuildSetsTelemetryOptions(): void $portProp = $ref->getProperty('serverPort'); $this->assertSame(443, $portProp->getValue($transport)); } + + public function testConstructAndSetTelemetryOptionsIgnoreInvalidHostname(): void + { + $transport = new GrpcTransport('dns:///localhost:7469', [ + 'credentials' => ChannelCredentials::createSsl(), + ]); + + $tracerProvider = $this->createMock(TracerProviderInterface::class); + $transport->setTelemetryOptions([ + 'openTelemetryTracerProvider' => $tracerProvider, + ]); + + $ref = new ReflectionClass($transport); + $addrProp = $ref->getProperty('serverAddress'); + $portProp = $ref->getProperty('serverPort'); + $this->assertNull($addrProp->getValue($transport)); + $this->assertNull($portProp->getValue($transport)); + } } From f24d7ced380baa23a9aa87b2d6cb12c4cb416aa3 Mon Sep 17 00:00:00 2001 From: Brent Shaffer Date: Thu, 8 Oct 2026 17:54:29 +0000 Subject: [PATCH 6/7] refactor(Gax): make setTelemetryOptions protected and remove initTelemetry --- Gax/src/Middleware/TracingMiddleware.php | 2 +- Gax/src/Telemetry/TelemetryTrait.php | 14 ++------------ Gax/src/Transport/GrpcTransport.php | 8 +++++--- Gax/tests/Unit/Transport/GrpcTransportTest.php | 15 +++++++++++---- 4 files changed, 19 insertions(+), 20 deletions(-) diff --git a/Gax/src/Middleware/TracingMiddleware.php b/Gax/src/Middleware/TracingMiddleware.php index 1a71f61ed2fc..52abf1278485 100644 --- a/Gax/src/Middleware/TracingMiddleware.php +++ b/Gax/src/Middleware/TracingMiddleware.php @@ -74,7 +74,7 @@ public function __construct( $this->serverAddress = $serverAddress ?: null; $this->serverPort = $serverPort; $this->systemName = $systemName; - $this->initTelemetry($telemetryOptions); + $this->setTelemetryOptions($telemetryOptions); } /** diff --git a/Gax/src/Telemetry/TelemetryTrait.php b/Gax/src/Telemetry/TelemetryTrait.php index 995fffb64342..b43d5b277f7c 100644 --- a/Gax/src/Telemetry/TelemetryTrait.php +++ b/Gax/src/Telemetry/TelemetryTrait.php @@ -56,21 +56,11 @@ trait TelemetryTrait * @param array $telemetryOptions * @return $this */ - public function setTelemetryOptions(array $telemetryOptions): self - { - $this->initTelemetry($telemetryOptions); - return $this; - } - - /** - * Initializes telemetry properties from an options array. - * - * @param array $telemetryOptions - */ - private function initTelemetry(array $telemetryOptions): void + protected function setTelemetryOptions(array $telemetryOptions): self { $this->openTelemetryTracerProvider = $telemetryOptions['openTelemetryTracerProvider'] ?? null; $this->clientVersion = $telemetryOptions['clientVersion'] ?? null; + return $this; } /** diff --git a/Gax/src/Transport/GrpcTransport.php b/Gax/src/Transport/GrpcTransport.php index 47dc01b9dc07..64b3a9eb3e33 100644 --- a/Gax/src/Transport/GrpcTransport.php +++ b/Gax/src/Transport/GrpcTransport.php @@ -68,7 +68,9 @@ class GrpcTransport extends BaseStub implements TransportInterface use GrpcSupportTrait; use ServiceAddressTrait; use LoggingTrait; - use TelemetryTrait; + use TelemetryTrait { + setTelemetryOptions as private traitSetTelemetryOptions; + } private null|LoggerInterface $logger; private string $hostname; @@ -118,9 +120,9 @@ public function __construct( * @param array $telemetryOptions * @return $this */ - public function setTelemetryOptions(array $telemetryOptions): self + protected function setTelemetryOptions(array $telemetryOptions): self { - $this->initTelemetry($telemetryOptions); + $this->traitSetTelemetryOptions($telemetryOptions); if ($this->openTelemetryTracerProvider && $this->hostname !== '') { try { [$addr, $port] = self::normalizeServiceAddress($this->hostname); diff --git a/Gax/tests/Unit/Transport/GrpcTransportTest.php b/Gax/tests/Unit/Transport/GrpcTransportTest.php index c1aef355722c..2bd57fdeb117 100644 --- a/Gax/tests/Unit/Transport/GrpcTransportTest.php +++ b/Gax/tests/Unit/Transport/GrpcTransportTest.php @@ -784,7 +784,7 @@ public function testStartUnaryCallEmitsT4ClientSpanOnSuccess(): void ->willReturn([$response, $status]); $transport = new MockGrpcTransport($unaryCall->reveal()); - $transport->setTelemetryOptions([ + $this->setTelemetryOptions($transport, [ 'openTelemetryTracerProvider' => $tracerProvider, 'clientVersion' => '1.0.0', ]); @@ -840,7 +840,7 @@ public function testStartUnaryCallEmitsT4ClientSpanOnFailure(): void ->willReturn([null, $status]); $transport = new MockGrpcTransport($unaryCall->reveal()); - $transport->setTelemetryOptions([ + $this->setTelemetryOptions($transport, [ 'openTelemetryTracerProvider' => $tracerProvider, ]); @@ -883,7 +883,7 @@ public function testStartUnaryCallEndsSpanOnSynchronousException(): void ->willThrow(new \RuntimeException('Auth callback failed')); $transport = new MockGrpcTransport(null); - $transport->setTelemetryOptions([ + $this->setTelemetryOptions($transport, [ 'openTelemetryTracerProvider' => $tracerProvider, ]); @@ -928,7 +928,7 @@ public function testConstructAndSetTelemetryOptionsIgnoreInvalidHostname(): void ]); $tracerProvider = $this->createMock(TracerProviderInterface::class); - $transport->setTelemetryOptions([ + $this->setTelemetryOptions($transport, [ 'openTelemetryTracerProvider' => $tracerProvider, ]); @@ -938,4 +938,11 @@ public function testConstructAndSetTelemetryOptionsIgnoreInvalidHostname(): void $this->assertNull($addrProp->getValue($transport)); $this->assertNull($portProp->getValue($transport)); } + + private function setTelemetryOptions(GrpcTransport $transport, array $options): void + { + (new ReflectionClass(GrpcTransport::class)) + ->getMethod('setTelemetryOptions') + ->invoke($transport, $options); + } } From 525e7b878eaec70e3db67e046eadd055946f5ccf Mon Sep 17 00:00:00 2001 From: Brent Shaffer Date: Thu, 8 Oct 2026 21:33:17 +0000 Subject: [PATCH 7/7] refactor(Gax): consolidate private setTelemetryOptions in TelemetryTrait --- Gax/src/Middleware/TracingMiddleware.php | 2 -- Gax/src/Telemetry/TelemetryTrait.php | 18 ++++++++++- Gax/src/Transport/GrpcTransport.php | 31 ++----------------- .../Unit/Transport/GrpcTransportTest.php | 11 ++++--- 4 files changed, 26 insertions(+), 36 deletions(-) diff --git a/Gax/src/Middleware/TracingMiddleware.php b/Gax/src/Middleware/TracingMiddleware.php index 52abf1278485..7f3442ab37e9 100644 --- a/Gax/src/Middleware/TracingMiddleware.php +++ b/Gax/src/Middleware/TracingMiddleware.php @@ -52,8 +52,6 @@ class TracingMiddleware implements MiddlewareInterface /** @var MiddlewareInterface|callable */ private $nextHandler; - private ?string $serverAddress; - private ?int $serverPort; private string $systemName; /** diff --git a/Gax/src/Telemetry/TelemetryTrait.php b/Gax/src/Telemetry/TelemetryTrait.php index b43d5b277f7c..b3f54957e987 100644 --- a/Gax/src/Telemetry/TelemetryTrait.php +++ b/Gax/src/Telemetry/TelemetryTrait.php @@ -34,6 +34,8 @@ namespace Google\ApiCore\Telemetry; use Google\ApiCore\ApiException; +use Google\ApiCore\ServiceAddressTrait; +use Google\ApiCore\ValidationException; use OpenTelemetry\API\Trace\SpanInterface; use OpenTelemetry\API\Trace\SpanKind; use OpenTelemetry\API\Trace\StatusCode; @@ -47,19 +49,33 @@ */ trait TelemetryTrait { + use ServiceAddressTrait; + private ?TracerProviderInterface $openTelemetryTracerProvider = null; private ?string $clientVersion = null; + private ?string $serverAddress = null; + private ?int $serverPort = null; /** * Sets telemetry options and initializes tracing properties. * * @param array $telemetryOptions + * @param string|null $apiEndpoint * @return $this */ - protected function setTelemetryOptions(array $telemetryOptions): self + private function setTelemetryOptions(array $telemetryOptions, ?string $apiEndpoint = null): self { $this->openTelemetryTracerProvider = $telemetryOptions['openTelemetryTracerProvider'] ?? null; $this->clientVersion = $telemetryOptions['clientVersion'] ?? null; + if ($this->openTelemetryTracerProvider && $apiEndpoint) { + try { + [$addr, $port] = self::normalizeServiceAddress($apiEndpoint); + $this->serverAddress = $addr; + $this->serverPort = (int) $port; + } catch (ValidationException $e) { + // Ignore invalid apiEndpoint formats when setting span attributes + } + } return $this; } diff --git a/Gax/src/Transport/GrpcTransport.php b/Gax/src/Transport/GrpcTransport.php index 64b3a9eb3e33..8bb451b68e75 100644 --- a/Gax/src/Transport/GrpcTransport.php +++ b/Gax/src/Transport/GrpcTransport.php @@ -68,14 +68,9 @@ class GrpcTransport extends BaseStub implements TransportInterface use GrpcSupportTrait; use ServiceAddressTrait; use LoggingTrait; - use TelemetryTrait { - setTelemetryOptions as private traitSetTelemetryOptions; - } + use TelemetryTrait; private null|LoggerInterface $logger; - private string $hostname; - private ?string $serverAddress = null; - private ?int $serverPort = null; /** * @param string $hostname @@ -111,28 +106,6 @@ public function __construct( parent::__construct($hostname, $opts, $channel); $this->logger = $logger; - $this->hostname = $hostname; - } - - /** - * Sets telemetry options and initializes tracing properties. - * - * @param array $telemetryOptions - * @return $this - */ - protected function setTelemetryOptions(array $telemetryOptions): self - { - $this->traitSetTelemetryOptions($telemetryOptions); - if ($this->openTelemetryTracerProvider && $this->hostname !== '') { - try { - [$addr, $port] = self::normalizeServiceAddress($this->hostname); - $this->serverAddress = $addr; - $this->serverPort = (int) $port; - } catch (ValidationException $e) { - // Ignore invalid hostname formats when setting span attributes - } - } - return $this; } /** @@ -195,7 +168,7 @@ public static function build(string $apiEndpoint, array $config = []) $config['logger'] = null; } $transport = new GrpcTransport($host, $stubOpts, $channel, $config['interceptors'], $config['logger']); - $transport->setTelemetryOptions($config); + $transport->setTelemetryOptions($config, $host); return $transport; } catch (Exception $ex) { throw new ValidationException( diff --git a/Gax/tests/Unit/Transport/GrpcTransportTest.php b/Gax/tests/Unit/Transport/GrpcTransportTest.php index 2bd57fdeb117..5dcc57636e6e 100644 --- a/Gax/tests/Unit/Transport/GrpcTransportTest.php +++ b/Gax/tests/Unit/Transport/GrpcTransportTest.php @@ -930,7 +930,7 @@ public function testConstructAndSetTelemetryOptionsIgnoreInvalidHostname(): void $tracerProvider = $this->createMock(TracerProviderInterface::class); $this->setTelemetryOptions($transport, [ 'openTelemetryTracerProvider' => $tracerProvider, - ]); + ], 'dns:///localhost:7469'); $ref = new ReflectionClass($transport); $addrProp = $ref->getProperty('serverAddress'); @@ -939,10 +939,13 @@ public function testConstructAndSetTelemetryOptionsIgnoreInvalidHostname(): void $this->assertNull($portProp->getValue($transport)); } - private function setTelemetryOptions(GrpcTransport $transport, array $options): void - { + private function setTelemetryOptions( + GrpcTransport $transport, + array $options, + ?string $apiEndpoint = null + ): void { (new ReflectionClass(GrpcTransport::class)) ->getMethod('setTelemetryOptions') - ->invoke($transport, $options); + ->invoke($transport, $options, $apiEndpoint); } }