From f6ed0f9db85433c5426346a7f72d40e15f6a99e8 Mon Sep 17 00:00:00 2001 From: Yenfry Herrera Feliz Date: Wed, 13 Nov 2024 17:24:52 -0800 Subject: [PATCH] chore: address PR feedback - Use static mapping instead of multiple if/else-ifs. - Remove the endpoint_id metric tracking since it was removed from the spec. - Make method's name to be verb based. - Remove the getMetricsBuilder from the CommandInterface. --- src/AwsClient.php | 4 +- src/Command.php | 3 +- src/CommandInterface.php | 7 --- src/Credentials/CredentialSources.php | 2 +- src/EndpointV2/EndpointV2Middleware.php | 4 -- src/MetricsBuilder.php | 84 ++++++++++--------------- src/UserAgentMiddleware.php | 48 +++++++------- tests/MetricsBuilderTest.php | 23 ------- tests/UserAgentMiddlewareTest.php | 35 ----------- 9 files changed, 60 insertions(+), 150 deletions(-) diff --git a/src/AwsClient.php b/src/AwsClient.php index 63f67f61b7..bbdd2099ee 100644 --- a/src/AwsClient.php +++ b/src/AwsClient.php @@ -438,7 +438,6 @@ private function addSignatureMiddleware(array $args) $name = $this->config['signing_name']; $region = $this->config['signing_region']; $signingRegionSet = $this->signingRegionSet; - $handlerList = $this->getHandlerList(); if (isset($args['signature_version']) || isset($this->config['configured_signature_version']) @@ -457,8 +456,7 @@ private function addSignatureMiddleware(array $args) $region, $signatureVersion, $configuredSignatureVersion, - $signingRegionSet, - $handlerList + $signingRegionSet ) { if (!$configuredSignatureVersion) { if (!empty($command['@context']['signing_region'])) { diff --git a/src/Command.php b/src/Command.php index f6c3991355..949f7f4ed8 100644 --- a/src/Command.php +++ b/src/Command.php @@ -121,7 +121,8 @@ public function get($name) } /** - * @inheridoc + * Returns the metrics builder instance tied up to this command. + * * @internal * * @return MetricsBuilder diff --git a/src/CommandInterface.php b/src/CommandInterface.php index 853338039a..b35c75d37b 100644 --- a/src/CommandInterface.php +++ b/src/CommandInterface.php @@ -39,11 +39,4 @@ public function hasParam($name); * @return HandlerList */ public function getHandlerList(); - - /** - * Returns the metrics builder instance tied up to this command. - * - * @return MetricsBuilder - */ - public function getMetricsBuilder(); } diff --git a/src/Credentials/CredentialSources.php b/src/Credentials/CredentialSources.php index 6480b7c838..08dd678e0d 100644 --- a/src/Credentials/CredentialSources.php +++ b/src/Credentials/CredentialSources.php @@ -5,7 +5,7 @@ /** * @internal */ -class CredentialSources +final class CredentialSources { const STATIC = 'static'; const ENVIRONMENT = 'env'; diff --git a/src/EndpointV2/EndpointV2Middleware.php b/src/EndpointV2/EndpointV2Middleware.php index 0f141037c6..c271baae15 100644 --- a/src/EndpointV2/EndpointV2Middleware.php +++ b/src/EndpointV2/EndpointV2Middleware.php @@ -103,10 +103,6 @@ public function __invoke(CommandInterface $command) $command->getMetricsBuilder()->append(MetricsBuilder::RESOLVED_ACCOUNT_ID); } $endpoint = $this->endpointProvider->resolveEndpoint($providerArgs); - $command->getMetricsBuilder()->identifyMetricByValueAndAppend( - 'account_id_endpoint', - $endpoint->getUrl() - ); if (!empty($authSchemes = $endpoint->getProperty('authSchemes'))) { $this->applyAuthScheme( $authSchemes, diff --git a/src/MetricsBuilder.php b/src/MetricsBuilder.php index a267088702..d27634f290 100644 --- a/src/MetricsBuilder.php +++ b/src/MetricsBuilder.php @@ -132,7 +132,6 @@ public function identifyMetricByValueAndAppend( static $appendMetricFns = [ 'signature' => 'appendSignatureMetric', 'request_compression' => 'appendRequestCompressionMetric', - 'account_id_endpoint' => 'appendAccountIdEndpoint', 'request_checksum' => 'appendRequestChecksumMetric', 'credentials' => 'appendCredentialsMetric' ]; @@ -171,22 +170,6 @@ private function appendRequestCompressionMetric(string $format): void } } - /** - * Appends the account id endpoint metric by validating if the - * endpoint contains an account id in its URL. - * - * @param string $endpoint - * - * @return void - */ - private function appendAccountIdEndpoint(string $endpoint): void - { - $regex = "/^(https?:\/\/\d{12}\.[^\s\/$.?#].\S*)$/"; - if (preg_match($regex, $endpoint)) { - $this->append(MetricsBuilder::ACCOUNT_ID_ENDPOINT); - } - } - /** * Appends the request checksum metric based on the algorithm. * @@ -227,30 +210,34 @@ private function appendCredentialsMetric( return; } - if ($source === CredentialSources::STATIC) { - $this->append(MetricsBuilder::CREDENTIALS_CODE); - } elseif ($source === CredentialSources::ENVIRONMENT) { - $this->append(MetricsBuilder::CREDENTIALS_ENV_VARS); - } elseif ($source === CredentialSources::ENVIRONMENT_STS_WEB_ID_TOKEN) { - $this->append(MetricsBuilder::CREDENTIALS_ENV_VARS_STS_WEB_ID_TOKEN); - } elseif ($source === CredentialSources::STS_ASSUME_ROLE) { - $this->append(MetricsBuilder::CREDENTIALS_STS_ASSUME_ROLE); - } elseif ($source === CredentialSources::STS_WEB_ID_TOKEN) { - $this->append(MetricsBuilder::CREDENTIALS_STS_ASSUME_ROLE_WEB_ID); - } elseif ($source === CredentialSources::PROFILE) { - $this->append(MetricsBuilder::CREDENTIALS_PROFILE); - } elseif ($source === CredentialSources::IMDS) { - $this->append(MetricsBuilder::CREDENTIALS_IMDS); - } elseif ($source === CredentialSources::ECS) { - $this->append(MetricsBuilder::CREDENTIALS_HTTP); - } elseif ($source === CredentialSources::PROFILE_STS_WEB_ID_TOKEN) { - $this->append(MetricsBuilder::CREDENTIALS_PROFILE_STS_WEB_ID_TOKEN); - } elseif ($source === CredentialSources::PROCESS) { - $this->append(MetricsBuilder::CREDENTIALS_PROCESS); - } elseif ($source === CredentialSources::SSO) { - $this->append(MetricsBuilder::CREDENTIALS_SSO); - } elseif ($source === CredentialSources::SSO_LEGACY) { - $this->append(MetricsBuilder::CREDENTIALS_SSO_LEGACY); + static $credentialsMetricMapping = [ + CredentialSources::STATIC => + MetricsBuilder::CREDENTIALS_CODE, + CredentialSources::ENVIRONMENT => + MetricsBuilder::CREDENTIALS_ENV_VARS, + CredentialSources::ENVIRONMENT_STS_WEB_ID_TOKEN => + MetricsBuilder::CREDENTIALS_ENV_VARS_STS_WEB_ID_TOKEN, + CredentialSources::STS_ASSUME_ROLE => + MetricsBuilder::CREDENTIALS_STS_ASSUME_ROLE, + CredentialSources::STS_WEB_ID_TOKEN => + MetricsBuilder::CREDENTIALS_STS_ASSUME_ROLE_WEB_ID, + CredentialSources::PROFILE => + MetricsBuilder::CREDENTIALS_PROFILE, + CredentialSources::IMDS => + MetricsBuilder::CREDENTIALS_IMDS, + CredentialSources::ECS => + MetricsBuilder::CREDENTIALS_HTTP, + CredentialSources::PROFILE_STS_WEB_ID_TOKEN => + MetricsBuilder::CREDENTIALS_PROFILE_STS_WEB_ID_TOKEN, + CredentialSources::PROCESS => + MetricsBuilder::CREDENTIALS_PROCESS, + CredentialSources::SSO => + MetricsBuilder::CREDENTIALS_SSO, + CredentialSources::SSO_LEGACY => + MetricsBuilder::CREDENTIALS_SSO_LEGACY, + ]; + if (isset($credentialsMetricMapping[$source])) { + $this->append($credentialsMetricMapping[$source]); } } @@ -269,25 +256,18 @@ private function appendCredentialsMetric( */ private function canMetricBeAppended(string $newMetric): bool { + if ($newMetric === "") { + return false; + } + if ($this->metricsSize + (strlen($newMetric) + strlen(self::$METRIC_SEPARATOR)) > self::$MAX_METRICS_SIZE ) { - @trigger_error( - "The metric `{$newMetric}` " - . "can not be added due to size constraints", - E_USER_WARNING - ); - return false; } if (isset($this->metrics[$newMetric])) { - @trigger_error( - 'The metric ' . $newMetric. ' is already appended!', - E_USER_WARNING - ); - return false; } diff --git a/src/UserAgentMiddleware.php b/src/UserAgentMiddleware.php index 1ec0da2dd2..066e133270 100644 --- a/src/UserAgentMiddleware.php +++ b/src/UserAgentMiddleware.php @@ -16,20 +16,20 @@ class UserAgentMiddleware { const AGENT_VERSION = 2.1; static $userAgentFnList = [ - 'sdkVersion', - 'userAgentVersion', - 'hhvmVersion', - 'osName', - 'langVersion', - 'execEnv', - 'endpointDiscovery', - 'appId', - 'metrics' + 'getSdkVersion', + 'getUserAgentVersion', + 'getHhvmVersion', + 'getOsName', + 'getLangVersion', + 'getExecEnv', + 'getEndpointDiscovery', + 'getAppId', + 'getMetrics' ]; static $metricsFnList = [ - 'endpointMetric', - 'accountIdModeMetric', - 'retryConfigMetric', + 'appendEndpointMetric', + 'appendAccountIdModeMetric', + 'appendRetryConfigMetric', ]; /** @var callable */ @@ -138,7 +138,7 @@ private function buildUserAgentValue(): array * * @return string */ - private function sdkVersion(): string + private function getSdkVersion(): string { return 'aws-sdk-php/' . Sdk::VERSION; } @@ -148,7 +148,7 @@ private function sdkVersion(): string * * @return string */ - private function userAgentVersion(): string + private function getUserAgentVersion(): string { return 'ua/' . self::AGENT_VERSION; } @@ -159,7 +159,7 @@ private function userAgentVersion(): string * * @return string */ - private function hhvmVersion(): string + private function getHhvmVersion(): string { if (defined('HHVM_VERSION')) { return 'HHVM/' . HHVM_VERSION; @@ -173,7 +173,7 @@ private function hhvmVersion(): string * * @return string */ - private function osName(): string + private function getOsName(): string { $disabledFunctions = explode(',', ini_get('disable_functions')); if (function_exists('php_uname') @@ -193,7 +193,7 @@ private function osName(): string * * @return string */ - private function langVersion(): string + private function getLangVersion(): string { return 'lang/php#' . phpversion(); } @@ -203,7 +203,7 @@ private function langVersion(): string * * @return string */ - private function execEnv(): string + private function getExecEnv(): string { if ($executionEnvironment = getenv('AWS_EXECUTION_ENV')) { return $executionEnvironment; @@ -218,7 +218,7 @@ private function execEnv(): string * * @return string */ - private function endpointDiscovery(): string + private function getEndpointDiscovery(): string { $args = $this->args; if (isset($args['endpoint_discovery'])) { @@ -243,7 +243,7 @@ private function endpointDiscovery(): string * * @return string */ - private function appId(): string + private function getAppId(): string { if (empty($this->args['app_id'])) { return ""; @@ -257,7 +257,7 @@ private function appId(): string * * @return string */ - private function metrics(): string + private function getMetrics(): string { foreach (self::$metricsFnList as $fn) { $this->{$fn}(); @@ -275,7 +275,7 @@ private function metrics(): string * Appends the endpoint metric into the metrics builder, * just if a custom endpoint was provided at client construction. */ - private function endpointMetric(): void + private function appendEndpointMetric(): void { if (!empty($this->args['endpoint'])) { $this->metricsBuilder->append(MetricsBuilder::ENDPOINT_OVERRIDE); @@ -286,7 +286,7 @@ private function endpointMetric(): void * Appends the account id endpoint mode metric into the metrics builder, * based on the account id endpoint mode provide as client argument. */ - private function accountIdModeMetric(): void + private function appendAccountIdModeMetric(): void { $accountIdMode = $this->args['account_id_endpoint_mode'] ?? null; if ($accountIdMode === null) { @@ -306,7 +306,7 @@ private function accountIdModeMetric(): void * Appends the retry mode metric into the metrics builder, * based on the resolved retry config mode. */ - private function retryConfigMetric(): void + private function appendRetryConfigMetric(): void { $retries = $this->args['retries'] ?? null; if ($retries === null) { diff --git a/tests/MetricsBuilderTest.php b/tests/MetricsBuilderTest.php index 99c9e23cd4..eda1a7f9d9 100644 --- a/tests/MetricsBuilderTest.php +++ b/tests/MetricsBuilderTest.php @@ -61,29 +61,6 @@ static function ( $errno, $errstr ) { } } - public function testEmitMetricsSizeConstraintWarning() - { - try { - // Prevent deprecation warning for expectWarning - set_error_handler( - static function ( $errno, $errstr ) { - throw new \Exception( $errstr, $errno ); - }, - E_ALL - ); - $this->expectException(Exception::class); - $this->expectExceptionMessage( - "The metric `A` can not be added due to size constraints" - ); - $metricsBuilder = new MetricsBuilder(); - $firstMetric = str_repeat("*", 1024); - $metricsBuilder->append($firstMetric); - $metricsBuilder->append("A"); - } finally { - restore_error_handler(); - } - } - public function testGetMetricsBuilderFromCommand() { $command = new Command('TestCommand', [], new HandlerList()); diff --git a/tests/UserAgentMiddlewareTest.php b/tests/UserAgentMiddlewareTest.php index 000205061c..2cc14c4396 100644 --- a/tests/UserAgentMiddlewareTest.php +++ b/tests/UserAgentMiddlewareTest.php @@ -650,41 +650,6 @@ public function testUserAgentCaptureGzipRequestCompressionMetric() ]); } - /** - * Tests user agent captures the account id endpoint metric. - * - * @return void - */ - public function testUserAgentCaptureAccountIdEndpointMetric() - { - $dynamoDbClient = new DynamoDbClient([ - 'region' => 'us-east-2', - 'credentials' => new Credentials( - 'foo', - 'foo', - 'foo', - null, - '123456789012' - ), - 'http_handler' => function ( - RequestInterface $request - ) { - $metrics = $this->getMetricsAsArray($request); - - $this->assertTrue( - in_array(MetricsBuilder::ACCOUNT_ID_ENDPOINT, $metrics) - ); - - return new Response( - 200, - [], - '{}' - ); - } - ]); - $dynamoDbClient->listTables(); - } - /** * Tests user agent captures a resolved account id metric. *